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

Unwrap CGLIB proxy when invoking non-proxied methods in ReflectionTestUtils #33429

Closed
drachenpalme opened this issue Aug 26, 2024 · 4 comments
Closed
Assignees
Labels
in: test Issues in the test module type: enhancement A general enhancement
Milestone

Comments

@drachenpalme
Copy link

drachenpalme commented Aug 26, 2024

Overview

We are starting to use Spring AOP and have some issues with existing tests using ReflectionTestUtils.

In one test, a private method of spring-bean A is invoked via ReflectionTestUtils.invokeMethod. This method calls a method of an injected spring-bean B.

With spring-aop in use, this does not work anymore. CGlib will not provide a proxied version of the private method. Therefore the private method of the proxy is invoked, and the proxy does not have the DI-beans present and therefore we encounter a NullPointerException.

Now, my original assumption was that ReflectionTestUtils should handle this (as the setField/getField methods explicitly deal with Spring proxies).

My question is: Is my assumption wrong – and if so, how do we invoke private methods in test – or is this an omission in the ReflectionTestUtils?

Form a user's perspective I would have hoped that ReflectionTestUtils would at least handle the invocation of private methods (there should never be a reason to invoke private methods on a proxy, should there?).

I found issue #18622 which seems to be the point where AOP invocations in setField/getField were considered.

Related Issues

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 26, 2024
@sbrannen sbrannen added the in: test Issues in the test module label Aug 26, 2024
@sbrannen sbrannen changed the title ReflectionTestUtils and Spring-AOP ReflectionTestUtils and Spring-AOP Aug 26, 2024
@sbrannen sbrannen changed the title ReflectionTestUtils and Spring-AOP ReflectionTestUtils.invokeMethod() and Spring-AOP Aug 26, 2024
@sbrannen sbrannen changed the title ReflectionTestUtils.invokeMethod() and Spring-AOP ReflectionTestUtils.invokeMethod() and Spring AOP Aug 26, 2024
@sbrannen sbrannen self-assigned this Aug 26, 2024
@sbrannen sbrannen changed the title ReflectionTestUtils.invokeMethod() and Spring AOP ReflectionTestUtils should automatically unwrap proxies when invoking methods Aug 26, 2024
@sbrannen sbrannen changed the title ReflectionTestUtils should automatically unwrap proxies when invoking methods ReflectionTestUtils should automatically unwrap proxies when invoking private methods Aug 26, 2024
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Aug 26, 2024
@sbrannen sbrannen added this to the 6.2.0-RC1 milestone Aug 27, 2024
@sbrannen sbrannen added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Aug 27, 2024
@sbrannen
Copy link
Member

Hi @drachenpalme,

Thanks for raising the issue.

Now, my original assumption was that ReflectionTestUtils should handle this (as the setField/getField methods explicitly deal with Spring proxies).

My question is: Is my assumption wrong – and if so, how do we invoke private methods in test – or is this an omission in the ReflectionTestUtils?

ReflectionTestUtils does not currently handle that scenario, simply because no one ever asked for such support before, and we've never needed it in our own tests.

The way to achieve that now is by invoking AopTestUtils.getUltimateTargetObject() yourself and passing the result of that to ReflectionTestUtils. For example:

ReflectionTestUtils.invokeGetterMethod(AopTestUtils.getUltimateTargetObject(bean), "privateProperty");

Form a user's perspective I would have hoped that ReflectionTestUtils would at least handle the invocation of private methods (there should never be a reason to invoke private methods on a proxy, should there?).

That's correct: there should never be a reason to invoke a private method (or, more specifically, a non-proxied method) on a CGLIB proxy.

I found issue #18622 which seems to be the point where AOP invocations in setField/getField were considered.

That's correct. We'll do something similar for method invocations with ReflectionTestUtils in Spring Framework 6.2, but we'll limit that to non-proxied methods invoked on a CGLIB proxy.

In the interim, please use AopTestUtils.getUltimateTargetObject() manually as mentioned above.

@sbrannen sbrannen changed the title ReflectionTestUtils should automatically unwrap proxies when invoking private methods Unwrap CGLIB proxy when invoking non-proxied methods in ReflectionTestUtils Aug 28, 2024
@drachenpalme
Copy link
Author

Hi @sbrannen ,

thanks a lot for the quick reaction.
Should anyone else stumble upon this: As there are a lot of places in our code, which are affected by this, we choose to create a drop-in replacement for the ReflectionTestUtils in a different namespace which anticipates your change - so we can simply exchange imports application-wide.

After some further digging: invokeGetterMethod and invokeSetterMethod do seem to have the same problem. We are not affected by this (as we do not use them), yet, consistency-wise, would it be a good idea to adapt them as well?

@sbrannen
Copy link
Member

thanks a lot for the quick reaction.

You're welcome.

Should anyone else stumble upon this: As there are a lot of places in our code, which are affected by this, we choose to create a drop-in replacement for the ReflectionTestUtils in a different namespace which anticipates your change - so we can simply exchange imports application-wide.

Sounds like a good plan.

After some further digging: invokeGetterMethod and invokeSetterMethod do seem to have the same problem. We are not affected by this (as we do not use them), yet, consistency-wise, would it be a good idea to adapt them as well?

Yes, I have already added the new support to the following locally.

  • org.springframework.test.util.ReflectionTestUtils.invokeSetterMethod(Object, String, Object, Class<?>)
  • org.springframework.test.util.ReflectionTestUtils.invokeGetterMethod(Object, String)
  • org.springframework.test.util.ReflectionTestUtils.invokeMethod(Object, Class<?>, String, Object...)

@sbrannen
Copy link
Member

This has been addressed in main for inclusion in Spring Framework 6.2 RC1.

For details, see commit 38a56b3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants
@sbrannen @spring-projects-issues @drachenpalme and others