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

BeanNameAutoProxyCreator proxies the wrong beans when custom TargetSourceCreator specified #24915

Closed
kennymacleod opened this issue Apr 16, 2020 · 6 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@kennymacleod
Copy link

Spring 5.1.8 (but also probably earlier versions)

Short version: if you use BeanNameAutoProxyCreator with a custom TargetSourceCreator, you'll get proxies created for all of your beans, not just the ones listed in the beanNames property.

Longer version: BeanNameAutoProxyCreator allows you to specify the beanNames for which you want proxies to be created. It also allows you to specify one of more TargetSourceCreators. However, the TargetSourceCreator will be asked to create TargetSource for every bean in the context, not just the beans listed in beanNames.

To reproduce the problem, I created a simple context with 2 beans and a unit test (see below). The context contains a BeanNameAutoProxyCreator and specifies that only beanA should be proxied. Both beanA and beanB are declared as lazy-init, and I've specified a LazyInitTargetSourceCreator to be used (the lazy-init-ness isn't particularly important, it's just an off-the-shelf implementation of TargetSourceCreator that demonstrates the problem).

The unit test asserts that BeanA is a proxy and beanB is not, but then fails because both beans have been proxied.

This is at the very least unexpected and confusing (and should be documented), and is probably a bug. The TargetSourceCreator is being asked for a TargetSource for every bean in the context, without checking for which beans should be candidates for being proxied.

<?xml version="1.0" encoding="utf-8"?>
<beans xmlns="http://www.springframework.org/schema/beans"
       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xsi:schemaLocation=" http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd">

    <bean id="beanA" class="test.BeanNameAutoProxyBugTest.ClassA" lazy-init="true"/>
    <bean id="beanB" class="test.BeanNameAutoProxyBugTest.ClassB" lazy-init="true"/>

    <bean class="org.springframework.aop.framework.autoproxy.BeanNameAutoProxyCreator">
        <property name="beanNames" value="beanA"/>
        <property name="customTargetSourceCreators">
            <bean class="org.springframework.aop.framework.autoproxy.target.LazyInitTargetSourceCreator"/>
        </property>
    </bean>
</beans>
package test;

import org.junit.Test;
import org.junit.runner.RunWith;
import org.springframework.aop.SpringProxy;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.TestExecutionListeners;
import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
import org.springframework.test.context.support.DependencyInjectionTestExecutionListener;

import javax.inject.Inject;

import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertThat;

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(locations = "/context.xml")
@TestExecutionListeners(DependencyInjectionTestExecutionListener.class)
public class BeanNameAutoProxyBugTest {

    @Inject
    ClassA beanA;

    @Inject
    ClassB beanB;

    @Test
    public void test() {
        assertThat("beanA should be proxied", beanA, instanceOf(SpringProxy.class));
        assertThat("beanB should not be proxied", beanB, not(instanceOf(SpringProxy.class)));
    }

    static class ClassA {
    }

    static class ClassB {
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 16, 2020
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 11, 2020
@sbrannen sbrannen added this to the 5.3 M1 milestone May 11, 2020
@sbrannen
Copy link
Member

Thanks for pointing this out.

In your particular example, the LazyInitTargetSourceCreator only returns a LazyInitTargetSource if the bean definition for a given bean is configured as lazy-init. Otherwise, LazyInitTargetSourceCreator would return null for the custom TargetSource and no proxy would be created based solely on the configuration of the custom TargetSourceCreator.

However, since beanB is configured with lazy-init="true", it also gets proxied even though it's not in the configured beanNames list.

That's the part that is counter-intuitive, and we'll address that in 5.3 by ensuring that a custom TargetSourceCreator is only applied to beans whose names match the configured beanNames list.

@sbrannen sbrannen self-assigned this May 11, 2020
@kennymacleod
Copy link
Author

Hi @sbrannen. The use of LazyInitTargetSourceCreator was just a convenient example, the problem occurs regardless of what the TargetSourceCreator does. If any TargetSourceCreator is specified, then the beanNames collection will be ignored and every bean will be passed to the TargetSourceCreator. That's more than just counterintuitive, surely, that's a bug, essentially that BeanNameAutoProxyCreator simply does not work as designed when used with a TargetSourceCreator.

@sbrannen
Copy link
Member

The use of LazyInitTargetSourceCreator was just a convenient example, the problem occurs regardless of what the TargetSourceCreator does. If any TargetSourceCreator is specified, then the beanNames collection will be ignored and every bean will be passed to the TargetSourceCreator.

Indeed. I was merely pointing out that a TargetSourceCreator can actively decide if it should create a TargetSource. The LazyInitTargetSourceCreator does actively decide, and yet it still gets applied when you wouldn't expect it to in the example you provided. So I think we're in agreement on that point. 😉

That's more than just counterintuitive, surely, that's a bug, essentially that BeanNameAutoProxyCreator simply does not work as designed when used with a TargetSourceCreator.

I think one can certainly view that as a bug in the sense that it was probably unintentional. However, this behavior has been in place for a long time, and people may have come to rely on this "accidental feature". That's why we have currently slated the fix for 5.3 in order to avoid breaking code relying on this in previous releases.

Speaking of which, are you hoping to see this fix applied to 5.2.7 or any earlier branches?

sbrannen added a commit to sbrannen/spring-framework that referenced this issue May 12, 2020
@sbrannen
Copy link
Member

The proposed fix can be viewed here: sbrannen@5eda469

@kennymacleod
Copy link
Author

Speaking of which, are you hoping to see this fix applied to 5.2.7 or any earlier branches?

That was my hope, but I appreciate that this is old code, old enough as you say that people my be relying on the "broken" behaviour, which makes it hard to back-port.

@sbrannen
Copy link
Member

Thanks for the feedback, @kennymacleod.

This has been addressed in 3c3e8e6 for Spring Framework 5.3.

sbrannen added a commit that referenced this issue May 12, 2020
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
Prior to this commit, if a BeanNameAutoProxyCreator was configured with
a custom TargetSourceCreator, the TargetSourceCreator was applied to
all beans in the ApplicationContext. Thus, the list of supported
beanNames was effectively ignored when applying any
TargetSourceCreator. Consequently, if a TargetSourceCreator returned a
non-null TargetSource for a given bean, the BeanNameAutoProxyCreator
proxied the bean even if the bean name had not been configured in the
beanNames list.

This commit addresses this issue by ensuring that a custom
TargetSourceCreator is only applied to beans whose names match the
configured beanNames list in a BeanNameAutoProxyCreator.

Closes spring-projectsgh-24915
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
kenny5he pushed a commit to kenny5he/spring-framework that referenced this issue Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants