spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.27k stars 37.98k forks source link

@RequestMapping produces condition should not impact error responses [SPR-16318] #20865

Closed spring-projects-issues closed 5 years ago

spring-projects-issues commented 6 years ago

Ammar Husain opened SPR-16318 and commented

For a vanilla REST endpoint created in Spring Boot (1.5.9 Release) as below

@RequestMapping(value = "/foo", method = RequestMethod.GET, produces = "text/csv")
public String getCsv() {
    throw new IllegalArgumentException();
}

if request has header Accept: application/json,text/csv it should respond with the JSON representation of error / HTTP 500, instead it gives HTTP 406 - Not Acceptable.

Thus Spring seems not to respect the Accept header as it has following exception in DEBUG logs -> org.springframework.web.HttpMediaTypeNotAcceptableException: Could not find acceptable representation

Even after adding additional message converters (StringMessageConverter) the error persists.

Moreover configuring the @RestControllerAdvice has no effect.


Affects: 4.3.13

Reference URL: https://stackoverflow.com/questions/47831530/406-when-exception-thrown-in-spring-controller-with-accept-header-of-text-csv

Issue Links:

Referenced from: commits https://github.com/spring-projects/spring-framework/commit/395792b30292bc62a56897a4bdc15ab61c689dc1

0 votes, 7 watchers

spring-projects-issues commented 6 years ago

Rossen Stoyanchev commented

The "produces" condition determines the media type to use for the response to be "text/csv". For a success scenario that makes sense, but when rendering an exception (e.g. with a JSON body) that becomes a problem and leads to a 406 instead.

It is surprising this error has not surfaced for as long. We can try and fix it in ExceptionHandlerExceptionResolver by removing the HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE request attribute since at that point the produces condition from the original mapping should no longer apply.

That alone will not help Boot which does an ERROR dispatch to its ErrorController. For that we can add a check to make sure we never use the HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE request attribute as part of an ERROR dispatch.

In all of this there is some potential for regression. I've set 4.3.14 as a target but we'll have to evaluate once the changes are made.

spring-projects-issues commented 6 years ago

Rossen Stoyanchev commented

After some further thought, I am moving this issue to 5.1 since the current behavior has existed for so long and it has potential for regression with existing controller methods that have come to depend on the present behavior, knowingly or not. For example a controller method declared to produce "application/json" might not produce the same response any more when it handles exceptions depending on the exact return type and client Accept header.

A good reason to keep this in 5.x only is the recent fix #20720 which gives an @ExceptionHandler method control over the content-type of the response. So in case of a regression there is an easy option to address it.

In the mean time as a workaround, in your @RestControllerAdvice remove the request attribute with the name HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, and if you still want Boot to handle it, re-throw the exception and that will cause exception handling to continue.

spring-projects-issues commented 6 years ago

Rossen Stoyanchev commented

Modified title (was: "Accept header not respected by Spring (Http response 406 recieved)").

spring-projects-issues commented 6 years ago

Rossen Stoyanchev commented

A little more background. RFC 7807 suggests the use of error specific media types (e.g. "application/problem+json", "application/problem+xml") which suggests those might be appended to the Accept header in addition to media types for a successful response. So while the "produces" condition on an @RequestMapping is binding and forces the response to use that media type, once an error is raised, we need a reset and let the error response make a fresh choice about the media type to use.

spring-projects-issues commented 6 years ago

member sound commented

Will this fix also handle cases where the get-query parameter takes precedence over http accept header?

Which is achieved using the following content negotiation configuration:

@Configuration
public class ContentNegotiationConfiguration implements WebMvcConfigurer {
    @Override
    public void configureContentNegotiation(ContentNegotiationConfigurer configurer) {
        configurer
            .favorParameter(true) //favor &format= parameter over http accept header
            .defaultContentType(MediaType.APPLICATION_JSON);
    }
}
spring-projects-issues commented 6 years ago

Rossen Stoyanchev commented

I don't think so since the ContentNegotiationConfigure only impacts how the requested content type is extracted from the request, while this ticket is about how the resulting value is used thereafter when handling exceptions. Also if there is configuration for what you need, it doesn't sound like it is an actual "issue".

In any case please refrain from commenting further on this ticket which will never be re-opened now that the associated fix is released. If you suspect an issue, please create a new ticket with a fresh description.