AbstractMessageConverterMethodProcessor ignores HttpEntityMethodProcessor's Content-Type header [SPR-16172]

[spring-projects-issues]

Liam Bryan opened SPR-16172 and commented

Context:
We have a controller method which returns ResponseEntity<?> (because several conversions are supported based on the client's request). This means that there is no produces specified in the @RequestMapping (or more directly, @GetMapping) annotation.

Using:
return ResponseEntity.ok().contentType(...).body(...);

I believe it should be possible to directly specify the Content-Type of the returned entity.

Problem
The HttpEntityMethodProcessor does add the specified Content-Type header to the outputHeaders (line 184); however the call to AbstractMessageConverterMethodProcessor.writeWithMessageConverters does not pay attention to this header's presence. getProducibleMediaTypes then falls through to looking at the produces part of the @RequestMapping annotation (which is empty) and so declares support for everything. In our case this means that it ends up being handled by the wrong (or at least, from our perspective, not the intended) HttpMessageConverter.

Current workaround
We are getting around this at present by adding a call to HttpServletRequest.setAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, Collections.singleton(...)); (which then directly overrides the return value from getProducibleMediaTypes).

I'm not 100% certain this is a bug, but it certainly makes using the ResponseEntity.BodyBuilder.contentType method appear misleading in this context.

Tested on both 4.3.7.RELEASE and 4.3.12.RELEASE.

---

Affects: 4.3.7, 4.3.12

Issue Links:

• WebMVC RequestMapping Produces Problem [SPR-16448] #20993 WebMVC RequestMapping Produces Problem ("is duplicated by")
• Breaking change in MVC: Accept handling is now more strict [SPR-16251] #20798 Breaking change in MVC: Accept handling is now more strict

Referenced from: commits 9a894ab

[spring-projects-issues]

Rossen Stoyanchev commented

This is by design indeed. The idea is that the content type to use is determined through a fairly involved content negotiation algorithm. So the content type shouldn't come from the ResponseEntity unless the controller is checking against requested media types paying attention to specificity, quality parameter, and so on. Chances the controller isn't doing all that and it shouldn't have to. How are you actually determining what content type to produce and why don't you let that be picked based on the requested content type and the Object returned? The ResponseEntity<?> in the method signature isn't a problem because we also check the actual value returned for the body.

[spring-projects-issues]

Liam Bryan commented

Fair questions.

We are determining the content type based on the submitted content type for the entity being retrieved, alongside a couple of additional parameters specifying any (or no) conversions requested. For that reason, */* is the only applicable produces attribute for the mapping in question.

The issue here occurred due to a combination of two factors:

1. A client not providing an Accept header (they have been asked to update this, but it appears they may not always have knowledge of what the submitted Content-Type was, and when not asking for any conversions have no real desire to know), meaning that it is defaulting to \*/\* as an Accept.

Thus, the acceptableContentTypes list is populated to specify support for everything.

2. A GsonHttpMessageConverter being registered before (in the list of converters) the more specific HttpMessageConverter for the value to be returned (which would not have content type application/json) - order of converters is effectively out of our control. This resulted in the GsonHttpMessageConverter declaring that it can support the object and being the first match between produces and acceptable and so being used - which in turn returned an empty json object ({}) due to configuration and lack of @Expose annotations on the returned item.

Returning a ResponseEntity with a Content-Type seemed like it would get around this - allowing us to, in the absence of an Accept header (or with a matching one), return the item as submitted; however this was not the case. If not for specifying the Content-Type of the returned entity, what is the ResponseEntity.contentType method intended for?

[spring-projects-issues]

Rossen Stoyanchev commented

Maybe something could be done about the order of converters. You're aware of the extendHandlerExceptionResolvers callback on WebMvcConfigurer? It's invoked at the end after converters have been registered and you're given the list to inspect and modify as needed. For example you can find the Gson converter and insert before it, etc.

As for the content type on ResponseEntity, I believe the original intent in writing it to the output headers was to provide control over how the Content-Type is written (e.g. with charset added) but that makes no sense since content negotiation could come up with an entirely different content type. Unless Arjen Poutsma has any more insight?

I think we could take the content type from the ResponseEntity, if one was present, and use it as a fallback when there is no "produces" media type present. We would still cross-check this against the requested media types, and then try to find a matching converter, so it could result in a 406 or a 500 if we can't find a compatible media type and converter, but at least we're not bypassing the content negotiation process.

[spring-projects-issues]

Arjen Poutsma commented

If I remember correctly, HttpEntity predates the produces element, and the original intent was to be able to specify both HTTP headers and a body in one type, not just to change the character set of the content-type.

In my opinion, if there is a Content-Type specified in the ResponseEntity, then that should be the Content-Type that is written to the HTTP output. After all, the user added it explicitly, and we simply cannot ignore it or change it to another value, as that violates the principle of least astonishment. If the method that returns the ResponseEntity with Content-Type also has a produces element, then the Content-Type in the entity should still take precedence, as code takes precedence over metadata. If the specified Content-Type and the produces element are incompatible, then this should result in an exception and a 5xx status error, as the error is clearly on the server side.

[spring-projects-issues]

Rossen Stoyanchev commented

Thanks Arjen Poutsma I think we're generally in agreement.

One small correction -- compatibility between the provided Content-Type and the produces attribute is an indirect check against the request against which the produces attribute was matched in the first place. We might as well check directly against the request. In other words it doesn't matter if the request was routed to a method because it had a specific produces condition. As long as the provided content type still matches the request, all is good. We need to do that anyway and a produces condition may not even be present.

For the most part I think produces conditions and an explicitly provided content type are naturally, mutually exclusive. Unless one wants to override what is declared in metadata but I think the most likely case is you're using one or the other rather than both. In any case taking the provided Content-Type first, before the produces attribute, is a good idea.

Now the question is do we backport to 4.3? Even if the new behavior is better, there is bound to be existing code that relies on the current behavior.

[spring-projects-issues]

Juergen Hoeller commented

This is indeed tricky to backport. Let's rather fix this in 5.0.2 for a start and see how it goes.

[spring-projects-issues]

Liam Bryan commented

@Rossen: Interesting about the extendHandlerExceptionResolvers. I was aware of the method; but honestly had assumed that it would be a completely separately maintained List (specifically for Exception resolution) rather than the same one. I believe the main hold up on reordering was that it would need to happen in every override of configureMessageConverters; otherwise it will depend on the order the classes are scanned when loading the context. Simply adding an @Order(Ordered.HIGHEST_PRECEDENCE) annotation to the less-specific converters, and sorting based on that in one of these callbacks should get around that part of the issue however.

The excellently put point of "least astonishment" still stands however for general usage. Thank you all for taking the time to respond.

[spring-projects-issues]

Rossen Stoyanchev commented

The Content-Type from the ResponseEntity, if present, is now used to initialize the list of producible media types.

[spring-projects-issues]

Francisco Lozano commented

I'm late to this discussion, but suffering now the consequences. This change forcibly breaks an acceptable (not recommended but acceptable) behaviour in HTTP, which is returning a content-type that is not compatible with the one that the client expects as per the Accept header.

#20798.

[spring-projects-issues]

Rossen Stoyanchev commented

#20798 is fixed now.