MVC Controllers proxied with JDK Proxy (using <aop:aspectj-autoproxy/>) are not resolved properly [SPR-8353]

[spring-projects-issues]

Shai Yallin opened SPR-8353 and commented

When a @Controller implements a single interface, the aop:aspectj-autoproxy declaration automatically creates a JDK proxy for it, instead of a CGLib proxy. While this is mostly a desired behavior, i.e. when using AOP in a single system where the business logic is hidden behind an interface, it creates a problem if the controller implements a utility interface, such as ApplicationContextAware, or is a Scala class (all scala classes automatically extend the ScalaObject trait, which translates into an interface at the bytecode level).

Specifically, the problem is in AnnotationMethodHandlerAdapter#getMethodResolver, which calls ClassUtils.getUserClass(handler). This utility method does not check whether handler is a proxy. The solution should be simple - check for proxy and extract the target class, if needed.

Note that this prevents usage of Spring MVC Scala Controllers in an application where Spring AOP is also used, which is problematic to say the least.

---

Affects: 3.0.5

1 votes, 2 watchers

[spring-projects-issues]

Rossen Stoyanchev commented

Are the annotations on an interface or on the class? If they're on the interface, which they need to be, they should get discovered correctly.

[spring-projects-issues]

Shai Yallin commented

You haven't read my bug report. The interface is not a Controller interface - it's a utility interface, such as ApplicationContextAware, or even worse - the ScalaObject trait, which is automatically attached to all Scala classes. Only the class is annotated.

[spring-projects-issues]

Rossen Stoyanchev commented

The solution should be simple - check for proxy and extract the target class, if needed.

This is intentional. If the controller is proxied only interface methods are introspected. In the end the controller is invoked as a proxy and not the as the target class. Hence my question about where the annotations are.

... it's a utility interface, such as ApplicationContextAware, or even worse - the ScalaObject trait

What I don't quite understand yet is why you have a proxy around the controller? Are you actually applying any AOP logic to these interface methods?

[spring-projects-issues]

Shai Yallin commented

Again - let's assume that I have a controller written in Scala. The Scala compiler adds the ScalaObject trait to all Scala classes, so that in the bytecode level it looks like it implements the ScalaObject interface. Let's also assume that I have some AOP facility in place, like declarative transaction support, or (in our case) declarative logging. I would like Spring to create a CGLib proxy (use proxyTargetClass), because otherwise getMethodResolver() looks for request mappings only on the ScalaObject interface and not on my controller class. This makes usage of Scala controllers with aop:aspectj-autoproxy/ impossible, unless you specify proxyTargetClass=true, in which case stuff like autowiring stops working properly.

[spring-projects-issues]

Rossen Stoyanchev commented

This makes usage of Scala controllers with aop:aspectj-autoproxy/ impossible...

This is a rather sweeping statement. aop:aspectj-autoproxy enables use of AspectJ annotations. I assume you have a pointcut in one of those annotations that targets your controllers. If you don't want the controllers to be proxied, change your pointcut. Unless you have a reason for proxying them. Hence my question: do you have a reason for proxying the controllers?

If the answer is no, then make sure they're aren't proxied. Either by changing the pointucts or if you have two application contexts - root (via ContextLoaderListener) and a DispatcherServlet context, then you may be able to declare aop:aspectj-autoproxy/ and the @Aspect classes in the root context only. That would not proxy the controllers.

If the answer is yes, then you'll need a Controller interface with the annotations on it if using JDK dynamic proxies.

[spring-projects-issues]

Shai Yallin commented

I have a reason for proxying, I have a logging aspect. Also, as I said, "preset" aspects such as @Transactional is another case where the problem arises.

What I've been claiming for a month now is that adding a controller interface in order to overcome this issue IS NOT THE PROPER SOLUTION. It's a bad behavior, which I've already solved simply by always forcing @Controller classes to be proxied using CGLib, using a custom auto-proxy creator.

Please, think about this for a moment, I'm sure you'll realize that what you're suggesting is absurd; the whole point of @Controller classes is for them to be as simple as possible, and requiring the use to add an annotated interface just because he happens to be implementing a utility interface is just WRONG.

