From ff72652890d1a476fa3506facc92ffafa0e28a59 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 13 Jan 2025 13:03:25 +0100 Subject: [PATCH 1/2] Defensively check expected type for qualified bean Closes gh-34187 --- .../BeanFactoryAnnotationUtils.java | 18 ++-- .../EnableTransactionManagementTests.java | 84 ++++++++++++++----- .../TransactionInterceptorTests.java | 3 +- 3 files changed, 76 insertions(+), 29 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/BeanFactoryAnnotationUtils.java b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/BeanFactoryAnnotationUtils.java index 7df476fe540..0e1ecd046ff 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/annotation/BeanFactoryAnnotationUtils.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/annotation/BeanFactoryAnnotationUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -95,7 +95,7 @@ public abstract class BeanFactoryAnnotationUtils { // Full qualifier matching supported. return qualifiedBeanOfType(lbf, beanType, qualifier); } - else if (beanFactory.containsBean(qualifier)) { + else if (beanFactory.containsBean(qualifier) && beanFactory.isTypeMatch(qualifier, beanType)) { // Fallback: target bean at least found by bean name. return beanFactory.getBean(qualifier, beanType); } @@ -110,16 +110,16 @@ public abstract class BeanFactoryAnnotationUtils { /** * Obtain a bean of type {@code T} from the given {@code BeanFactory} declaring a qualifier * (for example, {@code } or {@code @Qualifier}) matching the given qualifier). - * @param bf the factory to get the target bean from + * @param beanFactory the factory to get the target bean from * @param beanType the type of bean to retrieve * @param qualifier the qualifier for selecting between multiple bean matches * @return the matching bean of type {@code T} (never {@code null}) */ - private static T qualifiedBeanOfType(ListableBeanFactory bf, Class beanType, String qualifier) { - String[] candidateBeans = BeanFactoryUtils.beanNamesForTypeIncludingAncestors(bf, beanType); + private static T qualifiedBeanOfType(ListableBeanFactory beanFactory, Class beanType, String qualifier) { + String[] candidateBeans = BeanFactoryUtils.beanNamesForTypeIncludingAncestors(beanFactory, beanType); String matchingBean = null; for (String beanName : candidateBeans) { - if (isQualifierMatch(qualifier::equals, beanName, bf)) { + if (isQualifierMatch(qualifier::equals, beanName, beanFactory)) { if (matchingBean != null) { throw new NoUniqueBeanDefinitionException(beanType, matchingBean, beanName); } @@ -127,11 +127,11 @@ public abstract class BeanFactoryAnnotationUtils { } } if (matchingBean != null) { - return bf.getBean(matchingBean, beanType); + return beanFactory.getBean(matchingBean, beanType); } - else if (bf.containsBean(qualifier)) { + else if (beanFactory.containsBean(qualifier) && beanFactory.isTypeMatch(qualifier, beanType)) { // Fallback: target bean at least found by bean name - probably a manually registered singleton. - return bf.getBean(qualifier, beanType); + return beanFactory.getBean(qualifier, beanType); } else { throw new NoSuchBeanDefinitionException(qualifier, "No matching " + beanType.getSimpleName() + diff --git a/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java b/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java index add722ae685..b3849246c07 100644 --- a/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java +++ b/spring-tx/src/test/java/org/springframework/transaction/annotation/EnableTransactionManagementTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -58,6 +58,7 @@ import static org.springframework.transaction.annotation.RollbackOn.ALL_EXCEPTIO * @author Juergen Hoeller * @author Stephane Nicoll * @author Sam Brannen + * @author Yanming Zhou * @since 3.1 */ class EnableTransactionManagementTests { @@ -243,8 +244,8 @@ class EnableTransactionManagementTests { } @Test - void spr11915TransactionManagerAsManualSingleton() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Spr11915Config.class); + void transactionManagerAsManualSingleton() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(ManualSingletonConfig.class); TransactionalTestBean bean = ctx.getBean(TransactionalTestBean.class); CallCountingTransactionManager txManager = ctx.getBean("qualifiedTransactionManager", CallCountingTransactionManager.class); @@ -264,25 +265,49 @@ class EnableTransactionManagementTests { } @Test - void gh24291TransactionManagerViaQualifierAnnotation() { - AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Gh24291Config.class); - TransactionalTestBean bean = ctx.getBean(TransactionalTestBean.class); - CallCountingTransactionManager txManager = ctx.getBean("qualifiedTransactionManager", CallCountingTransactionManager.class); + void transactionManagerViaQualifierAnnotation() { + AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(QualifiedTransactionConfig.class); + + TransactionalTestBean bean = ctx.getBean("testBean", TransactionalTestBean.class); + TransactionalTestBeanWithNonExistentQualifier beanWithNonExistentQualifier = ctx.getBean( + "testBeanWithNonExistentQualifier", TransactionalTestBeanWithNonExistentQualifier.class); + TransactionalTestBeanWithInvalidQualifier beanWithInvalidQualifier = ctx.getBean( + "testBeanWithInvalidQualifier", TransactionalTestBeanWithInvalidQualifier.class); + + CallCountingTransactionManager qualified = ctx.getBean("qualifiedTransactionManager", + CallCountingTransactionManager.class); + CallCountingTransactionManager primary = ctx.getBean("primaryTransactionManager", + CallCountingTransactionManager.class); bean.saveQualifiedFoo(); - assertThat(txManager.begun).isEqualTo(1); - assertThat(txManager.commits).isEqualTo(1); - assertThat(txManager.rollbacks).isEqualTo(0); + assertThat(qualified.begun).isEqualTo(1); + assertThat(qualified.commits).isEqualTo(1); + assertThat(qualified.rollbacks).isEqualTo(0); bean.saveQualifiedFooWithAttributeAlias(); - assertThat(txManager.begun).isEqualTo(2); - assertThat(txManager.commits).isEqualTo(2); - assertThat(txManager.rollbacks).isEqualTo(0); + assertThat(qualified.begun).isEqualTo(2); + assertThat(qualified.commits).isEqualTo(2); + assertThat(qualified.rollbacks).isEqualTo(0); bean.findAllFoos(); - assertThat(txManager.begun).isEqualTo(3); - assertThat(txManager.commits).isEqualTo(3); - assertThat(txManager.rollbacks).isEqualTo(0); + assertThat(qualified.begun).isEqualTo(3); + assertThat(qualified.commits).isEqualTo(3); + assertThat(qualified.rollbacks).isEqualTo(0); + + beanWithNonExistentQualifier.findAllFoos(); + assertThat(primary.begun).isEqualTo(1); + assertThat(primary.commits).isEqualTo(1); + assertThat(primary.rollbacks).isEqualTo(0); + + beanWithInvalidQualifier.findAllFoos(); + assertThat(primary.begun).isEqualTo(2); + assertThat(primary.commits).isEqualTo(2); + assertThat(primary.rollbacks).isEqualTo(0); + + // no further access to qualified transaction manager + assertThat(qualified.begun).isEqualTo(3); + assertThat(qualified.commits).isEqualTo(3); + assertThat(qualified.rollbacks).isEqualTo(0); ctx.close(); } @@ -386,6 +411,16 @@ class EnableTransactionManagementTests { public static class TransactionalTestBeanSubclass extends TransactionalTestBean { } + @Service + @Qualifier("nonExistentBean") + public static class TransactionalTestBeanWithNonExistentQualifier extends TransactionalTestBean { + } + + @Service + @Qualifier("transactionalTestBeanWithInvalidQualifier") + public static class TransactionalTestBeanWithInvalidQualifier extends TransactionalTestBean { + } + @Configuration static class PlaceholderConfig { @@ -558,7 +593,7 @@ class EnableTransactionManagementTests { @Configuration @EnableTransactionManagement @Import(PlaceholderConfig.class) - static class Spr11915Config { + static class ManualSingletonConfig { @Autowired public void initializeApp(ConfigurableApplicationContext applicationContext) { @@ -581,7 +616,7 @@ class EnableTransactionManagementTests { @Configuration @EnableTransactionManagement @Import(PlaceholderConfig.class) - static class Gh24291Config { + static class QualifiedTransactionConfig { @Autowired public void initializeApp(ConfigurableApplicationContext applicationContext) { @@ -596,7 +631,18 @@ class EnableTransactionManagementTests { } @Bean - public CallCountingTransactionManager otherTxManager() { + public TransactionalTestBeanWithNonExistentQualifier testBeanWithNonExistentQualifier() { + return new TransactionalTestBeanWithNonExistentQualifier(); + } + + @Bean + public TransactionalTestBeanWithInvalidQualifier testBeanWithInvalidQualifier() { + return new TransactionalTestBeanWithInvalidQualifier(); + } + + @Bean + @Primary + public CallCountingTransactionManager primaryTransactionManager() { return new CallCountingTransactionManager(); } } 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 adea2bb54a8..de82fa7c9d0 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -296,6 +296,7 @@ class TransactionInterceptorTests extends AbstractTransactionAspectTests { private PlatformTransactionManager associateTransactionManager(BeanFactory beanFactory, String name) { PlatformTransactionManager transactionManager = mock(); given(beanFactory.containsBean(name)).willReturn(true); + given(beanFactory.isTypeMatch(name, TransactionManager.class)).willReturn(true); given(beanFactory.getBean(name, TransactionManager.class)).willReturn(transactionManager); return transactionManager; } From 8771b9ea21016efe9969b21ac8b937f7fc2d1ceb Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Mon, 13 Jan 2025 13:04:42 +0100 Subject: [PATCH 2/2] Defensively acquire singleton lock for FactoryBean type check Closes gh-34247 --- .../support/AbstractAutowireCapableBeanFactory.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java index 1ddddbe82db..131c0313cfd 100644 --- a/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java +++ b/spring-beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -991,7 +991,11 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac */ @Nullable private FactoryBean getSingletonFactoryBeanForTypeCheck(String beanName, RootBeanDefinition mbd) { - this.singletonLock.lock(); + boolean locked = this.singletonLock.tryLock(); + if (!locked) { + return null; + } + try { BeanWrapper bw = this.factoryBeanInstanceCache.get(beanName); if (bw != null) {