Ensure CGLIB proxy is created for mixin with IntroductionInterceptor

[BinaryOracle]

Resolve the issue of creating mixin classes using IntroductionInterceptor resulting in dynamic proxies instead of CGLIB proxies ;

Corresponding issue address: #31304

[pivotal-cla]

@BinaryOracle Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@BinaryOracle Thank you for signing the Contributor License Agreement!

[sbrannen]

Please introduce a test case that fails before the proposed changes and passes after the changes.

Thanks

[BinaryOracle]

@sbrannen
The following is a previously failed test case :

import org.springframework.aop.framework.ProxyFactory;
import org.springframework.aop.support.DelegatingIntroductionInterceptor;

public class TestMain {
    public static void main(String[] args) {
        People peo = new People();
        ProxyFactory pf = new ProxyFactory();
        DelegatingIntroductionInterceptor dii = new DelegatingIntroductionInterceptor((Developer) () -> System.out.println("Coding"));
        pf.addAdvice(dii);
        pf.setTarget(peo);
        peo = (People) pf.getProxy();
        peo.drink();
        peo.eat();
        Developer developer = (Developer) peo;
        developer.code();
    }

    public static class People {
        void eat() {
            System.out.println("eat");
        }

        void drink() {
            System.out.println("drink");
        }
    }

    public interface Developer {
        void code();
    }
}

The exception result thrown before the issue was resolved：

Exception in thread "main" java.lang.ClassCastException: class com.sun.proxy.$Proxy0 cannot be cast to class com.spring.TestMain$People (com.sun.proxy.$Proxy0 and com.spring.TestMain$People are in unnamed module of loader 'app')
	at com.spring.TestMain.main(TestMain.java:13)

The modified implementation of the DefaultAopProxyFactory class results in the expected output:

drink
eat
Coding

[sbrannen]

The following is a previously failed test case :

Please introduce an equivalent of that in our test suite in the form of a JUnit Jupiter based test.

If you need help determining where such a test should reside, let us know.

[BinaryOracle]

@sbrannen

Sure, thank you. It's getting late today, but I will submit a more comprehensive set of test cases by tomorrow evening.

[BinaryOracle]

@sbrannen

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.springframework.aop.framework.ProxyFactory;
import org.springframework.aop.support.AopUtils;
import org.springframework.aop.support.DelegatePerTargetObjectIntroductionInterceptor;
import org.springframework.aop.support.DelegatingIntroductionInterceptor;

/**
 * @author 占道宏
 * @create 2023/10/10 9:59
 */
public class ProxyFactoryTests {

    /**
     * The target object does not implement any interfaces, and in this case, you want to use CGLIB for dynamic proxying.
     */
    @Test
    public void testDelegatingIntroductionInterceptorWithoutInterface() {
        People peo = new People();
        ProxyFactory pf = new ProxyFactory();
        DelegatingIntroductionInterceptor dii = new DelegatingIntroductionInterceptor((Developer) () -> System.out.println("Coding"));
        pf.addAdvice(dii);
        pf.setTarget(peo);

        Object proxy = pf.getProxy();
        Assertions.assertTrue(AopUtils.isCglibProxy(proxy));
        Assertions.assertTrue(proxy instanceof People);
        Assertions.assertTrue(proxy instanceof Developer);

        People people = (People) proxy;
        Assertions.assertDoesNotThrow(people::eat);

        Developer developer = (Developer) proxy;
        Assertions.assertDoesNotThrow(developer::code);
    }

    /**
     * The target object implements the Teacher interface, and in this case, you want to use JDK for dynamic proxying
     */
    @Test
    public void testDelegatingIntroductionInterceptorWithInterface() {
        Teacher teacher = () -> System.out.println("teach");
        ProxyFactory pf = new ProxyFactory();
        DelegatingIntroductionInterceptor dii = new DelegatingIntroductionInterceptor((Developer) () -> System.out.println("Coding"));
        pf.addAdvice(dii);
        pf.addInterface(Teacher.class);
        pf.setTarget(teacher);

        Object proxy = pf.getProxy();
        Assertions.assertTrue(AopUtils.isJdkDynamicProxy(proxy));
        Assertions.assertTrue(proxy instanceof Teacher);
        Assertions.assertTrue(proxy instanceof Developer);

        Teacher teacher1 = (Teacher) proxy;
        Assertions.assertDoesNotThrow(teacher1::teach);

        Developer developer = (Developer) proxy;
        Assertions.assertDoesNotThrow(developer::code);
    }

    /**
     * The target object does not implement any interfaces, and in this case, you want to use CGLIB for dynamic proxying.
     */
    @Test
    public void testDelegatePerTargetObjectIntroductionInterceptorWithoutInterface() {
        People peo = new People();
        ProxyFactory pf = new ProxyFactory();
        DelegatePerTargetObjectIntroductionInterceptor dii = new DelegatePerTargetObjectIntroductionInterceptor(DeveloperImpl.class, Developer.class);
        pf.addAdvice(dii);
        pf.setTarget(peo);

        Object proxy = pf.getProxy();
        Assertions.assertTrue(AopUtils.isCglibProxy(proxy));
        Assertions.assertTrue(proxy instanceof People);
        Assertions.assertTrue(proxy instanceof Developer);

        People people = (People) proxy;
        Assertions.assertDoesNotThrow(people::eat);

        Developer developer = (Developer) proxy;
        Assertions.assertDoesNotThrow(developer::code);
    }

