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

ReflectionTestUtils should automatically unwrap proxies [SPR-14050] #18622

Closed
spring-projects-issues opened this issue Mar 14, 2016 · 7 comments
Closed
Assignees
Labels
in: test type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 14, 2016

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 #1006

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 14, 2016

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.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 15, 2016

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?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 15, 2016

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

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 16, 2016

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.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 16, 2016

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. ;-)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 16, 2016

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!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 16, 2016

Sam Brannen commented

Merged into master in the following commit:

9439a00

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

No branches or pull requests

2 participants