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

Avoid 406 Not Acceptable for error pages #19522

Closed
bclozel opened this issue Jan 3, 2020 · 7 comments
Closed

Avoid 406 Not Acceptable for error pages #19522

bclozel opened this issue Jan 3, 2020 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@bclozel
Copy link
Member

bclozel commented Jan 3, 2020

Given a Spring MVC Controller like:

@RestController
public class TestController {
	@GetMapping(path = "/test", produces = "text/plain")
	public String test() {
		throw new ExpectedException();
	}

	@ResponseStatus(value = HttpStatus.BAD_REQUEST, reason = "Expected!")
	static class ExpectedException extends RuntimeException {

	}
}

When clients send requests to the "/test" endpoint and the Accept: text/plain request header, the error response rendered by Spring Boot's ErrorController has an empty body and a "406 Not Acceptable" HTTP status. Although the client is not able to read the response body, we should not hide the original HTTP response status as it might give important information about the error.

This happens because the ErrorController tries to render the error map with the requested media type, which is text/plain. Since it is not possible to serialize the error map in that format, the original HTTP status response is overridden and the client gets a 406 instead.

In these cases, we should not write the response body and instead use the orignal HTTP response status with an empty response body.

We've seen instances of this problem in #13163 and spring-projects/spring-framework#23421

@bclozel bclozel added the type: enhancement A general enhancement label Jan 3, 2020
@bclozel bclozel added this to the 2.3.0.M1 milestone Jan 3, 2020
@bclozel bclozel self-assigned this Jan 3, 2020
@bclozel bclozel closed this as completed in cc154bb Jan 3, 2020
@snicoll
Copy link
Member

snicoll commented Jan 3, 2020

@bclozel would it be possible to backport this one?

@bclozel
Copy link
Member Author

bclozel commented Jan 3, 2020

@snicoll I'm not sure. This is improving the error handling support but it changes the existing behavior which is not strictly a bug. So I'm wondering if developers are expecting this somehow in tests, or (in a strange way) relying on this.

@rstoyanchev
Copy link
Contributor

I think there is a good case for a backport. It seems unlikely that an application can do something meaningful that would depend on the current behavior. A test might be fail somewhere but that's an easy change.

@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Jan 6, 2020
@bclozel bclozel removed the for: team-attention An issue we'd like other members of the team to review label Jan 6, 2020
bclozel added a commit that referenced this issue Jan 6, 2020
Prior to this commit, the `ErrorController` would override the original
error response status if the error map cannot be written due to content
negotiation with the HTTP client. In that case, the error handling
infrastructure returns a `406 Not Acceptable` response.

This commit improves the `ErrorController` so that
`HttpMediaTypeNotAcceptableException` instances thrown by that
controller are not returned as is but instead we write the error
response with an empty body and the original HTTP error status.

Fixes gh-19545
See gh-19522
@rstoyanchev
Copy link
Contributor

It would be really useful to have this one in 2.2.x. I keep running into issues where the original status is obscured.

@wilkinsona
Copy link
Member

It was back ported in 2f78c72 which was in 2.2.3. Assuming that you're seeing the problem with 2.2.3 or later, it sounds like we need to investigate to learn why you're still seeing a problem. @rstoyanchev is there a sample project somewhere that we can look at?

@rstoyanchev
Copy link
Contributor

Ah yes indeed it was, and it is working as expected, my bad. Cheers!

@mojtaba-ghasemi
Copy link

mojtaba-ghasemi commented Jul 11, 2023

in spring boot: imagine that we want to return in web service method same as:

ResponseEntity.status(HttpStatus.CREATED)
        .body(myObject);

myObject class need to use @Data annotation from Lombok package

@Data
public class myClass{
...
}

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

No branches or pull requests

5 participants