spring-framework
spring-framework copied to clipboard
spring-projects
DefaultRestClient: readWithMessageConverters: retain the statusCode
Affects: 6.1.10
org.springframework.web.client.DefaultRestClient:
If a response cannot be parsed (readWithMessageConverters), a RestClientException (or a subclass) gets thrown instead of the specific subclass RestClientResponseException, making the original responseBody and statusCode very hidden in nested causes.
In such cases a RestClientResponseException should be thrown, so that there is a clean API for getting the statusCode and the original responseBody.
Common base class for exceptions that contain actual HTTP response data.
https://github.com/spring-projects/spring-framework/blob/4a099d213e12eb7985dcb56e433b66cb20cd4274/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java#L202
So far, RestClientResponseException has been used for specific cases like HTTP responses with status > 400.
Expanding RestClientResponseException could have significant repercussions as the body is buffered entirely in memory as a byte[], whereas the HttpMessageNotReadableException thrown by converters contains the HttpInputMessage with the body as input stream. We would need to buffer the entire body into a byte array to make this work, which could consume a lot of memory if the response body is large. Some might even consider this a security issue.
Is that what you had in mind?
Currently I'm mainly interesseted in the statusCode. If we get a statusCode and it is in the 2xx-family, we know the remote service could process everything, we "just" could not parse the response (deserialization step). There are a couple of scenarios where there is actually really no statusCode (connect timeout, message serialization exception during writing the InputStream etc.). In those cases RestClientException is fine as there is no status code to report. But detecting those different cases is important for proper error handling.
I guess the byte[] responseBody in the RestClientResponseException class usually holds smaller (error) messages and I guess in the case we are talking about you could get the original message from the log output for analyzing purposes. So I guess we could omit that for now, if you think that is an issue.
But to get the statusCode I need currently to do something like this:
} catch (RestClientException rce) {
if (rce.getCause() instanceof HttpMessageNotReadableException hmnre) {
if (hmnre.getHttpInputMessage() instanceof ClientHttpResponse clientHttpResponse) {
try {
HttpStatusCode statusCode = clientHttpResponse.getStatusCode();
} catch (IOException e) {
// ...
}
}
}
And this looks quite brittle.
Interestingly, the WebClient API throws in such cases a DecodingException, which is not a subclass of WebClientException. You still don't know the exact statusCode, but since you can clearly distinguish it from the other errors, you at least know there must have been a statusCode in the 2xx-family.
Also note: if no message converter is found, an UnknownContentTypeException gets thrown. This class contains the statusCode as well as the responseBody (!) but extends RestClientException not RestClientResponseException .
The WebClient API throws a WebClientResponseException with statusCode but with an empty (!) responseBody in such cases (the cause is UnsupportedMediaTypeException).
The problem with an HttpMessageNotReadableException is that some of the content may already have been read, e.g. JSON parsing error half way through, and in that case we can't create RestClientResponseException because it expects the body and while that's nullable, "no body" is not the same as "body that can't be exposed" because it's partly consumed.
To put it differently, RestClientException vs RestClientResponseException separates requests that went wrong before we got a full response. An IOException or HttpMessageNotReadableException as the cause further indicates the content couldn't be read. I'm not sure if the exact status really matters in that case. It's definitely something in the success range.
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.
Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.
From the discussion so far I would suggest to add a new sub-class of RestClientException similar to UnknownContentTypeException , which at least retains the statusCode. If you want to omit the responseBody, I think this should be fine with our main use case as well. I changed the title of this issue accordingly.
You could name the exception RestClientResponseDecodingException or similar. HttpMessageNotReadableException could/should still be kept as the cause of this new issue.
Like this we could do error handling like the following (compared to the rather brittle solution I mentioned above):
} catch (RestClientResponseDecodingException rcrde) {
HttpStatusCode statusCode = rcrde.getStatusCode();
// ...
} catch (RestClientException rce) {
// ...
}
On further thought, I'm not sure about another exception. There is RestClientResponseException for 4xx-5xx responses, and RestClientException for anything earlier in which case there is no complete response or no response at all. In other words the following :
}
catch (RestClientResponseException ex) {
// 4xx-5xx
}
catch (RestClientException ex) {
if (ex.getCause() instanceof HttpMessageNotReadableException subEx) {
// failed to read response
}
else {
// other, earlier failure
}
}
From an API point of view I strongly discourage the current solution. It's neither obvious nor documented, as far as I know. And it's boilerplate code.
This is a common case every client has to be prepared for: "the call was executed correctly, but the client fails to parse the response".
Especially if the remote service is not under your control. For writing operations this means the state on the remote service has changed, although you get an exception. There really should be a proper API for this.
The DefaultRestClient#readWithMessageConverters even already has the status information. It just gets lost in the catch-block. Just my 2 cents.
This is a common case every client has to be prepared for: "the call was executed correctly, but the client fails to parse the response".
I'm not sure how adding an exception changes that. One way or another, this is a case that needs to be handled.
For writing operations this means the state on the remote service has changed, although you get an exception.
This is the what I am looking to understand, why the status code matters, and it is becoming more clear, but still, does knowing the specific 2xx status help in some concrete way?
The error handling can be different, if you get an HTTP status in the 2xx-family. You know the service accepted and processed your request and is in a defined state. You "just" could not parse the response. If you don't need the data from the response in that step you could even safely ignore such an exception. Otherwise you could retrieve it with a GET operation.
But for this you have to detect such an error. You can distinguish it from a client and server error, because the HTTP status is in a different HTTP status family. And you can distinguish it from connect errors and read/ write timeouts (which could result in an unknowen and/or undefined server state and thus requires a different error handling) by the presence/ absence of a status code - if the exception carries this information, which it currently does not, unfortunately (just very hidden in the cause chain).
Why is the below not sufficient then:
}
catch (RestClientResponseException ex) {
// <1> 4xx-5xx
}
catch (RestClientException ex) {
if (ex.getCause() instanceof HttpMessageNotReadableException subEx) {
// <2> failed to read response
}
else {
// <3> other, earlier failure
}
}
At <2>, we know there was a response but we couldn't read it, and the status is 2xx because it's not 4xx-5xx. At <3> we didn't even get to an actual response.