Marten Deinum opened SPR-14050 and commented

When using Spring AOP and in a test case you use ReflectionTestUtils.setField, this fails when using (class-based) proxies. Specifically, the value is set in the proxy itself rather than in the underlying instance.

Currently one would first have to call AopTestUtils.getUltimateTargetObject and then pass that result to the ReflectionTestUtils.setField method.

Object actualTarget = AopTestUtils.getUltimateTargetObject(injectedProxy);
ReflectionTestUtils.setField(actualTarget, "field", "new-value");

Arguably calling ReflectionTestUtils.setField on a proxied class isn't the best practice, but it would be nice if the call to AopTestUtils.getUltimateTargetObject could be embedded in the ReflectionTestUtils class.

Affects: 4.2.5

Referenced from: pull request https://github.com/spring-projects/spring-framework/pull/1006

0 votes, 5 watchers

Comment From: spring-projects-issues

Marten Deinum commented

I've created a pull request for this.

In the pull request the targetObject is unwrapped when using setField or getField the other methods for invoking methods on the object are left untouched.

Comment From: spring-projects-issues

Sam Brannen commented

Hi Marten Deinum,

Thanks for the PR.

I've fixed the bugs in the code and polished everything. You can see the results in my branch:

https://github.com/sbrannen/spring-framework/commits/SPR-14050

However, after having reviewed the work, I'm not sure that we should be making these changes to the existing methods by default. The reason is that it will break any existing code that expects the class-based proxy to return values set via ReflectionTestUtils.

Thus, I'm leaning toward not implementing this at all, or introducing overloaded versions of the affected methods that accept a boolean flag to turn on this feature.

Thoughts?

Comment From: spring-projects-issues

Juergen Hoeller commented

Hmm, is there a valid case for storing values in a CGLIB proxy instance? Feels like a mistake in any sane scenario. From that perspective, always passing values through to the actual target object doesn't seem so wrong.

That said, I'm not sure ReflectionTestUtils has to do this. After all, user code can also do this manually, and it's just about test setups.

Juergen

Comment From: spring-projects-issues

Marten Deinum commented

The main pain point is that (and I've had that on a couple of projects now) is that when you have working testcases and introduce a mechanism that introduces a proxy, for instance enabling transactions or async methods on that class. All of a sudden tests start to break, because the values aren't set, because they are now set on the proxy. And I have to agree with Juergen on this that that feels like a mistake.

It comes as a surprise to people that tests suddenly start failing.

Agree it isn't very hard to solve but it still is surpising and people not aware of the fact that proxies are in play have a hard time figuring out why this happens.

I guess it should be at least documented if it isn't going to be resolved and the overloaded methods don't seem like a bad choice to have as that makes it more explicit that proxies might be playing a part.

Comment From: spring-projects-issues

Sam Brannen commented

Hmm, is there a valid case for storing values in a CGLIB proxy instance?

Yes, in fact, there is.

Consider the current test for this new functionality:

@Test
public void setFieldAndGetFieldOnCglibProxiedInstance() throws Exception {
    ProxyFactory pf = new ProxyFactory(this.person);
    pf.setProxyTargetClass(true);
    Person proxyPerson = (Person) pf.getProxy();

    // Set reflectively via Proxy
    setField(proxyPerson, "name", "Tom");

    // Get directly from Target via getter methods
    assertEquals("name (protected field)", "Tom", this.person.getName());

    // This FAILS!
    assertEquals("name (protected field)", "Tom", proxyPerson.getName());

    // Get reflectively via Proxy
    assertEquals("Tom", getField(proxyPerson, "name"));
}

Note that this.person.getName() properly returns "Tom" as expected. However... an invocation of proxyPerson.getName() returns null.

Let's assume that proxyPerson is a Spring-managed bean with transactional support applied via AOP. In such a scenario, any component that has the proxyPerson injected as a dependency would expect invocations of this.person.getName() and proxyPerson.getName() to both return the same thing. In the very least, I would expect that clients of the proxy would see the changed state in the target; otherwise, what is the point in changing the state of the target if clients of the proxy never see the affected state?

Marten Deinum, can you please expound on your use case? Specifically, how does the functionality proposed in this issue help your tests to pass? Furthermore, doesn't the proposed functionality actually cause other tests to fail? Or do you not have any tests that interact with the target via the proxy after using reflection to change state in the target?

Juergen Hoeller, I'm looking forward to your feedback, too. ;-)

Comment From: spring-projects-issues

Sam Brannen commented

Yes, in fact, there is.

Well, let me rephrase that...

The only valid case is if the getter methods for such fields are final in the class being proxied (which is the case for the Person class in the tests). In such scenarios, the CGLIB proxy cannot override the getter methods, and that's why the aforementioned test returns null via the proxy... since getName() can only see the state of the proxy and not the target's state.

Marten Deinum, with that in mind, I don't think I need any answers to my previous questions. ;)

In summary, people just have to make sure that their getter methods are not final if they are using CGLIB proxies!

Comment From: spring-projects-issues

Sam Brannen commented

Merged into master in the following commit:

https://github.com/spring-projects/spring-framework/commit/9439a008d7a4ec7359fc024fce60453995f2ebd1