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

AbstractMessageConverterMethodProcessor results in 406 even when ResponseEntity presets content type #23205

Closed
xak2000 opened this issue Jun 27, 2019 · 3 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@xak2000
Copy link
Contributor

xak2000 commented Jun 27, 2019

The fixes introduced in #20720 and #20798 added ability to return ResponseEntity with defined ContentType header. Like this:

    @GetMapping("/test")
    public ResponseEntity<MyModel> test() {
        return ResponseEntity.ok().contentType(MediaType.APPLICATION_XML).body(new MyModel());
    }

AbstractMessageConverterMethodProcessor now takes into account provided Content-Type header and uses it instead of Accept header from request. So it just ignores Accept header. It is ok and good.

But in this case, when no HttpMessageConverter found that can write the response with selected Content-Type, then AbstractMessageConverterMethodProcessor throws HttpMediaTypeNotAcceptableException.

		if (body != null) {
			throw new HttpMediaTypeNotAcceptableException(this.allSupportedMediaTypes);
		}

I think this is not right, because it leads to 406 response, like if no acceptable representation was found, while the real reason of this error is that no converter found for selected representation (and Accept header was ignored altogether). So, this clearly indicates a server misconfiguration (server tries to return a response using provided content type without registering proper HttpMessageConverter, that supports this content type with this response body type). So it should lead to error 500 and should throw HttpMessageNotWritableException.

It should do this only when selectedMediaType is selected based on outputMessage.getHeaders().getContentType() (the Accept header was not taken into account), of course. In other cases HttpMediaTypeNotAcceptableException is still good.

Affected version:

Spring 5.1.8.RELEASE

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 27, 2019
@sbrannen sbrannen added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jun 29, 2019
@rstoyanchev rstoyanchev self-assigned this Jul 4, 2019
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jul 4, 2019
@rstoyanchev rstoyanchev added this to the 5.2 RC1 milestone Jul 4, 2019
@rstoyanchev
Copy link
Contributor

Fair point, in this case it's the server application that chose the content type of the response, and that choice could not be matched to a compatible converter.

@rstoyanchev rstoyanchev changed the title AbstractMessageConverterMethodProcessor throws HttpMediaTypeNotAcceptableException when no converter found for content type selected based on existing Content-Type header AbstractMessageConverterMethodProcessor results in 406 even when ResponseEntity presets content type Jul 5, 2019
@xak2000
Copy link
Contributor Author

xak2000 commented Jul 6, 2019

Thanks, @rstoyanchev !

Just want to ask one question: isn't HttpMessageNotWritableException is more appropriate in this case than IllegalStateException?

In similar case several lines upper the writeWithMessageConverters method throws it:

if (body != null && producibleTypes.isEmpty()) {
    throw new HttpMessageNotWritableException(
            "No converter found for return value of type: " + valueType);
}

This case is similar, except in this case no converters for return type found at all, while in new case no converters for [return type and required content-type] found. But, because the server is responsible for selecting this content-type, then I think HttpMessageNotWritableException is still applies here. In both cases it is illegal configuration of server. And both cases mean: the response can't be written by provided HttpMessageConverters - add more converers or change return type/conten-type.

Also, writeWithMessageConverters method declares HttpMessageNotWritableException as possible result. Unfortunately without mention it in javadoc (what causes this exception?), while two other declared exceptions IOException and HttpMediaTypeNotAcceptableException are present in javadoc, describing cases when they are thrown.

I think, it is worth to add HttpMessageNotWritableException to javadoc of this method with quick description of what causes it to throw. And if you really think that IllegalStateException is more appropriate than HttpMessageNotWritableException in case of preset Content-Type, then maybe it is worth to add it to javadoc (and method declaration) too?


P.S. Actually, while I writing this comment, I found another incorrect behavior. If you declare a controller like this:

@GetMapping(path = "/xml", produces = "application/xml")
public Map<String, Object> producesContentTypeThatIsNotSupportedByAnyConverter() {
	return ImmutableMap.of("foo", "bar");
}

And make a request like this (httpie):

http -v :9080/xml

Then the result will be:

GET /xml HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: localhost:9080
User-Agent: HTTPie/0.9.9



HTTP/1.1 406
...

AbstractMessageConverterMethodProcessor#writeWithMessageConverters method will throw org.springframework.web.HttpMediaTypeNotAcceptableException: Could not find acceptable representation.

This is exactly the same case as with ResponseEntity - the server is responsible for selecting content-type application/xml and no converters found that can write the result using this content-type.

The Accept header was taken into account but it was Accept: */* so it is obviously not "Could not find acceptable representation" case, but more a "Cound not find http message converter for totally acceptable representation" (application/xml is acceptable for client, that requsted it with Accept: */*).

rstoyanchev added a commit that referenced this issue Jul 13, 2019
As a follow-up to the recent commit #37f9ce, this change replaces the
raised IllegalStateException with HttpMessageNotWritableException.

See gh-23205
@rstoyanchev
Copy link
Contributor

I've made a change to raise HttpMesageNotWritableException and updated the Javadoc. As for the issue with produces that one is potentially more nuanced. Can you open a separate ticket for consideration?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants