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

If controller method has produces="*/*" in 5.2.3 response is 500 instead of 406 #24466

Closed
smaldini opened this issue Jan 31, 2020 · 6 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@smaldini
Copy link
Contributor

With the following commit, if a RequestMapping specifies produces="*/*and the request headers don't specify any accept, it will override the response with a 500 as the code below suggests :

34d3240#diff-24571dc5c7743b2a69f7a5df4f2109b2R316

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 31, 2020
@smaldini
Copy link
Contributor Author

smaldini commented Jan 31, 2020

Can reproduce with a simple controller:

@RestController
@RequestMapping("foo")
public class ResourceController {
    @GetMapping(produces = MediaType.ALL_VALUE)
    public void foo(final HttpServletResponse response) throws IOException {
        response.setHeader("Content-Range", "bytes */foo");
        response.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE); //416
    }
}
 RestAssured
            .given(//**.....**//)
            .header(HttpHeaders.RANGE, "bytes=10000-")
            .when()
            .port(this.port)
            .get("/foo")
            .then()
           .statusCode(Matchers.is(HttpStatus.REQUESTED_RANGE_NOT_SATISFIABLE.value())); //fails

@smaldini
Copy link
Contributor Author

In spring 5.2.2 the behavior didn't seem all too right neither in fact, because instead of returning 416 status it would return 406 because content type doesn't match. Is it on purpose ?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 3, 2020

Yes, overriding 406 (Not Acceptable) with a 500 has some recent history.

Starting in 5.2 and #23205, if a controller method returns a ResponseEntity with a "Content-Type" set, then the controller has effectively fixed the content-type (rather than allow the content negotiation algorithm determine it). In that case if there is no suitable converter, it's a server-side error.

In 5.2.3 and #23287, this was further extended for the case where a controller method declares what it produces, and yet fails to produce it. Arguably through we should be double checking that the declared mime type is concrete, so I'm scheduling a follow-up fix.

That said, a produces="*/*" is rather unexpected here since a controller method supports all media types by default. Is there a reason for declaring it like that?

@rstoyanchev rstoyanchev self-assigned this Feb 3, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 3, 2020
@rstoyanchev rstoyanchev added this to the 5.2.4 milestone Feb 3, 2020
@rstoyanchev rstoyanchev changed the title Regression in 5.2.3: If content type "*/*" is produced, converter now fails the request If controller method has produces="*/*" in 5.2.3 response is 500 instead of 406 Feb 3, 2020
@rstoyanchev
Copy link
Contributor

On closer look, the problem is not what seems.

This happens after Boot's BasicErrorController tries to render the error. It fails because the request attribute HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, which reflects the media types from the target controller method's producible condition, is still lingering.

Typically this attribute would be removed in case of an exception from the controller, so as to not interfere with error response rendering, but in this case the controller method operates directly on the response and calls sendError. We can clear this attribute in all cases after controller method handling.

Keep in mind however, this fix wouldn't help because on some Servlet container, sendError works as a forward and we wouldn't even have a chance to do anything until it's too late. So generally I would advise against handling the response directly like this unless it's really necessary. If you must however, as a workaround you can do this:

@GetMapping(produces = MediaType.ALL_VALUE)
public void foo(final HttpServletRequest request, final HttpServletResponse response) throws IOException {
	response.setHeader("Content-Range", "bytes */foo");
	request.removeAttribute(HandlerMapping.PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE); // <---
	response.sendError(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE); 
}

Note also that Spring MVC does support HTTP ranges. If you return a Resource it will automatically handle requests for specific ranges:

@GetMapping(produces = MediaType.ALL_VALUE)
public Resource foo() {
    Resource resource = ... ;
    return resource;
}

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 5, 2020

I should also note with the above workaround in place, and Boot 2.2.4, I get a 416 response.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Feb 5, 2020

You should also see the following spring-projects/spring-boot#19522 which is coming in Boot 2.3 as an improvement so that error detail rendering does not end up overriding the original error response if it fails to find a compatible media type (say client wants text, but we want to render a JSON error response).

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

3 participants