fix: Fix handling of null responses in rest transport (#1668)

Null response should not ever happen, but apparently it is possible in cases when errors occur somewhere on network/authentication level
googleapis · Apr 29, 2022 · 8def947 · 8def947
1 parent 4750384
commit 8def947
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 22 deletions.
diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonClientCalls.java
@@ -130,7 +130,16 @@ public void onMessage(T message) {
     @Override
     public void onClose(int statusCode, HttpJsonMetadata trailers) {
       if (!future.isDone()) {
-        future.setException(trailers.getException());
+        if (trailers == null || trailers.getException() == null) {
+          future.setException(
+              new HttpJsonStatusRuntimeException(
+                  statusCode,
+                  "Exception during a client call closure",
+                  new NullPointerException(
+                      "Both response message and response exception were null")));
+        } else {
+          future.setException(trailers.getException());
+        }
       } else if (statusCode < 200 || statusCode >= 400) {
         LOGGER.log(
             Level.WARNING, "Received error for unary call after receiving a successful response");

diff --git a/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java b/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpRequestRunnable.java
@@ -131,7 +131,9 @@ public void run() {
       result.setStatusCode(e.getStatusCode());
       result.setResponseHeaders(HttpJsonMetadata.newBuilder().setHeaders(e.getHeaders()).build());
       result.setResponseContent(
-          new ByteArrayInputStream(e.getContent().getBytes(StandardCharsets.UTF_8)));
+          e.getContent() != null
+              ? new ByteArrayInputStream(e.getContent().getBytes(StandardCharsets.UTF_8))
+              : null);
       trailers.setStatusMessage(e.getStatusMessage());
       trailers.setException(e);
     } catch (Throwable e) {

diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/HttpJsonDirectCallableTest.java
@@ -138,14 +138,7 @@ public void testSuccessfulUnaryResponse() throws ExecutionException, Interrupted
 
     Field request;
     Field expectedResponse;
-    request =
-        expectedResponse =
-            Field.newBuilder() // "echo" service
-                .setName("imTheBestField")
-                .setNumber(2)
-                .setCardinality(Cardinality.CARDINALITY_OPTIONAL)
-                .setDefaultValue("blah")
-                .build();
+    request = expectedResponse = createTestMessage();
 
     MOCK_SERVICE.addResponse(expectedResponse);
 
@@ -164,27 +157,65 @@ public void testErrorUnaryResponse() throws InterruptedException {
 
     HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel);
 
-    Field request;
-    request =
-        Field.newBuilder() // "echo" service
-            .setName("imTheBestField")
-            .setNumber(2)
-            .setCardinality(Cardinality.CARDINALITY_OPTIONAL)
-            .setDefaultValue("blah")
-            .build();
-
     ApiException exception =
         ApiExceptionFactory.createException(
             new Exception(), FakeStatusCode.of(Code.NOT_FOUND), false);
     MOCK_SERVICE.addException(exception);
 
     try {
-      callable.futureCall(request, callContext).get();
+      callable.futureCall(createTestMessage(), callContext).get();
       Assert.fail("No exception raised");
     } catch (ExecutionException e) {
       HttpResponseException respExp = (HttpResponseException) e.getCause();
       assertThat(respExp.getStatusCode()).isEqualTo(400);
       assertThat(respExp.getContent()).isEqualTo(exception.toString());
     }
   }
+
+  @Test
+  public void testErrorNullContentSuccessfulResponse() throws InterruptedException {
+    HttpJsonDirectCallable<Field, Field> callable =
+        new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR);
+
+    HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel);
+
+    MOCK_SERVICE.addNullResponse();
+
+    try {
+      callable.futureCall(createTestMessage(), callContext).get();
+      Assert.fail("No exception raised");
+    } catch (ExecutionException e) {
+      HttpJsonStatusRuntimeException respExp = (HttpJsonStatusRuntimeException) e.getCause();
+      assertThat(respExp.getStatusCode()).isEqualTo(200);
+      assertThat(respExp.getCause().getMessage())
+          .isEqualTo("Both response message and response exception were null");
+    }
+  }
+
+  @Test
+  public void testErrorNullContentFailedResponse() throws InterruptedException {
+    HttpJsonDirectCallable<Field, Field> callable =
+        new HttpJsonDirectCallable<>(FAKE_METHOD_DESCRIPTOR);
+
+    HttpJsonCallContext callContext = HttpJsonCallContext.createDefault().withChannel(channel);
+    MOCK_SERVICE.addNullResponse(400);
+
+    try {
+      callable.futureCall(createTestMessage(), callContext).get();
+      Assert.fail("No exception raised");
+    } catch (ExecutionException e) {
+      HttpResponseException respExp = (HttpResponseException) e.getCause();
+      assertThat(respExp.getStatusCode()).isEqualTo(400);
+      assertThat(respExp.getContent()).isNull();
+    }
+  }
+
+  private Field createTestMessage() {
+    return Field.newBuilder() // "echo" service
+        .setName("imTheBestField")
+        .setNumber(2)
+        .setCardinality(Cardinality.CARDINALITY_OPTIONAL)
+        .setDefaultValue("blah")
+        .build();
+  }
 }
diff --git a/gax-httpjson/src/test/java/com/google/api/gax/httpjson/testing/MockHttpService.java b/gax-httpjson/src/test/java/com/google/api/gax/httpjson/testing/MockHttpService.java
@@ -87,10 +87,15 @@ public synchronized void addResponse(Object response) {
     responseHandlers.add(new MessageResponseFactory(endpoint, serviceMethodDescriptors, response));
   }
 
+  /** Add an expected null response (empty HTTP response body) with a custom status code. */
+  public synchronized void addNullResponse(int statusCode) {
+    responseHandlers.add(
+        (httpMethod, targetUrl) -> new MockLowLevelHttpResponse().setStatusCode(statusCode));
+  }
+
   /** Add an expected null response (empty HTTP response body). */
   public synchronized void addNullResponse() {
-    responseHandlers.add(
-        (httpMethod, targetUrl) -> new MockLowLevelHttpResponse().setStatusCode(200));
+    addNullResponse(200);
   }
 
   /** Add an Exception to the response queue. */