Skip to content

Fix CreateAopProxyInterceptor in the Spring core-patch indeed changes the implementation of the Spring AOP proxy #739

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.

Sign up for GitHub

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

Merged
wu-sheng merged 2 commits into from
Dec 15, 2024
Merged

Fix CreateAopProxyInterceptor in the Spring core-patch indeed changes the implementation of the Spring AOP proxy #739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebcb8d0
#12858:CreateAopProxyInterceptor in spring core-patch indeed changed …
Dec 14, 2024
995c37c
#12858:CreateAopProxyInterceptor in spring core-patch indeed changed …
Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ Release Notes.
* Add empty judgment for constructorInterceptPoint
* Bump up gRPC to 1.68.1
* Bump up netty to 4.1.115.Final
* Fix the `CreateAopProxyInterceptor` in the Spring core-patch to prevent it from changing the implementation of the Spring AOP proxy

All issues and pull requests are [here](https://github.com/apache/skywalking/milestone/222?closed=1)

Expand Down
36 changes: 29 additions & 7 deletions ...rc/main/java/org/apache/skywalking/apm/plugin/spring/patch/CreateAopProxyInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@

package org.apache.skywalking.apm.plugin.spring.patch;

import java.lang.reflect.Method;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
import org.springframework.aop.SpringProxy;
import org.springframework.aop.framework.AdvisedSupport;

import java.lang.reflect.Method;

/**
* <code>CreateAopProxyInterceptor</code> check that the bean has been implement {@link EnhancedInstance}.
* if yes, true will be returned.
Expand All @@ -32,25 +34,45 @@ public class CreateAopProxyInterceptor implements InstanceMethodsAroundIntercept

@Override
public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
MethodInterceptResult result) throws Throwable {
MethodInterceptResult result) throws Throwable {

}

@Override
public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
Object ret) throws Throwable {
Object ret) throws Throwable {
AdvisedSupport advisedSupport = (AdvisedSupport) allArguments[0];

Class targetClass = advisedSupport.getTargetClass();
if (targetClass != null && EnhancedInstance.class.isAssignableFrom(targetClass)) {
return true;
if (maybeHasUserSuppliedProxyInterfaces(ret)) {
Class targetClass = advisedSupport.getTargetClass();
if (targetClass != null) {
if (onlyImplementsEnhancedInstance(advisedSupport) || onlyImplementsEnhancedInstanceAndSpringProxy(advisedSupport)) {
return true;
}
}
}
return ret;
}

private boolean maybeHasUserSuppliedProxyInterfaces(Object ret) {
return !(Boolean) ret;
}

private boolean onlyImplementsEnhancedInstanceAndSpringProxy(AdvisedSupport advisedSupport) {
Class<?>[] ifcs = advisedSupport.getProxiedInterfaces();
Class targetClass = advisedSupport.getTargetClass();
return ifcs.length == 2 && EnhancedInstance.class.isAssignableFrom(targetClass) && SpringProxy.class.isAssignableFrom(targetClass);
Copy link
Member

@wu-sheng wu-sheng Dec 15, 2024

Choose a reason for hiding this comment

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

When is SpringProxy.class added into the hierarchy tree?

Copy link
Contributor Author

@youjie23 youjie23 Dec 15, 2024

Choose a reason for hiding this comment

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

Thank you for the review.
As shown below, the hasNoUserSuppliedProxyInterfaces method in DefaultAopProxyFactory is defined as follows:
spring4/spring5

         /**
	 * Determine whether the supplied {@link AdvisedSupport} has only the
	 * {@link org.springframework.aop.SpringProxy} interface specified
	 * (or no proxy interfaces specified at all).
	 */
	private boolean hasNoUserSuppliedProxyInterfaces(AdvisedSupport config) {
		Class<?>[] ifcs = config.getProxiedInterfaces();
		return (ifcs.length == 0 || (ifcs.length == 1 && SpringProxy.class.isAssignableFrom(ifcs[0])));
	}

spring3

         /**
	 * Determine whether the supplied {@link AdvisedSupport} has only the
	 * {@link org.springframework.aop.SpringProxy} interface specified
	 * (or no proxy interfaces specified at all).
	 */
	private boolean hasNoUserSuppliedProxyInterfaces(AdvisedSupport config) {
		Class[] interfaces = config.getProxiedInterfaces();
		return (interfaces.length == 0 || (interfaces.length == 1 && SpringProxy.class.equals(interfaces[0])));
	}

Copy link
Member

@wu-sheng wu-sheng Dec 15, 2024

Choose a reason for hiding this comment

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

OK, this seems ok. Could you update the changelog? Then I could merge this PR.

Copy link
Contributor Author

@youjie23 youjie23 Dec 15, 2024

Choose a reason for hiding this comment

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

Thank you for the review.
The Changes.md file has been updated.

}

private boolean onlyImplementsEnhancedInstance(AdvisedSupport advisedSupport) {
Class<?>[] ifcs = advisedSupport.getProxiedInterfaces();
Class targetClass = advisedSupport.getTargetClass();
return ifcs.length == 1 && EnhancedInstance.class.isAssignableFrom(targetClass);
}

@Override
public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, Throwable t) {
Class<?>[] argumentsTypes, Throwable t) {

}
}
139 changes: 134 additions & 5 deletions ...est/java/org/apache/skywalking/apm/plugin/spring/patch/CreateAopProxyInterceptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.aop.SpringProxy;
import org.springframework.aop.framework.AdvisedSupport;

