From 8def947620610b7aab526413a1ac14fd4f890dbf Mon Sep 17 00:00:00 2001 From: Vadym Matsishevskyi <25311427+vam-google@users.noreply.github.com> Date: Fri, 29 Apr 2022 11:41:47 -0700 Subject: [PATCH] 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 --- .../api/gax/httpjson/HttpJsonClientCalls.java | 11 ++- .../api/gax/httpjson/HttpRequestRunnable.java | 4 +- .../httpjson/HttpJsonDirectCallableTest.java | 67 ++++++++++++++----- .../gax/httpjson/testing/MockHttpService.java | 9 ++- 4 files changed, 69 insertions(+), 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 index ad8320eba..9512537cd 100644 --- 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 index 621335f89..2edb4ceb2 100644 --- 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 index d7a88e7e9..310f13e4a 100644 --- 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,22 +157,13 @@ 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(); @@ -187,4 +171,51 @@ public void testErrorUnaryResponse() throws InterruptedException { assertThat(respExp.getContent()).isEqualTo(exception.toString()); } } + + @Test + public void testErrorNullContentSuccessfulResponse() throws InterruptedException { + HttpJsonDirectCallable 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 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 index a682088ed..3a4d81ceb 100644 --- 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. */