Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spring-projects-issues opened this issue Dec 21, 2017 · 6 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Dec 21, 2017

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:

  • #21738 @RequestMapping get 406 Not Acceptable when I use MediaType.IMAGE_PNG_VALUE ("is duplicated by")
  • #18982 Content negotiation fails with the conjonction of text/plain and ExceptionHandler ("is duplicated by")
  • #18982 Content negotiation fails with the conjonction of text/plain and ExceptionHandler
  • #21927 Content-Type not set correctly on @ControllerAdvice

Referenced from: commits 395792b

0 votes, 7 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Dec 21, 2017

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
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 3, 2018

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
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 3, 2018

Rossen Stoyanchev commented

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

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jan 5, 2018

Rossen Stoyanchev commented

A little more background. RFC 7808 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
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 7, 2018

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
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 4, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants