Fix CreateAopProxyInterceptor in the Spring core-patch indeed changes…

… the implementation of the Spring AOP proxy (#739)
apache · Dec 15, 2024 · 26600e7 · 26600e7
1 parent 18e971c
commit 26600e7
Show file tree

Hide file tree

Showing 3 changed files with 164 additions and 12 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -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)
 

diff --git a/...rc/main/java/org/apache/skywalking/apm/plugin/spring/patch/CreateAopProxyInterceptor.java b/...rc/main/java/org/apache/skywalking/apm/plugin/spring/patch/CreateAopProxyInterceptor.java
@@ -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.
@@ -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);
+    }
+
+    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) {
 
     }
 }
diff --git a/...est/java/org/apache/skywalking/apm/plugin/spring/patch/CreateAopProxyInterceptorTest.java b/...est/java/org/apache/skywalking/apm/plugin/spring/patch/CreateAopProxyInterceptorTest.java
@@ -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;
@@ -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)));
     }
 
-    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() {
@@ -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();
+
+    }
+
 }