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

Mixin created with IntroductionInterceptor results in dynamic proxy instead of CGLIB proxy #31304

Closed
BinaryOracle opened this issue Sep 24, 2023 · 4 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@BinaryOracle
Copy link

BinaryOracle commented Sep 24, 2023

public class TestMain {
    public static void main(String[] args) {
        // 1. Prepare the target object to be proxied.
        People peo = new People();
        // 2. Prepare the proxy factory.
        ProxyFactory pf = new ProxyFactory();
        // 3. Prepare the introduction advice. The advice holds the additional interface Developer and its implementation.
        DelegatingIntroductionInterceptor dii = new DelegatingIntroductionInterceptor((Developer) () -> System.out.println("Coding"));
        // 4. Add the advice and the interfaces that the proxy object should inherit.
        pf.addAdvice(dii);
        // 5. Set the target object to be proxied.
        pf.setTarget(peo);
        // 6. Type casting here will fail because the proxy object uses JDK dynamic proxying, which only implements the Developer interface and Spring AOP internal interfaces.
        // It should ideally use CGLIB proxying instead!!!
        peo = (People) pf.getProxy();
        peo.drink();
        peo.eat();
        // 7. Forced casting to the Developer interface. The actual method calls will be intercepted by the introduction advice, and the request will be forwarded to the Developer interface implementation held by the advice.
        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 result of running the code is:

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:20)

The original expectation here was for the proxy object to be able to use Cglib for proxying because the target object did not implement any interfaces. However, this situation occurred due to a special handling of IntroductionAdvisor within the ProxyFactory. It added all the interfaces provided by IntroductionAdvisor to the AdvisedSupport's interface collection. Consequently, when DefaultAopProxyFactory eventually executed the proxying process, it chose to use JDK dynamic proxy instead of Cglib.

As a result, the proxy object we obtain is actually implemented using JDK dynamic proxy. It implements internal Spring AOP module-related interfaces and the Developer interface. When we forcefully attempt to cast the proxy object to the People type, a type casting exception is thrown.

The Spring AOP module version is: 5.3.9

image

Reason:

When AdvisedSupport adds advice, it specifically handles Advice of the IntroductionInfo type and adds the interfaces implemented by it to the interfaces collection.

@Override
	public void addAdvice(Advice advice) throws AopConfigException {
		int pos = this.advisors.size();
		addAdvice(pos, advice);
	}

	@Override
	public void addAdvice(int pos, Advice advice) throws AopConfigException {
		Assert.notNull(advice, "Advice must not be null");
		if (advice instanceof IntroductionInfo) {
			// We don't need an IntroductionAdvisor for this kind of introduction:
			// It's fully self-describing.
			addAdvisor(pos, new DefaultIntroductionAdvisor(advice, (IntroductionInfo) advice));
		}
		...
	}

	@Override
	public void addAdvisor(int pos, Advisor advisor) throws AopConfigException {
		if (advisor instanceof IntroductionAdvisor) {
			validateIntroductionAdvisor((IntroductionAdvisor) advisor);
		}
		addAdvisorInternal(pos, advisor);
	}

	private void validateIntroductionAdvisor(IntroductionAdvisor advisor) {
		advisor.validateInterfaces();
		// If the advisor passed validation, we can make the change.
		Class<?>[] ifcs = advisor.getInterfaces();
		for (Class<?> ifc : ifcs) {
			addInterface(ifc);
		}
	}

At this point, even if the target object does not implement any interfaces, the interfaces collection will not be empty :

	private List<Class<?>> interfaces = new ArrayList<>();

This results in DefaultAopProxyFactory making an incorrect choice between using JDK or CGLIB for dynamic proxying. It chooses JDK instead of CGLIB for dynamic proxying, even when the target object does not implement any interfaces. Consequently, the resulting proxy object cannot be cast to the target object type, which is not in line with our expected behavior.

@Override
	public AopProxy createAopProxy(AdvisedSupport config) throws AopConfigException {
		if (!NativeDetector.inNativeImage() &&
				(config.isOptimize() || config.isProxyTargetClass() || hasNoUserSuppliedProxyInterfaces(config))) {
			Class<?> targetClass = config.getTargetClass();
			if (targetClass == null) {
				throw new AopConfigException("TargetSource cannot determine target class: " +
						"Either an interface or a target is required for proxy creation.");
			}
			if (targetClass.isInterface() || Proxy.isProxyClass(targetClass)) {
				return new JdkDynamicAopProxy(config);
			}
			return new ObjenesisCglibAopProxy(config);
		}
		else {
			return new JdkDynamicAopProxy(config);
		}
	}
    
