Improve response message on DecodingException

[PyvesB]

Affects: 5.2.0

---

Hello,

When a DecodingException is thrown from a reactive handler chain, it is mapped to a ServerWebInputException, with a generic Failed to read HTTP message reason:
https://github.com/spring-projects/spring-framework/blob/d1aee0e8691c41753621332ff69b17be3f7c8ba2/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java#L73

As a consequence, all bad request bodies are similar, regardless of what caused them in the first place:

{"timestamp":"2019-10-10T13:32:32.210+0000","path":"/some-path","status":400,"error":"Bad Request","message":"Failed to read HTTP message"}

The client is not provided with any helpful message indicating why the request was unsuccessful, it's not even clear that this was caused by a failure to decode the body in the first place.

For example, when using a AbstractJackson2Decoder, the DecodingException contains much more useful information, such as:

JSON decoding error: Missing required creator property 'xxxxxx'

Could such information be included as part of the Bad Request response?

Thanks for reading!

Pyves

[rstoyanchev]

On second thought, I wonder if this is the right place to fix this. The ServerWebInputException is created with the root cause, so it does contain the reason for the 400 error. The error message in the response body however is likely written by Spring Boot. There is nothing in the framework that writes error details to the response body.

Taking a look at DefaultErrorAttributes it does seem to write only the message of the top level exception. Is there some room for improvement there @bclozel to also write the cause, or is there a reason why the cause isn't included?

[bclozel]

Spring Boot is providing configuration properties for writing the actual exception class (server.error.include-exception) and the full stacktrace (server.error.include-stacktrace). Spring Boot is currently unwrapping Servlet exceptions (for Servlet containers) and ResponseStatusException (including subclasses like ServerWebInputException), but we're only including that information with the exception and the stacktrace themselves.

I assume this is because we don't want to expose information about application internals by default, and we're leaving that choice to developers.

[PyvesB]

@rstoyanchev thanks for your answer! Those were some of my thoughts as well when browsing through the code. As far as I can tell, there are two main ways to improve this:

• expanding the exception message in spring-framework.
• including the cause message in the response body in spring-boot.

[bclozel]

@PyvesB Are you sure you're using Spring Boot 2.2? The error message should include the requestId now and I'm not seeing that in your sample.

I'm wondering if including the cause by default would not leak important internal information about the application.

[PyvesB]

@bclozel at the time of opening the issue, Spring Boot 2.2 was not yet released. So I did check out in the latest milestone, but took an error message from our production servers. Here's what it looks like with the latest:

{"timestamp":"2019-10-30T09:20:45.988+0000","path":"/some-path","status":400,"error":"Bad Request","message":"Failed to read HTTP message","requestId":"bccb992f"}

Only server.error.include-stacktrace gives access to some of the missing information and contains JSON decoding error: Missing required creator property 'xxxxxx'. But that also comes with the entire stack trace, which creates a giant payload, is extremely noisy from the API user's point of view and does consequently disclose much more information about the application.

I think a middle ground has to be found, where the user is not confusingly in the dark ("Failed to read HTTP message"), but has some indication of why his request is invalid.

[bclozel]

I've transferred this on the Spring Boot tracker so we can consider an enhancement here.
A sensible middle-ground would be to add a "cause" attribute to the DefaultErrorAttributes (in both MVC and WebFlux).

[rstoyanchev]

I'm wondering if including the cause by default would not leak important internal information about the application.

I think including the top level message isn't all that different from including the root cause message, and arguably both could have been included, but starting to do that at this stage is almost certainly going to cause a surprise somewhere. Having the option to enable that seems reasonable to me. It is already possible by including the full stack trace, so this would be a sort of in-between option.

[bclozel]

The Spring Boot team has been discussing this and we're not 100% sure yet about that change.
I'll implement the change locally and try it out with various exceptions (codecs, IO errors, etc) and see how things look like. This feature would be useful in that very case but we're not sure that:

• it's useful in all cases as the "relevant" information might be in the middle of the exception hierarchy
• it might expose too much information
• it might not really help more because the error message might be very implementation specific

I'll report back here with my findings.