Inability to override response status with BasicErrorController in 2.1.10

[rafaljaw]

Version: 2.1.10.RELEASE

After upgrading from spring boot 2.1.8 to 2.1.10 we start retrieving HTTP 500 errors instead of HTTP 400 or 409 that were thrown before. After some investigation we've found custom error handling in our code base for some of the errors thrown by spring jpa/hibernate/mongo/etc which was detected in our custom implementation of ErrorAttributes and apart of returning custom error message we were also modifying 'javax.servlet.error.status_code' attribute of the WebRequest sent as input parameter. This was working so far without any problem because response status was determined after resolving error attributes as you can see below.

public class BasicErrorController extends AbstractErrorController {

	@RequestMapping
	public ResponseEntity<Map<String, Object>> error(HttpServletRequest request) {
		Map<String, Object> body = getErrorAttributes(request, isIncludeStackTrace(request, MediaType.ALL)); // HERE we could still override javax.servlet.error.status_code which is used as HTTP response status
		HttpStatus status = getStatus(request);
		return new ResponseEntity<>(body, status);
	}

With latest code the status is resolved before retrieving error attributes, and hence overriding response status has no effect and the old code no longer works for us. Please restore the original behavior so that overriding javax.servlet.error.status_code in ErrorAttributes:getErrorAttributes affects the response status returned in ResponseEntity.

Workaround

As a workaround we already extended BasicErrorController and override its error method:

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

        return status == HttpStatus.NO_CONTENT ? new ResponseEntity<>(status) : new ResponseEntity<>(body, status);
    }

however I believe it would make much more sense to modify the BasicErrorController so that people relying on similar behavior can still use this.

[wilkinsona]

Thanks for raising an issue after the brief discussion on a related commit.

Can you please provide some details about your use case and how you came to rely on mutating the request as a side-effect of retrieving the error attributes? I’d like to understand why something like an exception handler wasn’t used.

[rafaljaw]

I'm not saying it is not possible to achieve the same through exception error handler - we just don't want to invest in that for now. There is some common code written old time ago where exception handling is implemented by providing custom ErrorAttributes and HandlerExceptionResolver. This common code is imported by ~20 different microservices and it just works so we never were interested in replacing it by something else. It may be too expensive to migrate it to exception handler since this common error handling is reused in all our microservices.

In theory I believe it should be possible to replace this by different spring mechanism - we just don't want to invest in doing that. Although some parts of current error handling may be tricky to implement with exception handler since for some exceptions (e.g. org.hibernate.exception.ConstraintViolationException) we return different response status depending on the error message in the cause, in other words the same exception may result in different HTTP status depending on what exactly is causing that (for some errors we do parsing of error message).

[rafaljaw]

I'm also aware that the way this error handling is implemented is not as elegant as it could/should be and we can probably live with the workaround. If there is no one else relying on this behavior I'm fine with not getting this fixed - it may be even an argument for us for considering rewriting it to exception handler in the future. On the other hand fixing this seems to be trivial so I still would be happy to see this fixed:)

[wilkinsona]

We discussed this as a team today and decided that, given that you have a workaround, we will leave things as they are. Mutating the request's attributes while getting the error attributes to include in the error controller's response isn't something that we want to encourage. Thanks anyway for bringing this to our attention.