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

ExceptionHandlerExceptionResolver advice applicability check may fail against interface-based controller proxy [SPR-16496] #21039

Closed
spring-issuemaster opened this issue Feb 14, 2018 · 5 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Feb 14, 2018

Vladislav Kisel opened SPR-16496 and commented

o.s.w.s.m.m.a.ExceptionHandlerExceptionResolver uses HandlerMethod#getBeanType during handler lookup. This method may return com.sun.proxy.Proxy when controller impl is a proxy.

Thus ControllerAdvice basePackages/basePackageClasses/assignableTypes are not working correctly, because ControllerAdviceBean#isApplicableToBeanType receives Proxy class as an argument.


Affects: 4.3.14, 5.0.3

Attachments:

Referenced from: pull request #1684

Backported to: 4.3.15

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Feb 14, 2018

Vladislav Kisel commented

Hi,
Im going to prepare a pull request soon

upd: #1684

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Feb 14, 2018

Juergen Hoeller commented

We essentially do the same for @RequestMapping methods in AbstractMethodHandlerMapping.detectHandlerMethods, not specifically handling interface-based proxies. What does your arrangement look like there? Could you simply expose your exception mapping annotations on the interface, like you have to do for your regular handler methods as well? It seems inconsistent to perform exception handler lookups on the target class while the regular mappings are obtained from the proxy (and have to be exposed on the proxy interface). Or am I missing something?

Granted, MVC controller beans with interface-based proxies are generally a rather unintuitive affair. I would rather recommend target-class proxies via CGLIB there.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Feb 14, 2018

Vladislav Kisel commented

Using cglib would help indeed. But assuming that I have to stick with dynamic proxy (which is my case), I think all handlers should not use getBeanType method. I've attached a sample project which reproduces the bug. Run a spring boot app and call /test uri. When using dynamic proxy config, default tomcat 403 is returned, after switching to cglib an error is catched by spring handler.

I will check requestMapping handler later, but I think it is working correctly somehow, otherwise our controllers wont be mapped at all

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Feb 15, 2018

Juergen Hoeller commented

I've addressed this through an AOP target class lookup for the controller advice applicability check, now consistently against the user-declared class even for an interface-based proxy. We're still performing local exception handler lookups on the proxy, just the advice applicability check on the underlying target class.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Feb 15, 2018

Vladislav Kisel commented

Thank you for fast fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.