AspectJExpressionPointcut consistently resolves superinterface methods

Includes efficient check for same ClassLoader in ClassUtils.isVisible, efficient MethodMatchers check for IntroductionAwareMethodMatcher, and supertype method resolution in MethodMapTransactionAttributeSource.

Issue: SPR-16723

Author: jhoeller

spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -49,13 +49,13 @@
 import org.springframework.aop.framework.autoproxy.ProxyCreationContext;
 import org.springframework.aop.interceptor.ExposeInvocationInterceptor;
 import org.springframework.aop.support.AbstractExpressionPointcut;
-import org.springframework.aop.support.AopUtils;
 import org.springframework.beans.factory.BeanFactory;
 import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.BeanFactoryUtils;
 import org.springframework.beans.factory.FactoryBean;
 import org.springframework.beans.factory.annotation.BeanFactoryAnnotationUtils;
 import org.springframework.beans.factory.config.ConfigurableBeanFactory;
+import org.springframework.core.BridgeMethodResolver;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
@@ -289,10 +289,9 @@ public boolean matches(Class<?> targetClass) {
 	}
 
 	@Override
-	public boolean matches(Method method, @Nullable Class<?> targetClass, boolean beanHasIntroductions) {
+	public boolean matches(Method method, @Nullable Class<?> targetClass, boolean hasIntroductions) {
 		obtainPointcutExpression();
-		Method targetMethod = AopUtils.getMostSpecificMethod(method, targetClass);
-		ShadowMatch shadowMatch = getShadowMatch(targetMethod, method);
+		ShadowMatch shadowMatch = getTargetShadowMatch(method, targetClass);
 
 		// Special handling for this, target, @this, @target, @annotation
 		// in Spring - we can optimize since we know we have exactly this class,
@@ -305,7 +304,7 @@ else if (shadowMatch.neverMatches()) {
 		}
 		else {
 			// the maybe case
-			if (beanHasIntroductions) {
+			if (hasIntroductions) {
 				return true;
 			}
 			// A match test returned maybe - if there are any subtype sensitive variables
@@ -331,8 +330,7 @@ public boolean isRuntime() {
 	@Override
 	public boolean matches(Method method, @Nullable Class<?> targetClass, Object... args) {
 		obtainPointcutExpression();
-		ShadowMatch shadowMatch = getShadowMatch(AopUtils.getMostSpecificMethod(method, targetClass), method);
-		ShadowMatch originalShadowMatch = getShadowMatch(method, method);
+		ShadowMatch shadowMatch = getTargetShadowMatch(method, targetClass);
 
 		// Bind Spring AOP proxy to AspectJ "this" and Spring AOP target to AspectJ target,
 		// consistent with return of MethodInvocationProceedingJoinPoint
@@ -367,7 +365,7 @@ public boolean matches(Method method, @Nullable Class<?> targetClass, Object...
 			 * <p>See SPR-2979 for the original bug.
 			 */
 			if (pmi != null && thisObject != null) {  // there is a current invocation
-				RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch);
+				RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(getShadowMatch(method, method));
 				if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) {
 					return false;
 				}
@@ -427,16 +425,33 @@ private void bindParameters(ProxyMethodInvocation invocation, JoinPointMatch jpm
 		invocation.setUserAttribute(resolveExpression(), jpm);
 	}
 
+	private ShadowMatch getTargetShadowMatch(Method method, @Nullable Class<?> targetClass) {
+		Method targetMethod = method;
+		if (targetClass != null) {
+			targetMethod = ClassUtils.getMostSpecificMethod(method, ClassUtils.getUserClass(targetClass));
+			if (targetMethod.getDeclaringClass().isInterface()) {
+				Set<Class<?>> ifcs = ClassUtils.getAllInterfacesForClassAsSet(targetClass);
+				if (ifcs.size() > 1) {
+					Class<?> compositeInterface = ClassUtils.createCompositeInterface(
+							ClassUtils.toClassArray(ifcs), targetClass.getClassLoader());
+					targetMethod = ClassUtils.getMostSpecificMethod(targetMethod, compositeInterface);
+				}
+			}
+		}
+		targetMethod = BridgeMethodResolver.findBridgedMethod(targetMethod);
+		return getShadowMatch(targetMethod, method);
+	}
+
 	private ShadowMatch getShadowMatch(Method targetMethod, Method originalMethod) {
 		// Avoid lock contention for known Methods through concurrent access...
 		ShadowMatch shadowMatch = this.shadowMatchCache.get(targetMethod);
 		if (shadowMatch == null) {
 			synchronized (this.shadowMatchCache) {
 				// Not found - now check again with full lock...
 				PointcutExpression fallbackExpression = null;
-				Method methodToMatch = targetMethod;
 				shadowMatch = this.shadowMatchCache.get(targetMethod);
 				if (shadowMatch == null) {
+					Method methodToMatch = targetMethod;
 					try {
 						try {
 							shadowMatch = obtainPointcutExpression().matchesMethodExecution(methodToMatch);
@@ -459,7 +474,7 @@ private ShadowMatch getShadowMatch(Method targetMethod, Method originalMethod) {
 							try {
 								shadowMatch = obtainPointcutExpression().matchesMethodExecution(methodToMatch);
 							}
-							catch (ReflectionWorldException ex3) {
+							catch (ReflectionWorldException ex) {
 								// Could neither introspect the target class nor the proxy class ->
 								// let's try the original method's declaring class before we give up...
 								try {
@@ -468,7 +483,7 @@ private ShadowMatch getShadowMatch(Method targetMethod, Method originalMethod) {
 										shadowMatch = fallbackExpression.matchesMethodExecution(methodToMatch);
 									}
 								}
-								catch (ReflectionWorldException ex4) {
+								catch (ReflectionWorldException ex2) {
 									fallbackExpression = null;
 								}
 							}

spring-aop/src/main/java/org/springframework/aop/support/MethodMatchers.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -90,8 +90,8 @@ public static MethodMatcher intersection(MethodMatcher mm1, MethodMatcher mm2) {
 	 */
 	public static boolean matches(MethodMatcher mm, Method method, @Nullable Class<?> targetClass, boolean hasIntroductions) {
 		Assert.notNull(mm, "MethodMatcher must not be null");
-		return ((mm instanceof IntroductionAwareMethodMatcher &&
-				((IntroductionAwareMethodMatcher) mm).matches(method, targetClass, hasIntroductions)) ||
+		return (mm instanceof IntroductionAwareMethodMatcher ?
+				((IntroductionAwareMethodMatcher) mm).matches(method, targetClass, hasIntroductions) :
 				mm.matches(method, targetClass));
 	}
 

spring-beans/src/test/java/org/springframework/tests/sample/beans/AgeHolder.java

@@ -0,0 +1,29 @@
+/*
+ * Copyright 2002-2018 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.tests.sample.beans;
+
+public interface AgeHolder {
+
+	default int age() {
+		return getAge();
+	}
+
+	int getAge();
+
+	void setAge(int age);
+
+}

spring-beans/src/test/java/org/springframework/tests/sample/beans/ITestBean.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2007 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,11 +27,7 @@
  * @author Rod Johnson
  * @author Juergen Hoeller
  */
-public interface ITestBean {
-
-	int getAge();
-
-	void setAge(int age);
+public interface ITestBean extends AgeHolder {
 
 	String getName();
 

spring-context/src/test/java/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -185,7 +185,7 @@ public void testAspectsAndAdvisorAreAppliedEvenIfComingFromParentFactory() {
 		// Create a child factory with a bean that should be woven
 		RootBeanDefinition bd = new RootBeanDefinition(TestBean.class);
 		bd.getPropertyValues().addPropertyValue(new PropertyValue("name", "Adrian"))
-				.addPropertyValue(new PropertyValue("age", new Integer(34)));
+				.addPropertyValue(new PropertyValue("age", 34));
 		childAc.registerBeanDefinition("adrian2", bd);
 		// Register the advisor auto proxy creator with subclass
 		childAc.registerBeanDefinition(AnnotationAwareAspectJAutoProxyCreator.class.getName(), new RootBeanDefinition(
@@ -271,24 +271,44 @@ public void testPerTargetAspect() throws SecurityException, NoSuchMethodExceptio
 	}
 
 	@Test
-	public void testTwoAdviceAspectSingleton() {
-		doTestTwoAdviceAspectWith("twoAdviceAspect.xml");
+	public void testTwoAdviceAspect() {
+		ClassPathXmlApplicationContext bf = newContext("twoAdviceAspect.xml");
+
+		ITestBean adrian1 = (ITestBean) bf.getBean("adrian");
+		testAgeAspect(adrian1, 0, 2);
 	}
 
 	@Test
-	public void testTwoAdviceAspectPrototype() {
-		doTestTwoAdviceAspectWith("twoAdviceAspectPrototype.xml");
+	public void testTwoAdviceAspectSingleton() {
+		ClassPathXmlApplicationContext bf = newContext("twoAdviceAspectSingleton.xml");
+
+		ITestBean adrian1 = (ITestBean) bf.getBean("adrian");
+		testAgeAspect(adrian1, 0, 1);
+		ITestBean adrian2 = (ITestBean) bf.getBean("adrian");
+		assertNotSame(adrian1, adrian2);
+		testAgeAspect(adrian2, 2, 1);
 	}
 
-	private void doTestTwoAdviceAspectWith(String location) {
-		ClassPathXmlApplicationContext bf = newContext(location);
+	@Test
+	public void testTwoAdviceAspectPrototype() {
+		ClassPathXmlApplicationContext bf = newContext("twoAdviceAspectPrototype.xml");
 
-		boolean aspectSingleton = bf.isSingleton("aspect");
 		ITestBean adrian1 = (ITestBean) bf.getBean("adrian");
-		testPrototype(adrian1, 0);
+		testAgeAspect(adrian1, 0, 1);
 		ITestBean adrian2 = (ITestBean) bf.getBean("adrian");
 		assertNotSame(adrian1, adrian2);
-		testPrototype(adrian2, aspectSingleton ? 2 : 0);
+		testAgeAspect(adrian2, 0, 1);
+	}
+
+	private void testAgeAspect(ITestBean adrian, int start, int increment) {
+		assertTrue(AopUtils.isAopProxy(adrian));
+		adrian.setName("");
+		assertEquals(start, adrian.age());
+		int newAge = 32;
+		adrian.setAge(newAge);
+		assertEquals(start + increment, adrian.age());
+		adrian.setAge(0);
+		assertEquals(start + increment * 2, adrian.age());
 	}
 
 	@Test
@@ -312,18 +332,6 @@ public void testIncludeMechanism() {
 		assertEquals(68, adrian.getAge());
 	}
 
-	private void testPrototype(ITestBean adrian1, int start) {
-		assertTrue(AopUtils.isAopProxy(adrian1));
-		//TwoAdviceAspect twoAdviceAspect = (TwoAdviceAspect) bf.getBean(TwoAdviceAspect.class.getName());
-		adrian1.setName("");
-		assertEquals(start++, adrian1.getAge());
-		int newAge = 32;
-		adrian1.setAge(newAge);
-		assertEquals(start++, adrian1.getAge());
-		adrian1.setAge(0);
-		assertEquals(start++, adrian1.getAge());
-	}
-
 	@Test
 	public void testForceProxyTargetClass() {
 		ClassPathXmlApplicationContext bf = newContext("aspectsWithCGLIB.xml");

spring-context/src/test/java/test/aspect/PerTargetAspect.java

@@ -1,6 +1,19 @@
-/**
+/*
+ * Copyright 2002-2018 the original author or authors.
  *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
  */
+
 package test.aspect;
 
 import org.aspectj.lang.annotation.Around;

spring-context/src/test/java/test/aspect/TwoAdviceAspect.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2008 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -23,15 +23,17 @@
 
 @Aspect
 public class TwoAdviceAspect {
+
 	private int totalCalls;
 
-	@Around("execution(* getAge())")
+	@Around("execution(* org.springframework.tests.sample.beans.ITestBean.age())")
 	public int returnCallCount(ProceedingJoinPoint pjp) throws Exception {
 		return totalCalls;
 	}
 
-	@Before("execution(* setAge(int)) && args(newAge)")
+	@Before("execution(* org.springframework.tests.sample.beans.ITestBean.setAge(int)) && args(newAge)")
 	public void countSet(int newAge) throws Exception {
 		++totalCalls;
 	}
-}
+
+}

spring-context/src/test/resources/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests-twoAdviceAspect.xml

@@ -7,9 +7,13 @@
 
 	<bean id="aspect" class="test.aspect.TwoAdviceAspect"/>
 
-	<bean id="adrian" class="org.springframework.tests.sample.beans.TestBean" scope="prototype">
-		<property name="name" value="adrian"/>
-		<property name="age" value="34"/>
+	<bean id="adrian" class="org.springframework.aop.framework.ProxyFactoryBean">
+		<property name="target">
+			<bean class="org.springframework.tests.sample.beans.TestBean">
+				<property name="name" value="adrian"/>
+				<property name="age" value="34"/>
+			</bean>
+		</property>
 	</bean>
 
 </beans>

spring-context/src/test/resources/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests-twoAdviceAspectPrototype.xml

@@ -5,8 +5,7 @@
 
 	<bean class="org.springframework.aop.aspectj.annotation.AnnotationAwareAspectJAutoProxyCreator"/>
 
-	<bean id="aspect" class="test.aspect.TwoAdviceAspect"
-			scope="prototype"/>
+	<bean id="aspect" class="test.aspect.TwoAdviceAspect" scope="prototype"/>
 
 	<bean id="adrian" class="org.springframework.tests.sample.beans.TestBean" scope="prototype">
 		<property name="name" value="adrian"/>

spring-context/src/test/resources/org/springframework/aop/aspectj/autoproxy/AspectJAutoProxyCreatorTests-twoAdviceAspectSingleton.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE beans PUBLIC "-//SPRING//DTD BEAN 2.0//EN" "http://www.springframework.org/dtd/spring-beans-2.0.dtd">
+
+<beans>
+
+	<bean class="org.springframework.aop.aspectj.annotation.AnnotationAwareAspectJAutoProxyCreator"/>
+
+	<bean id="aspect" class="test.aspect.TwoAdviceAspect"/>
+
+	<bean id="adrian" class="org.springframework.tests.sample.beans.TestBean" scope="prototype">
+		<property name="name" value="adrian"/>
+		<property name="age" value="34"/>
+	</bean>
+
+</beans>