AsyncRestTemplate changes in SPR-13413 made its harder to detect Client/Server errors [SPR-14353] #18925
Description
Oleg Mikhailenko opened SPR-14353 and commented
After migrating to 4.2.6 our response statuses were broken for our client side. It always showed 500 error status. It turned out our client's exception handler (@ControllerAdvice) caught ExecutionException even though a server responded with 4xx/5xx error statuses.
Spring's error hander for AsyncRestTemplate (DefaultResponseErrorHandler) contains code:
public void handleError(ClientHttpResponse response) throws IOException {
HttpStatus statusCode = getHttpStatusCode(response);
switch (statusCode.series()) {
case CLIENT_ERROR:
throw new HttpClientErrorException(statusCode, response.getStatusText(),
response.getHeaders(), getResponseBody(response), getCharset(response));
case SERVER_ERROR:
throw new HttpServerErrorException(statusCode, response.getStatusText(),
response.getHeaders(), getResponseBody(response), getCharset(response));
default:
throw new RestClientException("Unknown status code [" + statusCode + "]");
}
}It's OK but then (after #17992) any of these exceptions wrapped into ExecutionException in ResponseExtractorFuture (look throw new ExecutionException(ex);):
@Override
protected final T adapt(ClientHttpResponse response) throws ExecutionException {
try {
if (!getErrorHandler().hasError(response)) {
logResponseStatus(this.method, this.url, response);
}
else {
handleResponseError(this.method, this.url, response);
}
return convertResponse(response);
}
catch (Throwable ex) {
throw new ExecutionException(ex);
}
finally {
if (response != null) {
response.close();
}
}
}So as result it does not matter what error response sent by server HttpClientErrorException/HttpServerErrorException. It always wrapped to ExecutionException. And to fetch response status and body I have to get inner cause and cast to appropriate type. What is not so convenient.
It would be great if you could change the code of ResponseExtractorFuture to something like:
@Override
protected final T adapt(ClientHttpResponse response) throws ExecutionException {
try {
if (!getErrorHandler().hasError(response)) {
logResponseStatus(this.method, this.url, response);
}
else {
handleResponseError(this.method, this.url, response);
}
return convertResponse(response);
}
catch (RestClientException ex) {
throw ex;
}
catch (Throwable ex) {
throw new ExecutionException(ex);
}
finally {
if (response != null) {
response.close();
}
}
}Affects: 4.2.6
Issue Links:
- @ExceptionHandler should match cause as well (e.g. for exception thrown from argument formatter) [SPR-14291] #18863
@ExceptionHandlershould match cause as well (e.g. for exception thrown from argument formatter) - AsyncRestTemplate should wrap RuntimeExceptions in ExecutionException [SPR-13413] #17992 AsyncRestTemplate should wrap RuntimeExceptions in ExecutionException
1 votes, 5 watchers