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

JdkDynamicAopProxy constructs ReflectiveMethodInvocation with EmptyTargetSource.EMPTY_TARGET for static methods [SPR-15651] #20210

Closed
spring-issuemaster opened this issue Jun 10, 2017 · 3 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Jun 10, 2017

Rob Winch opened SPR-15651 and commented

The recent changes to EmptyTargetSource have broken Spring Security's tests.

Spring Security relies on checking to see if a MethodInvocation.getThis() is null to determine if the method is static. This seems to be a safe assumption based on the javadoc which states:

Returns:
the object (can be null if the accessible object is static).

What's more is it would seem non-obvious for me to use the EmptyTargetSource.EMPTY_TARGET (which is not accessible, so I need to use EmptyTargetSource.INSTANCE.getTarget() from Spring Framework to compare against the result of MethodInvocation.getThis() from aopalliance.

I think it makes sense to ensure that the construction of ReflectiveMethodInvocation contains target=null and targetClass=null for static methods.


Affects: 5.0 RC2

Referenced from: commits spring-projects/spring-framework-issues@8011c93

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 10, 2017

Juergen Hoeller commented

Good point, Rob. This change was largely meant to be internal since we're effectively just using such an EmptyTargetSource as a dummy, in particular for client-side remoting proxies where there is no "real" target object (it's all handled by the interceptor) for interface-defined service methods (which are of course non-static). A null target artificially complicates the contract and the proxy implementations there and arguably also gives a false indication of a static method.

In any case, as you said, we should fix this towards MethodInvocation.getThis() returning null for actual static invocations. How did you actually arrive that, since we're only really intercepting instance-based methods to begin with? Are you just validating against static methods there, or are you trying to do anything specific with them?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2017

Rob Winch commented

Juergen Hoeller I added a somewhat minimal sample for you in https://github.com/spring-projects/spring-framework-issues/tree/master/SPR-15651

The test does not pass when using the latest 5.0.0.BUILD-SNAPSHOT of Spring. If you change the version of Spring to 5.0.0.RC1 the build passes.

A relevant debug point is at AbstractFallbackMethodSecurityMetadataSource. You will note that targetClass is Object with the latest SNAPSHOT of Spring Framework, but null with previous versions.

This is due to the fact that in AbstractMethodSecurityMetadataSource obtains the targetClass from the MethodInvocation.getThis().

In master we currently have a workaround to ensure this works, but it is not idea.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 12, 2017

Juergen Hoeller commented

I've largely reverted the EmptyTargetSource change to return null from getTarget and therefore also from MethodInvocation.getThis(). This is not so much for actual static methods but rather for interceptor-based scenarios without an actual target (like in your HTTP Invoker case) where we need to retain compatibility with our previous MethodInvocation semantics.

Instead, CglibAopProxy defensively handles null targets in a consistent fashion now, only calling TargetSource.releaseTarget for non-null target objects on a non-static TargetSource.

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