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

BasicErrorController returns body even if NO_CONTENT status causing 406 later #18136

Closed
aleksanderlech opened this issue Sep 5, 2019 · 5 comments
Labels
type: bug A general bug
Milestone

Comments

@aleksanderlech
Copy link

Having exception defined like this:

@ResponseStatus(HttpStatus.NO_CONTENT)
public class NoContentException extends RuntimeException {

    public NoContentException(String id) {
        super("ID " + id + " has no content.");
    }
}

And the controller method as following:

    @GetMapping(path = "/{id}/content", produces = {
            "text/plain",
            "audio/wav",
            "image/tiff"
    })
    public ResponseEntity<FileSystemResource> getContent(@PathVariable Id id, @RequestHeader(value = HttpHeaders.ACCEPT, required = false) MediaType requestedMediaType) {
        throw new NoContentException(id.toString()));
    }

I would expect that the response code will be no content no matter what but while executing

curl 'http://localhost:8080/rest/1/content' -i -X GET -H 'Accept: audio/wav'

I am getting 406 (Not Acceptable) because the BasicErrorController that always attaches the body no matter what the status code is:

@RequestMapping
public ResponseEntity<Map<String, Object>> error(HttpServletRequest request) {
	Map<String, Object> body = getErrorAttributes(request, isIncludeStackTrace(request, MediaType.ALL));
	HttpStatus status = getStatus(request);
	return new ResponseEntity<>(body, status);
}

will later cause 406 as the body cannot be converted to the requested type. It seems to be a bug to me as according to the spec the no content response should never contain body.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 5, 2019
@philwebb philwebb added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 5, 2019
@philwebb philwebb added this to the 2.1.x milestone Sep 5, 2019
@s2agrahari
Copy link

s2agrahari commented Sep 8, 2019

By default the registered message converter in spring-boot-starter-web are "application/json, application/*+json".

Thats the reason for getting 406 as there is no message converter found for audio/wav and the BasicErrorController sends the control to unsupportedMediaType().

To get the desired result of 204, try updating the MessageConverter by overriding WebMvcConfigurationSupport's configureMessageConverters()

@Configuration
public class WebConfig extends WebMvcConfigurationSupport {

    @Override
    protected void configureMessageConverters(List<HttpMessageConverter<?>> converters) {
        MappingJackson2HttpMessageConverter converter = new MappingJackson2HttpMessageConverter();
        converter.setSupportedMediaTypes(Collections.singletonList(MediaType.ALL));
        converters.add(converter);
        super.configureMessageConverters(converters);
    }
}

@wilkinsona
Copy link
Member

Thanks for trying to help, @s2agrahari.

The 406 only occurs because the error controller is returning a body. If the response had no body, Spring MVC would not throw a HttpMediaTypeNotAcceptableException and, therefore, would not return a 406.

@wilkinsona
Copy link
Member

wilkinsona commented Sep 9, 2019

@aleksanderlech Using an exception to return a 204 is, perhaps, a little unusual. It's also a little inefficient, which may or may not be a problem depending on how often your application will return such a response. You may want to consider returning a ResponseEntity with a 204 status directly instead:

@GetMapping(path = "/{id}/content")
public ResponseEntity<FileSystemResource> getContent(@PathVariable String id) {
    return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}

This will take Boot's error handling and error controller completely out of the picture.

@aleksanderlech
Copy link
Author

@aleksanderlech Using an exception to return a 204 is, perhaps, a little unusual. It's also a little inefficient, which may or may not be a problem depending on how often your application will return such a response. You may want to consider returning a ResponseEntity with a 204 status directly instead:

@GetMapping(path = "/{id}/content")
public ResponseEntity<FileSystemResource> getContent(@PathVariable String id) {
    return new ResponseEntity<>(HttpStatus.NO_CONTENT);
}

This will take Boot's error handling and error controller completely out of the picture.

Have in mind that this is just example to reproduce the error. In bigger projects using exceptions can be more handy and allows you to abstract from the ResponseEntity.

Anyway this is not discussion about the use cases but just a bug report I found in BasicErrorController :)

@aleksanderlech
Copy link
Author

Thanks for trying to help, @s2agrahari.

The 406 only occurs because the error controller is returning a body. If the response had no body, Spring MVC would not throw a HttpMediaTypeNotAcceptableException and, therefore, would not return a 406.

This is exactly what happens. When the exception its thrown the JSON body is generated no matter what causing the HttpMediaTypeNotAcceptableException later on. I think NO_CONTENT should be handled there different way (either not attach the body or check before throwing the exception).

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

No branches or pull requests

5 participants