Content negotiation fails with the conjonction of text/plain and ExceptionHandler [SPR-14411]

[spring-projects-issues]

Mathieu POUSSE opened SPR-14411 and commented

When a client request an url with an "Accept" header such as "text/plain, application/json, */*" and the handler method that produces "text/plain" throws and exception that is handled with an @ExceptionHandler that returns an object to serialize as json, the framework fails to negotiate the content and returns a 406.

This is because in org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodProcessor#writeWithMessageConverters(T, org.springframework.core.MethodParameter, org.springframework.http.server.ServletServerHttpRequest, org.springframework.http.server.ServletServerHttpResponse) at line 195, we take the first MediaType matching the requested and the productible (in this case text/plain).

Unfortunatly, there is no HttpMessageConverter able to serialize the object to text/plain and then it returns the 406.

Instead of that, it should proceed with the other MediaType matching the requested and productible types.

---

Affects: 4.2.6

Reference URL: https://github.com/mathieu-pousse/spring-framework-issues/tree/SPR-14411

Issue Links:

• @RequestMapping produces condition should not impact error responses [SPR-16318] #20865 @RequestMapping produces condition should not impact error responses ("duplicates")
• @RequestMapping get '406 Not Acceptable' when change 'produces' order [SPR-16205] #20753 @RequestMapping get '406 Not Acceptable' when change 'produces' order ("is duplicated by")
• @RequestMapping produces condition should not impact error responses [SPR-16318] #20865 @RequestMapping produces condition should not impact error responses

[spring-projects-issues]

Mathieu POUSSE commented

Proposed PR #1093

[spring-projects-issues]

Rossen Stoyanchev commented

Unfortunatly, there is no HttpMessageConverter able to serialize the object to text/plain and then it returns the 406.

This part doesn't make sense. The "producible" media types is what we can produce. Please provide a sample or a repro project.

[spring-projects-issues]

Mathieu POUSSE commented

I thought the gist was enough. Anyway, here is the complete test project with integration tests: https://github.com/mathieu-pousse/spring-framework-issues/tree/SPR-14411 (do you want me to PR that ?)

I meant that spring tries to negotiate content and ends up with the "compatibleMediaTypes" that contains the values text and json

Spring picks the first concrete type from the list (based on client preferences) and tries to convert the result.
As there is no message converter able to serialize an exception to text/plain, it gives up and return the 406.

Instead, it should try to use the remaining compatibleMediaTypes.

And this is only when all the negotiated "compatibleMediaTypes" have failed that it return 406.

Hope it is clearer
M.

[spring-projects-issues]

Mathieu POUSSE commented

just completed test cases

[spring-projects-issues]

Rossen Stoyanchev commented

Okay thanks for the test project. I have a better idea.

The gist of it is is an @RequestMapping(produces="text/plain") and an @ExceptionHandler returning a Map. In case of an error we attempt to render the Map as "text/plain" which clearly can't work. Unfortunately looping over the "compatibleMediaTypes" isn't a solution. If the method has only produces="text/plain" that list would contain only one content type.

The real problem is that the produces condition is "sticky" which is by design and it's meant to pre-determine the response content type. In this case with error handling however it gets in the way because the error response is meant to be JSON. In fact I'm guessing you want the error response content type to be fixed (always JSON) and not subject to content negotiation?

There may be ways to improve how produces works with regards to error handling. However I'd like to ask if you do need all the produces conditions? It's hard to judge from the given code but take for example the controller method that produces both text and json, that doesn't make sense because the return value is a Map. Generally speaking produces is useful as a deal breaker between two controller methods when the request mappings have the same URL path mapping and HTTP method. Perhaps reducing the use of produces conditions a little may be just enough to help in your case. In particular the produces="text/plain" seem a bit redundant. If the mappings work out without them as I expect they would, error handling should work better too.

[spring-projects-issues]

Mathieu POUSSE commented

Yes, the example is probably not complex enough to cover a real life case.

The idea is to have an endpoint that produces something different than json (text/plain, images, binary, ..), and in case of error during the process we must return a properly formatted error which is in my case json but could be xml.

I agree the "produces" is is used to negotiate which method of the controller can handle the request but it is kind of a workaround I was trying.

Somehow we should be able to serialize "normal" method return accordingly to the "produces" on the controller' method and the mapped exception as json/xml or whatever.

Then the serialization phase should renegotiate how to serialize the returned object.

Is there a place where we can define how the error can be serialized and then automatically add this MediaType as "productible" type in case of mapped exception ?

[spring-projects-issues]

Brian Clozel commented

Resuming the discussion here since we've just got a duplicate report for this in #20753.

When Spring MVC is handling errors with @ExceptionHandler methods, I'm wondering if we shouldn't completely remove the HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE request attribute and start over content negotiation.

This would solve the following issue (and I guess #18982 and #20753):

@Controller
public class TestController {

	@ExceptionHandler(IllegalArgumentException.class)
	public ResponseEntity<ErrorMessage> handleErrors(IllegalArgumentException exc) {
		return ResponseEntity.ok().contentType(MediaType.APPLICATION_JSON).body(new ErrorMessage("another message"));
	}

	@GetMapping(path = "/", produces = MediaType.TEXT_PLAIN_VALUE)
	public ResponseEntity<String> throwException() {
		throw new IllegalArgumentException("test exception message");
	}
}

The original handler can only produce "text/plain" and this is the only producible content type, but the error handler wants to write JSON.

But that change is a breaking one and could cause confusion in existing applications, like:

@Controller
public class TestController {

	@ExceptionHandler(IllegalArgumentException.class)
        @ResponseBody
	public ErrorMessage handleErrors(IllegalArgumentException exc) {
		return new ErrorMessage("another message");
	}

	@GetMapping(path = "/", produces = MediaType.APPLICATION_XML_VALUE)
	@ResponseBody
	public List<User> listAllUsers() {
		throw new IllegalArgumentException("test exception message");
	}
}

In that case, an application is exposing an HTTP API in XML format; when an error occurs, the handler will return an object and is not providing any content type information. A new content negotiation takes place and JSON might be selected. This would be really confusing as many applications would have to enforce a content type information where they didn't need before. This might cause other breaking changes as well.

Right now I think that fixing the fact that we send HTTP 406 instead of error responses outweigh the possible breaking changes.

Note: Spring Boot error handling mechanism, through an error dispatch, does not keep the content type selected in the first place and leaves that to the error page support. This is rather different as this is a truly global error handling mechanism; but this might show that starting over the content negotiation might not be bad after all in many cases.

[spring-projects-issues]

Loi Huu Mai commented

Hi Brian,

"does not keep the content type selected in the first place and leaves that to the error page support." I am building a restful service, "error page support" is not a solution for me. My current solution is put the content type which @ExceptionHandler produces stand first. It is kind of work around. So I still look for a solution.