Bean overriding by type uses isAutowireCandidate for matching

This commit uses the bean factory `isAutowiredCandidate` method directly
in `BeanOverrideBeanFactoryPostProcessor` to select a single match among
multiple candidates when matching by type.

The expected consequence, in most cases, is that this will delegate to
a `@Qualifier`-aware `QualifierAnnotationAutowireCandidateResolver`.
In that sense, bean overriding by-type matching is now potentially
taking Qualifier annotations or meta-annotations into account.

It also changes the way existing bean definitions are checked in case
a bean name has been specified: factory beans are now taken into account
when checking the type of an existing definition matches the expected
bean override type.

Closes gh-32822

Author: simonbasle

framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-mockitobean.adoc

@@ -6,17 +6,18 @@ the test's `ApplicationContext` with a Mockito mock or spy, respectively. In the
 case, the original bean definition is not replaced, but instead an early instance of the
 bean is captured and wrapped by the spy.
 
-Users are encouraged to make bean overriding as explicit and unambiguous as possible,
-typically by specifying a bean `name` in the annotation.
-If no bean `name` is specified, the annotated field's type is used to search for candidate
-definitions to override.
+By default, the annotated field's type is used to search for candidate definitions to
+override, but note that `@Qualifier` annotations are also taken into account for the
+purpose of matching. Users can also make things entirely explicit by specifying a bean
+`name` in the annotation.
+
 Each annotation also defines Mockito-specific attributes to fine-tune the mocking details.
 
 The `@MockitoBean` annotation uses the `REPLACE_OR_CREATE_DEFINITION`
 xref:testing/testcontext-framework/bean-overriding.adoc#testcontext-bean-overriding-custom[strategy for test bean overriding].
 
-It requires that at most one candidate definition exists if a bean name is specified,
-or exactly one if no bean name is specified.
+It requires that at most one matching candidate definition exists if a bean name
+is specified, or exactly one if no bean name is specified.
 
 The `@MockitoSpyBean` annotation uses the `WRAP_BEAN`
 xref:testing/testcontext-framework/bean-overriding.adoc#testcontext-bean-overriding-custom[strategy],

framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-testbean.adoc

@@ -11,10 +11,10 @@ but the annotation allows for a specific method name to be provided.
 The `@TestBean` annotation uses the `REPLACE_DEFINITION`
 xref:testing/testcontext-framework/bean-overriding.adoc#testcontext-bean-overriding-custom[strategy for test bean overriding].
 
-Users are encouraged to make bean overriding as explicit and unambiguous as possible,
-typically by specifying a bean `name` in the annotation.
-If no bean `name` is specified, the annotated field's type is used to search for candidate
-definitions to override. In that case it is required that exactly one definition matches.
+By default, the annotated field's type is used to search for candidate definitions to override.
+In that case it is required that exactly one definition matches, but note that `@Qualifier`
+annotations are also taken into account for the purpose of matching.
+Users can also make things entirely explicit by specifying a bean `name` in the annotation.
 
 The following example shows how to fully configure the `@TestBean` annotation, with
 explicit values equivalent to the defaults:

framework-docs/modules/ROOT/pages/testing/testcontext-framework/bean-overriding.adoc

@@ -61,11 +61,11 @@ In contrast to Spring's autowiring mechanism (for example, resolution of an `@Au
 field), the bean overriding infrastructure in the TestContext framework has limited
 heuristics it can perform to locate a bean. Either the `BeanOverrideProcessor` can compute
 the name of the bean to override, or it can be unambiguously selected given the type of
-the annotated field.
+the annotated field and its qualifying annotations.
+
+Typically, the bean is selected by type by the `BeanOverrideFactoryPostProcessor`.
+Alternatively, the user can directly provide the bean name in the custom annotation.
 
-Typically, the user directly provides the bean name in the custom annotation in order to
-make things as explicit as possible. Alternatively, the bean is selected by type by the
-`BeanOverrideFactoryPostProcessor`.
 Some `BeanOverrideProcessor`s could also internally compute a bean name based on a
 convention or another advanced method.
 ====

spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessor.java

@@ -31,6 +31,7 @@
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanFactoryPostProcessor;
 import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
+import org.springframework.beans.factory.config.DependencyDescriptor;
 import org.springframework.beans.factory.config.SmartInstantiationAwareBeanPostProcessor;
 import org.springframework.beans.factory.support.BeanDefinitionRegistry;
 import org.springframework.beans.factory.support.RootBeanDefinition;
