From d45982610e660b5a83f89c57ee45b5a913bec7cb Mon Sep 17 00:00:00 2001 From: Rickard Blomkvist Date: Wed, 25 Oct 2023 09:52:07 +0200 Subject: [PATCH 1/3] Disregard charset in Content-Type when parsing response, to avoid unnecessary logging if it differs from the default application/json or the Produces annotation on the resource method. --- .../reactivewizard/client/HttpClient.java | 3 +- .../reactivewizard/client/HttpClientTest.java | 60 ++++++++++++++----- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java b/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java index 77b6cf4f..b09dca5c 100644 --- a/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java +++ b/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java @@ -10,6 +10,7 @@ import com.fasterxml.jackson.databind.type.TypeFactory; import com.google.common.collect.Sets; import io.netty.handler.codec.http.HttpResponseStatus; +import io.netty.handler.codec.http.HttpUtil; import io.netty.handler.timeout.ReadTimeoutException; import jakarta.inject.Inject; import org.reactivestreams.Publisher; @@ -281,7 +282,7 @@ protected Flux parseResponseSingle(Method method, RwHttpClientResponse r } private String resolveContentType(Method method, RwHttpClientResponse response) { - String contentType = response.getHttpClientResponse().responseHeaders().get(CONTENT_TYPE); + String contentType = HttpUtil.getMimeType(response.getHttpClientResponse().responseHeaders().get(CONTENT_TYPE)).toString(); // Override response content-type if resource method is annotated with a non-empty @Produces JaxRsMeta jaxRsMeta = new JaxRsMeta(method); diff --git a/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java b/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java index 8467e4e0..730207b5 100644 --- a/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java +++ b/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java @@ -207,7 +207,7 @@ private TestResource getHttpProxyWithClientReturningEmpty(DisposableServer dispo } protected void withServer(Consumer serverConsumer) { - server = startServer(HttpResponseStatus.OK, "\"OK\""); + server = startServer(OK, "\"OK\""); serverConsumer.accept(server); } @@ -594,7 +594,7 @@ void shouldSupportBeanParamRecord() throws Exception { @Test void shouldSupportSingleSource() { - server = startServer(HttpResponseStatus.OK, "\"OK\""); + server = startServer(OK, "\"OK\""); TestResource resource = getHttpProxy(server.port()); resource.getSingle().toBlocking().value(); @@ -602,14 +602,14 @@ void shouldSupportSingleSource() { @Test void shouldHandleLargeResponses() { - server = startServer(HttpResponseStatus.OK, generateLargeString(10)); + server = startServer(OK, generateLargeString(10)); TestResource resource = getHttpProxy(server.port()); resource.getHello().toBlocking().single(); } @Test void shouldThrowIllegalArgumentExceptionWhenPathParamIsNull() { - server = startServer(HttpResponseStatus.OK, generateLargeString(10)); + server = startServer(OK, generateLargeString(10)); TestResource resource = getHttpProxy(server.port()); assertThatExceptionOfType(IllegalArgumentException.class) .isThrownBy(() -> resource.getPathParam(null).block()) @@ -618,7 +618,7 @@ void shouldThrowIllegalArgumentExceptionWhenPathParamIsNull() { @Test void shouldLogErrorOnTooLargeResponse() { - server = startServer(HttpResponseStatus.OK, generateLargeString(11)); + server = startServer(OK, generateLargeString(11)); TestResource resource = getHttpProxy(server.port()); assertThatExceptionOfType(WebException.class) .isThrownBy(() -> resource.getHello().toBlocking().single()) @@ -704,7 +704,7 @@ void shouldRedactSensitiveHeaderInLogsAndExceptionMessage() throws Exception { @Test void shouldReturnBadRequestOnTooLargeResponses() throws URISyntaxException { - server = startServer(HttpResponseStatus.OK, "\"derp\""); + server = startServer(OK, "\"derp\""); HttpClientConfig config = new HttpClientConfig("127.0.0.1:" + server.port()); config.setMaxResponseSize(5); TestResource resource = getHttpProxy(config); @@ -718,7 +718,7 @@ void shouldReturnBadRequestOnTooLargeResponses() throws URISyntaxException { @Test void shouldSupportByteArrayResponse() { - server = startServer(HttpResponseStatus.OK, "hej"); + server = startServer(OK, "hej"); TestResource resource = getHttpProxy(server.port()); byte[] result = resource.getAsBytes().toBlocking().single(); assertThat(new String(result)).isEqualTo("hej"); @@ -852,7 +852,7 @@ void shouldThrowNettyReadTimeoutIfRequestTakesLongerThanClientIsConfigured() { @Test void shouldHandleMultipleChunks() { server = HttpServer.create().port(0).handle((request, response) -> { - response.status(HttpResponseStatus.OK); + response.status(OK); return response.sendString(just("\"he") .concatWith(defer(() -> just("llo\""))) .concatWith(defer(Flux::empty))); @@ -923,18 +923,18 @@ private DisposableServer createTestServer(Consumer serverLog) { return HttpServer.create().host("localhost").port(0).handle((request, response) -> { serverLog.accept(request.fullPath()); if (request.fullPath().equals("/hello/servertest/fast")) { - response.status(HttpResponseStatus.OK); + response.status(OK); return response.sendString(just("\"fast\"")); } if (request.fullPath().equals("/hello/servertest/slowHeaders")) { return Flux.defer(() -> { - response.status(HttpResponseStatus.OK); + response.status(OK); return response.sendString(just("\"slowHeaders\"")); }) .delaySubscription(Duration.ofMillis(5000)); } if (request.fullPath().equals("/hello/servertest/slowBody")) { - response.status(HttpResponseStatus.OK); + response.status(OK); return response.sendString( just("\"slowBody: ") .concatWith(just("1", "2", "3").delaySequence(Duration.ofMillis(10000))) @@ -990,7 +990,7 @@ void shouldShouldNotGivePoolExhaustedIfServerDoesNotCloseConnection() throws URI server = HttpServer.create().port(0) .handle((request, response) -> { return Flux.defer(() -> { - response.status(HttpResponseStatus.OK); + response.status(OK); return response.sendString(Mono.just("\"hello\"")); }) .delaySubscription(Duration.ofMillis(50000)) @@ -1235,7 +1235,7 @@ void shouldAllowBodyInPostPutDeletePatchCalls() { void shouldBeAbleToSendAndReceiveRecordBody() { server = HttpServer.create().port(0).handle((request, response) -> request.receive().aggregate().flatMap(buf -> - response.status(HttpResponseStatus.OK).send(Mono.just(buf)).then() + response.status(OK).send(Mono.just(buf)).then() )).bindNow(); var resource = getHttpProxy(server.port()); @@ -1275,7 +1275,7 @@ void shouldSetTimeoutOnResource() throws URISyntaxException { @Test void shouldExecutePreRequestHooks() throws URISyntaxException { server = HttpServer.create().port(0).handle((request, response) -> { - response.status(HttpResponseStatus.OK); + response.status(OK); return response.sendString(Mono.just("\"hi\"")); }).bindNow(); @@ -1523,6 +1523,35 @@ void shouldFailIfBodyIsNotStringOrBytes() { } } + @Test + void shouldDisregardCharsetInContentTypeWhenComparingToResourceAnnotation() { + String someRecordJson = """ + [{ + "param1": "a", + "param2": "b" + }, + { + "param1": "c", + "param2": "d" + } + ]"""; + server = HttpServer.create().port(0).handle((request, response) -> + response.status(OK) + .header("Content-Type", "application/json; charset=utf-8") + .sendString(Mono.just(someRecordJson)) + .then()).bindNow(); + TestResource resource = getHttpProxy(server.port()); + + StepVerifier.create(resource.producesJson()) + .expectNext(new TestResource.SomeRecord("a", "b")) + .expectNext(new TestResource.SomeRecord("c", "d")) + .verifyComplete(); + loggingVerifier.assertThatLogs() + .noneSatisfy(logEvent -> + assertThat(logEvent.getMessage().getFormattedMessage()) + .contains("does not match the Content-Type")); + } + @Path("/hello") public interface TestResource { @@ -1656,6 +1685,9 @@ Observable postForm(@FormParam("paramA") String a, @Consumes("application/xml") Observable sendXml(Pojo pojo); + @GET + Flux producesJson(); + } static class Wrapper { From 7dd2e811fccc2883215721ae796e2a6dac6640c5 Mon Sep 17 00:00:00 2001 From: Rickard Blomkvist Date: Wed, 25 Oct 2023 10:04:10 +0200 Subject: [PATCH 2/3] Minor refactoring/cleanup. --- .../reactivewizard/client/HttpClient.java | 21 ++++--- .../reactivewizard/client/HttpClientTest.java | 56 +++++++++---------- 2 files changed, 36 insertions(+), 41 deletions(-) diff --git a/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java b/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java index b09dca5c..2e8ffa3b 100644 --- a/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java +++ b/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java @@ -234,7 +234,7 @@ private static Mono> flattenResponse(Mono>> res */ public static Mono>> getFullResponse(Flux source) { Optional>>> responseFlux = ReactiveDecorator.getDecoration(source); - if (!responseFlux.isPresent()) { + if (responseFlux.isEmpty()) { throw new IllegalArgumentException("Must be used with Flux returned from api call"); } return responseFlux.get(); @@ -249,7 +249,7 @@ public static Mono>> getFullResponse(Flux source) { */ public static Mono> getFullResponse(Mono source) { Optional>>> responseFlux = ReactiveDecorator.getDecoration(source); - if (!responseFlux.isPresent()) { + if (responseFlux.isEmpty()) { throw new IllegalArgumentException("Must be used with Mono returned from api call"); } return flattenResponse(responseFlux.get()); @@ -434,13 +434,13 @@ protected void addContent(Method method, Object[] arguments, RequestBuilder requ } } } - if (output.length() > 0) { + if (!output.isEmpty()) { requestBuilder.setContent(output.toString()); } } protected void addFormParamToOutput(StringBuilder output, Object value, FormParam formParam) { - if (output.length() != 0) { + if (!output.isEmpty()) { output.append("&"); } output.append(formParam.value()).append("=").append(urlEncode(value.toString())); @@ -496,7 +496,7 @@ private String formatHeaders(reactor.netty.http.client.HttpClientResponse client private HttpClient.DetailedError getDetailedError(String data, Throwable cause) { HttpClient.DetailedError detailedError = new HttpClient.DetailedError(cause); - if (data != null && data.length() > 0) { + if (data != null && !data.isEmpty()) { try { objectMapper.readerForUpdating(detailedError).readValue(data); } catch (IOException e) { @@ -667,13 +667,12 @@ protected String getPath(Method method, Object[] arguments, JaxRsMeta meta) { private List getBeanParamGetters(Class beanParamType) { List result = new ArrayList<>(); for (Field field : getDeclaredFieldsFromClassAndAncestors(beanParamType)) { - Optional> getter = ReflectionUtil.getter(beanParamType, field.getName()); - if (getter.isPresent()) { + Optional> optionalGetter = ReflectionUtil.getter(beanParamType, field.getName()); + optionalGetter.ifPresent(getter -> result.add(new BeanParamProperty( - getter.get(), + getter, field.getAnnotations() - )); - } + ))); } return result; } @@ -682,7 +681,7 @@ private List getBeanParamGetters(Class beanParamType) { * Recursive function getting all declared fields from the passed in class and its ancestors. * * @param clazz the clazz fetching fields from - * @return list of fields + * @return set of fields */ private static Set getDeclaredFieldsFromClassAndAncestors(Class clazz) { final HashSet declaredFields = new HashSet<>(asList(clazz.getDeclaredFields())); diff --git a/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java b/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java index 730207b5..a19d9dee 100644 --- a/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java +++ b/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java @@ -329,7 +329,7 @@ void shouldNotRetryIfEmptyReturnedOnPost() { } @Test - void shouldNotReportUnhealthyWhenPoolIsExhaustedRatherSequentiatingTheRequests() throws URISyntaxException { + void shouldNotReportUnhealthyWhenPoolIsExhaustedRatherSequentiatingTheRequests() { AtomicBoolean wasUnhealthy = new AtomicBoolean(false); @@ -350,7 +350,7 @@ public boolean logStatus(Object key, boolean currentStatus) { config.setRetryCount(0); TestResource resource = getHttpProxy(config); - List> results = new ArrayList>(); + List> results = new ArrayList<>(); for (int i = 0; i < 10; i++) { results.add(resource.getHello()); } @@ -419,7 +419,7 @@ void shouldReportHealthyWhenPoolIsNotExhausted() { withServer(server -> { TestResource resource = getHttpProxy(server.port(), 5); - List> results = new ArrayList>(); + List> results = new ArrayList<>(); for (int i = 0; i < 5; i++) { results.add(resource.getHello()); } @@ -480,14 +480,13 @@ void shouldReturnErrorJsonBody() { TestResource resource = getHttpProxy(server.port()); StepVerifier.create(resource.getHelloMono()) - .verifyErrorSatisfies(error -> { + .verifyErrorSatisfies(error -> assertThat(error) .isInstanceOf(WebException.class) .hasFieldOrPropertyWithValue("body", errorJson) .cause() .isInstanceOf(HttpClient.DetailedError.class) - .hasMessage("My message"); - }); + .hasMessage("My message")); } @Test @@ -497,14 +496,13 @@ void shouldReturnErrorNonJsonBody() { TestResource resource = getHttpProxy(server.port()); StepVerifier.create(resource.getHelloMono()) - .verifyErrorSatisfies(error -> { + .verifyErrorSatisfies(error -> assertThat(error) .isInstanceOf(WebException.class) .hasFieldOrPropertyWithValue("body", errorNonJson) .cause() .isInstanceOf(HttpClient.DetailedError.class) - .hasMessage(errorNonJson); - }); + .hasMessage(errorNonJson)); } @Test @@ -988,16 +986,14 @@ void shouldShouldNotGivePoolExhaustedIfServerDoesNotCloseConnection() throws URI try { server = HttpServer.create().port(0) - .handle((request, response) -> { - return Flux.defer(() -> { + .handle((request, response) -> + Flux.defer(() -> { response.status(OK); return response.sendString(Mono.just("\"hello\"")); }) .delaySubscription(Duration.ofMillis(50000)) - .doOnError(e -> { - e.printStackTrace(); - }); - }).bindNow(); + .doOnError(Throwable::printStackTrace) + ).bindNow(); // this config ensures that the autocleanup will run before the hystrix timeout HttpClientConfig config = new HttpClientConfig("localhost:" + server.port()); @@ -1022,7 +1018,7 @@ void shouldShouldNotGivePoolExhaustedIfServerDoesNotCloseConnection() throws URI @Test void willRequestWithMultipleCookies() { Consumer reqLog = mock(Consumer.class); - server = startServer(OK, "", reqLog::accept); + server = startServer(OK, "", reqLog); String cookie1Value = "stub1"; String cookie2Value = "stub2"; @@ -1291,9 +1287,9 @@ void shouldExecutePreRequestHooks() throws URISyntaxException { TestResource resource = client.create(TestResource.class); resource.getHello().toBlocking().single(); - verify(preRequestHook, times(1)).apply((TestUtil.matches(requestBuilder -> { - assertThat(requestBuilder.getFullUrl()).isEqualToIgnoringCase(url + "/hello"); - }))); + verify(preRequestHook, times(1)).apply((TestUtil.matches(requestBuilder -> + assertThat(requestBuilder.getFullUrl()).isEqualToIgnoringCase(url + "/hello") + ))); } @Test @@ -1427,7 +1423,7 @@ private Injector injectorWithProgrammaticHttpClientConfig(HttpClientConfig httpC @Test void assertRequestContainsHost() { Consumer reqLog = mock(Consumer.class); - server = startServer(OK, "", reqLog::accept); + server = startServer(OK, "", reqLog); String host = "localhost"; @@ -1442,7 +1438,7 @@ void assertRequestContainsHost() { @Test void assertRequestContainsHostFromHeaderParam() { Consumer reqLog = mock(Consumer.class); - server = startServer(OK, "", reqLog::accept); + server = startServer(OK, "", reqLog); String host = "globalhost"; @@ -1526,15 +1522,15 @@ void shouldFailIfBodyIsNotStringOrBytes() { @Test void shouldDisregardCharsetInContentTypeWhenComparingToResourceAnnotation() { String someRecordJson = """ - [{ - "param1": "a", - "param2": "b" - }, - { - "param1": "c", - "param2": "d" - } - ]"""; + [{ + "param1": "a", + "param2": "b" + }, + { + "param1": "c", + "param2": "d" + } + ]"""; server = HttpServer.create().port(0).handle((request, response) -> response.status(OK) .header("Content-Type", "application/json; charset=utf-8") From b3ec501d397ef17927193508a5838a4c3f73fa33 Mon Sep 17 00:00:00 2001 From: Rickard Blomkvist Date: Wed, 25 Oct 2023 15:42:45 +0200 Subject: [PATCH 3/3] Handle case where there is no Content-Type header. --- .../fortnox/reactivewizard/client/HttpClient.java | 5 ++++- .../reactivewizard/client/HttpClientTest.java | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java b/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java index 2e8ffa3b..e3c23fc7 100644 --- a/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java +++ b/client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java @@ -282,7 +282,10 @@ protected Flux parseResponseSingle(Method method, RwHttpClientResponse r } private String resolveContentType(Method method, RwHttpClientResponse response) { - String contentType = HttpUtil.getMimeType(response.getHttpClientResponse().responseHeaders().get(CONTENT_TYPE)).toString(); + String contentType = Optional.ofNullable(response.getHttpClientResponse().responseHeaders().get(CONTENT_TYPE)) + .map(HttpUtil::getMimeType) + .map(CharSequence::toString) + .orElse(null); // Override response content-type if resource method is annotated with a non-empty @Produces JaxRsMeta jaxRsMeta = new JaxRsMeta(method); diff --git a/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java b/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java index a19d9dee..a36b24cb 100644 --- a/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java +++ b/client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java @@ -1548,6 +1548,19 @@ void shouldDisregardCharsetInContentTypeWhenComparingToResourceAnnotation() { .contains("does not match the Content-Type")); } + @Test + void shouldHandleMissingContentTypeInResponse() { + server = HttpServer.create().port(0).handle((request, response) -> + response.status(OK) + .sendString(Mono.just("[]")) + .then()).bindNow(); + TestResource resource = getHttpProxy(server.port()); + + StepVerifier.create(resource.producesJson()) + .expectNextCount(1) + .verifyComplete(); + } + @Path("/hello") public interface TestResource {