AbstractAdvisingBeanPostProcessor uses target class check for existing proxy but checks against actual exposed object otherwise (catching introductions)

Issue: SPR-11725

spring-aop/src/main/java/org/springframework/aop/framework/AbstractAdvisingBeanPostProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -91,9 +91,11 @@ public abstract class AbstractAdvisingBeanPostProcessor extends ProxyConfig
 			// Ignore AOP infrastructure such as scoped proxies.
 			return bean;
 		}
-		if (isEligible(bean, beanName)) {
-			if (bean instanceof Advised) {
-				Advised advised = (Advised) bean;
+
+		if (bean instanceof Advised) {
+			Advised advised = (Advised) bean;
+			if (!advised.isFrozen() && isEligible(AopUtils.getTargetClass(bean))) {
+				// Add our local Advisor to the existing proxy's Advisor chain...
 				if (this.beforeExistingAdvisors) {
 					advised.addAdvisor(0, this.advisor);
 				}
@@ -102,32 +104,47 @@ public abstract class AbstractAdvisingBeanPostProcessor extends ProxyConfig
 				}
 				return bean;
 			}
-			else {
-				ProxyFactory proxyFactory = new ProxyFactory(bean);
-				// Copy our properties (proxyTargetClass etc) inherited from ProxyConfig.
-				proxyFactory.copyFrom(this);
-				proxyFactory.addAdvisor(this.advisor);
-				return proxyFactory.getProxy(this.beanClassLoader);
-			}
 		}
-		else {
-			// No async proxy needed.
-			return bean;
+
+		if (isEligible(bean, beanName)) {
+			ProxyFactory proxyFactory = new ProxyFactory(bean);
+			// Copy our properties (proxyTargetClass etc) inherited from ProxyConfig.
+			proxyFactory.copyFrom(this);
+			proxyFactory.addAdvisor(this.advisor);
+			return proxyFactory.getProxy(this.beanClassLoader);
 		}
+
+		// No async proxy needed.
+		return bean;
 	}
 
 	/**
 	 * Check whether the given bean is eligible for advising with this
 	 * post-processor's {@link Advisor}.
-	 * <p>Implements caching of {@code canApply} results per bean target class.
+	 * <p>Delegates to {@link #isEligible(Class)} for target class checking.
 	 * Can be overridden e.g. to specifically exclude certain beans by name.
+	 * <p>Note: Only called for regular bean instances but not for existing
+	 * proxy instances which implement {@link Advised} and allow for adding
+	 * the local {@link Advisor} to the existing proxy's {@link Advisor} chain.
+	 * For the latter, {@link #isEligible(Class)} is being called directly,
+	 * with the actual target class behind the existing proxy (as determined
+	 * by {@link AopUtils#getTargetClass(Object)}).
 	 * @param bean the bean instance
 	 * @param beanName the name of the bean
-	 * @see AopUtils#getTargetClass(Object)
-	 * @see AopUtils#canApply(Advisor, Class)
+	 * @see #isEligible(Class)
 	 */
 	protected boolean isEligible(Object bean, String beanName) {
-		Class<?> targetClass = AopUtils.getTargetClass(bean);
+		return isEligible(bean.getClass());
+	}
+
+	/**
+	 * Check whether the given class is eligible for advising with this
+	 * post-processor's {@link Advisor}.
+	 * <p>Implements caching of {@code canApply} results per bean target class.
+	 * @param targetClass the class to check against
+	 * @see AopUtils#canApply(Advisor, Class)
+	 */
+	protected boolean isEligible(Class<?> targetClass) {
 		Boolean eligible = this.eligibleBeans.get(targetClass);
 		if (eligible != null) {
 			return eligible;

spring-context/src/test/java/org/springframework/scheduling/annotation/AsyncExecutionTests.java

@@ -18,19 +18,22 @@ package org.springframework.scheduling.annotation;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
+import java.util.HashMap;
 import java.util.concurrent.Future;
 
-import org.junit.Before;
+import org.aopalliance.intercept.MethodInterceptor;
+import org.aopalliance.intercept.MethodInvocation;
 import org.junit.Test;
 
+import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.aop.framework.autoproxy.DefaultAdvisorAutoProxyCreator;
+import org.springframework.aop.support.DefaultIntroductionAdvisor;
+import org.springframework.beans.factory.FactoryBean;
 import org.springframework.beans.factory.support.RootBeanDefinition;
 import org.springframework.context.ApplicationEvent;
 import org.springframework.context.ApplicationListener;
 import org.springframework.context.support.GenericApplicationContext;
 import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;
-import org.springframework.tests.Assume;
-import org.springframework.tests.TestGroup;
 
 import static org.junit.Assert.*;
 
@@ -46,10 +49,6 @@ public class AsyncExecutionTests {
 
 	private static int listenerConstructed = 0;
 
-	@Before
-	public void setUp() {
-		Assume.group(TestGroup.PERFORMANCE);
-	}
 
 	@Test
 	public void asyncMethods() throws Exception {
@@ -135,6 +134,46 @@ public class AsyncExecutionTests {
 		assertEquals("20", future.get());
 	}
 
+	@Test
+	public void asyncClassWithPostProcessor() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncClassBean.class));
+		context.registerBeanDefinition("asyncProcessor", new RootBeanDefinition(AsyncAnnotationBeanPostProcessor.class));
+		context.refresh();
+		AsyncClassBean asyncTest = context.getBean("asyncTest", AsyncClassBean.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
+	@Test
+	public void asyncClassWithInterface() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncClassBeanWithInterface.class));
+		context.registerBeanDefinition("autoProxyCreator", new RootBeanDefinition(DefaultAdvisorAutoProxyCreator.class));
+		context.registerBeanDefinition("asyncAdvisor", new RootBeanDefinition(AsyncAnnotationAdvisor.class));
+		context.refresh();
+		RegularInterface asyncTest = context.getBean("asyncTest", RegularInterface.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
+	@Test
+	public void asyncClassWithInterfaceAndPostProcessor() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncClassBeanWithInterface.class));
+		context.registerBeanDefinition("asyncProcessor", new RootBeanDefinition(AsyncAnnotationBeanPostProcessor.class));
+		context.refresh();
+		RegularInterface asyncTest = context.getBean("asyncTest", RegularInterface.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
 	@Test
 	public void asyncInterface() throws Exception {
 		originalThreadName = Thread.currentThread().getName();
@@ -149,6 +188,46 @@ public class AsyncExecutionTests {
 		assertEquals("20", future.get());
 	}
 
+	@Test
+	public void asyncInterfaceWithPostProcessor() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncInterfaceBean.class));
+		context.registerBeanDefinition("asyncProcessor", new RootBeanDefinition(AsyncAnnotationBeanPostProcessor.class));
+		context.refresh();
+		AsyncInterface asyncTest = context.getBean("asyncTest", AsyncInterface.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
+	@Test
+	public void dynamicAsyncInterface() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(DynamicAsyncInterfaceBean.class));
+		context.registerBeanDefinition("autoProxyCreator", new RootBeanDefinition(DefaultAdvisorAutoProxyCreator.class));
+		context.registerBeanDefinition("asyncAdvisor", new RootBeanDefinition(AsyncAnnotationAdvisor.class));
+		context.refresh();
+		AsyncInterface asyncTest = context.getBean("asyncTest", AsyncInterface.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
+	@Test
+	public void dynamicAsyncInterfaceWithPostProcessor() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(DynamicAsyncInterfaceBean.class));
+		context.registerBeanDefinition("asyncProcessor", new RootBeanDefinition(AsyncAnnotationBeanPostProcessor.class));
+		context.refresh();
+		AsyncInterface asyncTest = context.getBean("asyncTest", AsyncInterface.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
 	@Test
 	public void asyncMethodsInInterface() throws Exception {
 		originalThreadName = Thread.currentThread().getName();
@@ -164,6 +243,33 @@ public class AsyncExecutionTests {
 		assertEquals("20", future.get());
 	}
 
+	@Test
+	public void asyncMethodsInInterfaceWithPostProcessor() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncMethodsInterfaceBean.class));
+		context.registerBeanDefinition("asyncProcessor", new RootBeanDefinition(AsyncAnnotationBeanPostProcessor.class));
+		context.refresh();
+		AsyncMethodsInterface asyncTest = context.getBean("asyncTest", AsyncMethodsInterface.class);
+		asyncTest.doNothing(5);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
+	@Test
+	public void dynamicAsyncMethodsInInterfaceWithPostProcessor() throws Exception {
+		originalThreadName = Thread.currentThread().getName();
+		GenericApplicationContext context = new GenericApplicationContext();
+		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(DynamicAsyncMethodsInterfaceBean.class));
+		context.registerBeanDefinition("asyncProcessor", new RootBeanDefinition(AsyncAnnotationBeanPostProcessor.class));
+		context.refresh();
+		AsyncMethodsInterface asyncTest = context.getBean("asyncTest", AsyncMethodsInterface.class);
+		asyncTest.doSomething(10);
+		Future<String> future = asyncTest.returnSomething(20);
+		assertEquals("20", future.get());
+	}
+
 	@Test
 	public void asyncMethodListener() throws Exception {
 		originalThreadName = Thread.currentThread().getName();
@@ -304,6 +410,28 @@ public class AsyncExecutionTests {
 	}
 
 
+	public interface RegularInterface {
+
+		void doSomething(int i);
+
+		Future<String> returnSomething(int i);
+	}
+
+
+	@Async
+	public static class AsyncClassBeanWithInterface implements RegularInterface {
+
+		public void doSomething(int i) {
+			assertTrue(!Thread.currentThread().getName().equals(originalThreadName));
+		}
+
+		public Future<String> returnSomething(int i) {
+			assertTrue(!Thread.currentThread().getName().equals(originalThreadName));
+			return new AsyncResult<String>(Integer.toString(i));
+		}
+	}
+
+
 	@Async
 	public interface AsyncInterface {
 
@@ -328,6 +456,44 @@ public class AsyncExecutionTests {
 	}
 
 
+	public static class DynamicAsyncInterfaceBean implements FactoryBean<AsyncInterface> {
+
+		private final AsyncInterface proxy;
+
+		public DynamicAsyncInterfaceBean() {
+			ProxyFactory pf = new ProxyFactory(new HashMap<>());
+			DefaultIntroductionAdvisor advisor = new DefaultIntroductionAdvisor(new MethodInterceptor() {
+				@Override
+				public Object invoke(MethodInvocation invocation) throws Throwable {
+					assertTrue(!Thread.currentThread().getName().equals(originalThreadName));
+					if (Future.class.equals(invocation.getMethod().getReturnType())) {
+						return new AsyncResult<String>(invocation.getArguments()[0].toString());
+					}
+					return null;
+				}
+			});
+			advisor.addInterface(AsyncInterface.class);
+			pf.addAdvisor(advisor);
+			this.proxy = (AsyncInterface) pf.getProxy();
+		}
+
+		@Override
+		public AsyncInterface getObject() {
+			return this.proxy;
+		}
+
+		@Override
+		public Class<?> getObjectType() {
+			return this.proxy.getClass();
+		}
+
+		@Override
+		public boolean isSingleton() {
+			return true;
+		}
+	}
+
+
 	public interface AsyncMethodsInterface {
 
 		void doNothing(int i);
@@ -360,6 +526,44 @@ public class AsyncExecutionTests {
 	}
 
 
+	public static class DynamicAsyncMethodsInterfaceBean implements FactoryBean<AsyncMethodsInterface> {
+
+		private final AsyncMethodsInterface proxy;
+
+		public DynamicAsyncMethodsInterfaceBean() {
+			ProxyFactory pf = new ProxyFactory(new HashMap<>());
+			DefaultIntroductionAdvisor advisor = new DefaultIntroductionAdvisor(new MethodInterceptor() {
+				@Override
+				public Object invoke(MethodInvocation invocation) throws Throwable {
+					assertTrue(!Thread.currentThread().getName().equals(originalThreadName));
+					if (Future.class.equals(invocation.getMethod().getReturnType())) {
+						return new AsyncResult<String>(invocation.getArguments()[0].toString());
+					}
+					return null;
+				}
+			});
+			advisor.addInterface(AsyncMethodsInterface.class);
+			pf.addAdvisor(advisor);
+			this.proxy = (AsyncMethodsInterface) pf.getProxy();
+		}
+
+		@Override
+		public AsyncMethodsInterface getObject() {
+			return this.proxy;
+		}
+
+		@Override
+		public Class<?> getObjectType() {
+			return this.proxy.getClass();
+		}
+
+		@Override
+		public boolean isSingleton() {
+			return true;
+		}
+	}
+
+
 	public static class AsyncMethodListener implements ApplicationListener<ApplicationEvent> {
 
 		@Override