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

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<TransactionInfo>("Current aspect-driven transaction");
 
 
-	private final ConcurrentHashMap<String, PlatformTransactionManager> transactionManagerCache =
+	private final ConcurrentHashMap<Object, PlatformTransactionManager> transactionManagerCache =
-			new ConcurrentHashMap<String, PlatformTransactionManager>();
+			new ConcurrentHashMap<Object, PlatformTransactionManager>();
 
 	/**
 	 * 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,10 +342,8 @@ 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) {
+		if (this.beanFactory != null) {
-			return this.transactionManager;
+			String qualifier = txAttr != null ? txAttr.getQualifier() : null;
-		}
-		String qualifier = txAttr.getQualifier();
 			if (StringUtils.hasText(qualifier)) {
 				PlatformTransactionManager txManager = this.transactionManagerCache.get(qualifier);
 				if (txManager == null) {
@@ -361,13 +361,18 @@ public abstract class TransactionAspectSupport implements BeanFactoryAware, Init
 					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);
 				}
-		else {
+				return defaultTransactionManager;
-			// Look up the default transaction manager and cache it for subsequent calls
-			this.transactionManager = this.beanFactory.getBean(PlatformTransactionManager.class);
-			return this.transactionManager;
 			}
 		}
+		return getTransactionManager();
+	}
 
 	/**
 	 * Convenience method to return a String representation of this Method

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);
+		PlatformTransactionManager txManager = associateTransactionManager(beanFactory, "fooTransactionManager");
-		given(beanFactory.containsBean("fooTransactionManager")).willReturn(true);
-		given(beanFactory.getBean("fooTransactionManager", PlatformTransactionManager.class)).willReturn(txManager);
 
 		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);
+		PlatformTransactionManager txManager = 	associateTransactionManager(beanFactory, "fooTransactionManager");
-		given(beanFactory.getBean("fooTransactionManager", PlatformTransactionManager.class)).willReturn(txManager);
 
 		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();
+		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.