    /**
     * The target object implements the Teacher interface, and in this case, you want to use JDK for dynamic proxying
     */
    @Test
    public void testDelegatePerTargetObjectIntroductionInterceptorWithInterface() {
        Teacher teacher = () -> System.out.println("teach");
        ProxyFactory pf = new ProxyFactory();
        DelegatePerTargetObjectIntroductionInterceptor dii = new DelegatePerTargetObjectIntroductionInterceptor(DeveloperImpl.class, Developer.class);
        pf.addAdvice(dii);
        pf.addInterface(Teacher.class);
        pf.setTarget(teacher);

        Object proxy = pf.getProxy();
        Assertions.assertTrue(AopUtils.isJdkDynamicProxy(proxy));
        Assertions.assertTrue(proxy instanceof Teacher);
        Assertions.assertTrue(proxy instanceof Developer);

        Teacher teacher1 = (Teacher) proxy;
        Assertions.assertDoesNotThrow(teacher1::teach);

        Developer developer = (Developer) proxy;
        Assertions.assertDoesNotThrow(developer::code);
    }

    /**
     * The target object does not implement any interfaces, so it is necessary to use CGLIB for proxying
     */
    @Test
    public void testProxyFactoryWithoutInterface() {
        People people = new People();
        ProxyFactory pf = new ProxyFactory();
        pf.setTarget(people);
        Object proxy = pf.getProxy();

        Assertions.assertTrue(AopUtils.isCglibProxy(proxy));
        Assertions.assertTrue(proxy instanceof People);
        Assertions.assertDoesNotThrow(((People)proxy)::eat);

        pf.addInterface(Teacher.class);
        proxy = pf.getProxy();
        Assertions.assertTrue(AopUtils.isCglibProxy(proxy));
        Assertions.assertTrue(proxy instanceof Teacher);
        Assertions.assertTrue(proxy instanceof People);
        Assertions.assertDoesNotThrow(((People)proxy)::eat);
    }

    /**
     * When the target object implements the Teacher interface
     * but we have not explicitly called the addInterface method,
     * we expect to use CGLIB; however, after calling it, we expect to use JDK
     */
    @Test
    public void testProxyFactoryWithInterface() {
        Teacher teacher = () -> System.out.println("teach");
        ProxyFactory pf = new ProxyFactory();
        pf.setTarget(teacher);
        Object proxy = pf.getProxy();

        Assertions.assertTrue(AopUtils.isCglibProxy(proxy));
        Assertions.assertTrue(proxy instanceof Teacher);
        Assertions.assertDoesNotThrow(((Teacher)proxy)::teach);

        pf.addInterface(Teacher.class);
        proxy = pf.getProxy();
        Assertions.assertTrue(AopUtils.isJdkDynamicProxy(proxy));
        Assertions.assertTrue(proxy instanceof Teacher);
        Assertions.assertDoesNotThrow(((Teacher)proxy)::teach);
    }

    public static class People {
        void eat() {
            System.out.println("eat");
        }
    }

    public interface Teacher {
        void teach();
    }

    public interface Developer {
        void code();
    }

    public static class DeveloperImpl implements Developer {
        @Override
        public void code() {
            System.out.println("Coding");
        }
    }
}

Before the issue was corrected, only three out of the six test cases could pass. The specific results were as follows:

success:
testDelegatingIntroductionInterceptorWithInterface  
testDelegatePerTargetObjectIntroductionInterceptorWithInterface
testProxyFactoryWithInterface

failure:
testDelegatingIntroductionInterceptorWithoutInterface
testDelegatePerTargetObjectIntroductionInterceptorWithoutInterface
testProxyFactoryWithoutInterface

After the issue was corrected, all test cases passed successfully

[snicoll]

@BinaryOracle the tests need to be added to the PR, not as a comment here. Can you please upgrade the PR? You can push more commits to your branch.

[BinaryOracle]

@snicoll Okay, I'll go update it right away

[sbrannen]

Thanks for supplying the tests!

I've requested a few minor changes.

[sbrannen]

Suggested change

	private boolean hashNoInterfaceImplement(org.springframework.aop.framework.AdvisedSupport config) {
	Class<?> targetClass = config.getTargetClass();
	if (targetClass == null) return false;
	return targetClass.getInterfaces().length == 0;
	}
	private boolean targetDoesNotImplementInterfaces(AdvisedSupport config) {
	Class<?> targetClass = config.getTargetClass();
	return (targetClass != null ? targetClass.getInterfaces().length == 0 : false);
	}

Let's revise this method like this.

Also, please add Javadoc for this method, similar to the Javadoc for hasNoUserSuppliedProxyInterfaces(...).

[sbrannen]

Suggested change

	* @author 占道宏
	* @create 2023/10/10 9:59
	* @author 占道宏
	* @since 6.1

If possible, please provide your full name using the Latin alphabet for the @author tag.

[sbrannen]

Please use AssertJ for assertions.

The build will fail if you attempt to use JUnit Jupiter's Assertions class.

Speaking of which, please run a full build locally before submitting a PR (./gradlew check) to ensure that the build succeeds.

[sbrannen]

Instead of asserting that the code does not throw an exception, please change the void methods in your test fixture to return a String whose value can be asserted.

[sbrannen]

Same as above.

[sbrannen]

Same as above.

[sbrannen]

We do not use System.out.println() in tests.

Please change the void methods to return the String instead.

[jhoeller]

I'm addressing this in a slightly different way for 6.2, reopening #31304 for that purpose.