Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

URL and HTTP method in rest client exception message #26480

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -20,11 +20,13 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.Reader;
import java.net.URI;
import java.nio.CharBuffer;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;

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;
Expand Down Expand Up @@ -109,20 +111,40 @@ protected boolean hasError(int unknownStatusCode) {
* {@link HttpStatus} enum range.
* </ul>
* @throws UnknownHttpStatusCodeException in case of an unresolvable status code
* @see #handleError(ClientHttpResponse, HttpStatus)
* @see #handleError(URI, HttpMethod, ClientHttpResponse, HttpStatus)
*/
@Override
public void handleError(ClientHttpResponse response) throws IOException {
handleError(null, null, response);
}

/**
* Handle the error in the given response with the given resolved status code.
* <p>The default implementation throws:
* <ul>
* <li>{@link HttpClientErrorException} if the status code is in the 4xx
* series, or one of its sub-classes such as
* {@link HttpClientErrorException.BadRequest} and others.
* <li>{@link HttpServerErrorException} if the status code is in the 5xx
* series, or one of its sub-classes such as
* {@link HttpServerErrorException.InternalServerError} and others.
* <li>{@link UnknownHttpStatusCodeException} for error status codes not in the
* {@link HttpStatus} enum range.
* </ul>
* @throws UnknownHttpStatusCodeException in case of an unresolvable status code
* @see #handleError(URI, HttpMethod, ClientHttpResponse, HttpStatus)
*/
@Override
public void handleError(URI url, HttpMethod method, ClientHttpResponse response) throws IOException {
HttpStatus statusCode = HttpStatus.resolve(response.getRawStatusCode());
if (statusCode == null) {
byte[] body = getResponseBody(response);
String message = getErrorMessage(response.getRawStatusCode(),
response.getStatusText(), body, getCharset(response));
String message = getErrorMessage(response.getRawStatusCode(), response.getStatusText(), body, getCharset(response), url, method);
throw new UnknownHttpStatusCodeException(message,
response.getRawStatusCode(), response.getStatusText(),
response.getHeaders(), body, getCharset(response));
}
handleError(response, statusCode);
handleError(url, method, response, statusCode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the protected method that's now bypassed.

Copy link
Author

@jerzykrlk jerzykrlk Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

Then it can definitely wait for a future Spring release. I'm not in a particular hurry - any custom extensions would well enough.

Should I close the PR, or just keep it here?

Also - is it something that you'd see easy to port into WebClient? Would it make sense to make these two in sync?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can close it for now. On the WebClient side it looks we do have the URL and HTTP method in WebClientResponseException.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @rstoyanchev !

Is there a chance to squeeze this in a future Spring Framework release? Should I come back once it's announced?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course. We can probably re-open this PR then.

}

/**
Expand All @@ -132,9 +154,10 @@ public void handleError(ClientHttpResponse response) throws IOException {
* </pre>
*/
private String getErrorMessage(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, I'd make this method protected - so that I can override it to extract some more specific information from responses that return HTML, XML or some other nonstandard data.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering something beyond 5.3.x - what about this one?

I'd love to have an extension point where I can further customize the response body preview - format it right, remove some tags, etc.

A long time ago, here: https://github.com/spring-projects/spring-framework/pull/1956/files - I even thought about a separate class to extract the body. But a protected method would be just fine.

int rawStatusCode, String statusText, @Nullable byte[] responseBody, @Nullable Charset charset) {
int rawStatusCode, String statusText, @Nullable byte[] responseBody, @Nullable Charset charset, @Nullable URI url, @Nullable HttpMethod method) {

String preface = getPreface(rawStatusCode, statusText, url, method);

String preface = rawStatusCode + " " + statusText + ": ";
if (ObjectUtils.isEmpty(responseBody)) {
return preface + "[no body]";
}
Expand Down Expand Up @@ -162,6 +185,15 @@ private String getErrorMessage(
}
}

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(" ");
}
preface.append(": ");
return preface.toString();
}

/**
* Handle the error based on the resolved status code.
*
Expand All @@ -175,11 +207,29 @@ private String getErrorMessage(
* @see HttpServerErrorException#create
*/
protected void handleError(ClientHttpResponse response, HttpStatus statusCode) throws IOException {
handleError(null, null, response, statusCode);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a protected method that is now bypassed. Something to consider for a minor/major release but not at this stage of the 5.3.x branch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into that!

Initially, I didn't think of squeezing it into 5.3. I'd be very happy to do so - but I think the previous changes broke something in spring-integration, so it might not be that easy.

I tried to not to change the behaviour of this method at all. I can also try to rewrite this to make it more 5.3-compatible. But If you think it makes more sense to wait - let's wait.


/**
* 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(@Nullable URI url, @Nullable HttpMethod method, ClientHttpResponse response,
HttpStatus statusCode) throws IOException {

String statusText = response.getStatusText();
HttpHeaders headers = response.getHeaders();
byte[] body = getResponseBody(response);
Charset charset = getCharset(response);
String message = getErrorMessage(statusCode.value(), statusText, body, charset);
String message = getErrorMessage(statusCode.value(), statusText, body, charset, url, method);

switch (statusCode.series()) {
case CLIENT_ERROR:
Expand Down
Expand Up @@ -18,13 +18,15 @@

import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.util.function.Function;

import org.junit.jupiter.api.Test;
import reactor.core.publisher.Flux;

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;
Expand Down Expand Up @@ -78,6 +80,22 @@ public void handleError() throws Exception {
.satisfies(ex -> assertThat(ex.getResponseHeaders()).isSameAs(headers));
}

@Test
public void handleErrorWithUrlAndMethod() throws Exception {
HttpHeaders headers = new HttpHeaders();
headers.setContentType(MediaType.TEXT_PLAIN);

given(response.getRawStatusCode()).willReturn(HttpStatus.NOT_FOUND.value());
given(response.getStatusText()).willReturn("Not Found");
given(response.getHeaders()).willReturn(headers);
given(response.getBody()).willReturn(new ByteArrayInputStream("Hello World".getBytes(StandardCharsets.UTF_8)));

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));
}

@Test
public void handleErrorWithLongBody() throws Exception {

Expand Down
Expand Up @@ -243,6 +243,7 @@ void notFound(ClientHttpRequestFactory clientHttpRequestFactory) {
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]");
});
}

Expand All @@ -254,7 +255,7 @@ void badRequest(ClientHttpRequestFactory clientHttpRequestFactory) {
template.execute(baseUrl + "/status/badrequest", HttpMethod.GET, null, null))
.satisfies(ex -> {
assertThat(ex.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST);
assertThat(ex.getMessage()).isEqualTo("400 Client Error: [no body]");
assertThat(ex.getMessage()).isEqualTo("400 Client Error after GET http://localhost:" + port + "/status/badrequest : [no body]");
});
}

Expand All @@ -268,6 +269,7 @@ void serverError(ClientHttpRequestFactory clientHttpRequestFactory) {
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]");
});
}

Expand Down