[spring-projects-issues]

Rossen Stoyanchev commented

Shai, first of all there is no need to uppercase to get your point across.

Secondly I didn't mean to suggest a Controller interface is the right solution. I only meant to say that when using JDK dynamic proxies, the annotations would need to be on an interface.

That said I am not familiar with the issue with cglib and autowiring. Is there a ticket opened for that? I have also asked Chris Beams to comment.

Last, personally I always recommend not using AOP where there are natural alternatives. If your main goal for controllers is logging why not use HandlerInterceptors? It's much simpler and the afterCompletion method is guaranteed to be invoked even in case of exceptions.

[spring-projects-issues]

Shai Yallin commented

I'm sorry for the uppercase, but I felt like you're not really trying to understand the issue...
As to the CGLib issue - I don't recall the exact use case, but sometimes, I think that mainly when you have two aspects around the same class, injection of autowired CGLib proxies fail, supposedly because the proxy subclass is not compatible with the expected class, for some reason.
As to the alternatives - I don't want to go into details but you'll have to trust me that we do indeed need AOP support for our logging concerns as well as other concerns.

My point, bottom line, is that you should support a mixed model where some classes are proxied using CGLib while others using JDK Proxy, which is actually very simple to implement, as I've already said.

[spring-projects-issues]

Chris Beams commented

Shai:

unless you specify proxyTargetClass=true, in which case stuff like autowiring stops working properly

Rossen:

That said I am not familiar with the issue with cglib and autowiring. Is there a ticket opened for that?

Shai:

As to the CGLib issue - I don't recall the exact use case, but sometimes, I think that mainly when you have two aspects around the same class, injection of autowired CGLib proxies fail, supposedly because the proxy subclass is not compatible with the expected class, for some reason.

I'd like to dig into the problem discussed above, because it sounds to me like the root cause here. Shai, your suggestion about detecting and unwrapping the proxy in AnnotationMethodHandlerAdapter may be an option, but also may have side effects. Setting proxyTargetClass=true is a simpler solution and one widely understood by existing users.

I am unfamiliar with the cglib/autowiring issue as described above. And a moderate amount of searching JIRA doesn't ring any bells either. Is this something you've personally experienced? Is it something you experience now in your application if you actually try it?

If so, let's shift gears and track it down. It would be extremely helpful if you could submit a small project that reproduces this behavior.

If not, perhaps the solution is as simple as switching proxyTargetClass to true and carrying on from there? i.e. the problem you describe may well exist, but if it does not in fact affect you, then you can at least move forward with your current effort.

In any case, your complaint makes sense and under no circumstances should a user be forced to implement an interface for a @Controller if it is not otherwise natural or desired to do so.

Thanks,

Chris

[spring-projects-issues]

Jun Yamog commented

Hi,

We have got a similar issue. In our case we are using scala + jersey + spring. What happens is that jersey scala uses the same ClassUtils.getUserClass method to register the resources as beans. Since we got an existing AOP for logging just as Shai has on his project.

We can then turn on proxyTargetClass=true however it seems some legacy java code are final classes. It would be great if we can just turn on proxyTargetClass for each specific config. However currently its not possible as per doc:

---

Multiple aop:config/ sections are collapsed into a single unified auto-proxy creator at runtime, which applies the strongest proxy settings that any of the aop:config/ sections (typically from different XML bean definition files) specified. This also applies to the tx:annotation-driven/ and aop:aspectj-autoproxy/ elements.

---

Is there other ways to turn on cglib for particular classes? As indicated by Shai above our jersey resources automatically implements an interface which is the scala object.

Or maybe get ClassUtils.getUserClass to unwrap the proxy object.

[spring-projects-issues]

Jun Yamog commented

I can seem to edit my previous comment.

I meant "What happens is that jersey SPRING uses the same ClassUtils.getUserClass method to register the resources as beans."

[spring-projects-issues]

Chris Beams commented

@Jun,

Would you be willing to put together a very simple reproduction project per the instructions at https://github.com/SpringSource/spring-framework-issues#readme? This will really help in getting to a solution quickly.

Thanks!