Spring Framework
  1. Spring Framework
  2. SPR-5084

@RequestMapping-based controllers do not work for JDK proxies with annotated interfaces

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.4
    • Fix Version/s: 3.0 RC1
    • Component/s: Web
    • Labels:
      None
    • Last commented by a User:
      true

      Description

      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.

        Activity

        Hide
        Bruno Navert added a comment -

        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 @RequestMapping at method level, so it can't find any handler methods, and the controller cannot be used. A similar fix should be made for both HandlerMethodResolver AND DefaultAnnotationHandlerMapping (when no type-level @RequestMapping is present).

        AnnotationUtils should have a method that does the interface lookup for methods as well as types.

        Show
        Bruno Navert added a comment - 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 @RequestMapping at method level, so it can't find any handler methods, and the controller cannot be used. A similar fix should be made for both HandlerMethodResolver AND DefaultAnnotationHandlerMapping (when no type-level @RequestMapping is present). AnnotationUtils should have a method that does the interface lookup for methods as well as types.
        Hide
        Grant Gochnauer added a comment -

        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

        Show
        Grant Gochnauer added a comment - 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
        Hide
        Grant Gochnauer added a comment -

        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!!

        Show
        Grant Gochnauer added a comment - 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!!
        Hide
        Arjen Poutsma added a comment -

        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.

        Show
        Arjen Poutsma added a comment - 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.
        Hide
        Grant Gochnauer added a comment -

        Thanks Arjen. Does this mean that this is already available in M4 as I haven't seen any new trunk commits lately?

        Show
        Grant Gochnauer added a comment - Thanks Arjen. Does this mean that this is already available in M4 as I haven't seen any new trunk commits lately?
        Hide
        Arjen Poutsma added a comment -

        No, it is not in M4; I committed it this morning.

        Show
        Arjen Poutsma added a comment - No, it is not in M4; I committed it this morning.
        Hide
        Andrew Ebaugh added a comment -

        Will a similar fix help in the AnnotationMethodHandlerExceptionResolver, for dealing with the @ExceptionHandler method-level annotation?

        It seems related to SPR-5959, which also does not pick up the @ExceptionHandler annotation (but that is logged for a CGLIB class proxy).

        Show
        Andrew Ebaugh added a comment - Will a similar fix help in the AnnotationMethodHandlerExceptionResolver, for dealing with the @ExceptionHandler method-level annotation? It seems related to SPR-5959 , which also does not pick up the @ExceptionHandler annotation (but that is logged for a CGLIB class proxy).

          People

          • Assignee:
            Arjen Poutsma
            Reporter:
            Bruno Navert
            Last updater:
            Trevor Marshall
          • Votes:
            1 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              4 years, 33 weeks, 5 days ago

              Time Tracking

              Estimated:
              Original Estimate - Not Specified
              Not Specified
              Remaining:
              Remaining Estimate - 0d
              0d
              Logged:
              Time Spent - 4d
              4d