Error controller does not process a controller exception anymore

[snicoll]

No idea if this is a regression in Boot or in Spring Framework at this point.

Consider the following

@ResponseStatus(value = HttpStatus.NOT_ACCEPTABLE)
@SuppressWarnings("serial")
private static class NoReasonExpectedException extends RuntimeException {

    public NoReasonExpectedException(String message) {
        super(message);
    }
}

and a controller that throws such exception with a dummy message. With 1.2 you get the standard JSON output as expected. With master the controller does not seem to process the request at all (and therefore an empty response is sent).

Seems like ResponseStatusExceptionResolver is doing something different in 4.2 now.

[bclozel]

This is probably linked to SPR-11193.

In a nutshell: previously, our ResponseStatusExceptionResolver would call HttpServletResponse.sendError in all cases, thus enforcing servlet error pages and breaking the API contract.

The new behavior:

• @ResponseStatus(code=HttpStatus.NOT_ACCEPTABLE) will only call HttpServletResponse.setStatus
• @ResponseStatus(code=HttpStatus.NOT_ACCEPTABLE, reason="some reason") will call HttpServletResponse.sendError

I guess that with the new first case, Boot's ErrorController is not called.

As pointed out by Stéphane, this test case covers only the second use case, not the first.

[snicoll]

The first use case is "covered" now - Fails with master, works with 1.2

[dsyer]

How do you want to do this then? I don't really know why an empty body is a useful default in Spring. Is there something else we can do to handle the response after the standard ResponseStatusExceptionResolver? Must we register a handler for every status code in 4xx?

[bclozel]

I've looked into this and I came up with two solutions.

Adapt Boot behavior to handle those exceptions

In that particular use case, HttpServletResponse.sendError is not called, but Spring Framework's DispatcherServlet.processHandlerException notices that this exception has been handled but its body is empty, so it sets an attribute on the request with the exception itself request.setAttribute(EXCEPTION_ATTRIBUTE, ex);.

With that case, Boot could catch those cases (with a Filter or an Interceptor?) and re-map those responses to the errorController by calling sendError.

Rollback SPR-11193 and clarify javadoc

This example shows some inconsistency in Spring Framework. @ResponseStatus(code=HttpStatus.NOT_ACCEPTABLE, reason="some reason") and @ResponseStatus(code=HttpStatus.NOT_ACCEPTABLE) are quite different and take opposite code paths. With those changes, applications with annotated Exceptions @ResponseStatus(code=HttpStatus.NOT_ACCEPTABLE) will get empty pages instead of container error pages.

I find that telling developers "@ResponseStatus is Servlet only, if you want to handle REST exceptions you'd better use ResponseEntity" sounds better than "@ResponseStatus deals differently with content negotiation and behaves differently depending on its attributes".
So right now I'm leaning towards using sendError in any case in ResponseStatusExceptionResolver and clearly stating that in @ResponseStatus javadoc as it reflects the behavior it had until now anyway and won't break applications.

What do you think @sdeleuze @rstoyanchev ?

[jhoeller]

From my perspective, this setStatus vs sendError arrangement felt awkward to begin with. I'd rather roll it back towards consistent sendError usage and recommend ResponseEntity instead.

[snicoll]

👍

[sdeleuze]

👍

[bclozel]

I've got a commit ready for master on my fork. Leaving this open for a while in case someone wants to comment on this.

[jhoeller]

Discussing this with Rossen, it seems that we have no choice but to revert the change and go with sendError consistently in ResponseStatusExceptionResolver.

So Brian, could you please apply this to master and re-open / re-resolve SPR-13226 (previously marked as "Works as Designed" but I guess we changed our mind on that one now).