Fix APC registration for @EnableTransactionManagement

Prior to this change, @EnableTransactionManagement (via the
ProxyTransactionManagementConfiguration class) did not properly
register its auto-proxy creator through the usual AopConfigUtils
methods.  It was trying to register the APC as a normal @Bean method,
but this causes issues (SPR-8494) with the logic in
AopConfigUtils#registerOrEscalateApcAsRequired, which expects the APC
bean definition to have a beanClassName property.  When the APC is
registered via a @Bean definition, it is actually a
factoryBean/factoryMethod situation with no directly resolvable
beanClass/beanClassName.

To solve this problem, ImportSelector#selectImports has been refactored
to accept an ImportSelector.Context instance. This object contains the
AnnotationMetadata of the importing class as well as the enclosing
BeanDefinitionRegistry to allow for the kind of conditional bean
registration necessary here. In this case, the bean definition that
must be registered conditionally is that of the auto-proxy creator.
It should only be registered if AdviceMode == PROXY, and thus the
ImportSelector is an appropriate place to make this happen.  It must
happen as a BeanDefinition (rather than a @Bean method) for
compatibility with AopConfigUtils, and working with the
BeanDefinitionRegistry API allows for that. This change does mean that
in certain cases like this one, #selectImports has container modifying
side effects. Documentation has been updated to reflect.

Issue: SPR-8411, SPR-8494

git-svn-id: https://src.springframework.org/svn/spring-framework/trunk@4686 50f2f4bb-b051-0410-bef5-90022cba6387

org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java

