Polish "Include URL and HTTP method in DefaultResponseErrorHandler"

See gh-28958

Author: snicoll

spring-web/src/main/java/org/springframework/web/client/DefaultResponseErrorHandler.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -131,15 +131,17 @@ protected boolean hasError(int statusCode) {
 	 * {@link HttpStatus} enum range.
 	 * </ul>
 	 * @throws UnknownHttpStatusCodeException in case of an unresolvable status code
-	 * @see #handleError(URI, HttpMethod, ClientHttpResponse, HttpStatusCode)
+	 * @see #handleError(ClientHttpResponse, HttpStatusCode, URI, HttpMethod)
 	 */
 	@Override
 	public void handleError(ClientHttpResponse response) throws IOException {
-		handleError(null, null, response);
+		HttpStatusCode statusCode = response.getStatusCode();
+		handleError(response, statusCode, null, null);
 	}
 
 	/**
-	 * Handle the error in the given response with the given resolved status code.
+	 * 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.
 	 * <p>The default implementation throws:
 	 * <ul>
 	 * <li>{@link HttpClientErrorException} if the status code is in the 4xx
@@ -152,58 +154,51 @@ public void handleError(ClientHttpResponse response) throws IOException {
 	 * {@link HttpStatus} enum range.
 	 * </ul>
 	 * @throws UnknownHttpStatusCodeException in case of an unresolvable status code
-	 * @see #handleError(URI, HttpMethod, ClientHttpResponse, HttpStatusCode)
+	 * @since 6.2
+	 * @see #handleError(ClientHttpResponse, HttpStatusCode, URI, HttpMethod)
 	 */
 	@Override
 	public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException {
 		HttpStatusCode statusCode = response.getStatusCode();
-		handleError(url, method, response, statusCode);
+		handleError(response, statusCode, url, method);
 	}
 
 	/**
 	 * Return error message with details from the response body. For example:
 	 * <pre>
-	 * 404 Not Found: [{'id': 123, 'message': 'my message'}]
+	 * 404 Not Found on GET request for "https://example.com": [{'id': 123, 'message': 'my message'}]
 	 * </pre>
 	 */
-	private String getErrorMessage(
-			int rawStatusCode, String statusText, @Nullable byte[] responseBody, @Nullable Charset charset, @Nullable URI url, @Nullable HttpMethod method) {
+	private String getErrorMessage(int rawStatusCode, String statusText, @Nullable byte[] responseBody, @Nullable Charset charset,
+			@Nullable URI url, @Nullable HttpMethod method) {
 
-		String preface = getPreface(rawStatusCode, statusText, url, 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)) {
-			return preface + "[no body]";
+			msg.append("[no body]");
 		}
-
-		charset = (charset != null ? charset : StandardCharsets.UTF_8);
-
-		String bodyText = new String(responseBody, charset);
-		bodyText = LogFormatUtils.formatValue(bodyText, -1, true);
-
-		return preface + bodyText;
-	}
-
-	private String getPreface(int rawStatusCode, String statusText, @Nullable URI url, @Nullable HttpMethod method) {
-		StringBuilder preface = new StringBuilder(rawStatusCode + " " + statusText);
-		if (!ObjectUtils.isEmpty(method) && !ObjectUtils.isEmpty(url)) {
-			preface.append(" after ").append(method).append(" ").append(url).append(" ");
+		else {
+			charset = (charset != null ? charset : StandardCharsets.UTF_8);
+			String bodyText = new String(responseBody, charset);
+			bodyText = LogFormatUtils.formatValue(bodyText, -1, true);
+			msg.append(bodyText);
 		}
-		preface.append(": ");
-		return preface.toString();
-	}
-
-	/**
-	 * Handle the error based on the resolved status code.
-	 *
-	 * <p>The default implementation delegates to
-	 * {@link HttpClientErrorException#create} for errors in the 4xx range, to
-	 * {@link HttpServerErrorException#create} for errors in the 5xx range,
-	 * or otherwise raises {@link UnknownHttpStatusCodeException}.
-	 * @since 5.0
-	 * @see HttpClientErrorException#create
-	 * @see HttpServerErrorException#create
-	 */
-	protected void handleError(ClientHttpResponse response, HttpStatusCode statusCode) throws IOException {
-		handleError(null, null, response, statusCode);
+		return msg.toString();
 	}
 
 	/**
@@ -213,12 +208,11 @@ protected void handleError(ClientHttpResponse response, HttpStatusCode statusCod
 	 * {@link HttpClientErrorException#create} for errors in the 4xx range, to
 	 * {@link HttpServerErrorException#create} for errors in the 5xx range,
 	 * or otherwise raises {@link UnknownHttpStatusCodeException}.
-	 * @since 5.0
+	 * @since 6.2
 	 * @see HttpClientErrorException#create
 	 * @see HttpServerErrorException#create
 	 */
-	protected void handleError(@Nullable URI url, @Nullable HttpMethod method, ClientHttpResponse response,
-			HttpStatusCode statusCode) throws IOException {
+	protected void handleError(ClientHttpResponse response, HttpStatusCode statusCode, @Nullable URI url, @Nullable HttpMethod method) throws IOException {
 		String statusText = response.getStatusText();
 		HttpHeaders headers = response.getHeaders();
 		byte[] body = getResponseBody(response);

spring-web/src/main/java/org/springframework/web/client/ExtractingResponseErrorHandler.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2022 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -138,7 +138,12 @@ protected boolean hasError(HttpStatusCode statusCode) {
 	}
 
 	@Override
-	public void handleError(ClientHttpResponse response, HttpStatusCode statusCode) throws IOException {
+	public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException {
+		handleError(response, response.getStatusCode(), url, method);
+	}
+
+	@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);
 		}
@@ -147,17 +152,12 @@ public void handleError(ClientHttpResponse response, HttpStatusCode statusCode)
 			extract(this.seriesMapping.get(series), response);
 		}
 		else {
-			super.handleError(response, statusCode);
+			super.handleError(response, statusCode, url, method);
 		}
 	}
 
-	@Override
-	public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException {
-		handleError(response, response.getStatusCode());
-	}
-
 	private void extract(@Nullable Class<? extends RestClientException> exceptionClass,
-						ClientHttpResponse response) throws IOException {
+			ClientHttpResponse response) throws IOException {
 
 		if (exceptionClass == null) {
 			return;

spring-web/src/test/java/org/springframework/web/client/DefaultResponseErrorHandlerTests.java

@@ -29,6 +29,7 @@
 import org.springframework.http.HttpStatusCode;
 import org.springframework.http.MediaType;
 import org.springframework.http.client.ClientHttpResponse;
+import org.springframework.lang.Nullable;
 import org.springframework.util.StreamUtils;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -80,19 +81,38 @@ void handleError() throws Exception {
 	}
 
 	@Test
-	public void handleErrorWithUrlAndMethod() throws Exception {
-		HttpHeaders headers = new HttpHeaders();
-		headers.setContentType(MediaType.TEXT_PLAIN);
+	void handleErrorWithUrlAndMethod() throws Exception {
+		setupClientHttpResponse(HttpStatus.NOT_FOUND, "Hello World");
+		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\": \"Hello World\"");
+	}
 
-		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)));
+	@Test
+	void handleErrorWithUrlAndQueryParameters() throws Exception {
+		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\"");
+	}
 
