Make ICANN reporting not fail on success upload (#791)

* Make ICANN reporting not fail on success upload

According to the spec
(https://tools.ietf.org/html/draft-lozano-icann-registry-interfaces-13#page-16),
when an upload succeeds (HTTP response code 200), the result code
contained in the response message is always 1000 (success). So there is
no need to parse the response content and check the result code. Given
that we are having a problem parsing the response content due to encoding,
it is best that we don't check it so as to not get false negative
alerts when the upload is successful.

The current logic also has a bug: HttpRequest.execute() will by default
throw when the response code is non-20X. Therefore for a 400 response,
our parsing logic never runs on it. Coincidentally, this month when we
uploaded the July activity report (due to stale cursors), we get 400
responses (due to existing reports on the ICANN servers). The stack
trace printed for the thrown exceptions from the 400 responses contained
correctly parsed response contents. This lead us to believe that the issue with
encoding was transient last month. However when we tried again to upload this
month's report, our parser failed again (because the response code was 200 this
time, and our parser actually ran on the response contents).

This seems to suggest that ICANN is sending back readable response
contents, but our parser somehow failed to understand it, assuming that
ICANN is using the same encoding for 200 (which we tried and failed to
parse) and 400 response contents (which caused an exception and was printed
corrected in the stack trace).

This PR changed the transport behavior so that it doesn't throw
automatically for non-20X responses. We will print the content for both
200 and 400 responses, but only try to parse 400 response content. We
put the 400 response in an HttpResponseException and print stack trace
from it, which should display the content correctly so that we can
compare it with the result of our own parsing.

* Add tests

core/src/main/java/google/registry/reporting/icann/IcannHttpReporter.java

@@ -24,6 +24,8 @@ import com.google.api.client.http.GenericUrl;
 import com.google.api.client.http.HttpHeaders;
 import com.google.api.client.http.HttpRequest;
 import com.google.api.client.http.HttpResponse;
+import com.google.api.client.http.HttpResponseException;
+import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.http.HttpTransport;
 import com.google.common.base.Ascii;
 import com.google.common.base.Splitter;
@@ -80,6 +82,7 @@ public class IcannHttpReporter {
     headers.setContentType(CSV_UTF_8.toString());
     request.setHeaders(headers);
     request.setFollowRedirects(false);
+    request.setThrowExceptionOnExecuteError(false);
 
     HttpResponse response = null;
     logger.atInfo().log(
@@ -87,6 +90,12 @@ public class IcannHttpReporter {
     boolean success = true;
     try {
       response = request.execute();
+      // Only responses with a 200 or 400 status have a body. For everything else, throw so that
+      // the caller catches it and prints the stack trace.
+      if (response.getStatusCode() != HttpStatusCodes.STATUS_CODE_OK
+          && response.getStatusCode() != HttpStatusCodes.STATUS_CODE_BAD_REQUEST) {
+        throw new HttpResponseException(response);
+      }
       byte[] content;
       try {
         content = ByteStreams.toByteArray(response.getContent());
@@ -94,13 +103,24 @@ public class IcannHttpReporter {
         response.getContent().close();
       }
       logger.atInfo().log(
-          "Received response code %d with content: %s\n\nResponse content in hex: %s",
+          "Received response code %d\n\n"
+              + "Response headers: %s\n\n"
+              + "Response content in UTF-8: %s\n\n"
+              + "Response content in HEX: %s",
           response.getStatusCode(),
+          response.getHeaders(),
           new String(content, UTF_8),
           BaseEncoding.base16().encode(content));
-      XjcIirdeaResult result = parseResult(content);
+      // For reasons unclear at the moment, when we parse the response content using UTF-8 we get
-      if (result.getCode().getValue() != 1000) {
+      // garbled texts. Since we know that an HTTP 200 response can only contain a result code of
+      // 1000 (i. e. success), there is no need to parse it.
+      if (response.getStatusCode() == HttpStatusCodes.STATUS_CODE_BAD_REQUEST) {
         success = false;
+        // To debug if there is a problem with our parsing, we wrap the response and print the stack
+        // trace of it. As far as we can tell, the stack trace for such an exception contains the
+        // response content that is decoded correctly using the expected charset.
+        new HttpResponseException(response).printStackTrace();
+        XjcIirdeaResult result = parseResult(content);
         logger.atWarning().log(
             "PUT rejected, status code %s:\n%s\n%s",
             result.getCode(), result.getMsg(), result.getDescription());

core/src/test/java/google/registry/reporting/icann/IcannHttpReporterTest.java

@@ -21,6 +21,8 @@ import static google.registry.testing.DatastoreHelper.createTld;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.google.api.client.http.HttpResponseException;
+import com.google.api.client.http.HttpStatusCodes;
 import com.google.api.client.http.LowLevelHttpRequest;
 import com.google.api.client.http.LowLevelHttpResponse;
 import com.google.api.client.testing.http.MockHttpTransport;
@@ -50,7 +52,8 @@ class IcannHttpReporterTest {
   AppEngineExtension appEngineRule =
       new AppEngineExtension.Builder().withDatastoreAndCloudSql().build();
 
-  private MockHttpTransport createMockTransport(final ByteSource iirdeaResponse) {
+  private MockHttpTransport createMockTransport(
+      int statusCode, final ByteSource iirdeaResponse) {
     return new MockHttpTransport() {
       @Override
       public LowLevelHttpRequest buildRequest(String method, String url) {
@@ -59,7 +62,7 @@ class IcannHttpReporterTest {
               @Override
               public LowLevelHttpResponse execute() throws IOException {
                 MockLowLevelHttpResponse response = new MockLowLevelHttpResponse();
-                response.setStatusCode(200);
+                response.setStatusCode(statusCode);
                 response.setContentType(PLAIN_TEXT_UTF_8.toString());
                 response.setContent(iirdeaResponse.read());
                 return response;
@@ -71,6 +74,10 @@ class IcannHttpReporterTest {
     };
   }
 
+  private MockHttpTransport createMockTransport(final ByteSource iirdeaResponse) {
+    return createMockTransport(HttpStatusCodes.STATUS_CODE_OK, iirdeaResponse);
+  }
+
   @BeforeEach
   void beforeEach() {
     createTld("test");
@@ -117,10 +124,21 @@ class IcannHttpReporterTest {
   @Test
   void testFail_BadIirdeaResponse() throws Exception {
     IcannHttpReporter reporter = createReporter();
-    reporter.httpTransport = createMockTransport(IIRDEA_BAD_XML);
+    reporter.httpTransport =
+        createMockTransport(HttpStatusCodes.STATUS_CODE_BAD_REQUEST, IIRDEA_BAD_XML);
     assertThat(reporter.send(FAKE_PAYLOAD, "test-transactions-201706.csv")).isFalse();
   }
 
+  @Test
+  void testFail_transportException() throws Exception {
+    IcannHttpReporter reporter = createReporter();
+    reporter.httpTransport =
+        createMockTransport(HttpStatusCodes.STATUS_CODE_FORBIDDEN, ByteSource.empty());
+    assertThrows(
+        HttpResponseException.class,
+        () -> reporter.send(FAKE_PAYLOAD, "test-transactions-201706.csv"));
+  }
+
   @Test
   void testFail_invalidFilename_nonSixDigitYearMonth() {
     IcannHttpReporter reporter = createReporter();