From cdddf09c206bf538adcbe131c173450b5a62df39 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 28 Nov 2024 17:03:39 +0000 Subject: [PATCH 1/3] Polishing in DefaultResponseErrorHandler See gh-34231 --- .../client/DefaultResponseErrorHandler.java | 140 +++++++++--------- .../ExtractingResponseErrorHandler.java | 50 ++++--- .../DefaultResponseErrorHandlerTests.java | 24 +-- .../ExtractingResponseErrorHandlerTests.java | 44 +++--- 4 files changed, 127 insertions(+), 131 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java index 485e70b132a..11ecdb5795f 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java @@ -38,7 +38,6 @@ import org.springframework.http.converter.HttpMessageConverter; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; -import org.springframework.util.FileCopyUtils; import org.springframework.util.ObjectUtils; /** @@ -135,8 +134,7 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { */ @Override public void handleError(ClientHttpResponse response) throws IOException { - HttpStatusCode statusCode = response.getStatusCode(); - handleError(response, statusCode, null, null); + handleError(response, response.getStatusCode(), null, null); } /** @@ -159,46 +157,7 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { */ @Override public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException { - HttpStatusCode statusCode = response.getStatusCode(); - handleError(response, statusCode, url, method); - } - - /** - * Return error message with details from the response body. For example: - * 
-	 * 404 Not Found on GET request for "https://example.com": [{'id': 123, 'message': 'my message'}]
-	 *
- */ - private String getErrorMessage(int rawStatusCode, String statusText, @Nullable byte[] responseBody, @Nullable Charset charset, - @Nullable URI url, @Nullable HttpMethod method) { - - StringBuilder msg = new StringBuilder(rawStatusCode + " " + statusText); - if (method != null) { - msg.append(" on ").append(method).append(" request"); - } - if (url != null) { - msg.append(" for \""); - String urlString = url.toString(); - int idx = urlString.indexOf('?'); - if (idx != -1) { - msg.append(urlString, 0, idx); - } - else { - msg.append(urlString); - } - msg.append("\""); - } - msg.append(": "); - if (ObjectUtils.isEmpty(responseBody)) { - msg.append("[no body]"); - } - else { - charset = (charset != null ? charset : StandardCharsets.UTF_8); - String bodyText = new String(responseBody, charset); - bodyText = LogFormatUtils.formatValue(bodyText, -1, true); - msg.append(bodyText); - } - return msg.toString(); + handleError(response, response.getStatusCode(), url, method); } /** @@ -211,7 +170,8 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { * @see HttpClientErrorException#create * @see HttpServerErrorException#create */ - protected void handleError(ClientHttpResponse response, HttpStatusCode statusCode, + protected void handleError( + ClientHttpResponse response, HttpStatusCode statusCode, @Nullable URI url, @Nullable HttpMethod method) throws IOException { String statusText = response.getStatusText(); @@ -238,6 +198,68 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { throw ex; } + /** + * Read the body of the given response (for inclusion in a status exception). + * @param response the response to inspect + * @return the response body as a byte array, + * or an empty byte array if the body could not be read + * @since 4.3.8 + */ + protected byte[] getResponseBody(ClientHttpResponse response) { + return RestClientUtils.getBody(response); + } + + /** + * Determine the charset of the response (for inclusion in a status exception). + * @param response the response to inspect + * @return the associated charset, or {@code null} if none + * @since 4.3.8 + */ + @Nullable + protected Charset getCharset(ClientHttpResponse response) { + MediaType contentType = response.getHeaders().getContentType(); + return (contentType != null ? contentType.getCharset() : null); + } + + /** + * Return an error message with details from the response body. For example: + * 
+	 * 404 Not Found on GET request for "https://example.com": [{'id': 123, 'message': 'my message'}]
+	 *
+ */ + private String getErrorMessage( + int rawStatusCode, String statusText, @Nullable byte[] responseBody, @Nullable Charset charset, + @Nullable URI url, @Nullable HttpMethod method) { + + StringBuilder msg = new StringBuilder(rawStatusCode + " " + statusText); + if (method != null) { + msg.append(" on ").append(method).append(" request"); + } + if (url != null) { + msg.append(" for \""); + String urlString = url.toString(); + int idx = urlString.indexOf('?'); + if (idx != -1) { + msg.append(urlString, 0, idx); + } + else { + msg.append(urlString); + } + msg.append("\""); + } + msg.append(": "); + if (ObjectUtils.isEmpty(responseBody)) { + msg.append("[no body]"); + } + else { + charset = (charset != null ? charset : StandardCharsets.UTF_8); + String bodyText = new String(responseBody, charset); + bodyText = LogFormatUtils.formatValue(bodyText, -1, true); + msg.append(bodyText); + } + return msg.toString(); + } + /** * Return a function for decoding the error content. This can be passed to * {@link RestClientResponseException#setBodyConvertFunction(Function)}. @@ -265,34 +287,4 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { }; } - /** - * Read the body of the given response (for inclusion in a status exception). - * @param response the response to inspect - * @return the response body as a byte array, - * or an empty byte array if the body could not be read - * @since 4.3.8 - */ - protected byte[] getResponseBody(ClientHttpResponse response) { - try { - return FileCopyUtils.copyToByteArray(response.getBody()); - } - catch (IOException ex) { - // ignore - } - return new byte[0]; - } - - /** - * Determine the charset of the response (for inclusion in a status exception). - * @param response the response to inspect - * @return the associated charset, or {@code null} if none - * @since 4.3.8 - */ - @Nullable - protected Charset getCharset(ClientHttpResponse response) { - HttpHeaders headers = response.getHeaders(); - MediaType contentType = headers.getContentType(); - return (contentType != null ? contentType.getCharset() : null); - } - } diff --git a/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java index c411b133771..578a124abaa 100644 --- a/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java @@ -32,26 +32,27 @@ import org.springframework.lang.Nullable; import org.springframework.util.CollectionUtils; /** - * Implementation of {@link ResponseErrorHandler} that uses {@link HttpMessageConverter - * HttpMessageConverters} to convert HTTP error responses to {@link RestClientException - * RestClientExceptions}. + * Implementation of {@link ResponseErrorHandler} that uses + * {@link HttpMessageConverter HttpMessageConverters} to convert HTTP error + * responses to {@link RestClientException RestClientExceptions}. * *

To use this error handler, you must specify a * {@linkplain #setStatusMapping(Map) status mapping} and/or a - * {@linkplain #setSeriesMapping(Map) series mapping}. If either of these mappings has a match - * for the {@linkplain ClientHttpResponse#getStatusCode() status code} of a given - * {@code ClientHttpResponse}, {@link #hasError(ClientHttpResponse)} will return - * {@code true}, and {@link #handleError(ClientHttpResponse)} will attempt to use the - * {@linkplain #setMessageConverters(List) configured message converters} to convert the response - * into the mapped subclass of {@link RestClientException}. Note that the - * {@linkplain #setStatusMapping(Map) status mapping} takes precedence over - * {@linkplain #setSeriesMapping(Map) series mapping}. + * {@linkplain #setSeriesMapping(Map) series mapping}. If either of these + * mappings has a match for the {@linkplain ClientHttpResponse#getStatusCode() + * status code} of a given {@code ClientHttpResponse}, + * {@link #hasError(ClientHttpResponse)} will return {@code true}, and + * {@link #handleError(ClientHttpResponse)} will attempt to use the + * {@linkplain #setMessageConverters(List) configured message converters} to + * convert the response into the mapped subclass of {@link RestClientException}. + * Note that the {@linkplain #setStatusMapping(Map) status mapping} takes + * precedence over {@linkplain #setSeriesMapping(Map) series mapping}. * *

If there is no match, this error handler will default to the behavior of - * {@link DefaultResponseErrorHandler}. Note that you can override this default behavior - * by specifying a {@linkplain #setSeriesMapping(Map) series mapping} from - * {@code HttpStatus.Series#CLIENT_ERROR} and/or {@code HttpStatus.Series#SERVER_ERROR} - * to {@code null}. + * {@link DefaultResponseErrorHandler}. Note that you can override this default + * behavior by specifying a {@linkplain #setSeriesMapping(Map) series mapping} + * from {@code HttpStatus.Series#CLIENT_ERROR} and/or + * {@code HttpStatus.Series#SERVER_ERROR} to {@code null}. * * @author Simon Galperin * @author Arjen Poutsma @@ -126,11 +127,11 @@ public class ExtractingResponseErrorHandler extends DefaultResponseErrorHandler @Override protected boolean hasError(HttpStatusCode statusCode) { if (this.statusMapping.containsKey(statusCode)) { - return this.statusMapping.get(statusCode) != null; + return (this.statusMapping.get(statusCode) != null); } HttpStatus.Series series = HttpStatus.Series.resolve(statusCode.value()); if (this.seriesMapping.containsKey(series)) { - return this.seriesMapping.get(series) != null; + return (this.seriesMapping.get(series) != null); } else { return super.hasError(statusCode); @@ -138,15 +139,14 @@ public class ExtractingResponseErrorHandler extends DefaultResponseErrorHandler } @Override - public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException { - handleError(response, response.getStatusCode(), url, method); - } + protected void handleError( + ClientHttpResponse response, HttpStatusCode statusCode, + @Nullable URI url, @Nullable HttpMethod method) throws IOException { - @Override - protected void handleError(ClientHttpResponse response, HttpStatusCode statusCode, @Nullable URI url, @Nullable HttpMethod method) throws IOException { if (this.statusMapping.containsKey(statusCode)) { extract(this.statusMapping.get(statusCode), response); } + HttpStatus.Series series = HttpStatus.Series.resolve(statusCode.value()); if (this.seriesMapping.containsKey(series)) { extract(this.seriesMapping.get(series), response); @@ -156,8 +156,9 @@ public class ExtractingResponseErrorHandler extends DefaultResponseErrorHandler } } - private void extract(@Nullable Class exceptionClass, - ClientHttpResponse response) throws IOException { + private void extract( + @Nullable Class exceptionClass, ClientHttpResponse response) + throws IOException { if (exceptionClass == null) { return; @@ -165,6 +166,7 @@ public class ExtractingResponseErrorHandler extends DefaultResponseErrorHandler HttpMessageConverterExtractor extractor = new HttpMessageConverterExtractor<>(exceptionClass, this.messageConverters); + RestClientException exception = extractor.extractData(response); if (exception != null) { throw exception; diff --git a/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java b/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java index 967b2c4fbe2..576070361dd 100644 --- a/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java @@ -19,7 +19,6 @@ package org.springframework.web.client; import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.URI; -import java.nio.charset.StandardCharsets; import org.junit.jupiter.api.Test; @@ -32,6 +31,7 @@ import org.springframework.http.client.ClientHttpResponse; import org.springframework.lang.Nullable; import org.springframework.util.StreamUtils; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.catchThrowable; @@ -72,7 +72,7 @@ class DefaultResponseErrorHandlerTests { given(response.getStatusCode()).willReturn(HttpStatus.NOT_FOUND); given(response.getStatusText()).willReturn("Not Found"); given(response.getHeaders()).willReturn(headers); - given(response.getBody()).willReturn(new ByteArrayInputStream("Hello World".getBytes(StandardCharsets.UTF_8))); + given(response.getBody()).willReturn(new ByteArrayInputStream("Hello World".getBytes(UTF_8))); assertThatExceptionOfType(HttpClientErrorException.class) .isThrownBy(() -> handler.handleError(response)) @@ -90,18 +90,20 @@ class DefaultResponseErrorHandlerTests { @Test void handleErrorWithUrlAndQueryParameters() throws Exception { + String url = "https://example.com/resource"; setupClientHttpResponse(HttpStatus.NOT_FOUND, "Hello World"); assertThatExceptionOfType(HttpClientErrorException.class) - .isThrownBy(() -> handler.handleError(URI.create("https://example.com/resource?access_token=123"), HttpMethod.GET, response)) - .withMessage("404 Not Found on GET request for \"https://example.com/resource\": \"Hello World\""); + .isThrownBy(() -> handler.handleError(URI.create(url + "?access_token=123"), HttpMethod.GET, response)) + .withMessage("404 Not Found on GET request for \"" + url + "\": \"Hello World\""); } @Test void handleErrorWithUrlAndNoBody() throws Exception { + String url = "https://example.com"; setupClientHttpResponse(HttpStatus.NOT_FOUND, null); assertThatExceptionOfType(HttpClientErrorException.class) - .isThrownBy(() -> handler.handleError(URI.create("https://example.com"), HttpMethod.GET, response)) - .withMessage("404 Not Found on GET request for \"https://example.com\": [no body]"); + .isThrownBy(() -> handler.handleError(URI.create(url), HttpMethod.GET, response)) + .withMessage("404 Not Found on GET request for \"" + url + "\": [no body]"); } private void setupClientHttpResponse(HttpStatus status, @Nullable String textBody) throws Exception { @@ -110,7 +112,7 @@ class DefaultResponseErrorHandlerTests { given(response.getStatusText()).willReturn(status.getReasonPhrase()); if (textBody != null) { headers.setContentType(MediaType.TEXT_PLAIN); - given(response.getBody()).willReturn(new ByteArrayInputStream(textBody.getBytes(StandardCharsets.UTF_8))); + given(response.getBody()).willReturn(new ByteArrayInputStream(textBody.getBytes(UTF_8))); } given(response.getHeaders()).willReturn(headers); } @@ -187,7 +189,7 @@ class DefaultResponseErrorHandlerTests { headers.setContentType(MediaType.TEXT_PLAIN); String responseBody = "Hello World"; - TestByteArrayInputStream body = new TestByteArrayInputStream(responseBody.getBytes(StandardCharsets.UTF_8)); + TestByteArrayInputStream body = new TestByteArrayInputStream(responseBody.getBytes(UTF_8)); given(response.getStatusCode()).willReturn(statusCode); given(response.getStatusText()).willReturn(statusText); @@ -227,7 +229,7 @@ class DefaultResponseErrorHandlerTests { headers.setContentType(MediaType.TEXT_PLAIN); String responseBody = "Hello World"; - TestByteArrayInputStream body = new TestByteArrayInputStream(responseBody.getBytes(StandardCharsets.UTF_8)); + TestByteArrayInputStream body = new TestByteArrayInputStream(responseBody.getBytes(UTF_8)); given(response.getStatusCode()).willReturn(statusCode); given(response.getStatusText()).willReturn(statusText); @@ -250,7 +252,7 @@ class DefaultResponseErrorHandlerTests { public void bodyAvailableAfterHasErrorForUnknownStatusCode() throws Exception { HttpHeaders headers = new HttpHeaders(); headers.setContentType(MediaType.TEXT_PLAIN); - TestByteArrayInputStream body = new TestByteArrayInputStream("Hello World".getBytes(StandardCharsets.UTF_8)); + TestByteArrayInputStream body = new TestByteArrayInputStream("Hello World".getBytes(UTF_8)); given(response.getStatusCode()).willReturn(HttpStatusCode.valueOf(999)); given(response.getStatusText()).willReturn("Custom status code"); @@ -259,7 +261,7 @@ class DefaultResponseErrorHandlerTests { assertThat(handler.hasError(response)).isFalse(); assertThat(body.isClosed()).isFalse(); - assertThat(StreamUtils.copyToString(response.getBody(), StandardCharsets.UTF_8)).isEqualTo("Hello World"); + assertThat(StreamUtils.copyToString(response.getBody(), UTF_8)).isEqualTo("Hello World"); } diff --git a/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java b/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java index e28b623cf5e..a65951ea4f3 100644 --- a/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java @@ -19,6 +19,8 @@ package org.springframework.web.client; import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; import java.util.Collections; +import java.util.List; +import java.util.Map; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -36,8 +38,11 @@ import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; /** + * Unit tests for {@link ExtractingResponseErrorHandler}. + * * @author Arjen Poutsma */ +@SuppressWarnings("ALL") class ExtractingResponseErrorHandlerTests { private ExtractingResponseErrorHandler errorHandler; @@ -48,13 +53,10 @@ class ExtractingResponseErrorHandlerTests { @BeforeEach void setup() { HttpMessageConverter converter = new MappingJackson2HttpMessageConverter(); - this.errorHandler = new ExtractingResponseErrorHandler( - Collections.singletonList(converter)); + this.errorHandler = new ExtractingResponseErrorHandler(List.of(converter)); - this.errorHandler.setStatusMapping( - Collections.singletonMap(HttpStatus.I_AM_A_TEAPOT, MyRestClientException.class)); - this.errorHandler.setSeriesMapping(Collections - .singletonMap(HttpStatus.Series.SERVER_ERROR, MyRestClientException.class)); + this.errorHandler.setStatusMapping(Map.of(HttpStatus.I_AM_A_TEAPOT, MyRestClientException.class)); + this.errorHandler.setSeriesMapping(Map.of(HttpStatus.Series.SERVER_ERROR, MyRestClientException.class)); } @@ -72,8 +74,7 @@ class ExtractingResponseErrorHandlerTests { @Test void hasErrorOverride() throws Exception { - this.errorHandler.setSeriesMapping(Collections - .singletonMap(HttpStatus.Series.CLIENT_ERROR, null)); + this.errorHandler.setSeriesMapping(Collections.singletonMap(HttpStatus.Series.CLIENT_ERROR, null)); given(this.response.getStatusCode()).willReturn(HttpStatus.I_AM_A_TEAPOT); assertThat(this.errorHandler.hasError(this.response)).isTrue(); @@ -96,9 +97,9 @@ class ExtractingResponseErrorHandlerTests { responseHeaders.setContentLength(body.length); given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); - assertThatExceptionOfType(MyRestClientException.class).isThrownBy(() -> - this.errorHandler.handleError(this.response)) - .satisfies(ex -> assertThat(ex.getFoo()).isEqualTo("bar")); + assertThatExceptionOfType(MyRestClientException.class) + .isThrownBy(() -> this.errorHandler.handleError(this.response)) + .satisfies(ex -> assertThat(ex.getFoo()).isEqualTo("bar")); } @Test @@ -112,9 +113,9 @@ class ExtractingResponseErrorHandlerTests { responseHeaders.setContentLength(body.length); given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); - assertThatExceptionOfType(MyRestClientException.class).isThrownBy(() -> - this.errorHandler.handleError(this.response)) - .satisfies(ex -> assertThat(ex.getFoo()).isEqualTo("bar")); + assertThatExceptionOfType(MyRestClientException.class) + .isThrownBy(() -> this.errorHandler.handleError(this.response)) + .satisfies(ex -> assertThat(ex.getFoo()).isEqualTo("bar")); } @Test @@ -128,18 +129,17 @@ class ExtractingResponseErrorHandlerTests { responseHeaders.setContentLength(body.length); given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); - assertThatExceptionOfType(HttpClientErrorException.class).isThrownBy(() -> - this.errorHandler.handleError(this.response)) - .satisfies(ex -> { - assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); - assertThat(ex.getResponseBodyAsByteArray()).isEqualTo(body); - }); + assertThatExceptionOfType(HttpClientErrorException.class) + .isThrownBy(() -> this.errorHandler.handleError(this.response)) + .satisfies(ex -> { + assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); + assertThat(ex.getResponseBodyAsByteArray()).isEqualTo(body); + }); } @Test void handleNoMatchOverride() throws Exception { - this.errorHandler.setSeriesMapping(Collections - .singletonMap(HttpStatus.Series.CLIENT_ERROR, null)); + this.errorHandler.setSeriesMapping(Collections.singletonMap(HttpStatus.Series.CLIENT_ERROR, null)); given(this.response.getStatusCode()).willReturn(HttpStatus.NOT_FOUND); HttpHeaders responseHeaders = new HttpHeaders(); From a72855b2b3cf0dd62298b906d8c6282824edad3f Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 28 Nov 2024 17:57:32 +0000 Subject: [PATCH 2/3] DefaultResponseErrorHandler updates Deprecate handleError(response), and ensure it continues to be invoked if overridden. See gh-34231 --- .../client/DefaultResponseErrorHandler.java | 36 +++++++------------ .../ExtractingResponseErrorHandler.java | 25 +++++++------ .../web/client/NoOpResponseErrorHandler.java | 5 --- .../web/client/ResponseErrorHandler.java | 21 ++++++----- ...ltResponseErrorHandlerHttpStatusTests.java | 5 ++- .../DefaultResponseErrorHandlerTests.java | 15 ++++---- .../ExtractingResponseErrorHandlerTests.java | 10 +++--- .../client/RestClientObservationTests.java | 4 +-- .../client/RestTemplateObservationTests.java | 4 +-- 9 files changed, 62 insertions(+), 63 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java index 11ecdb5795f..e08ae70360d 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java @@ -49,8 +49,8 @@ import org.springframework.util.ObjectUtils; * {@link #hasError(HttpStatusCode)}. Unknown status codes will be ignored by * {@link #hasError(ClientHttpResponse)}. * - *

See {@link #handleError(ClientHttpResponse)} for more details on specific - * exception types. + *

See {@link #handleError(URI, HttpMethod, ClientHttpResponse)} for more + * details on specific exception types. * * @author Arjen Poutsma * @author Rossen Stoyanchev @@ -116,27 +116,6 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { return (series == HttpStatus.Series.CLIENT_ERROR || series == HttpStatus.Series.SERVER_ERROR); } - /** - * Handle the error in the given response with the given resolved status code. - *

The default implementation throws: - *

    - *
  • {@link HttpClientErrorException} if the status code is in the 4xx - * series, or one of its sub-classes such as - * {@link HttpClientErrorException.BadRequest} and others. - *
  • {@link HttpServerErrorException} if the status code is in the 5xx - * series, or one of its sub-classes such as - * {@link HttpServerErrorException.InternalServerError} and others. - *
  • {@link UnknownHttpStatusCodeException} for error status codes not in the - * {@link HttpStatus} enum range. - *
- * @throws UnknownHttpStatusCodeException in case of an unresolvable status code - * @see #handleError(ClientHttpResponse, HttpStatusCode, URI, HttpMethod) - */ - @Override - public void handleError(ClientHttpResponse response) throws IOException { - handleError(response, response.getStatusCode(), null, null); - } - /** * Handle the error in the given response with the given resolved status code * and extra information providing access to the request URL and HTTP method. @@ -157,9 +136,20 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { */ @Override public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException { + handleError(response); handleError(response, response.getStatusCode(), url, method); } + /** + * {@inheritDoc} + *

As of 6.2.1 this method is a no-op unless overridden. + */ + @SuppressWarnings("removal") + @Override + public void handleError(ClientHttpResponse response) throws IOException { + // no-op, but here for backwards compatibility + } + /** * Handle the error based on the resolved status code. *

The default implementation delegates to diff --git a/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java index 578a124abaa..e2c95a992b2 100644 --- a/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java @@ -42,11 +42,12 @@ import org.springframework.util.CollectionUtils; * mappings has a match for the {@linkplain ClientHttpResponse#getStatusCode() * status code} of a given {@code ClientHttpResponse}, * {@link #hasError(ClientHttpResponse)} will return {@code true}, and - * {@link #handleError(ClientHttpResponse)} will attempt to use the - * {@linkplain #setMessageConverters(List) configured message converters} to - * convert the response into the mapped subclass of {@link RestClientException}. - * Note that the {@linkplain #setStatusMapping(Map) status mapping} takes - * precedence over {@linkplain #setSeriesMapping(Map) series mapping}. + * {@link #handleError(ClientHttpResponse, HttpStatusCode, URI, HttpMethod)} + * will attempt to use the {@linkplain #setMessageConverters(List) configured + * message converters} to convert the response into the mapped subclass of + * {@link RestClientException}. Note that the + * {@linkplain #setStatusMapping(Map) status mapping} takes precedence over + * {@linkplain #setSeriesMapping(Map) series mapping}. * *

If there is no match, this error handler will default to the behavior of * {@link DefaultResponseErrorHandler}. Note that you can override this default @@ -98,9 +99,10 @@ public class ExtractingResponseErrorHandler extends DefaultResponseErrorHandler * If this mapping has a match * for the {@linkplain ClientHttpResponse#getStatusCode() status code} of a given * {@code ClientHttpResponse}, {@link #hasError(ClientHttpResponse)} will return - * {@code true} and {@link #handleError(ClientHttpResponse)} will attempt to use the - * {@linkplain #setMessageConverters(List) configured message converters} to convert the - * response into the mapped subclass of {@link RestClientException}. + * {@code true} and {@link #handleError(ClientHttpResponse, HttpStatusCode, URI, HttpMethod)} + * will attempt to use the {@linkplain #setMessageConverters(List) configured + * message converters} to convert the response into the mapped subclass of + * {@link RestClientException}. */ public void setStatusMapping(Map> statusMapping) { if (!CollectionUtils.isEmpty(statusMapping)) { @@ -113,9 +115,10 @@ public class ExtractingResponseErrorHandler extends DefaultResponseErrorHandler * If this mapping has a match * for the {@linkplain ClientHttpResponse#getStatusCode() status code} of a given * {@code ClientHttpResponse}, {@link #hasError(ClientHttpResponse)} will return - * {@code true} and {@link #handleError(ClientHttpResponse)} will attempt to use the - * {@linkplain #setMessageConverters(List) configured message converters} to convert the - * response into the mapped subclass of {@link RestClientException}. + * {@code true} and {@link #handleError(ClientHttpResponse, HttpStatusCode, URI, HttpMethod)} + * will attempt to use the {@linkplain #setMessageConverters(List) configured + * message converters} to convert the response into the mapped subclass of + * {@link RestClientException}. */ public void setSeriesMapping(Map> seriesMapping) { if (!CollectionUtils.isEmpty(seriesMapping)) { diff --git a/spring-web/src/main/java/org/springframework/web/client/NoOpResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/NoOpResponseErrorHandler.java index af0f22a52b1..4d1498b4030 100644 --- a/spring-web/src/main/java/org/springframework/web/client/NoOpResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/NoOpResponseErrorHandler.java @@ -43,9 +43,4 @@ public final class NoOpResponseErrorHandler implements ResponseErrorHandler { return false; } - @Override - public void handleError(ClientHttpResponse response) throws IOException { - // never actually called - } - } diff --git a/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java index db2329f8fef..4ca64425f80 100644 --- a/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java @@ -45,14 +45,6 @@ public interface ResponseErrorHandler { * Handle the error in the given response. *

This method is only called when {@link #hasError(ClientHttpResponse)} * has returned {@code true}. - * @param response the response with the error - * @throws IOException in case of I/O errors - */ - void handleError(ClientHttpResponse response) throws IOException; - - /** - * Alternative to {@link #handleError(ClientHttpResponse)} with extra - * information providing access to the request URL and HTTP method. * @param url the request URL * @param method the HTTP method * @param response the response with the error @@ -63,4 +55,17 @@ public interface ResponseErrorHandler { handleError(response); } + /** + * Handle the error in the given response. + *

This method is only called when {@link #hasError(ClientHttpResponse)} + * has returned {@code true}. + * @param response the response with the error + * @throws IOException in case of I/O errors + * @deprecated in favor of {@link #handleError(URI, HttpMethod, ClientHttpResponse)} + */ + @Deprecated(since = "6.2.1", forRemoval = true) + default void handleError(ClientHttpResponse response) throws IOException { + // no-op unless overridden + } + } diff --git a/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerHttpStatusTests.java b/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerHttpStatusTests.java index 328d0259ec4..e7d6a94eacf 100644 --- a/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerHttpStatusTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerHttpStatusTests.java @@ -16,6 +16,7 @@ package org.springframework.web.client; +import java.net.URI; import java.util.stream.Stream; import org.junit.jupiter.api.DisplayName; @@ -24,6 +25,7 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.client.ClientHttpResponse; @@ -80,7 +82,8 @@ class DefaultResponseErrorHandlerHttpStatusTests { given(this.response.getStatusCode()).willReturn(httpStatus); given(this.response.getHeaders()).willReturn(headers); - assertThatExceptionOfType(expectedExceptionClass).isThrownBy(() -> this.handler.handleError(this.response)); + assertThatExceptionOfType(expectedExceptionClass) + .isThrownBy(() -> this.handler.handleError(URI.create("/"), HttpMethod.GET, this.response)); } static Stream errorCodes() { diff --git a/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java b/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java index 576070361dd..fbe2f9d318c 100644 --- a/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java @@ -75,8 +75,8 @@ class DefaultResponseErrorHandlerTests { given(response.getBody()).willReturn(new ByteArrayInputStream("Hello World".getBytes(UTF_8))); assertThatExceptionOfType(HttpClientErrorException.class) - .isThrownBy(() -> handler.handleError(response)) - .withMessage("404 Not Found: \"Hello World\"") + .isThrownBy(() -> handler.handleError(URI.create("/"), HttpMethod.GET, response)) + .withMessage("404 Not Found on GET request for \"/\": \"Hello World\"") .satisfies(ex -> assertThat(ex.getResponseHeaders()).isEqualTo(headers)); } @@ -127,7 +127,8 @@ class DefaultResponseErrorHandlerTests { given(response.getHeaders()).willReturn(headers); given(response.getBody()).willThrow(new IOException()); - assertThatExceptionOfType(HttpClientErrorException.class).isThrownBy(() -> handler.handleError(response)); + assertThatExceptionOfType(HttpClientErrorException.class) + .isThrownBy(() -> handler.handleError(URI.create("/"), HttpMethod.GET, response)); } @Test @@ -140,7 +141,7 @@ class DefaultResponseErrorHandlerTests { given(response.getHeaders()).willReturn(headers); assertThatExceptionOfType(HttpClientErrorException.class).isThrownBy(() -> - handler.handleError(response)); + handler.handleError(URI.create("/"), HttpMethod.GET, response)); } @Test // SPR-16108 @@ -165,7 +166,7 @@ class DefaultResponseErrorHandlerTests { given(response.getHeaders()).willReturn(headers); assertThatExceptionOfType(UnknownHttpStatusCodeException.class).isThrownBy(() -> - handler.handleError(response)); + handler.handleError(URI.create("/"), HttpMethod.GET, response)); } @Test // SPR-17461 @@ -196,7 +197,7 @@ class DefaultResponseErrorHandlerTests { given(response.getHeaders()).willReturn(headers); given(response.getBody()).willReturn(body); - Throwable throwable = catchThrowable(() -> handler.handleError(response)); + Throwable throwable = catchThrowable(() -> handler.handleError(URI.create("/"), HttpMethod.GET, response)); // validate exception assertThat(throwable).isInstanceOf(HttpClientErrorException.class); @@ -236,7 +237,7 @@ class DefaultResponseErrorHandlerTests { given(response.getHeaders()).willReturn(headers); given(response.getBody()).willReturn(body); - Throwable throwable = catchThrowable(() -> handler.handleError(response)); + Throwable throwable = catchThrowable(() -> handler.handleError(URI.create("/"), HttpMethod.GET, response)); // validate exception assertThat(throwable).isInstanceOf(HttpServerErrorException.class); diff --git a/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java b/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java index a65951ea4f3..1af51071128 100644 --- a/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/ExtractingResponseErrorHandlerTests.java @@ -17,6 +17,7 @@ package org.springframework.web.client; import java.io.ByteArrayInputStream; +import java.net.URI; import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.List; @@ -26,6 +27,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.client.ClientHttpResponse; @@ -98,7 +100,7 @@ class ExtractingResponseErrorHandlerTests { given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); assertThatExceptionOfType(MyRestClientException.class) - .isThrownBy(() -> this.errorHandler.handleError(this.response)) + .isThrownBy(() -> this.errorHandler.handleError(URI.create("/"), HttpMethod.GET, this.response)) .satisfies(ex -> assertThat(ex.getFoo()).isEqualTo("bar")); } @@ -114,7 +116,7 @@ class ExtractingResponseErrorHandlerTests { given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); assertThatExceptionOfType(MyRestClientException.class) - .isThrownBy(() -> this.errorHandler.handleError(this.response)) + .isThrownBy(() -> this.errorHandler.handleError(URI.create("/"), HttpMethod.GET, this.response)) .satisfies(ex -> assertThat(ex.getFoo()).isEqualTo("bar")); } @@ -130,7 +132,7 @@ class ExtractingResponseErrorHandlerTests { given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); assertThatExceptionOfType(HttpClientErrorException.class) - .isThrownBy(() -> this.errorHandler.handleError(this.response)) + .isThrownBy(() -> this.errorHandler.handleError(URI.create("/"), HttpMethod.GET, this.response)) .satisfies(ex -> { assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); assertThat(ex.getResponseBodyAsByteArray()).isEqualTo(body); @@ -150,7 +152,7 @@ class ExtractingResponseErrorHandlerTests { responseHeaders.setContentLength(body.length); given(this.response.getBody()).willReturn(new ByteArrayInputStream(body)); - this.errorHandler.handleError(this.response); + this.errorHandler.handleError(URI.create("/"), HttpMethod.GET, this.response); } diff --git a/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java b/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java index 9a438ea866c..1624f25719e 100644 --- a/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/RestClientObservationTests.java @@ -345,12 +345,12 @@ class RestClientObservationTests { } @Override - public boolean hasError(ClientHttpResponse response) throws IOException { + public boolean hasError(ClientHttpResponse response) { return true; } @Override - public void handleError(ClientHttpResponse response) throws IOException { + public void handleError(URI uri, HttpMethod httpMethod, ClientHttpResponse response) { assertThat(this.observationRegistry.getCurrentObservationScope()).isNotNull(); } } diff --git a/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java b/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java index 9dff87051c1..b8a2ad99d31 100644 --- a/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/RestTemplateObservationTests.java @@ -242,12 +242,12 @@ class RestTemplateObservationTests { } @Override - public boolean hasError(ClientHttpResponse response) throws IOException { + public boolean hasError(ClientHttpResponse response) { return true; } @Override - public void handleError(ClientHttpResponse response) throws IOException { + public void handleError(URI uri, HttpMethod httpMethod, ClientHttpResponse response) { currentObservation = this.observationRegistry.getCurrentObservation(); } } From 07455b10f3a444ae64bc4bf09b33b5f7fc209811 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 29 Nov 2024 10:30:29 +0000 Subject: [PATCH 3/3] Use response decorator to check if error handled Closes gh-34231 --- .../client/DefaultResponseErrorHandler.java | 41 ++++++++++++++++--- .../web/client/ResponseErrorHandler.java | 1 - 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java index e08ae70360d..1388224300b 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java @@ -136,18 +136,29 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { */ @Override public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException { - handleError(response); + + // For backwards compatibility try handle(response) first + HandleErrorResponseDecorator decorator = new HandleErrorResponseDecorator(response); + handleError(decorator); + if (decorator.isHandled()) { + return; + } + handleError(response, response.getStatusCode(), url, method); } - /** - * {@inheritDoc} - *

As of 6.2.1 this method is a no-op unless overridden. - */ @SuppressWarnings("removal") @Override public void handleError(ClientHttpResponse response) throws IOException { - // no-op, but here for backwards compatibility + + // Called via handleError(url, method, response) + if (response instanceof HandleErrorResponseDecorator decorator) { + decorator.setNotHandled(); + return; + } + + // Called directly, so do handle + handleError(response, response.getStatusCode(), null, null); } /** @@ -277,4 +288,22 @@ public class DefaultResponseErrorHandler implements ResponseErrorHandler { }; } + + private static class HandleErrorResponseDecorator extends ClientHttpResponseDecorator { + + private boolean handled = true; + + public HandleErrorResponseDecorator(ClientHttpResponse delegate) { + super(delegate); + } + + public void setNotHandled() { + this.handled = false; + } + + public boolean isHandled() { + return this.handled; + } + } + } diff --git a/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java b/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java index 4ca64425f80..be96fb29a11 100644 --- a/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java +++ b/spring-web/src/main/java/org/springframework/web/client/ResponseErrorHandler.java @@ -65,7 +65,6 @@ public interface ResponseErrorHandler { */ @Deprecated(since = "6.2.1", forRemoval = true) default void handleError(ClientHttpResponse response) throws IOException { - // no-op unless overridden } }