Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doc: ControllerAdvice @ExceptionHandler matching for root vs cause in multi-advice scenario [SPR-16074] #20623

Closed
spring-issuemaster opened this issue Oct 16, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Oct 16, 2017

Franz Wong opened SPR-16074 and commented

I have ExceptionA and ExceptionB.

ExceptionB is the cause of ExceptionA.

ExceptionB exceptionB = new ExceptionB();
throw new ExceptionA(exceptionB);

The order of exception handlers for both exceptions are the same.
It means ExceptionBHandler "may" be checked before ExceptionAHandler in "ExceptionHandlerExceptionResolver.getExceptionHandlerMethod", thus it uses ExceptionBHandler instead of ExceptionAHandler.


Affects: 4.3.10

Referenced from: commits ea00c7c, d473506

Backported to: 4.3.13

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 16, 2017

Juergen Hoeller commented

If this is about exception handler methods on separate classes, there is indeed a precedence for earlier handlers, with any methods on those classes matching first... even if they match a cause while a later handler would match the root exception. This is arguably a feature, so we can't easily change this. Within exception handler methods on the same class, we do match the root exception first before even looking for causes.

Can you declare explicit ordering there? If not, maybe we could offer an explicit flag which would allow you to express your precedence rules?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 17, 2017

Franz Wong commented

I think I can put the exception handling methods into the same class. But I just wonder if it is the expected behaviour to match cause before the root exception, regardless it is difficult to change or not.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 17, 2017

Juergen Hoeller commented

Any reason why you couldn't declare your exception handler classes with a particular order? With ExceptionAHandler reliably coming first, it will always have the first chance to resolve ExceptionA in any of its exception handler methods... Alternatively, putting the related exception handler methods onto the same class will also work.

The algorithm is essentially as follows: Exception handler classes are checked in their declared order. For every such class, a given exception will be resolved against its exception handler methods, checking the root class as well as the cause. Only if an exception wasn't resolve against the current handler class at all, we'll move on to the next handler class in the chain, giving it the full root+cause matching opportunity.

So effectively, a cause match on a preceding handler class is rated higher than a root exception match on a lesser-ordered handler class. If there is no specific order, the order of handler class registration matters. Since exception handler classes may apply very different treatment to their matches, a cause match on a prioritized handler class may indeed be intentional. Separating exception handler methods onto dedicated handler classes with a particular order is a way to arrive at such an arrangement; if we treated all handler classes the same, there would be no way to express such override semantics even for cause matches.

All in all, even if this is arguably not obvious, the current behavior has its sweet spot. I'm sure there are plenty of applications relying on the current matching behavior in an ordered handler class arrangement, and I'm afraid we can't take this away from them. The best we can do is to properly document that behavior, so I'm taking the opportunity to turn this issue into a documentation task.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 18, 2017

Juergen Hoeller commented

I've covered this in the @ControllerAdvice javadoc and in the @ExceptionHandler chapter of the Web MVC reference docs.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 18, 2017

Franz Wong commented

Thanks for the update and effort :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.