HttpMessageConverterExtractor should wrap HttpMessageNotReadableException in RestClientExceptions [SPR-13592]

[spring-projects-issues]

Kristoffer Peterhänsel opened SPR-13592 and commented

When using a RestTemplate instance within a Spring MVC application, client exceptions may propagate in the MVC stack and can be wrongly mapped by server ExceptionHandlers, leading to a wrong HTTP response sent to the browser sending a request to the MVC application.

The RestTemplate instance uses HttpMessageConverter to decode the remote service responses; and when those fail decoding an HTTP response, they can throw an HttpMessageNotReadableException. That exception then bubbles up through the HttpMessageConverterExtractor, RestTemplate and the whole MVC stack, later mapped to HTTP 400 responses, since those exceptions can also be throws by the server stack when the incoming requests can't be deserialized.

Such exceptions should be nested under a RestClientException to avoid erroneous server responses and confusing situations for developers.

---

Affects: 4.1.5

Issue Links:

• Revisit CodecException vs ResponseStatusException in DecoderHttpMessageReader [SPR-15516] #20075 Revisit CodecException vs ResponseStatusException in DecoderHttpMessageReader
• Consistent throwing of HttpMessageNotReadableException in HttpMessageConverter implementations [SPR-16995] #21533 Consistent throwing of HttpMessageNotReadableException in HttpMessageConverter implementations

Referenced from: commits 81143a8, b84fefc

[spring-projects-issues]

Brian Clozel commented

I don't really get what alternative you'd like to see for #1. If we were to declare all possible exceptions there (and make them checked), then you'd be forced to do flow control with exceptions (i.e. implement te logic), which is a known anti-pattern. We're merely wrapping those with framework exceptions because we want the same experience for all possible HTTP clients we support, but also to provide a way to elegantly handle all those exceptions with a single code path.

Now about #2. You're experiencing this behavior because, at the Controller level, if an HTTP client sends an incorrect payload that can't be converted, then the underlying implementation will throw a HttpMessageNotReadableException which will be handled by ResponseEntityExceptionHandler.

Indeed here, this exception is not because of a client of your application, but your own HTTP client making 3rd party requests. In my opinion, the problem here is that you're not catching defensively those exceptions and adapting the behavior of your service (rethrowing a business exception?), and then mapping this business exception in your Controller using an @ExceptionHandler.

Thanks,

[spring-projects-issues]

Kristoffer Peterhänsel commented

My point is that my two issues combined is the default for the Spring Web framework. So basically it has been set up to produce incorrect HTTP status codes based on internal service calls. And there is no way for the caller of RestTemplate instances to know this will happen. So it will happen until the developer (as in me, in this instance) gets enough experience with it to learn that is the case.

And if I try to overwrite the handling of it on a more broader level (so I don't haven to make sure we do it every time we might be calling a RestTemplate - something that is not always obvious). It will break the regular handling of incorrect data being sent to my controllers.

So basically what I would ultimately ask is a more obvious way to handle this when calling RestTemplate instances. I suppose the easy way would be making sure all exceptions thrown inherit from RestClientException since it is already defined to be what will be thrown. And personally I would think that to be a better alternative to getting random exceptions that other parts of the framework clearly assumes are being generated for entirely different reasons.

Edit: I understand it is not being made much easier by the fact that the code that converts the Jackson exception to a Spring one is not aware of what is calling it either.

[spring-projects-issues]

Brian Clozel commented

Kristoffer Peterhänsel
I've rewritten the issue description to make things easier to read for a newcomer.
I've scheduled this issue for Spring 5.x since it's a significant change and applications may rely on that behavior.

Thanks for the report and your patience!

[spring-projects-issues]

Kristoffer Peterhänsel commented

Cool. It's always good to have things behave in a way that is easier to understand for people who didn't make it ;)