Suggested change to fix logging of handled exceptions #166
Conversation
|
Hi @TobiasMende, Thanks for the PR, we will take a look at this as soon as priorities allow. |
|
@johnkiely1 Cool! I am looking forward to your feedback. Meanwhile I have added two tests, to verify the behaviour. |
2ac8f75
to
a5e903a
Compare
| */ | ||
| @Override | ||
| public void extendHandlerExceptionResolvers(List<HandlerExceptionResolver> exceptionResolvers) { | ||
| final int position = exceptionResolvers.isEmpty() ? 0 : exceptionResolvers.size() - 1; |
There was a problem hiding this comment.
As far as I can tell the default exception resolvers are populated here.
My concern with this approach is that we don't have any control over the contents of this list. If the order or content of the default exception handlers is changed in the spring libraries then it could affect the way that the Bugsnag library works. If it does then we might end up in the situation of trying to support multiple versions of this mechanism as we can't guarantee what versions of the spring framework users are using.
Although there are problems with the current implementation, I believe that it is more resilient to change in the framework, and it's still possible to exclude unwanted exceptions using the beforeNotify callback, or by specifying "Discard by error class" in the project settings.
I had an idea how to fix #156 and would love some feedback on this approach. If the approach looks okay to you, I would spend some more time to add tests to verify the changed behaviour (namely
@ResponseStatusexceptions and@ExceptionHandlermethods get to handle the exception first and therefore can prevent BugSnag from logging it).Goal
Fix #156
Design
Just changing the Order of the
BugsnagMvcExceptionHandlerdoes not work, because all the three exception handlers have the same (lowest) precedence but are grouped in theHandlerExceptionResolverComposite. This approach injects BugSnag as the second last exception handler just before theDefaultHandlerExceptionResolver.Changeset
The
BugsnagMvcExceptionHandleris injected manually at the right position and not by Spring ordering.Testing
Existing tests still pass. I added two new test to test that exceptions handled by
@ResponseStatusand@ExceptionHandlerare not sent to BugSnag.