spring-projects / spring-framework

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

Append "*/*" To The defaultContentTypes List #26701

Open cgroneman opened 3 years ago

cgroneman commented 3 years ago

If a default content type is set in the ContentNegotiationConfigurer, endpoints that don't return that type can return a 406. I've attached a very simple example.

demo.zip

Bug Explanation: For an endpoint that produces some other content type (example in attached code is text/plain):

This behavior is wrong -- All of these should return a 200. Setting a default content type should not cause endpoints to result in 406. At a minimum, accept headers of 'garbage/garbage,*/*' and '*/*' should behave the same..

Extra explanation Why are we setting a default content type? Because for nearly all endpoints, we want the returned content to be available as XML or JSON, and default to JSON if nothing else is specified.

rstoyanchev commented 3 years ago

Indeed the end result from a client perspective doesn't make sense but this is up to your config. It is explicitly mentioned in the Javadoc that what you provide is what is used along with a hint about appending */* at the end:

     * Set the default content type(s) to use when no content type is requested
     * in order of priority.
     * <p>If destinations are present that do not support any of the given media
     * types, consider appending {@link MediaType#ALL} at the end.
cgroneman commented 3 years ago

You're right, and putting MediaType.ALL is what we ended up doing to resolve our issue.

I would suggest that this should be changed for these reasons:

Before this problem was brought to my attention, the developer who was working on it believed it to be a problem with the headers, because if he commented out .ignoreAcceptHeader(true), it started working in his browser. But that was because his browser was sending a list of accepts, ending with */*.

The documentation at https://docs.spring.io/spring-framework/docs/3.2.x/spring-framework-reference/html/mvc.html (not sure where that is in the current documentation) makes it sound like headers will be matched before the default content type is used. That's what seems natural as well. A header of '*/*' is not matched (but 'garbage/garbage,*/*' is matched, oddly enough). Needing to put ALL as the last default type feels like a hack. I believe the code should be changed such that no accept header, or '*/*', will match in the scenario I submitted. However, if it is determined that this is working as designed, I request that the documentation be updated to indicate that the default can take precedence over the headers in some situations.

rstoyanchev commented 3 years ago

I can improve the documentation to be more explicit about this. A change to the default would have to be in a minor or major version, not a maintenance version.

vmisra2018 commented 3 years ago

@rstoyanchev I stumbled upon this . Is my issue related to above - https://stackoverflow.com/questions/66752245/spring-5-jackson-dataformat-xml-forces-responsebody-with-xml Or how should I fix my issue by configuration. Please let me know

rstoyanchev commented 3 years ago

@vmisra2018 I don't think it's related. I've commented on SO.