From f99ab1cdd66165ccb64afb6daebfa21135a87d18 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Mon, 11 Jul 2011 01:17:19 +0000 Subject: [PATCH] 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 --- .../annotation/ConfigurationClassParser.java | 10 +++-- .../context/annotation/ImportSelector.java | 44 +++++++++++++++++-- .../AsyncConfigurationSelector.java | 11 +++-- ...TransactionManagementIntegrationTests.java | 38 ++++++++++++++++ .../transaction/annotation/enable-caching.xml | 10 +++++ ...oxyTransactionManagementConfiguration.java | 10 ----- ...actionManagementConfigurationSelector.java | 24 +++++++--- 7 files changed, 121 insertions(+), 26 deletions(-) create mode 100644 org.springframework.integration-tests/src/test/resources/org/springframework/transaction/annotation/enable-caching.xml diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java index 036766d2cea..a09f033833e 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ConfigurationClassParser.java +++ b/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); } diff --git a/org.springframework.context/src/main/java/org/springframework/context/annotation/ImportSelector.java b/org.springframework.context/src/main/java/org/springframework/context/annotation/ImportSelector.java index 5cc718f867a..76920bc97e9 100644 --- a/org.springframework.context/src/main/java/org/springframework/context/annotation/ImportSelector.java +++ b/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. * + *

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; + } + } } diff --git a/org.springframework.context/src/main/java/org/springframework/scheduling/annotation/AsyncConfigurationSelector.java b/org.springframework.context/src/main/java/org/springframework/scheduling/annotation/AsyncConfigurationSelector.java index 41539bb6f16..9f5791b46f6 100644 --- a/org.springframework.context/src/main/java/org/springframework/scheduling/annotation/AsyncConfigurationSelector.java +++ b/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} + *

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 enableAsync = importingClassMetadata.getAnnotationAttributes(EnableAsync.class.getName()); Assert.notNull(enableAsync, diff --git a/org.springframework.integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java b/org.springframework.integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java index e5e59cdee57..51bcaeebf5f 100644 --- a/org.springframework.integration-tests/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementIntegrationTests.java +++ b/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 caches = new ArrayList(); + caches.add(new ConcurrentMapCache()); + mgr.setCaches(caches); + return mgr; + } + } + @Configuration @EnableTransactionManagement diff --git a/org.springframework.integration-tests/src/test/resources/org/springframework/transaction/annotation/enable-caching.xml b/org.springframework.integration-tests/src/test/resources/org/springframework/transaction/annotation/enable-caching.xml new file mode 100644 index 00000000000..efc9ec29f30 --- /dev/null +++ b/org.springframework.integration-tests/src/test/resources/org/springframework/transaction/annotation/enable-caching.xml @@ -0,0 +1,10 @@ + + + + + + diff --git a/org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/ProxyTransactionManagementConfiguration.java b/org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/ProxyTransactionManagementConfiguration.java index 2dca350475b..48a6436ace1 100644 --- a/org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/ProxyTransactionManagementConfiguration.java +++ b/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; - } } diff --git a/org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/TransactionManagementConfigurationSelector.java b/org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/TransactionManagementConfigurationSelector.java index 69ce2b0b5fd..56290a8c00b 100644 --- a/org.springframework.transaction/src/main/java/org/springframework/transaction/annotation/TransactionManagementConfigurationSelector.java +++ b/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} + *

This implementation selects {@link ProxyTransactionManagementConfiguration} if + * {@link EnableTransactionManagement#mode()} equals {@code PROXY}, and otherwise selects + * {@link org.springframework.transaction.aspectj.AspectJTransactionManagementConfiguration * AspectJTransactionManagementConfiguration}. + *

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 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: