From 4a8c99c9ce6773a1b192d4b60969241b2370ac4f Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Wed, 26 Apr 2017 17:33:21 +0200 Subject: [PATCH] Consistent handling of 4xx/5xx status codes in WebClient This commit changes the handling of 4xx/5xx status codes in the WebClient to the following simple rule: if there is no way for the user to get the response status code, then a WebClientException is returned. If there is a way to get to the status code, then we do not return an exception. Issue: SPR-15486 --- .../function/client/ClientResponse.java | 16 +++---- .../client/DefaultClientResponse.java | 22 +--------- .../function/client/DefaultWebClient.java | 29 ++++++++++++- .../reactive/function/client/WebClient.java | 28 ++++++------ .../client/DefaultClientResponseTests.java | 38 +--------------- .../client/WebClientIntegrationTests.java | 43 ++++++++++++++++++- 6 files changed, 93 insertions(+), 83 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java index 4b0170e0eb..d8211f47af 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java @@ -59,9 +59,7 @@ public interface ClientResponse { MultiValueMap cookies(); /** - * Extract the body with the given {@code BodyExtractor}. Unlike {@link #bodyToMono(Class)} and - * {@link #bodyToFlux(Class)}; this method does not check for a 4xx or 5xx status code before - * extracting the body. + * Extract the body with the given {@code BodyExtractor}. * @param extractor the {@code BodyExtractor} that reads from the response * @param the type of the body returned * @return the extracted body @@ -69,22 +67,18 @@ public interface ClientResponse { T body(BodyExtractor extractor); /** - * Extract the body to a {@code Mono}. If the response has status code 4xx or 5xx, the - * {@code Mono} will contain a {@link WebClientException}. + * Extract the body to a {@code Mono}. * @param elementClass the class of element in the {@code Mono} * @param the element type - * @return a mono containing the body, or a {@link WebClientException} if the status code is - * 4xx or 5xx + * @return a mono containing the body of the given type {@code T} */ Mono bodyToMono(Class elementClass); /** - * Extract the body to a {@code Flux}. If the response has status code 4xx or 5xx, the - * {@code Flux} will contain a {@link WebClientException}. + * Extract the body to a {@code Flux}. * @param elementClass the class of element in the {@code Flux} * @param the element type - * @return a flux containing the body, or a {@link WebClientException} if the status code is - * 4xx or 5xx + * @return a flux containing the body of the given type {@code T} */ Flux bodyToFlux(Class elementClass); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java index 8387629a50..dc2b8a0a7d 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java @@ -21,11 +21,9 @@ import java.util.List; import java.util.Map; import java.util.Optional; import java.util.OptionalLong; -import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Stream; -import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -99,28 +97,12 @@ class DefaultClientResponse implements ClientResponse { @Override public Mono bodyToMono(Class elementClass) { - return bodyToPublisher(BodyExtractors.toMono(elementClass), Mono::error); + return body(BodyExtractors.toMono(elementClass)); } @Override public Flux bodyToFlux(Class elementClass) { - return bodyToPublisher(BodyExtractors.toFlux(elementClass), Flux::error); - } - - private > T bodyToPublisher( - BodyExtractor extractor, - Function errorFunction) { - - HttpStatus status = statusCode(); - if (status.is4xxClientError() || status.is5xxServerError()) { - WebClientException ex = new WebClientException( - "ClientResponse has erroneous status code: " + status.value() + - " " + status.getReasonPhrase()); - return errorFunction.apply(ex); - } - else { - return body(extractor); - } + return body(BodyExtractors.toFlux(elementClass)); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java index 084d9fb059..3ffd01a33b 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java @@ -32,13 +32,17 @@ import reactor.core.publisher.Mono; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; +import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.http.client.reactive.ClientHttpRequest; +import org.springframework.http.client.reactive.ClientHttpResponse; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; +import org.springframework.web.reactive.function.BodyExtractor; +import org.springframework.web.reactive.function.BodyExtractors; import org.springframework.web.reactive.function.BodyInserter; import org.springframework.web.reactive.function.BodyInserters; import org.springframework.web.util.DefaultUriBuilderFactory; @@ -350,14 +354,35 @@ class DefaultWebClient implements WebClient { @Override public Mono bodyToMono(Class bodyType) { - return this.responseMono.flatMap(clientResponse -> clientResponse.bodyToMono(bodyType)); + return this.responseMono.flatMap( + response -> bodyToPublisher(response, BodyExtractors.toMono(bodyType), + Mono::error)); } @Override public Flux bodyToFlux(Class elementType) { - return this.responseMono.flatMapMany(clientResponse -> clientResponse.bodyToFlux(elementType)); + return this.responseMono.flatMapMany( + response -> bodyToPublisher(response, BodyExtractors.toFlux(elementType), + Flux::error)); } + private > T bodyToPublisher(ClientResponse response, + BodyExtractor extractor, + Function errorFunction) { + + HttpStatus status = response.statusCode(); + if (status.is4xxClientError() || status.is5xxServerError()) { + WebClientException ex = new WebClientException( + "ClientResponse has erroneous status code: " + status.value() + + " " + status.getReasonPhrase()); + return errorFunction.apply(ex); + } + else { + return response.body(extractor); + } + } + + @Override public Mono> toEntity(Class bodyType) { return this.responseMono.flatMap(response -> diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java index 8e6d8d8d35..44ed674564 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java @@ -477,42 +477,46 @@ public interface WebClient { interface ResponseSpec { /** - * Extract the response body to an Object of type {@code } by - * invoking {@link ClientResponse#bodyToMono(Class)}. + * Extract the body to a {@code Mono}. If the response has status code 4xx or 5xx, the + * {@code Mono} will contain a {@link WebClientException}. * * @param bodyType the expected response body type * @param response body type - * @return {@code Mono} with the result + * @return a mono containing the body, or a {@link WebClientException} if the status code is + * 4xx or 5xx */ Mono bodyToMono(Class bodyType); /** - * Extract the response body to a stream of Objects of type {@code } - * by invoking {@link ClientResponse#bodyToFlux(Class)}. + * Extract the body to a {@code Flux}. If the response has status code 4xx or 5xx, the + * {@code Flux} will contain a {@link WebClientException}. * * @param elementType the type of element in the response * @param the type of elements in the response - * @return the body of the response + * @return a flux containing the body, or a {@link WebClientException} if the status code is + * 4xx or 5xx */ Flux bodyToFlux(Class elementType); /** - * A variant of {@link #bodyToMono(Class)} that also provides access to - * the response status and headers. + * Returns the response as a delayed {@code ResponseEntity}. Unlike + * {@link #bodyToMono(Class)} and {@link #bodyToFlux(Class)}, this method does not check + * for a 4xx or 5xx status code before extracting the body. * * @param bodyType the expected response body type * @param response body type - * @return {@code Mono} with the result + * @return {@code Mono} with the {@code ResponseEntity} */ Mono> toEntity(Class bodyType); /** - * A variant of {@link #bodyToFlux(Class)} collected via - * {@link Flux#collectList()} and wrapped in {@code ResponseEntity}. + * Returns the response as a delayed list of {@code ResponseEntity}s. Unlike + * {@link #bodyToMono(Class)} and {@link #bodyToFlux(Class)}, this method does not check + * for a 4xx or 5xx status code before extracting the body. * * @param elementType the expected response body list element type * @param the type of elements in the list - * @return {@code Mono} with the result + * @return {@code Mono} with the list of {@code ResponseEntity}s */ Mono>> toEntityList(Class elementType); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java index 813d6566a1..a3c5253268 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java @@ -29,7 +29,6 @@ import org.junit.Before; import org.junit.Test; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; -import reactor.test.StepVerifier; import org.springframework.core.codec.StringDecoder; import org.springframework.core.io.buffer.DataBuffer; @@ -48,7 +47,7 @@ import org.springframework.util.MultiValueMap; import static org.junit.Assert.*; import static org.mockito.Mockito.*; -import static org.springframework.web.reactive.function.BodyExtractors.*; +import static org.springframework.web.reactive.function.BodyExtractors.toMono; /** * @author Arjen Poutsma @@ -151,24 +150,6 @@ public class DefaultClientResponseTests { assertEquals("foo", resultMono.block()); } - @Test - public void bodyToMonoError() throws Exception { - HttpHeaders httpHeaders = new HttpHeaders(); - httpHeaders.setContentType(MediaType.TEXT_PLAIN); - when(mockResponse.getHeaders()).thenReturn(httpHeaders); - when(mockResponse.getStatusCode()).thenReturn(HttpStatus.NOT_FOUND); - - Set> messageReaders = Collections - .singleton(new DecoderHttpMessageReader<>(StringDecoder.allMimeTypes(true))); - when(mockExchangeStrategies.messageReaders()).thenReturn(messageReaders::stream); - - Mono resultMono = defaultClientResponse.bodyToMono(String.class); - - StepVerifier.create(resultMono) - .expectError(WebClientException.class) - .verify(); - } - @Test public void bodyToFlux() throws Exception { DefaultDataBufferFactory factory = new DefaultDataBufferFactory(); @@ -191,21 +172,4 @@ public class DefaultClientResponseTests { assertEquals(Collections.singletonList("foo"), result.block()); } - @Test - public void bodyToFluxError() throws Exception { - HttpHeaders httpHeaders = new HttpHeaders(); - httpHeaders.setContentType(MediaType.TEXT_PLAIN); - when(mockResponse.getHeaders()).thenReturn(httpHeaders); - when(mockResponse.getStatusCode()).thenReturn(HttpStatus.INTERNAL_SERVER_ERROR); - - Set> messageReaders = Collections - .singleton(new DecoderHttpMessageReader<>(StringDecoder.allMimeTypes(true))); - when(mockExchangeStrategies.messageReaders()).thenReturn(messageReaders::stream); - - Flux resultFlux = defaultClientResponse.bodyToFlux(String.class); - StepVerifier.create(resultFlux) - .expectError(WebClientException.class) - .verify(); - } - } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java index 63946eb45f..cf66db3652 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java @@ -334,7 +334,7 @@ public class WebClientIntegrationTests { } @Test - public void notFound() throws Exception { + public void exchangeNotFound() throws Exception { this.server.enqueue(new MockResponse().setResponseCode(404) .setHeader("Content-Type", "text/plain").setBody("Not Found")); @@ -351,6 +351,47 @@ public class WebClientIntegrationTests { Assert.assertEquals("/greeting?name=Spring", recordedRequest.getPath()); } + @Test + public void retrieveBodyToMonoNotFound() throws Exception { + this.server.enqueue(new MockResponse().setResponseCode(404) + .setHeader("Content-Type", "text/plain").setBody("Not Found")); + + Mono result = this.webClient.get() + .uri("/greeting?name=Spring") + .retrieve() + .bodyToMono(String.class); + + StepVerifier.create(result) + .expectError(WebClientException.class) + .verify(Duration.ofSeconds(3)); + + RecordedRequest recordedRequest = server.takeRequest(); + Assert.assertEquals(1, server.getRequestCount()); + Assert.assertEquals("*/*", recordedRequest.getHeader(HttpHeaders.ACCEPT)); + Assert.assertEquals("/greeting?name=Spring", recordedRequest.getPath()); + } + + @Test + public void retrieveToEntityNotFound() throws Exception { + this.server.enqueue(new MockResponse().setResponseCode(404) + .setHeader("Content-Type", "text/plain").setBody("Not Found")); + + Mono> result = this.webClient.get() + .uri("/greeting?name=Spring") + .retrieve() + .toEntity(String.class); + + StepVerifier.create(result) + .consumeNextWith(response -> assertEquals(HttpStatus.NOT_FOUND, response.getStatusCode())) + .expectComplete() + .verify(Duration.ofSeconds(3)); + + RecordedRequest recordedRequest = server.takeRequest(); + Assert.assertEquals(1, server.getRequestCount()); + Assert.assertEquals("*/*", recordedRequest.getHeader(HttpHeaders.ACCEPT)); + Assert.assertEquals("/greeting?name=Spring", recordedRequest.getPath()); + } + @Test public void buildFilter() throws Exception { this.server.enqueue(new MockResponse().setHeader("Content-Type", "text/plain").setBody("Hello Spring!"));