    // The `interfaces` collection is not empty at this point, so JDK dynamic proxying is used.
	private boolean hasNoUserSuppliedProxyInterfaces(AdvisedSupport config) {
		Class<?>[] ifcs = config.getProxiedInterfaces();
		return (ifcs.length == 0 || (ifcs.length == 1 && SpringProxy.class.isAssignableFrom(ifcs[0])));
	}

I'm not sure if this can be considered a bug. If possible, I would prefer that in this case, the IntroductionAdvisor's additional interface list be handled separately to avoid choosing JDK dynamic proxying when the target object does not implement interfaces.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 24, 2023
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Sep 26, 2023
@sbrannen
Copy link
Member

sbrannen commented Sep 26, 2023

As a workaround, you can invoke pf.setProxyTargetClass(true); before invoking getProxy(), and that will force the creation of a CGLIB-based proxy, which allows your sample application to run as expected.

I'm not sure if this can be considered a bug. If possible, I would prefer that in this case, the IntroductionAdvisor's additional interface list be handled separately to avoid choosing JDK dynamic proxying when the target object does not implement interfaces.

I can certainly understand the desire for that.

We'll have to investigate what options we have to improve that.

In the interim, I've added this to the general backlog.

@sbrannen sbrannen changed the title A bug related to IntroductionAdvisor Mixin created with IntroductionInterceptor results in dynamic proxy instead of CGLIB proxy Sep 26, 2023
@sbrannen sbrannen added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 26, 2023
@sbrannen sbrannen added this to the General Backlog milestone Sep 26, 2023
@BinaryOracle
Copy link
Author

BinaryOracle commented Oct 9, 2023

I've been quite busy recently, so I apologize for not getting back to you sooner. Regarding this issue, I think the solution is relatively straightforward, but I'm not sure if there might be any problems with the solution I'm about to suggest. Here are some of my thoughts:

When we invoke the setTarget method of the ProxyFactory and specify the target object to be proxied, it doesn't automatically check whether the target object implements an interface. In this case, it defaults to using CGLIB for dynamic proxy generation. Only if we manually call addInterface to add the interfaces implemented by the target object will it use JDK dynamic proxy.

public class TestMain {
    public static void main(String[] args) {
        Stu s = () -> System.out.println("Student");
        ProxyFactory pf = new ProxyFactory();
        pf.setTarget(s);
        // Call addInterface actively for ProxyFactory to use JDK dynamic proxy
        pf.addInterface(Stu.class);
        Stu stu = (Stu) pf.getProxy();
        stu.study();
    }

    public interface Stu {
        void study();
    }
}

image

image

In the scenario illustrated above, manually invoking the addInterface method to add an interface may not pose a significant issue. However, in the following scenario, there are potential problems:

public class TestMain {
    public static void main(String[] args) {
        Stu s = new Stu();
        ProxyFactory pf = new ProxyFactory();
        pf.setTarget(s);
        pf.addInterface(Boy.class);
        Object proxy = pf.getProxy();
        Stu stu = (Stu) proxy;
        stu.study();
    }

    public static class Stu {
        void study() {
            System.out.println("study");
        }
    }

    public interface Boy {
        void boy();
    }
}

In this case, a type casting exception will be thrown because JDK dynamic proxy is ultimately used. The proxy object only implements Spring AOP internal interfaces in addition to the Boy interface. As a result, the proxy object cannot be cast to the Stu type.

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

image

I believe that a proxy object is meant to enhance the behavior of the target object and should be capable of being cast to either the type of the target object itself or to one of its inherited interface types. However, in this scenario, the proxy object cannot be cast to the type of the target object, which goes against its original purpose.

The issue mentioned earlier, which arises due to the addition of extra interfaces through the addInterface method indirectly called by DelegatingIntroductionInterceptor, can be addressed by considering to check whether the target object implements any interfaces in the createAopProxy method of the DefaultAopProxyFactory class. If it doesn't implement any interfaces, then you can opt for CGLIB for dynamic proxy generation:

public class CustomDefaultAopProxyFactory implements AopProxyFactory, Serializable {

