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

Author: poutsma

spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java

@@ -59,32 +59,26 @@ public interface ClientResponse {
 	MultiValueMap<String, ResponseCookie> 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 <T> the type of the body returned
 	 * @return the extracted body
 	 */
 	<T> T body(BodyExtractor<T, ? super ClientHttpResponse> 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 <T> 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}
 	 */
 	<T> Mono<T> bodyToMono(Class<? extends T> 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 <T> 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}
 	 */
 	<T> Flux<T> bodyToFlux(Class<? extends T> elementClass);
 

spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java

@@ -21,11 +21,9 @@
 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 @@ public Map<String, Object> hints() {
 
 	@Override
 	public <T> Mono<T> bodyToMono(Class<? extends T> elementClass) {
-		return bodyToPublisher(BodyExtractors.toMono(elementClass), Mono::error);
+		return body(BodyExtractors.toMono(elementClass));
 	}
 
 	@Override
 	public <T> Flux<T> bodyToFlux(Class<? extends T> elementClass) {
-		return bodyToPublisher(BodyExtractors.toFlux(elementClass), Flux::error);
-	}
-
-	private <T extends Publisher<?>> T bodyToPublisher(
-			BodyExtractor<T, ? super ClientHttpResponse> extractor,
-			Function<WebClientException, T> 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));
 	}
 
 

spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java

@@ -32,13 +32,17 @@
 
 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 @@ private static class DefaultResponseSpec implements ResponseSpec {
 
 		@Override
 		public <T> Mono<T> bodyToMono(Class<T> bodyType) {
-			return this.responseMono.flatMap(clientResponse -> clientResponse.bodyToMono(bodyType));
+			return this.responseMono.flatMap(
+					response -> bodyToPublisher(response, BodyExtractors.toMono(bodyType),
+							Mono::error));
 		}
 
 		@Override
 		public <T> Flux<T> bodyToFlux(Class<T> elementType) {
-			return this.responseMono.flatMapMany(clientResponse -> clientResponse.bodyToFlux(elementType));
+			return this.responseMono.flatMapMany(
+					response -> bodyToPublisher(response, BodyExtractors.toFlux(elementType),
+							Flux::error));
 		}
 
+		private <T extends Publisher<?>> T bodyToPublisher(ClientResponse response,
+				BodyExtractor<T, ? super ClientHttpResponse> extractor,
+				Function<WebClientException, T> 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 <T> Mono<ResponseEntity<T>> toEntity(Class<T> bodyType) {
 			return this.responseMono.flatMap(response ->

spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java

@@ -477,42 +477,46 @@ interface RequestBodySpec extends RequestHeadersSpec<RequestBodySpec> {
 	interface ResponseSpec {
 
 		/**
-		 * Extract the response body to an Object of type {@code <T>} 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 <T> 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
 		 */
 		<T> Mono<T> bodyToMono(Class<T> bodyType);
 
 		/**
-		 * Extract the response body to a stream of Objects of type {@code <T>}
-		 * 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 <T> 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
 		 */
 		<T> Flux<T> bodyToFlux(Class<T> 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 <T> response body type
-		 * @return {@code Mono} with the result
+		 * @return {@code Mono} with the {@code ResponseEntity}
 		 */
 		<T> Mono<ResponseEntity<T>> toEntity(Class<T> 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 <T> the type of elements in the list
-		 * @return {@code Mono} with the result
+		 * @return {@code Mono} with the list of {@code ResponseEntity}s
 		 */
 		<T> Mono<ResponseEntity<List<T>>> toEntityList(Class<T> elementType);
 

spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java

@@ -29,7 +29,6 @@
 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 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 void bodyToMono() throws Exception {
 		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<HttpMessageReader<?>> messageReaders = Collections
-				.singleton(new DecoderHttpMessageReader<>(StringDecoder.allMimeTypes(true)));
-		when(mockExchangeStrategies.messageReaders()).thenReturn(messageReaders::stream);
-
-		Mono<String> 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 void bodyToFlux() throws Exception {
 		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<HttpMessageReader<?>> messageReaders = Collections
-				.singleton(new DecoderHttpMessageReader<>(StringDecoder.allMimeTypes(true)));
-		when(mockExchangeStrategies.messageReaders()).thenReturn(messageReaders::stream);
-
-		Flux<String> resultFlux = defaultClientResponse.bodyToFlux(String.class);
-		StepVerifier.create(resultFlux)
-				.expectError(WebClientException.class)
-				.verify();
-	}
-
 }

spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientIntegrationTests.java

@@ -334,7 +334,7 @@ public void cookies() throws Exception {
 	}
 
 	@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 void notFound() throws Exception {
 		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<String> 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<ResponseEntity<String>> 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!"));