import static org.hamcrest.core.Is.is;
Expand All @@ -48,18 +49,69 @@ public void setUp() {
}

@Test
public void testInterceptNormalObject() throws Throwable {
doReturn(Object.class).when(advisedSupport).getTargetClass();
public void testInterceptClassImplementsNoInterfaces() throws Throwable {
// doReturn(Object.class).when(advisedSupport).getTargetClass();
// doReturn(Object.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(true, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, true)));
}

@Test
public void testInterceptClassImplementsUserSuppliedInterface() throws Throwable {
doReturn(MockClassImplementsUserSuppliedInterface.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsUserSuppliedInterface.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(false, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
}

@Test
public void testInterceptEnhanceInstanceObject() throws Throwable {
doReturn(MockClass.class).when(advisedSupport).getTargetClass();
public void testInterceptClassImplementsSpringProxy() throws Throwable {
// doReturn(MockClassImplementsSpringProxy.class).when(advisedSupport).getTargetClass();
// doReturn(MockClassImplementsSpringProxy.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(true, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, true)));
}

@Test
public void testInterceptClassImplementsEnhancedInstance() throws Throwable {
doReturn(MockClassImplementsEnhancedInstance.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsEnhancedInstance.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(true, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
Copy link
Contributor

@lujiajing1126 lujiajing1126 Apr 30, 2025

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/45124660/the-bean-could-not-be-injected-as-a-type-because-it-is-a-jdk-dynamic-proxy-tha

According to the answer in this SO thread, we basically should not expect classes that both use @Transactional annotation and being enhanced by SkyWalking Agent to use JDK Proxy.

We are facing startup failures after upgrading to SkyWalking 9.4.

}

private class MockClass implements EnhancedInstance {
@Test
public void testClassImplementsEnhancedInstanceAndUserSuppliedInterface() throws Throwable {
doReturn(MockClassImplementsSpringProxyAndUserSuppliedInterface.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsSpringProxyAndUserSuppliedInterface.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(false, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
}

@Test
public void testInterceptClassImplementsSpringProxyAndEnhancedInstance() throws Throwable {
doReturn(MockClassImplementsSpringProxyAndEnhancedInstance.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsSpringProxyAndEnhancedInstance.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(true, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
}

@Test
public void testInterceptClassImplementsSpringProxyAndUserSuppliedInterface() throws Throwable {
doReturn(MockClassImplementsSpringProxyAndUserSuppliedInterface.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsSpringProxyAndUserSuppliedInterface.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(false, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
}

@Test
public void testInterceptClassImplementsEnhancedInstanceAndUserSuppliedInterface() throws Throwable {
doReturn(MockClassImplementsEnhancedInstanceAndUserSuppliedInterface.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsEnhancedInstanceAndUserSuppliedInterface.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(false, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
}

@Test
public void testInterceptClassImplementsSpringProxyAndEnhancedInstanceAndUserSuppliedInterface() throws Throwable {
doReturn(MockClassImplementsSpringProxyAndEnhancedInstanceAndUserSuppliedInterface.class).when(advisedSupport).getTargetClass();
doReturn(MockClassImplementsSpringProxyAndEnhancedInstanceAndUserSuppliedInterface.class.getInterfaces()).when(advisedSupport).getProxiedInterfaces();
assertThat(false, is(interceptor.afterMethod(enhancedInstance, null, new Object[] {advisedSupport}, new Class[] {Object.class}, false)));
}

private class MockClassImplementsEnhancedInstance implements EnhancedInstance {

@Override
public Object getSkyWalkingDynamicField() {
Expand All @@ -72,4 +124,81 @@ public void setSkyWalkingDynamicField(Object value) {
}
}

private class MockClassImplementsUserSuppliedInterface implements UserSuppliedInterface {

@Override
public void methodOfUserSuppliedInterface() {

}

}

private class MockClassImplementsSpringProxy implements SpringProxy {

}

private class MockClassImplementsSpringProxyAndEnhancedInstance implements EnhancedInstance, SpringProxy {

@Override
public Object getSkyWalkingDynamicField() {
return null;
}

@Override
public void setSkyWalkingDynamicField(Object value) {

}

}

private class MockClassImplementsEnhancedInstanceAndUserSuppliedInterface implements EnhancedInstance, UserSuppliedInterface {

@Override
public Object getSkyWalkingDynamicField() {
return null;
}

@Override
public void setSkyWalkingDynamicField(Object value) {

}

@Override
public void methodOfUserSuppliedInterface() {
}

}

private class MockClassImplementsSpringProxyAndUserSuppliedInterface implements SpringProxy, UserSuppliedInterface {

@Override
public void methodOfUserSuppliedInterface() {
}

}

private class MockClassImplementsSpringProxyAndEnhancedInstanceAndUserSuppliedInterface implements EnhancedInstance, SpringProxy, UserSuppliedInterface {

@Override
public Object getSkyWalkingDynamicField() {
return null;
}

@Override
public void setSkyWalkingDynamicField(Object value) {

}

@Override
public void methodOfUserSuppliedInterface() {
}

}

interface UserSuppliedInterface {

void methodOfUserSuppliedInterface();

}

}
Loading