@@ -84,6 +84,8 @@ class ConfigurationClassParser {
 
 	private final ResourceLoader resourceLoader;
 
+	private final BeanDefinitionRegistry registry;
+
 	private final ComponentScanAnnotationParser componentScanParser;
 
 
@@ -98,8 +100,9 @@ class ConfigurationClassParser {
 		this.problemReporter = problemReporter;
 		this.environment = environment;
 		this.resourceLoader = resourceLoader;
-
-		this.componentScanParser = new ComponentScanAnnotationParser(this.resourceLoader, this.environment, registry);
+		this.registry = registry;
+		this.componentScanParser =
+			new ComponentScanAnnotationParser(this.resourceLoader, this.environment, this.registry);
 	}
 
 
@@ -245,7 +248,8 @@ class ConfigurationClassParser {
 					// the candidate class is an ImportSelector -> delegate to it to determine imports
 					try {
 						ImportSelector selector = BeanUtils.instantiateClass(Class.forName(candidate), ImportSelector.class);
-						processImport(configClass, selector.selectImports(importingClassMetadata), false);
+						ImportSelector.Context context = new ImportSelector.Context(importingClassMetadata, this.registry);
+						processImport(configClass, selector.selectImports(context), false);
 					} catch (ClassNotFoundException ex) {
 						throw new IllegalStateException(ex);
 					}

org.springframework.context/src/main/java/org/springframework/context/annotation/ImportSelector.java

@@ -16,6 +16,7 @@
 
 package org.springframework.context.annotation;
 
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.core.type.AnnotationMetadata;
 
 /**
@@ -23,6 +24,10 @@ import org.springframework.core.type.AnnotationMetadata;
  * class(es) should be imported based on a given selection criteria, usually one or more
  * annotation attributes.
  *
+ * <p>In certain cases, an {@code ImportSelector} may register additional bean definitions
+ * through the {@link BeanDefinitionRegistry} available in the
+ * {@code Context} provided to the {@link #selectImports} method.
+ *
  * @author Chris Beams
  * @since 3.1
  * @see Import
@@ -31,10 +36,41 @@ import org.springframework.core.type.AnnotationMetadata;
 public interface ImportSelector {
 
 	/**
-	 * Select and return the names of which class(es) should be imported.
-	 * @param importingClassMetadata the AnnotationMetodata of the
-	 * importing @{@link Configuration} class.
+	 * Select and return the names of which class(es) should be imported based on
+	 * the {@code AnnotationMetadata} of the importing {@code @Configuration} class and
+	 * optionally register any {@code BeanDefinition}s necessary to support the selected
+	 * classes.
+	 * @param context containing the AnnotationMetadata of the importing @{@link
+	 * Configuration} class and the enclosing {@link BeanDefinitionRegistry}.
+	 */
+	String[] selectImports(Context context);
+
+
+	/**
+	 * Context object holding the {@link AnnotationMetadata} of the {@code @Configuration}
+	 * class that imported this {@link ImportSelector} as well as the enclosing
+	 * {@link BeanDefinitionRegistry} to allow for conditional bean definition
+	 * registration when necessary.
+	 *
+	 * @author Chris Beams
+	 * @since 3.1
 	 */
-	String[] selectImports(AnnotationMetadata importingClassMetadata);
+	static class Context {
+		private final AnnotationMetadata importingClassMetadata;
+		private final BeanDefinitionRegistry registry;
+
+		public Context(AnnotationMetadata importingClassMetadata, BeanDefinitionRegistry registry) {
+			this.importingClassMetadata = importingClassMetadata;
+			this.registry = registry;
+		}
+
+		public AnnotationMetadata getImportingClassMetadata() {
+			return this.importingClassMetadata;
+		}
+
+		public BeanDefinitionRegistry getBeanDefinitionRegistry() {
+			return registry;
+		}
+	}
 
 }

org.springframework.context/src/main/java/org/springframework/scheduling/annotation/AsyncConfigurationSelector.java

@@ -39,12 +39,15 @@ import org.springframework.util.Assert;
 public class AsyncConfigurationSelector implements ImportSelector {
 
 	/**
-	 * Import {@link ProxyAsyncConfiguration} if {@link EnableAsync#mode()} equals
-	 * {@code PROXY}, otherwise import
+	 * {@inheritDoc}
+	 * <p>This implementation selects {@link ProxyAsyncConfiguration} if
+	 * {@link EnableAsync#mode()} equals {@code PROXY}, and otherwise selects
 	 * {@link org.springframework.scheduling.aspectj.AspectJAsyncConfiguration
-	 * AspectJAsyncConfiguration}.
+	 * AspectJAsyncConfiguration}. No additional {@code BeanDefinition}s are registered
+	 * in either case.
 	 */
-	public String[] selectImports(AnnotationMetadata importingClassMetadata) {
+	public String[] selectImports(ImportSelector.Context context) {
+		AnnotationMetadata importingClassMetadata = context.getImportingClassMetadata();
 		Map<String, Object> enableAsync =
 			importingClassMetadata.getAnnotationAttributes(EnableAsync.class.getName());
 		Assert.notNull(enableAsync,

org.springframework.integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java

@@ -22,6 +22,7 @@ import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
@@ -32,9 +33,14 @@ import org.junit.Test;
 import org.springframework.aop.Advisor;
 import org.springframework.aop.framework.Advised;
 import org.springframework.aop.support.AopUtils;
+import org.springframework.cache.Cache;
+import org.springframework.cache.CacheManager;
+import org.springframework.cache.concurrent.ConcurrentMapCache;
+import org.springframework.cache.support.SimpleCacheManager;
 import org.springframework.context.annotation.AnnotationConfigApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.ImportResource;
 import org.springframework.context.config.AdviceMode;
 import org.springframework.jdbc.datasource.DataSourceTransactionManager;
 import org.springframework.jdbc.datasource.embedded.EmbeddedDatabaseBuilder;
@@ -152,6 +158,38 @@ public class EnableTransactionManagementIntegrationTests {
 		assertThat(txManager2.rollbacks, equalTo(0));
 	}
 
+	@Test
+	public void apcEscalation() {
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(EnableTxAndCachingConfig.class);
+		ctx.refresh();
+	}
+
+
+	@Configuration
+	@EnableTransactionManagement
+	@ImportResource("org/springframework/transaction/annotation/enable-caching.xml")
+	static class EnableTxAndCachingConfig {
+		@Bean
+		public PlatformTransactionManager txManager() {
+			return new CallCountingTransactionManager();
+		}
+
+		@Bean
+		public FooRepository fooRepository() {
+			return new DummyFooRepository();
+		}
+
+		@Bean
+		public CacheManager cacheManager() {
+			SimpleCacheManager mgr = new SimpleCacheManager();
+			ArrayList<Cache> caches = new ArrayList<Cache>();
+			caches.add(new ConcurrentMapCache());
+			mgr.setCaches(caches);
+			return mgr;
+		}
+	}
+
 
 	@Configuration
 	@EnableTransactionManagement

org.springframework.integration-tests/src/test/resources/org/springframework/transaction/annotation/enable-caching.xml

@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<beans xmlns="http://www.springframework.org/schema/beans"
+	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+	xmlns:cache="http://www.springframework.org/schema/cache"
+	xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans.xsd
+		http://www.springframework.org/schema/cache http://www.springframework.org/schema/cache/spring-cache.xsd">
+
+	<cache:annotation-driven/>
+
+</beans>

org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/ProxyTransactionManagementConfiguration.java

@@ -16,8 +16,6 @@
 
 package org.springframework.transaction.annotation;
 
-import org.springframework.aop.config.AopConfigUtils;
-import org.springframework.aop.framework.autoproxy.InfrastructureAdvisorAutoProxyCreator;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
@@ -67,12 +65,4 @@ public class ProxyTransactionManagementConfiguration extends AbstractTransaction
 		return interceptor;
 	}
 
-	// TODO: deal with escalation of APCs
-	@Bean(name=AopConfigUtils.AUTO_PROXY_CREATOR_BEAN_NAME)
-	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
-	public InfrastructureAdvisorAutoProxyCreator apc() {
-		InfrastructureAdvisorAutoProxyCreator apc = new InfrastructureAdvisorAutoProxyCreator();
-		apc.setProxyTargetClass((Boolean) this.enableTx.get("proxyTargetClass"));
-		return apc;
-	}
 }

org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/TransactionManagementConfigurationSelector.java

@@ -18,6 +18,9 @@ package org.springframework.transaction.annotation;
 
 import java.util.Map;
 
+import org.springframework.aop.config.AopConfigUtils;
+import org.springframework.beans.factory.support.BeanDefinitionRegistry;
+import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.ImportSelector;
 import org.springframework.context.config.AdviceMode;
 import org.springframework.core.type.AnnotationMetadata;
@@ -38,12 +41,19 @@ import org.springframework.util.Assert;
 public class TransactionManagementConfigurationSelector implements ImportSelector {
 
 	/**
-	 * Import {@link ProxyTransactionManagementConfiguration} if {@link
-	 * EnableTransactionManagement#mode()} equals {@code PROXY}, otherwise import {@link
-	 * org.springframework.transaction.aspectj.AspectJTransactionManagementConfiguration
+	 * {@inheritDoc}
+	 * <p>This implementation selects {@link ProxyTransactionManagementConfiguration} if
+	 * {@link EnableTransactionManagement#mode()} equals {@code PROXY}, and  otherwise selects
+	 * {@link org.springframework.transaction.aspectj.AspectJTransactionManagementConfiguration
 	 * AspectJTransactionManagementConfiguration}.
+	 * <p>If {@code #mode()} equals {@code PROXY}, an auto-proxy creator bean definition
+	 * will also be added to the enclosing {@link BeanDefinitionRegistry} and escalated
+	 * if necessary through the usual {@link AopConfigUtils} family of methods.
 	 */
-	public String[] selectImports(AnnotationMetadata importingClassMetadata) {
+	public String[] selectImports(ImportSelector.Context context) {
+		AnnotationMetadata importingClassMetadata = context.getImportingClassMetadata();
+		BeanDefinitionRegistry registry = context.getBeanDefinitionRegistry();
+
 		Map<String, Object> enableTx =
 			importingClassMetadata.getAnnotationAttributes(EnableTransactionManagement.class.getName());
 		Assert.notNull(enableTx,
@@ -52,7 +62,11 @@ public class TransactionManagementConfigurationSelector implements ImportSelecto
 
 		switch ((AdviceMode) enableTx.get("mode")) {
 			case PROXY:
-				return new String[] {ProxyTransactionManagementConfiguration.class.getName()};
+				AopConfigUtils.registerAutoProxyCreatorIfNecessary(registry);
+				if ((Boolean)enableTx.get("proxyTargetClass")) {
+					AopConfigUtils.forceAutoProxyCreatorToUseClassProxying(registry);
+				}
+				return new String[] { ProxyTransactionManagementConfiguration.class.getName() };
 			case ASPECTJ:
 				return new String[] {"org.springframework.transaction.aspectj.AspectJTransactionManagementConfiguration"};
 			default: