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

Ensure CGLIB proxy is created for mixin with IntroductionInterceptor #31389

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -59,7 +59,7 @@ public class DefaultAopProxyFactory implements AopProxyFactory, Serializable {

@Override
public AopProxy createAopProxy(AdvisedSupport config) throws AopConfigException {
if (config.isOptimize() || config.isProxyTargetClass() || hasNoUserSuppliedProxyInterfaces(config)) {
if (config.isOptimize() || config.isProxyTargetClass() || hashNoInterfaceImplement(config) || hasNoUserSuppliedProxyInterfaces(config)) {
Class<?> targetClass = config.getTargetClass();
if (targetClass == null) {
throw new AopConfigException("TargetSource cannot determine target class: " +
Expand All @@ -75,6 +75,12 @@ public AopProxy createAopProxy(AdvisedSupport config) throws AopConfigException
}
}

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

Comment on lines +78 to +83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* Determine whether the supplied {@link AdvisedSupport} has only the
* {@link org.springframework.aop.SpringProxy} interface specified
Expand Down
@@ -0,0 +1,175 @@
package java.org.springframework.aop.framework;

import org.junit.jupiter.api.Assertions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Comment on lines +11 to +12
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
public class ProxyFactoryWithIntroductionInterceptorTests {

/**
* 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

}

/**
* 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

}

public static class People {
void eat() {
System.out.println("eat");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please change the void methods to return the String instead.

}
}

public interface Teacher {
void teach();
}

public interface Developer {
void code();
}

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