Skip to content
This repository has been archived by the owner on Nov 26, 2024. It is now read-only.

Disregard charset in Content-Type when parsing response #631

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Disregard charset in Content-Type when parsing response #631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d459826
Disregard charset in Content-Type when parsing response, to avoid unn…
flowertwig Oct 25, 2023
7dd2e81
Minor refactoring/cleanup.
flowertwig Oct 25, 2023
b3ec501
Handle case where there is no Content-Type header.
flowertwig Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 15 additions & 12 deletions client/src/main/java/se/fortnox/reactivewizard/client/HttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -233,7 +234,7 @@ private static <T> Mono<Response<T>> flattenResponse(Mono<Response<Flux<T>>> res
*/
public static <T> Mono<Response<Flux<T>>> getFullResponse(Flux<T> source) {
Optional<Mono<Response<Flux<T>>>> 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();
Expand All @@ -248,7 +249,7 @@ public static <T> Mono<Response<Flux<T>>> getFullResponse(Flux<T> source) {
*/
public static <T> Mono<Response<T>> getFullResponse(Mono<T> source) {
Optional<Mono<Response<Flux<T>>>> 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());
Expand Down Expand Up @@ -281,7 +282,10 @@ protected Flux<Object> parseResponseSingle(Method method, RwHttpClientResponse r
}

private String resolveContentType(Method method, RwHttpClientResponse response) {
String contentType = response.getHttpClientResponse().responseHeaders().get(CONTENT_TYPE);
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);
Expand Down Expand Up @@ -433,13 +437,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()));
Expand Down Expand Up @@ -495,7 +499,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) {
Expand Down Expand Up @@ -666,13 +670,12 @@ protected String getPath(Method method, Object[] arguments, JaxRsMeta meta) {
private List<BeanParamProperty> getBeanParamGetters(Class beanParamType) {
List<BeanParamProperty> result = new ArrayList<>();
for (Field field : getDeclaredFieldsFromClassAndAncestors(beanParamType)) {
Optional<Function<Object, Object>> getter = ReflectionUtil.getter(beanParamType, field.getName());
if (getter.isPresent()) {
Optional<Function<Object, Object>> optionalGetter = ReflectionUtil.getter(beanParamType, field.getName());
optionalGetter.ifPresent(getter ->
result.add(new BeanParamProperty(
getter.get(),
getter,
field.getAnnotations()
));
}
)));
}
return result;
}
Expand All @@ -681,7 +684,7 @@ private List<BeanParamProperty> 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<Field> getDeclaredFieldsFromClassAndAncestors(Class clazz) {
final HashSet<Field> declaredFields = new HashSet<>(asList(clazz.getDeclaredFields()));
Expand Down
111 changes: 76 additions & 35 deletions client/src/test/java/se/fortnox/reactivewizard/client/HttpClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ private TestResource getHttpProxyWithClientReturningEmpty(DisposableServer dispo
}

protected void withServer(Consumer<DisposableServer> serverConsumer) {
server = startServer(HttpResponseStatus.OK, "\"OK\"");
server = startServer(OK, "\"OK\"");
serverConsumer.accept(server);
}

Expand Down Expand Up @@ -329,7 +329,7 @@ void shouldNotRetryIfEmptyReturnedOnPost() {
}

@Test
void shouldNotReportUnhealthyWhenPoolIsExhaustedRatherSequentiatingTheRequests() throws URISyntaxException {
void shouldNotReportUnhealthyWhenPoolIsExhaustedRatherSequentiatingTheRequests() {

AtomicBoolean wasUnhealthy = new AtomicBoolean(false);

Expand All @@ -350,7 +350,7 @@ public boolean logStatus(Object key, boolean currentStatus) {
config.setRetryCount(0);
TestResource resource = getHttpProxy(config);

List<Observable<String>> results = new ArrayList<Observable<String>>();
List<Observable<String>> results = new ArrayList<>();
for (int i = 0; i < 10; i++) {
results.add(resource.getHello());
}
Expand Down Expand Up @@ -419,7 +419,7 @@ void shouldReportHealthyWhenPoolIsNotExhausted() {
withServer(server -> {
TestResource resource = getHttpProxy(server.port(), 5);

List<Observable<String>> results = new ArrayList<Observable<String>>();
List<Observable<String>> results = new ArrayList<>();
for (int i = 0; i < 5; i++) {
results.add(resource.getHello());
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -594,22 +592,22 @@ 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();
}

@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())
Expand All @@ -618,7 +616,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())
Expand Down Expand Up @@ -704,7 +702,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);
Expand All @@ -718,7 +716,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");
Expand Down Expand Up @@ -852,7 +850,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)));
Expand Down Expand Up @@ -923,18 +921,18 @@ private DisposableServer createTestServer(Consumer<String> 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)))
Expand Down Expand Up @@ -988,16 +986,14 @@ void shouldShouldNotGivePoolExhaustedIfServerDoesNotCloseConnection() throws URI

try {
server = HttpServer.create().port(0)
.handle((request, response) -> {
return Flux.defer(() -> {
response.status(HttpResponseStatus.OK);
.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());
Expand All @@ -1022,7 +1018,7 @@ void shouldShouldNotGivePoolExhaustedIfServerDoesNotCloseConnection() throws URI
@Test
void willRequestWithMultipleCookies() {
Consumer<HttpServerRequest> reqLog = mock(Consumer.class);
server = startServer(OK, "", reqLog::accept);
server = startServer(OK, "", reqLog);

String cookie1Value = "stub1";
String cookie2Value = "stub2";
Expand Down Expand Up @@ -1235,7 +1231,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());
Expand Down Expand Up @@ -1275,7 +1271,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();

Expand All @@ -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
Expand Down Expand Up @@ -1427,7 +1423,7 @@ private Injector injectorWithProgrammaticHttpClientConfig(HttpClientConfig httpC
@Test
void assertRequestContainsHost() {
Consumer<HttpServerRequest> reqLog = mock(Consumer.class);
server = startServer(OK, "", reqLog::accept);
server = startServer(OK, "", reqLog);

String host = "localhost";

Expand All @@ -1442,7 +1438,7 @@ void assertRequestContainsHost() {
@Test
void assertRequestContainsHostFromHeaderParam() {
Consumer<HttpServerRequest> reqLog = mock(Consumer.class);
server = startServer(OK, "", reqLog::accept);
server = startServer(OK, "", reqLog);

String host = "globalhost";

Expand Down Expand Up @@ -1523,6 +1519,48 @@ 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"));
}

@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 {

Expand Down Expand Up @@ -1656,6 +1694,9 @@ Observable<String> postForm(@FormParam("paramA") String a,
@Consumes("application/xml")
Observable<Void> sendXml(Pojo pojo);

@GET
Flux<SomeRecord> producesJson();

}

static class Wrapper {
Expand Down