Fix redundant exception handling

[nkavian]

Describe the bug

When an unhandled exception occurs, the Spring framework iterates through a list of exception handlers looking for someone to consume the unhandled exception. This is the Java for loop in the Spring framework that is looking for 1 and only one exception handler to swallow the exception.

The existing BugSnag code explicitly sets BugsnagMvcExceptionHandler as the first Exception Handler by setting Ordered.HIGHEST_PRECEDENCE on line 22. The code on line 47-48 then explicitly lets some other exception handler try to consume the exceptions. This breaks the expectation that only 1 handler should be invoked.

It's more reasonable that the BugSnag library should run last in the chain and not first in the chain. Allowing other exception handlers to consume the exception first allows the application to consume or 'handle' the exception and swallow it before it reaches BugSnag.

For example, my Java application throws custom defined exceptions to perform URL redirects and display custom HTML error pages. I have a @ControllerAdvice class that then properly renders the content. However, the BugSnag library catches the exception, logs it to your API, and then finally my application code runs. Since I handled the exception, it should never have been logged by the library.

Another example, in my class with custom exception handling using @ControllerAdvice, for 1 particular exception, I explicitly invoke BugSnag to log an exception as handled. But then your code also logs it as unhandled in your own handler..

Steps to reproduce

Create a spring boot project
Create a class to handle a custom exception and create a @ControllerAdvice annotated class to custom handle the exception.
Invoke the exception and watch BugSnag log it as unhandled as well as watch the @ControllerAdvice annotated class handle the exception.

Environment

• Bugsnag version:3.6.1
• Java version: 8
• Spring framework version (if any): 2.1.4.RELEASE
• Maven version (if any): 3.6.3

Example

I created this branch to solve the issue, but I couldn't understand the test that I broke. This example fix and the above referenced parts of the code explain well what the observed yet expected result should be.

https://github.com/bugsnag/bugsnag-java/compare/master...nkavian:lower-precedence?expand=1

Change the order on BugsnagMvcExceptionHandler from highest to lowest. This allows the application to handle exceptions. If an exception truly was not handled by any one else, then finally it should be logged to BugSnag as unhandled.

Using bugsnag.setIgnoreClasses() is a workaround but it's undesirable as a long term solution. I have to maintain a list of 10+ classes to exclude here.

[tomlongridge]

@nkavian - in your change, I believe the tests fail because Spring's DefaultHandlerExceptionResolver handles the error and so it would not make it to Bugsnag as the lowest priority handler.

In order to allow ControllerAdvice beans to swallow exceptions before the Bugsnag handler and also to catch exceptions thrown by these beans, it looks like we could make the BugsnagMvcExceptionHandler a ControllerAdvice bean with the lowest priority. (A helpful explanation of the mechanism is here.)

However, I believe this would only work for Spring MVC, and I'm not sure we could find a solution that would not risk the handler not firing for different use cases. Therefore you may wish to consider such an approach using a fork of this repo, but I believe we'd need a more general purpose approach to change the library itself.

[nkavian]

Great link that's very detailed. Regarding your last paragraph, this issue is specific to Spring MVC and the suggested code change is to code within the bugsnag-spring folder, so there are no other use cases to be concerned with right?

[tomlongridge]

The ControllerAdvice is relevant to a Spring web application but not to a Spring console application, for example.

[nkavian]

I might have missed the nuance there, but that sounds like a good thing. This change would be to the Bugsnag MVC exception handler dealing with the Spring MVC.

https://github.com/nkavian/bugsnag-java/blob/master/bugsnag-spring/src/main/java/com/bugsnag/BugsnagMvcExceptionHandler.java#L22

[tomlongridge]

Yes - I think it's achievable. I've raised it in our system to have a look at when priorities allow.