+	@Test
+	void handleErrorWithUrlAndNoBody() throws Exception {
+		setupClientHttpResponse(HttpStatus.NOT_FOUND, null);
 		assertThatExceptionOfType(HttpClientErrorException.class)
 				.isThrownBy(() -> handler.handleError(URI.create("https://example.com"), HttpMethod.GET, response))
-				.withMessage("404 Not Found after GET https://example.com : \"Hello World\"")
-				.satisfies(ex -> assertThat(ex.getResponseHeaders()).isSameAs(headers));
+				.withMessage("404 Not Found on GET request for \"https://example.com\": [no body]");
+	}
+
+	private void setupClientHttpResponse(HttpStatus status, @Nullable String textBody) throws Exception {
+		HttpHeaders headers = new HttpHeaders();
+		given(response.getStatusCode()).willReturn(status);
+		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.getHeaders()).willReturn(headers);
 	}
 
 	@Test

spring-web/src/test/java/org/springframework/web/client/RestTemplateIntegrationTests.java

@@ -241,40 +241,48 @@ void patchForObject(ClientHttpRequestFactory clientHttpRequestFactory) {
 	void notFound(ClientHttpRequestFactory clientHttpRequestFactory) {
 		setUpClient(clientHttpRequestFactory);
 
+		String url = baseUrl + "/status/notfound";
 		assertThatExceptionOfType(HttpClientErrorException.class).isThrownBy(() ->
-				template.execute(baseUrl + "/status/notfound", HttpMethod.GET, null, null))
+				template.execute(url, HttpMethod.GET, null, null))
 			.satisfies(ex -> {
 				assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND);
 				assertThat(ex.getStatusText()).isNotNull();
 				assertThat(ex.getResponseBodyAsString()).isNotNull();
-				assertThat(ex.getMessage()).isEqualTo("404 Client Error after GET http://localhost:" + port + "/status/notfound : [no body]");
+				assertThat(ex.getMessage()).containsSubsequence("404", "on GET request for \"" + url + "\": [no body]");
+				assumeFalse(clientHttpRequestFactory instanceof JdkClientHttpRequestFactory, "JDK HttpClient does not expose status text");
+				assertThat(ex.getMessage()).isEqualTo("404 Client Error on GET request for \"" + url + "\": [no body]");
 			});
 	}
 
 	@ParameterizedRestTemplateTest
 	void badRequest(ClientHttpRequestFactory clientHttpRequestFactory) {
 		setUpClient(clientHttpRequestFactory);
 
+		String url = baseUrl + "/status/badrequest";
 		assertThatExceptionOfType(HttpClientErrorException.class).isThrownBy(() ->
-				template.execute(baseUrl + "/status/badrequest", HttpMethod.GET, null, null))
+				template.execute(url, HttpMethod.GET, null, null))
 			.satisfies(ex -> {
 				assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST);
+				assertThat(ex.getMessage()).containsSubsequence("400", "on GET request for \""+url+ "\": [no body]");
 				assumeFalse(clientHttpRequestFactory instanceof JdkClientHttpRequestFactory, "JDK HttpClient does not expose status text");
-				assertThat(ex.getMessage()).isEqualTo("400 Client Error after GET http://localhost:" + port + "/status/badrequest : [no body]");
+				assertThat(ex.getMessage()).isEqualTo("400 Client Error on GET request for \""+url+ "\": [no body]");
 			});
 	}
 
 	@ParameterizedRestTemplateTest
 	void serverError(ClientHttpRequestFactory clientHttpRequestFactory) {
 		setUpClient(clientHttpRequestFactory);
 
+		String url = baseUrl + "/status/server";
 		assertThatExceptionOfType(HttpServerErrorException.class).isThrownBy(() ->
-				template.execute(baseUrl + "/status/server", HttpMethod.GET, null, null))
+				template.execute(url, HttpMethod.GET, null, null))
 			.satisfies(ex -> {
 				assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
 				assertThat(ex.getStatusText()).isNotNull();
 				assertThat(ex.getResponseBodyAsString()).isNotNull();
-				assertThat(ex.getMessage()).isEqualTo("500 Server Error after GET http://localhost:" + port + "/status/server : [no body]");
+				assertThat(ex.getMessage()).containsSubsequence("500", "on GET request for \"" + url + "\": [no body]");
+				assumeFalse(clientHttpRequestFactory instanceof JdkClientHttpRequestFactory, "JDK HttpClient does not expose status text");
+				assertThat(ex.getMessage()).isEqualTo("500 Server Error on GET request for \"" + url + "\": [no body]");
 			});
 	}