From cec26e9ac4af524575d6c5a2e2cd15dca900b8ca Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 16 Dec 2014 15:05:00 +0100 Subject: [PATCH] Rework determineTransactionManager condition SPR-11954 introduced a regression that when the "default" transaction manager is cached, qualified transaction managers are not taken into account anymore. This commit rework the "determineTransactionManager" condition to favor qualifier and "named" transaction managers. If none of these apply, the default transaction manager is used as it should. Also reworked the caching infrastructure so that a single cache holds all transaction manager instances. Issue: SPR-12541 --- .../interceptor/TransactionAspectSupport.java | 73 ++++++++++--------- .../TransactionInterceptorTests.java | 73 +++++++++++++++++-- 2 files changed, 105 insertions(+), 41 deletions(-) diff --git a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java index 20e15440da..dcc512f878 100644 --- a/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java +++ b/spring-tx/src/main/java/org/springframework/transaction/interceptor/TransactionAspectSupport.java @@ -67,6 +67,11 @@ import org.springframework.util.StringUtils; */ public abstract class TransactionAspectSupport implements BeanFactoryAware, InitializingBean { + /** + * Key to use to store the default transaction manager. + */ + private final Object DEFAULT_TRANSACTION_MANAGER_KEY = new Object(); + // NOTE: This class must not implement Serializable because it serves as base // class for AspectJ aspects (which are not allowed to implement Serializable)! @@ -80,8 +85,8 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init new NamedThreadLocal("Current aspect-driven transaction"); - private final ConcurrentHashMap transactionManagerCache = - new ConcurrentHashMap(); + private final ConcurrentHashMap transactionManagerCache = + new ConcurrentHashMap(); /** * Subclasses can use this to return the current TransactionInfo. @@ -127,11 +132,6 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init */ private String transactionManagerBeanName; - /** - * Default transaction manager. - */ - private PlatformTransactionManager transactionManager; - private TransactionAttributeSource transactionAttributeSource; private BeanFactory beanFactory; @@ -159,14 +159,16 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init * @see #setTransactionManagerBeanName */ public void setTransactionManager(PlatformTransactionManager transactionManager) { - this.transactionManager = transactionManager; + if (transactionManager != null) { + this.transactionManagerCache.put(DEFAULT_TRANSACTION_MANAGER_KEY, transactionManager); + } } /** * Return the default transaction manager, or {@code null} if unknown. */ public PlatformTransactionManager getTransactionManager() { - return this.transactionManager; + return this.transactionManagerCache.get(DEFAULT_TRANSACTION_MANAGER_KEY); } /** @@ -239,7 +241,7 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init */ @Override public void afterPropertiesSet() { - if (this.transactionManager == null && this.beanFactory == null) { + if (getTransactionManager() == null && this.beanFactory == null) { throw new IllegalStateException( "Setting the property 'transactionManager' or running in a ListableBeanFactory is required"); } @@ -340,33 +342,36 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init * Determine the specific transaction manager to use for the given transaction. */ protected PlatformTransactionManager determineTransactionManager(TransactionAttribute txAttr) { - if (this.transactionManager != null || this.beanFactory == null || txAttr == null) { - return this.transactionManager; - } - String qualifier = txAttr.getQualifier(); - if (StringUtils.hasText(qualifier)) { - PlatformTransactionManager txManager = this.transactionManagerCache.get(qualifier); - if (txManager == null) { - txManager = BeanFactoryAnnotationUtils.qualifiedBeanOfType( - this.beanFactory, PlatformTransactionManager.class, qualifier); - this.transactionManagerCache.putIfAbsent(qualifier, txManager); + if (this.beanFactory != null) { + String qualifier = txAttr != null ? txAttr.getQualifier() : null; + if (StringUtils.hasText(qualifier)) { + PlatformTransactionManager txManager = this.transactionManagerCache.get(qualifier); + if (txManager == null) { + txManager = BeanFactoryAnnotationUtils.qualifiedBeanOfType( + this.beanFactory, PlatformTransactionManager.class, qualifier); + this.transactionManagerCache.putIfAbsent(qualifier, txManager); + } + return txManager; } - return txManager; - } - else if (StringUtils.hasText(this.transactionManagerBeanName)) { - PlatformTransactionManager txManager = this.transactionManagerCache.get(this.transactionManagerBeanName); - if (txManager == null) { - txManager = this.beanFactory.getBean( - this.transactionManagerBeanName, PlatformTransactionManager.class); - this.transactionManagerCache.putIfAbsent(this.transactionManagerBeanName, txManager); + else if (StringUtils.hasText(this.transactionManagerBeanName)) { + PlatformTransactionManager txManager = this.transactionManagerCache.get(this.transactionManagerBeanName); + if (txManager == null) { + txManager = this.beanFactory.getBean( + this.transactionManagerBeanName, PlatformTransactionManager.class); + this.transactionManagerCache.putIfAbsent(this.transactionManagerBeanName, txManager); + } + return txManager; + } else { + PlatformTransactionManager defaultTransactionManager = getTransactionManager(); + if (defaultTransactionManager == null) { + defaultTransactionManager = this.beanFactory.getBean(PlatformTransactionManager.class); + this.transactionManagerCache.putIfAbsent( + DEFAULT_TRANSACTION_MANAGER_KEY, defaultTransactionManager); + } + return defaultTransactionManager; } - return txManager; - } - else { - // Look up the default transaction manager and cache it for subsequent calls - this.transactionManager = this.beanFactory.getBean(PlatformTransactionManager.class); - return this.transactionManager; } + return getTransactionManager(); } /** diff --git a/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java b/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java index 4b4c7af169..1613357d5d 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/interceptor/TransactionInterceptorTests.java @@ -123,6 +123,22 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests assertTrue(ctas.getTransactionAttributeSources()[1] instanceof NameMatchTransactionAttributeSource); } + @Test + public void determineTransactionManagerWithNoBeanFactory() { + PlatformTransactionManager transactionManager = mock(PlatformTransactionManager.class); + TransactionInterceptor ti = createTestTransactionInterceptor(null, transactionManager); + + assertSame(transactionManager, ti.determineTransactionManager(new DefaultTransactionAttribute())); + } + + @Test + public void determineTransactionManagerWithNoBeanFactoryAndNoTransactionAttribute() { + PlatformTransactionManager transactionManager = mock(PlatformTransactionManager.class); + TransactionInterceptor ti = createTestTransactionInterceptor(null, transactionManager); + + assertSame(transactionManager, ti.determineTransactionManager(null)); + } + @Test public void determineTransactionManagerWithQualifierUnknown() { BeanFactory beanFactory = mock(BeanFactory.class); @@ -135,14 +151,41 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests ti.determineTransactionManager(attribute); } + @Test + public void determineTransactionManagerWithQualifierAndDefault() { + BeanFactory beanFactory = mock(BeanFactory.class); + PlatformTransactionManager transactionManager = mock(PlatformTransactionManager.class); + TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory, transactionManager); + PlatformTransactionManager fooTransactionManager = + associateTransactionManager(beanFactory, "fooTransactionManager"); + + DefaultTransactionAttribute attribute = new DefaultTransactionAttribute(); + attribute.setQualifier("fooTransactionManager"); + + assertSame(fooTransactionManager, ti.determineTransactionManager(attribute)); + } + + @Test + public void determineTransactionManagerWithQualifierAndDefaultName() { + BeanFactory beanFactory = mock(BeanFactory.class); + associateTransactionManager(beanFactory, "defaultTransactionManager"); + TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory); + ti.setTransactionManagerBeanName("defaultTransactionManager"); + + PlatformTransactionManager fooTransactionManager = + associateTransactionManager(beanFactory, "fooTransactionManager"); + DefaultTransactionAttribute attribute = new DefaultTransactionAttribute(); + attribute.setQualifier("fooTransactionManager"); + + assertSame(fooTransactionManager, ti.determineTransactionManager(attribute)); + } + @Test public void determineTransactionManagerWithQualifierSeveralTimes() { BeanFactory beanFactory = mock(BeanFactory.class); TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory); - PlatformTransactionManager txManager = mock(PlatformTransactionManager.class); - given(beanFactory.containsBean("fooTransactionManager")).willReturn(true); - given(beanFactory.getBean("fooTransactionManager", PlatformTransactionManager.class)).willReturn(txManager); + PlatformTransactionManager txManager = associateTransactionManager(beanFactory, "fooTransactionManager"); DefaultTransactionAttribute attribute = new DefaultTransactionAttribute(); attribute.setQualifier("fooTransactionManager"); @@ -162,8 +205,7 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests TransactionInterceptor ti = createTestTransactionInterceptor(beanFactory); ti.setTransactionManagerBeanName("fooTransactionManager"); - PlatformTransactionManager txManager = mock(PlatformTransactionManager.class); - given(beanFactory.getBean("fooTransactionManager", PlatformTransactionManager.class)).willReturn(txManager); + PlatformTransactionManager txManager = associateTransactionManager(beanFactory, "fooTransactionManager"); DefaultTransactionAttribute attribute = new DefaultTransactionAttribute(); PlatformTransactionManager actual = ti.determineTransactionManager(attribute); @@ -193,14 +235,31 @@ public class TransactionInterceptorTests extends AbstractTransactionAspectTests verify(beanFactory, times(1)).getBean(PlatformTransactionManager.class); } - private TransactionInterceptor createTestTransactionInterceptor(BeanFactory beanFactory) { + private TransactionInterceptor createTestTransactionInterceptor(BeanFactory beanFactory, + PlatformTransactionManager transactionManager) { TransactionInterceptor ti = new TransactionInterceptor(); - ti.setBeanFactory(beanFactory); + if (beanFactory != null) { + ti.setBeanFactory(beanFactory); + } + if (transactionManager != null) { + ti.setTransactionManager(transactionManager); + } ti.setTransactionAttributeSource(new NameMatchTransactionAttributeSource()); ti.afterPropertiesSet(); return ti; } + private TransactionInterceptor createTestTransactionInterceptor(BeanFactory beanFactory) { + return createTestTransactionInterceptor(beanFactory, null); + } + + private PlatformTransactionManager associateTransactionManager(BeanFactory beanFactory, String name) { + PlatformTransactionManager transactionManager = mock(PlatformTransactionManager.class); + given(beanFactory.containsBean(name)).willReturn(true); + given(beanFactory.getBean(name, PlatformTransactionManager.class)).willReturn(transactionManager); + return transactionManager; + } + /** * We won't use this: we just want to know it's serializable.