    @Override
    public AopProxy createAopProxy(AdvisedSupport config) throws AopConfigException {
        if (!NativeDetector.inNativeImage() &&
                (config.isOptimize() || config.isProxyTargetClass() ||
                // Modification: If the target object doesn't implement any interfaces, still use CGLIB for dynamic proxy
                hasNoInterfaceImplement(config) || hasNoUserSuppliedProxyInterfaces(config))) {
            Class<?> targetClass = config.getTargetClass();
            if (targetClass == null) {
                throw new AopConfigException("TargetSource cannot determine target class: " +
                        "Either an interface or a target is required for proxy creation.");
            }
            // This ensures that if targetClass is an interface, JDK dynamic proxy is still used
            if (targetClass.isInterface() || Proxy.isProxyClass(targetClass)) {
                return new JdkDynamicAopProxy(config);
            }
            return new ObjenesisCglibAopProxy(config);
        } else {
            return new JdkDynamicAopProxy(config);
        }
    }
    
    // New Logic !!!
    private boolean hasNoInterfaceImplement(AdvisedSupport config) {
        Class<?> targetClass = config.getTargetClass();
        if (targetClass == null) return false;
        return targetClass.getInterfaces().length == 0;
    }

    private boolean hasNoUserSuppliedProxyInterfaces(AdvisedSupport config) {
        Class<?>[] ifcs = config.getProxiedInterfaces();
        return (ifcs.length == 0 || (ifcs.length == 1 && SpringProxy.class.isAssignableFrom(ifcs[0])));
    }
}

Replace the default implementation of DefaultAopProxyFactory with custom implementation, and then continue running the tests:

public class TestMain {
    public static void main(String[] args) {
        Stu s = new Stu();
        ProxyFactory pf = new ProxyFactory();
        // Use a custom dynamic proxy creation factory
        pf.setAopProxyFactory(new CustomDefaultAopProxyFactory());
        pf.setTarget(s);
        pf.addInterface(Boy.class);
        Object proxy = pf.getProxy();
        Stu stu = (Stu) proxy;
        stu.study();
        // Output the interfaces implemented by the current proxy object
        for (Class<?> anInterface : proxy.getClass().getInterfaces()) {
            System.out.println(anInterface);
        }
    }

    public static class Stu {
        void study() {
            System.out.println("study");
        }
    }

    public interface Boy {
        void boy();
    }
}

The output result:

study
interface com.spring.TestMain$Boy
interface org.springframework.aop.SpringProxy
interface org.springframework.aop.framework.Advised
interface org.springframework.cglib.proxy.Factory

Execute the test for the problem scenario provided initially:

public class TestMain {
    public static void main(String[] args) {
        People peo = new People();
        ProxyFactory pf = new ProxyFactory();
        // Initial test case, replace the default proxy creation factory implementation here
        pf.setAopProxyFactory(new CustomDefaultAopProxyFactory());
        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 output result:

drink
eat
Coding

@sbrannen
Copy link
Member

sbrannen commented Oct 9, 2023

Hi @BinaryOracle,

Thanks for the feedback and proposal.

If I understand you correctly, you are proposing that we introduce a method like your hasNoInterfaceImplement(...) to address the issue.

If so, feel free to open a PR that introduces this method along with a test case that fails before the addition and passes after the addition.

Cheers,

Sam

p.s. whenever possible, please refrain from including screenshots in issues. If you would like to point to a specific section of the code, please include a GitHuB perma-link to the lines of the file in question.

@BinaryOracle
Copy link
Author

@sbrannen

Thank you for your feedback. Below is the preliminary solution I have provided. Here is the link to the PR:

#31389

@jhoeller jhoeller added the status: superseded An issue that has been superseded by another label Jan 12, 2024
@jhoeller jhoeller removed this from the General Backlog milestone Jan 12, 2024
@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Jan 12, 2024
@jhoeller jhoeller reopened this Sep 26, 2024
@jhoeller jhoeller added type: enhancement A general enhancement and removed type: bug A general bug status: superseded An issue that has been superseded by another labels Sep 26, 2024
@jhoeller jhoeller self-assigned this Sep 26, 2024
@jhoeller jhoeller added this to the 6.2.0-RC2 milestone Sep 26, 2024
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

4 participants