AopUtils.canApply properly matches package-visible methods (aligned with advice matching within proxies)

Also, AbstractAutoProxyCreator considers Pointcut as infrastructure class, analogous to Advice and Advisor.

Issue: SPR-14174

spring-aop/src/main/java/org/springframework/aop/framework/autoproxy/AbstractAutoProxyCreator.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -31,6 +31,7 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import org.springframework.aop.Advisor;
+import org.springframework.aop.Pointcut;
 import org.springframework.aop.TargetSource;
 import org.springframework.aop.framework.AopInfrastructureBean;
 import org.springframework.aop.framework.ProxyFactory;
@@ -370,6 +371,7 @@ public abstract class AbstractAutoProxyCreator extends ProxyProcessorSupport
 	 */
 	protected boolean isInfrastructureClass(Class<?> beanClass) {
 		boolean retVal = Advice.class.isAssignableFrom(beanClass) ||
+				Pointcut.class.isAssignableFrom(beanClass) ||
 				Advisor.class.isAssignableFrom(beanClass) ||
 				AopInfrastructureBean.class.isAssignableFrom(beanClass);
 		if (retVal && logger.isTraceEnabled()) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -222,6 +222,11 @@ public abstract class AopUtils {
 		}
 
 		MethodMatcher methodMatcher = pc.getMethodMatcher();
+		if (methodMatcher == MethodMatcher.TRUE) {
+			// No need to iterate the methods if we're matching any method anyway...
+			return true;
+		}
+
 		IntroductionAwareMethodMatcher introductionAwareMethodMatcher = null;
 		if (methodMatcher instanceof IntroductionAwareMethodMatcher) {
 			introductionAwareMethodMatcher = (IntroductionAwareMethodMatcher) methodMatcher;
@@ -230,7 +235,7 @@ public abstract class AopUtils {
 		Set<Class<?>> classes = new LinkedHashSet<Class<?>>(ClassUtils.getAllInterfacesForClassAsSet(targetClass));
 		classes.add(targetClass);
 		for (Class<?> clazz : classes) {
-			Method[] methods = clazz.getMethods();
+			Method[] methods = ReflectionUtils.getAllDeclaredMethods(clazz);
 			for (Method method : methods) {
 				if ((introductionAwareMethodMatcher != null &&
 						introductionAwareMethodMatcher.matches(method, targetClass, hasIntroductions)) ||

spring-context/src/test/java/org/springframework/aop/framework/autoproxy/AdvisorAutoProxyCreatorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -21,6 +21,7 @@ import java.io.IOException;
 import org.junit.Test;
 import test.mixin.Lockable;
 
+import org.springframework.aop.Advisor;
 import org.springframework.aop.framework.Advised;
 import org.springframework.aop.framework.autoproxy.target.AbstractBeanFactoryBasedTargetSourceCreator;
 import org.springframework.aop.support.AopUtils;
@@ -48,7 +49,7 @@ import static org.junit.Assert.*;
  * @author Chris Beams
  */
 @SuppressWarnings("resource")
-public final class AdvisorAutoProxyCreatorTests {
+public class AdvisorAutoProxyCreatorTests {
 
 	private static final Class<?> CLASS = AdvisorAutoProxyCreatorTests.class;
 	private static final String CLASSNAME = CLASS.getSimpleName();
@@ -59,6 +60,7 @@ public final class AdvisorAutoProxyCreatorTests {
 	private static final String QUICK_TARGETSOURCE_CONTEXT = CLASSNAME + "-quick-targetsource.xml";
 	private static final String OPTIMIZED_CONTEXT = CLASSNAME + "-optimized.xml";
 
+
 	/**
 	 * Return a bean factory with attributes and EnterpriseServices configured.
 	 */
@@ -66,6 +68,7 @@ public final class AdvisorAutoProxyCreatorTests {
 		return new ClassPathXmlApplicationContext(DEFAULT_CONTEXT, CLASS);
 	}
 
+
 	/**
 	 * Check that we can provide a common interceptor that will
 	 * appear in the chain before "specific" interceptors,
@@ -78,8 +81,8 @@ public final class AdvisorAutoProxyCreatorTests {
 		assertTrue(AopUtils.isAopProxy(test1));
 
 		Lockable lockable1 = (Lockable) test1;
-		NopInterceptor nop = (NopInterceptor) bf.getBean("nopInterceptor");
+		NopInterceptor nop1 = (NopInterceptor) bf.getBean("nopInterceptor");
-		assertEquals(0, nop.getCount());
+		NopInterceptor nop2 = (NopInterceptor) bf.getBean("pointcutAdvisor", Advisor.class).getAdvice();
 
 		ITestBean test2 = (ITestBean) bf.getBean("test2");
 		Lockable lockable2 = (Lockable) test2;
@@ -87,14 +90,28 @@ public final class AdvisorAutoProxyCreatorTests {
 		// Locking should be independent; nop is shared
 		assertFalse(lockable1.locked());
 		assertFalse(lockable2.locked());
-		// equals 2 calls on shared nop, because it's first
+		// equals 2 calls on shared nop, because it's first and sees calls
-		// and sees calls against the Lockable interface introduced
+		// against the Lockable interface introduced by the specific advisor
-		// by the specific advisor
+		assertEquals(2, nop1.getCount());
-		assertEquals(2, nop.getCount());
+		assertEquals(0, nop2.getCount());
 		lockable1.lock();
 		assertTrue(lockable1.locked());
 		assertFalse(lockable2.locked());
-		assertEquals(5, nop.getCount());
+		assertEquals(5, nop1.getCount());
+		assertEquals(0, nop2.getCount());
+
+		PackageVisibleMethod packageVisibleMethod = (PackageVisibleMethod) bf.getBean("packageVisibleMethod");
+		assertEquals(5, nop1.getCount());
+		assertEquals(0, nop2.getCount());
+		packageVisibleMethod.doSomething();
+		assertEquals(6, nop1.getCount());
+		assertEquals(1, nop2.getCount());
+		assertTrue(packageVisibleMethod instanceof Lockable);
+		Lockable lockable3 = (Lockable) packageVisibleMethod;
+		lockable3.lock();
+		assertTrue(lockable3.locked());
+		lockable3.unlock();
+		assertFalse(lockable3.locked());
 	}
 
 	/**
@@ -202,6 +219,7 @@ public final class AdvisorAutoProxyCreatorTests {
 
 }
 
+
 class SelectivePrototypeTargetSourceCreator extends AbstractBeanFactoryBasedTargetSourceCreator {
 
 	@Override

spring-context/src/test/java/org/springframework/aop/framework/autoproxy/AutoProxyCreatorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -56,7 +56,7 @@ import static org.junit.Assert.*;
  * @since 09.12.2003
  */
 @SuppressWarnings("resource")
-public final class AutoProxyCreatorTests {
+public class AutoProxyCreatorTests {
 
 	@Test
 	public void testBeanNameAutoProxyCreator() {
@@ -252,6 +252,23 @@ public final class AutoProxyCreatorTests {
 		assertEquals(2, tapc.testInterceptor.nrOfInvocations);
 	}
 
+	@Test
+	public void testAutoProxyCreatorWithPackageVisibleMethod() {
+		StaticApplicationContext sac = new StaticApplicationContext();
+		sac.registerSingleton("testAutoProxyCreator", TestAutoProxyCreator.class);
+		sac.registerSingleton("packageVisibleMethodToBeProxied", PackageVisibleMethod.class);
+		sac.refresh();
+
+		TestAutoProxyCreator tapc = (TestAutoProxyCreator) sac.getBean("testAutoProxyCreator");
+		tapc.testInterceptor.nrOfInvocations = 0;
+
+		PackageVisibleMethod tb = (PackageVisibleMethod) sac.getBean("packageVisibleMethodToBeProxied");
+		assertTrue(AopUtils.isCglibProxy(tb));
+		assertEquals(0, tapc.testInterceptor.nrOfInvocations);
+		tb.doSomething();
+		assertEquals(1, tapc.testInterceptor.nrOfInvocations);
+	}
+
 	@Test
 	public void testAutoProxyCreatorWithFactoryBean() {
 		StaticApplicationContext sac = new StaticApplicationContext();

spring-context/src/test/java/org/springframework/aop/framework/autoproxy/PackageVisibleMethod.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2002-2016 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.aop.framework.autoproxy;
+
+public class PackageVisibleMethod {
+
+	void doSomething() {
+	}
+
+}

spring-context/src/test/resources/org/springframework/aop/framework/autoproxy/AdvisorAutoProxyCreatorTests-common-interceptors.xml

@@ -10,37 +10,38 @@
 		Matches all Advisors in the factory: we don't use a prefix
 	</description>
 
-	<bean id="aapc"
+	<bean id="aapc" class="org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator">
-		class="org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator">
+		<!-- This common interceptor will be applied always, before custom lockable advisor -->
-	
-		<!-- This common interceptor will be applied always,
-		before custom lockable advisor -->
 		<property name="interceptorNames">
 			<value>nopInterceptor</value>
 		</property>
 	</bean>
 
-	<bean id="nopInterceptor" class="org.springframework.tests.aop.interceptor.NopInterceptor" />
+	<bean id="nopInterceptor" class="org.springframework.tests.aop.interceptor.NopInterceptor"/>
 
-	<!--
+	<bean id="pointcutAdvisor" class="org.springframework.aop.support.DefaultPointcutAdvisor">
-		Stateful mixin. Will apply to all objects
+		<property name="pointcut">
-		Note that singleton property is false.
+			<bean class="org.springframework.aop.support.NameMatchMethodPointcut">
-	-->
+				<property name="mappedName" value="doSomething"/>
-	<bean id="lockableAdvisor"
+			</bean>
-		class="test.mixin.LockMixinAdvisor"
+		</property>
-		scope="prototype"
+		<property name="advice">
-	/>
+			<bean class="org.springframework.tests.aop.interceptor.NopInterceptor"/>
-
+		</property>
-	<bean id="test1"
-		class="org.springframework.tests.sample.beans.TestBean">
-		<property name="age"><value>4</value></property>
 	</bean>
 
-	<bean id="test2"
+	<!-- Stateful mixin. Will apply to all objects. Note that singleton property is false. -->
-		class="org.springframework.tests.sample.beans.TestBean">
+	<bean id="lockableAdvisor" class="test.mixin.LockMixinAdvisor" scope="prototype"/>
-		<property name="age"><value>4</value></property>
+
+	<bean id="test1" class="org.springframework.tests.sample.beans.TestBean">
+		<property name="age" value="4"/>
 	</bean>
 
+	<bean id="test2" class="org.springframework.tests.sample.beans.TestBean">
+		<property name="age" value="4"/>
+	</bean>
 
-</beans>	
+	<bean id="packageVisibleMethod" class="org.springframework.aop.framework.autoproxy.PackageVisibleMethod"/>
+
+</beans>