@@ -127,28 +128,34 @@ private void registerReplaceDefinition(ConfigurableListableBeanFactory beanFacto
 
 		RootBeanDefinition beanDefinition = createBeanDefinition(overrideMetadata);
 		String beanName = overrideMetadata.getBeanName();
+		BeanDefinition existingBeanDefinition = null;
 		if (beanName == null) {
-			final String[] candidates = beanFactory.getBeanNamesForType(overrideMetadata.getBeanType());
-			if (candidates.length != 1) {
+			Set<String> candidates = getExistingBeanNamesByType(beanFactory, overrideMetadata, true);
+			if (candidates.size() != 1) {
 				Field f = overrideMetadata.getField();
 				throw new IllegalStateException("Unable to select a bean definition to override, " +
-						candidates.length+ " bean definitions found of type " + overrideMetadata.getBeanType() +
+						candidates.size() + " bean definitions found of type " + overrideMetadata.getBeanType() +
 						" (as required by annotated field '" + f.getDeclaringClass().getSimpleName() +
 						"." + f.getName() + "')");
 			}
-			beanName = candidates[0];
+			beanName = candidates.iterator().next();
+			existingBeanDefinition = beanFactory.getBeanDefinition(beanName);
+		}
+		else {
+			Set<String> candidates = getExistingBeanNamesByType(beanFactory, overrideMetadata, false);
+			if (candidates.contains(beanName)) {
+				existingBeanDefinition = beanFactory.getBeanDefinition(beanName);
+			}
+			else if (enforceExistingDefinition) {
+				throw new IllegalStateException("Unable to override bean '" + beanName + "'; there is no" +
+						" bean definition to replace with that name of type " + overrideMetadata.getBeanType());
+			}
 		}
 
-		BeanDefinition existingBeanDefinition = null;
-		if (beanFactory.containsBeanDefinition(beanName)) {
-			existingBeanDefinition = beanFactory.getBeanDefinition(beanName);
+		if (existingBeanDefinition != null) {
 			copyBeanDefinitionDetails(existingBeanDefinition, beanDefinition);
 			registry.removeBeanDefinition(beanName);
 		}
-		else if (enforceExistingDefinition) {
-			throw new IllegalStateException("Unable to override bean '" + beanName + "'; there is no" +
-					" bean definition to replace with that name");
-		}
 		registry.registerBeanDefinition(beanName, beanDefinition);
 
 		Object override = overrideMetadata.createOverride(beanName, existingBeanDefinition, null);
@@ -171,33 +178,40 @@ else if (enforceExistingDefinition) {
 	 * phase.
 	 */
 	private void registerWrapBean(ConfigurableListableBeanFactory beanFactory, OverrideMetadata metadata) {
-		Set<String> existingBeanNames = getExistingBeanNames(beanFactory, metadata.getBeanType());
 		String beanName = metadata.getBeanName();
 		if (beanName == null) {
-			if (existingBeanNames.size() != 1) {
+			Set<String> candidateNames = getExistingBeanNamesByType(beanFactory, metadata, true);
+			if (candidateNames.size() != 1) {
 				Field f = metadata.getField();
 				throw new IllegalStateException("Unable to select a bean to override by wrapping, " +
-						existingBeanNames.size() + " bean instances found of type " + metadata.getBeanType() +
+						candidateNames.size() + " bean instances found of type " + metadata.getBeanType() +
 						" (as required by annotated field '" + f.getDeclaringClass().getSimpleName() +
 						"." + f.getName() + "')");
 			}
-			beanName = existingBeanNames.iterator().next();
+			beanName = candidateNames.iterator().next();
 		}
-		else if (!existingBeanNames.contains(beanName)) {
-			throw new IllegalStateException("Unable to override bean '" + beanName + "' by wrapping; " +
-					"there is no existing bean instance with that name of type " + metadata.getBeanType());
+		else {
+			Set<String> candidates = getExistingBeanNamesByType(beanFactory, metadata, false);
+			if (!candidates.contains(beanName)) {
+				throw new IllegalStateException("Unable to override bean '" + beanName + "' by wrapping; there is no" +
+						" existing bean instance with that name of type " + metadata.getBeanType());
+			}
 		}
 		this.overrideRegistrar.markWrapEarly(metadata, beanName);
 		this.overrideRegistrar.registerNameForMetadata(metadata, beanName);
 	}
 
-	private RootBeanDefinition createBeanDefinition(OverrideMetadata metadata) {
+	RootBeanDefinition createBeanDefinition(OverrideMetadata metadata) {
 		RootBeanDefinition definition = new RootBeanDefinition();
 		definition.setTargetType(metadata.getBeanType());
+		definition.setQualifiedElement(metadata.getField());
 		return definition;
 	}
 
-	private Set<String> getExistingBeanNames(ConfigurableListableBeanFactory beanFactory, ResolvableType resolvableType) {
+	private Set<String> getExistingBeanNamesByType(ConfigurableListableBeanFactory beanFactory, OverrideMetadata metadata,
+			boolean checkAutowiredCandidate) {
+
+		ResolvableType resolvableType = metadata.getBeanType();
 		Set<String> beans = new LinkedHashSet<>(
 				Arrays.asList(beanFactory.getBeanNamesForType(resolvableType, true, false)));
 		Class<?> type = resolvableType.resolve(Object.class);
@@ -209,7 +223,14 @@ private Set<String> getExistingBeanNames(ConfigurableListableBeanFactory beanFac
 				beans.add(beanName);
 			}
 		}
-		beans.removeIf(ScopedProxyUtils::isScopedTarget);
+		if (checkAutowiredCandidate) {
+			DependencyDescriptor descriptor = new DependencyDescriptor(metadata.getField(), true);
+			beans.removeIf(beanName -> ScopedProxyUtils.isScopedTarget(beanName) ||
+					!beanFactory.isAutowireCandidate(beanName, descriptor));
+		}
+		else {
+			beans.removeIf(ScopedProxyUtils::isScopedTarget);
+		}
 		return beans;
 	}
 

spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBean.java

@@ -30,8 +30,9 @@
  *
  * <p>By default, the bean to override is inferred from the type of the
  * annotated field. This requires that exactly one matching definition is
- * present in the application context. To explicitly specify a bean name to
- * replace, set the {@link #value()} or {@link #name()} attribute.
+ * present in the application context. A {@code @Qualifier} annotation can be
+ * used to help disambiguate. Alternatively, you can explicitly specify a bean
+ * name to replace by setting the {@link #value()} or {@link #name()} attribute.
  *
  * <p>The instance is created from a zero-argument static factory method in the
  * test class whose return type is compatible with the annotated field. In the

spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoBean.java

@@ -32,7 +32,8 @@
  *
  * <p>If no explicit {@link #name()} is specified, a target bean definition is
  * selected according to the class of the annotated field, and there must be
- * exactly one such candidate definition in the context.
+ * exactly one such candidate definition in the context. A {@code @Qualifier}
+ * annotation can be used to help disambiguate.
  * If a {@link #name()} is specified, either the definition exists in the
  * application context and is replaced, or it doesn't and a new one is added to
  * the context.

spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBean.java

@@ -32,7 +32,8 @@
  *
  * <p>If no explicit {@link #name()} is specified, a target bean is selected
  * according to the class of the annotated field, and there must be exactly one
- * such candidate bean.
+ * such candidate bean. A {@code @Qualifier} annotation can be used to help
+ * disambiguate.
  * If a {@link #name()} is specified, it is required that a target bean of that
  * name has been previously registered in the application context.
  *

spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessorTests.java

@@ -16,6 +16,7 @@
 
 package org.springframework.test.context.bean.override;
 
+import java.lang.reflect.Field;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Predicate;
@@ -24,6 +25,7 @@
 
 import org.springframework.beans.BeanWrapper;
 import org.springframework.beans.factory.FactoryBean;
+import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.beans.factory.config.BeanDefinition;
 import org.springframework.beans.factory.config.BeanFactoryPostProcessor;
 import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
@@ -75,8 +77,8 @@ void cannotReplaceIfNoBeanMatching() {
 
 		assertThatIllegalStateException()
 				.isThrownBy(context::refresh)
-				.withMessage("Unable to override bean 'explicit'; " +
-						"there is no bean definition to replace with that name");
+				.withMessage("Unable to override bean 'explicit'; there is no bean definition " +
+						"to replace with that name of type org.springframework.test.context.bean.override.example.ExampleService");
 	}
 
 	@Test
@@ -176,6 +178,26 @@ void copyDefinitionPrimaryFallbackAndScope() {
 				.matches(Predicate.not(BeanDefinition::isPrototype), "!isPrototype");
 	}
 
+	@Test
+	void createDefinitionShouldSetQualifierElement() {
+		AnnotationConfigApplicationContext context = createContext(QualifiedBean.class);
+		context.registerBeanDefinition("singleton", new RootBeanDefinition(String.class, () -> "ORIGINAL"));
+		context.register(QualifiedBean.class);
+
+		assertThatNoException().isThrownBy(context::refresh);
+
+		assertThat(context.getBeanDefinition("singleton"))
+				.isInstanceOfSatisfying(RootBeanDefinition.class, this::isTheValueField);
+	}
+
+
+	private void isTheValueField(RootBeanDefinition def) {
+		assertThat(def.getQualifiedElement()).isInstanceOfSatisfying(Field.class, field -> {
+					assertThat(field.getDeclaringClass()).isEqualTo(QualifiedBean.class);
+					assertThat(field.getName()).as("annotated field name")
+							.isEqualTo("value");
+				});
+	}
 
 	private AnnotationConfigApplicationContext createContext(Class<?>... classes) {
 		AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext();
@@ -260,6 +282,18 @@ static String useThis() {
 		}
 	}
 
+	static class QualifiedBean {
+
+		@Qualifier("preferThis")
+		@ExampleBeanOverrideAnnotation(beanName = "singleton",
+				value = "useThis", createIfMissing = false)
+		private String value;
+
+		static String useThis() {
+			return "USED THIS";
+		}
+	}
+
 	static class TestFactoryBean implements FactoryBean<Object> {
 
 		@Override

spring-test/src/test/java/org/springframework/test/context/bean/override/convention/TestBeanByTypeIntegrationTests.java

@@ -21,9 +21,11 @@
 import org.junit.platform.testkit.engine.EngineExecutionResults;
 import org.junit.platform.testkit.engine.EngineTestKit;
 
+import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.test.context.bean.override.example.CustomQualifier;
 import org.springframework.test.context.bean.override.example.ExampleService;
 import org.springframework.test.context.bean.override.example.RealExampleService;
 import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;
@@ -39,10 +41,26 @@ public class TestBeanByTypeIntegrationTests {
 	@TestBean
 	ExampleService anyNameForService;
 
+	@TestBean(methodName = "someString")
+	@Qualifier("prefer")
+	StringBuilder anyNameForStringBuilder;
+
+	@TestBean(methodName = "someString2")
+	@CustomQualifier
+	StringBuilder anyNameForStringBuilder2;
+
 	static ExampleService anyNameForServiceTestOverride() {
 		return new RealExampleService("Mocked greeting");
 	}
 
+	static StringBuilder someString() {
+		return new StringBuilder("Prefer TestBean String");
+	}
+
+	static StringBuilder someString2() {
+		return new StringBuilder("CustomQualifier TestBean String");
+	}
+
 	@Test
 	void overrideIsFoundByType(ApplicationContext ctx) {
 		assertThat(this.anyNameForService)
@@ -52,6 +70,21 @@ void overrideIsFoundByType(ApplicationContext ctx) {
 		assertThat(this.anyNameForService.greeting()).isEqualTo("Mocked greeting");
 	}
 
+	@Test
+	void overrideIsFoundByTypeWithQualifierDisambiguation(ApplicationContext ctx) {
+		assertThat(this.anyNameForStringBuilder)
+				.as("direct qualifier")
+				.isSameAs(ctx.getBean("two"))
+				.hasToString("Prefer TestBean String");
+
+		assertThat(this.anyNameForStringBuilder2)
+				.as("meta qualifier")
+				.isSameAs(ctx.getBean("three"))
+				.hasToString("CustomQualifier TestBean String");
+
+		assertThat(ctx.getBean("one")).as("no qualifier needed").hasToString("Prod One");
+	}
+
 	@Test
 	void zeroCandidates() {
 		Class<?> caseClass = CaseNone.class;
@@ -92,6 +125,23 @@ static class ConfigByType {
 		ExampleService bean1() {
 			return new RealExampleService("Production hello");
 		}
+
+		@Bean("one")
+		StringBuilder beanString1() {
+			return new StringBuilder("Prod One");
+		}
+
+		@Bean("two")
+		@Qualifier("prefer")
+		StringBuilder beanString2() {
+			return new StringBuilder("Prod Two");
+		}
+
+		@Bean("three")
+		@CustomQualifier
+		StringBuilder beanString3() {
+			return new StringBuilder("Prod Three");
+		}
 	}
 
 	@SpringJUnitConfig(FailingNone.class)