-
Notifications
You must be signed in to change notification settings - Fork 37.7k
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
@RequestMapping-based controllers do not work for JDK proxies with annotated interfaces [SPR-5084] #9757
Comments
Bruno Navert commented Digging a little further, the above description is partially incorrect. The code in DefaultAnnotationHandlerMapping is correct if the URLs are defined at type level, since then it uses AnnotationUtils, and finds the annotation in the interface. It fails if the urls are defined at method level. When defined at type level, DefaultAnnotationHandlerMapping has the correct behaviour. However, it fails later on when DispatcherServlet calls AnnotationMethodHandlerAdapter.supports(handler), because it finds no supported handler methods. The problem is in the constructor of HandlerMethodResolver, which looks at the handler type for RequestMapping at method level. It doesn't find any, obviously, since the handler type is a proxy, and it does not look at interfaces either. So HandlerMethodResolver has the same problem, it doesn't detect the AnnotationUtils should have a method that does the interface lookup for methods as well as types. |
Grant Gochnauer commented I just ran into this issue with Spring 2.5.6 when trying to remove cglib from my classpath to reduce some memory issues we are having with it. I have some AOP advice applied to my controllers but I need to keep cglib in order to generate these proxies. This will be a welcome improvement |
Grant Gochnauer commented I'm hoping this doesn't get bumped from Spring 3.0 -- it's affecting our ability to completely remove cglib from our environment which is causing us to restart our production server after every redeployment. Thanks!! |
Arjen Poutsma commented This should work now. Note, however, that the annotations have to be on the interface, and cannot be on the implementing class. This is due to the fact that the AOP target might change at runtime, and it would be very inefficient to introspect the hander each time a web request comes in. |
Grant Gochnauer commented Thanks Arjen. Does this mean that this is already available in M4 as I haven't seen any new trunk commits lately? |
Arjen Poutsma commented No, it is not in M4; I committed it this morning. |
Andrew Ebaugh commented Will a similar fix help in the AnnotationMethodHandlerExceptionResolver, for dealing with the It seems related to #10627, which also does not pick up the |
Bruno Navert opened SPR-5084 and commented
When using JDK proxies (proxy-target-class=false), it is not possible to use
@RequestMapping
to automatically define controllers.This works very well if no proxying is involved. However, when AOP is used and a proxy of the controller is created (implementing the appropriate interface, where the interface has
@RequestMapping
at type and method level), then it fails.The code in DefaultAnnotationHandlerMapping properly finds
@RequestMapping
at type level in the interface (since it uses AnnotationUtils.findAnnotation), but then it looks for method-level annotations in DefaultAnnotationHandlerMapping.determineUrlsForHandlerMethods()This method only looks at the implementation class (the proxy), and finds no such annotation.
It should use the same kind of logic that AnnotationUtils does, and check the method declaration in the relevant interface(s) for any annotation there. Alternatively, perhaps declaring
@RequestMapping
as@Inheritable
would suffice? Not sure. In any case, it should be simple to find the interfaces, get the right method, and check for annotations there.The reason I prefer JDK proxies to CGLIB-based ones is because when we use CGLIB, it creates memory leaks when the application is reloaded in Tomcat, while JDK proxies don't have this problem.
Affects: 2.5.4
1 votes, 7 watchers
The text was updated successfully, but these errors were encountered: