Spring 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

Comment From: bclozel

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?

Comment From: puce77

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.

Comment From: puce77

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.

Comment From: puce77

Also note: if no message converter is found, an UnknownContentTypeExceptiongets thrown. This class contains the statusCodeas well as the responseBody(!) but extends RestClientExceptionnot RestClientResponseException.

The WebClient API throws a WebClientResponseException with statusCodebut with an empty (!) responseBodyin such cases (the cause is UnsupportedMediaTypeException).

Comment From: rstoyanchev

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.

Comment From: spring-projects-issues

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.

Comment From: spring-projects-issues

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.

Comment From: puce77

From the discussion so far I would suggest to add a new sub-class of RestClientExceptionsimilar 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) {
   // ...
 } 

Comment From: rstoyanchev

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

Comment From: puce77

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.

Comment From: rstoyanchev

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?

Comment From: puce77

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).

Comment From: rstoyanchev

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.