Exception swallowed in ResponseEntityExceptionHandler [SPR-16743]

[spring-projects-issues]

Jan Mares opened SPR-16743 and commented

Problem - after calling other service with RestTemplate, the call fails because of deserialization failure, but the exception is not logged

Expected behaviour - stack trace of the exception can be found in the logs

Actual behaviour - only this line appears in the log and the server returns 500

c.d.s.p.c.c.e.BadRequestExceptionHandler : Unknown exception type: org.springframework.web.client.RestClientException

Reason - exception is swallowed in ResponseEntityExceptionHandler (https://github.com/spring-projects/spring-framework/blob/v5.0.5.RELEASE/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java#L188) because not only type of the exception, but also the type of its cause is used to match method in ExceptionHandlerMethodResolver(https://github.com/spring-projects/spring-framework/blob/v5.0.5.RELEASE/spring-web/src/main/java/org/springframework/web/method/annotation/ExceptionHandlerMethodResolver.java#L136)

Code:

@ControllerAdvice
public class BadRequestExceptionHandler extends ResponseEntityExceptionHandler
{
 
    @ExceptionHandler(value = {BadRequestException.class})
    protected ResponseEntity<Object> handleBadRequest(BadRequestException ex, WebRequest request)
	{
        return handleExceptionInternal(ex, ex.getMessage(), new HttpHeaders(), HttpStatus.BAD_REQUEST, request);
    }
}

The swallowing most probably happens, because exception org.springframework.web.client.RestClientException is thrown with HttpMessageNotReadableException as its cause. When ExceptionHandlerMethodResolver looks for a handler method in resolveMethodByThrowable, first it does not find one, but when it tries a cause of the exception (HttpMessageNotReadableException) it finds one in ResponseEntityExceptionHandler::handleException. That results in this method being called, but the instance passed to the method is not the one of HttpMessageNotReadableException, but of RestClientException and therefore it does not match any of the instanceof ifs in the method and falls in the last else, resulting in logging only that little fragment shown above.

IMHO trying to match against cause as well as the exception itself is dangerous and rather unexpected. As we reuse code, we need to reuse the exceptions that come with it. But in this case an exception of type HttpMessageNotReadableException thrown from contoller trying to deserialize the request is something completely different to the same exception wrapped in RestClientException when trying to parse a response of dependant microservice. Also, why only one level down, why not two levels - cause of a cause, or three cause of a cause of a cause ...

Hacky temporary solution - as we cannot override handleException in ResponseEntityExceptionHandler , we had to override handleInternalException in the following way:

@Override
protected ResponseEntity<Object> handleExceptionInternal(
     final Exception ex,
     final Object body,
     final HttpHeaders headers,
     final HttpStatus status,
     final WebRequest request
)
{
     if (HttpStatus.INTERNAL_SERVER_ERROR.equals(status)) {
          // due to problem with cause and the fact the handleException in the parent is final
          logger.warn("Potentially unlogged exception. Re-logging: ", ex);
     }
     return super.handleExceptionInternal(ex, body, headers, status, request);
}

which is not very nice.

---

Affects: 4.3.16, 5.0.5

Issue Links:

• @ExceptionHandler should match cause as well (e.g. for exception thrown from argument formatter) [SPR-14291] #18863 @ExceptionHandler should match cause as well (e.g. for exception thrown from argument formatter)

Referenced from: commits a200df6, 318d04c, 7b894fe, 193c289, ed44262, f2cc70e

Backported to: 4.3.17

[spring-projects-issues]

Juergen Hoeller commented

From my perspective, ResponseEntityExceptionHandler should simply return null in case of an unknown exception. Its handler implementation decides whether it is only interested in top-level exceptions among its declared exception types, and if it is not, it should simply back out.

As for cause introspection there, this is a pretty old decision that we can't easily change. The differentiating exception can be wrapped in some cases, not least of it all in the common ServletException... and we only really care about top-level wrappers in such scenarios, not about any kind of deeper nesting. This is usually not a problem with the implementation pattern outlined above: However, for some reason ResponseEntityExceptionHandler wasn't following that pattern, assuming that its instanceof checks were exhaustively covering the potential variants and raising an assertion-like exception at that point... which seems accidental to  me.

Rossen Stoyanchev, does the above sound feasible to you?

[spring-projects-issues]

Jan Mares commented

@jhoeller I was not aware, that returning null will cause the handler to fallback to default error handling. That seems like a reasonable fix to me.

What concerns the causes and backwards compatibility, I was afraid, that might be a problem. And we (users of the framework) are very glad, that you keep BC breaks to minimum. An ability to choose which exceptions are going to be unwrapped might have sorted out the issue, you are describing, but I guess it is too late for that discussion.

Anyways, thank you for looking at this and for working on an amazing framework!

[spring-projects-issues]

Rossen Stoyanchev commented

Juergen Hoeller, since ResponseEntityExceptionHandler is an @ControllerAdvice, and not directly in the HandlerExceptionResolver chain, wouldn't returning null result in a 200 and empty body?

As for not checking the the cause, yes that is accidental.

[spring-projects-issues]

Juergen Hoeller commented

Rossen Stoyanchev, as far as I can tell, it would simply back out from advice-level resolution in ExceptionHandlerExceptionResolver and end up in ResponseStatusExceptionResolver / DefaultHandlerExceptionResolver as usual. I haven't done integration tests for this yet though.

[spring-projects-issues]

Jan Mares commented

@Rossen Stoyanchev I would just like to make clear, that returning 400 in this scenario is certainly not desirable for us either. The exception cause was HttpMessageNotReadableException, but the root is RestClientException, signalling that this happened when trying to parse a response of a microservice, that was called during handling the request. Expected behaviour is the same as for any other uncaught exception (500, internal error and logged).

If this is not possible due to BC limitations, it would be at least nice to be able to override the behaviour of ResponseEntityExceptionHandler#handleException and force it to stop handling exceptions, where the root should not be handled - e.g. by removing final from it.

[spring-projects-issues]

Juergen Hoeller commented

As for only handling the root exception level in those cases, this can easily be kept up. Arguably such an advice only means to resolve root-level exceptions, even if it will technically have the chance to introspect wrapped exceptions as well. Revising this towards lenient handling of an unexpected exception instance should do the job here while preserving existing expectations.

[spring-projects-issues]

Jan Mares commented

@Juerge Hoeller As I asked above - would not lenient handling (checking the causes in ResponseEntityExceptionHandler) turn this swallowed 500 to 400 with no log whatsoever?

[spring-projects-issues]

Juergen Hoeller commented

I actually mean to keep up our check for top-level exceptions in ResponseEntityExceptionHandler, simply ignoring nested exception matches by backing out with null if none of the existing top-level checks pass... being lenient in that sense, not considering a different exception an internal error at the end of that cascade. Since nested cause checks are specific to @ExceptionHandler methods, none of the other default resolvers should try to handle it either.

[spring-projects-issues]

Jan Mares commented

Alright, sorry for not understanding.

[spring-projects-issues]

Rossen Stoyanchev commented

it would simply back out from advice-level resolution in ExceptionHandlerExceptionResolver

As far as I can see, ExceptionHandlerExceptionResolver finds a matching @ExceptionHandler which returns a ResponseEntity. HttpEntityMethodProcessor leaves the response as is, for a null return and marking the request handled. ExceptionHandlerExceptionResolver then returns an empty ModelAndView.

In other words from the point of view of ExceptionHandlerExceptionResolver, the exception is handled, since it found and successfully invoked the @ExceptionHandler. 

[spring-projects-issues]

Juergen Hoeller commented

Rossen Stoyanchev, you're right indeed, that's a quite different code path for a null return value. I'm experimenting with a few tweaks at the moment, allowing for such an @ExceptionHandler method to back off, possibly through (re-)throwing an exception... not really ending up with the desired outcome yet though.

So we might be better off keeping the existing INTERNAL_SERVER_ERROR code path in ResponseEntityExceptionHandler, simply fully logging such an unexpected exception instead of just the exception type there. That seems to be the easiest way out, in particular for a 4.3.x / 5.0.x backport.

[spring-projects-issues]

Juergen Hoeller commented

It turns out that rethrowing the original exception from ResponseEntityExceptionHandler does have the intended effect (explicitly supported for @ExceptionHandler methods): with the exception going back to the HandlerExceptionResolver chain and eventually to DispatcherServlet's default processing. This seems like the cleanest way out here, so I'll go with that variant for the backports as well.

[spring-projects-issues]

Juergen Hoeller commented

Related to this, I've added explicit coverage of root vs cause exception matching to our MVC reference documentation, with hints on how to design custom @ExceptionHandler methods accordingly. This was unfortunately lacking since the cause matching feature is rather recent (#18863 in Spring Framework 4.3).

[spring-projects-issues]

Juergen Hoeller commented

Oh, and the actual change is available in the latest 5.0.6.BUILD-SNAPSHOT and 4.3.17.BUILD-SNAPSHOT for pre-release testing. We intend to go live tomorrow.

[spring-projects-issues]

Jan Mares commented

Thank you!