Reject non-singletons in Test Bean Override support

Prior to this commit, a non-singleton FactoryBean was silently replaced
by a singleton bean. In addition, bean definitions for prototype-scoped
and custom-scoped beans were replaced by singleton bean definitions
that were incapable of creating the desired bean instance. For example,
if the bean type of the original bean definition was a concrete class,
an attempt was made to invoke the default constructor which either
succeeded with undesirable results or failed with an exception if the
bean type did not have a default constructor. If the bean type of the
original bean definition was an interface or a FactoryBean that claimed
to create a bean of a certain interface type, an attempt was made to
instantiate the interface which always failed with a
BeanCreationException.

To address the aforementioned issues, this commit reworks the logic in
BeanOverrideBeanFactoryPostProcessor so that an exception is thrown
whenever an attempt is made to override a non-singleton bean.

Closes gh-33602

Author: sbrannen

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

@@ -36,6 +36,9 @@ xref:testing/testcontext-framework/bean-overriding.adoc#testcontext-bean-overrid
 and the original instance is wrapped in a Mockito spy. This strategy requires that
 exactly one candidate bean definition exists.
 
+NOTE: Only _singleton_ beans can be overridden. Any attempt to override a non-singleton
+bean will result in an exception.
+
 The following example shows how to use the default behavior of the `@MockitoBean` annotation:
 
 [tabs]

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

@@ -87,3 +87,6 @@ Alternatively, a factory method in an external class can be referenced via its
 fully-qualified method name following the syntax `<fully-qualified class name>#<method name>`
 – for example, `methodName = "org.example.TestUtils#createCustomService"`.
 ====
+
+NOTE: Only _singleton_ beans can be overridden. Any attempt to override a non-singleton
+bean will result in an exception.

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

@@ -69,3 +69,6 @@ Alternatively, the user can directly provide the bean name in the custom annotat
 Some `BeanOverrideProcessor` implementations could also internally compute a bean name
 based on a convention or another advanced method.
 ====
+
+NOTE: Only _singleton_ beans can be overridden. Any attempt to override a non-singleton
+bean will result in an exception.

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

@@ -40,6 +40,7 @@
 import org.springframework.core.Ordered;
 import org.springframework.core.PriorityOrdered;
 import org.springframework.core.ResolvableType;
+import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
 /**
@@ -116,14 +117,15 @@ private void registerBeanOverride(ConfigurableListableBeanFactory beanFactory, B
 	private void replaceDefinition(ConfigurableListableBeanFactory beanFactory, BeanDefinitionRegistry registry,
 			OverrideMetadata overrideMetadata, boolean enforceExistingDefinition) {
 
-		// The following is a "dummy" bean definition which should not be used to
+		// The following is a "pseudo" bean definition which MUST NOT be used to
 		// create an actual bean instance.
-		RootBeanDefinition beanDefinition = createBeanDefinition(overrideMetadata);
+		RootBeanDefinition pseudoBeanDefinition = createPseudoBeanDefinition(overrideMetadata);
 		String beanName = overrideMetadata.getBeanName();
 		String beanNameIncludingFactory;
 		BeanDefinition existingBeanDefinition = null;
 		if (beanName == null) {
-			beanNameIncludingFactory = getBeanNameForType(beanFactory, registry, overrideMetadata, beanDefinition, enforceExistingDefinition);
+			beanNameIncludingFactory = getBeanNameForType(
+					beanFactory, registry, overrideMetadata, pseudoBeanDefinition, enforceExistingDefinition);
 			beanName = BeanFactoryUtils.transformedBeanName(beanNameIncludingFactory);
 			if (registry.containsBeanDefinition(beanName)) {
 				existingBeanDefinition = beanFactory.getBeanDefinition(beanName);
@@ -145,25 +147,24 @@ else if (enforceExistingDefinition) {
 
 		// Process existing bean definition.
 		if (existingBeanDefinition != null) {
-			copyBeanDefinitionProperties(existingBeanDefinition, beanDefinition);
+			validateBeanDefinition(beanFactory, beanName);
+			copyBeanDefinitionProperties(existingBeanDefinition, pseudoBeanDefinition);
 			registry.removeBeanDefinition(beanName);
 		}
 
 		// At this point, we either removed an existing bean definition above, or
-		// there was no bean definition to begin with. So, we register the dummy bean
+		// there was no bean definition to begin with. So, we register the pseudo bean
 		// definition to ensure that a bean definition exists for the given bean name.
-		registry.registerBeanDefinition(beanName, beanDefinition);
+		registry.registerBeanDefinition(beanName, pseudoBeanDefinition);
 
 		Object override = overrideMetadata.createOverride(beanName, existingBeanDefinition, null);
 		overrideMetadata.track(override, beanFactory);
 		this.overrideRegistrar.registerNameForMetadata(overrideMetadata, beanNameIncludingFactory);
 
-		if (beanFactory.isSingleton(beanNameIncludingFactory)) {
-			// Now we have an instance (the override) that we can register. At this
-			// stage we don't expect a singleton instance to be present, and this call
-			// will throw an exception if there is such an instance already.
-			beanFactory.registerSingleton(beanName, override);
-		}
+		// Now we have an instance (the override) that we can register. At this stage, we don't
+		// expect a singleton instance to be present. If for some reason a singleton instance
+		// already exists, the following will throw an exception.
+		beanFactory.registerSingleton(beanName, override);
 	}
 
 	/**
@@ -196,6 +197,7 @@ private void wrapBean(ConfigurableListableBeanFactory beanFactory, OverrideMetad
 							.formatted(beanName, overrideMetadata.getBeanType()));
 			}
 		}
+		validateBeanDefinition(beanFactory, beanName);
 		this.overrideRegistrar.markWrapEarly(overrideMetadata, beanName);
 		this.overrideRegistrar.registerNameForMetadata(overrideMetadata, beanName);
 	}
@@ -276,23 +278,30 @@ private Set<String> getExistingBeanNamesByType(ConfigurableListableBeanFactory b
 	 * definition metadata available in the {@link BeanFactory} &mdash; for example,
 	 * for autowiring candidate resolution.
 	 */
-	private static RootBeanDefinition createBeanDefinition(OverrideMetadata metadata) {
+	private static RootBeanDefinition createPseudoBeanDefinition(OverrideMetadata metadata) {
 		RootBeanDefinition definition = new RootBeanDefinition(metadata.getBeanType().resolve());
 		definition.setTargetType(metadata.getBeanType());
 		definition.setQualifiedElement(metadata.getField());
 		return definition;
 	}
 
+	/**
+	 * Validate that the {@link BeanDefinition} for the supplied bean name is suitable
+	 * for being replaced by a bean override.
+	 */
+	private static void validateBeanDefinition(ConfigurableListableBeanFactory beanFactory, String beanName) {
+		Assert.state(beanFactory.isSingleton(beanName),
+				() -> "Unable to override bean '" + beanName + "': only singleton beans can be overridden.");
+	}
+
 	/**
 	 * Copy the following properties of the source {@link BeanDefinition} to the
-	 * target: the {@linkplain BeanDefinition#isPrimary() primary flag}, the
-	 * {@linkplain BeanDefinition#isFallback() fallback flag}, and the
-	 * {@linkplain BeanDefinition#getScope() scope}.
+	 * target: the {@linkplain BeanDefinition#isPrimary() primary flag} and the
+	 * {@linkplain BeanDefinition#isFallback() fallback flag}.
 	 */
 	private static void copyBeanDefinitionProperties(BeanDefinition source, RootBeanDefinition target) {
 		target.setPrimary(source.isPrimary());
 		target.setFallback(source.isFallback());
-		target.setScope(source.getScope());
 	}
 
 

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

@@ -49,6 +49,9 @@
  * instance creation} — for example, based on further processing of the
  * annotation or the annotated field.
  *
+ * <p><strong>NOTE</strong>: Only <em>singleton</em> beans can be overridden.
+ * Any attempt to override a non-singleton bean will result in an exception.
+ *
  * @author Simon Baslé
  * @author Stephane Nicoll
  * @author Sam Brannen

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

@@ -93,6 +93,9 @@
  *     }
  * }</code></pre>
  *
+ * <p><strong>NOTE</strong>: Only <em>singleton</em> beans can be overridden.
+ * Any attempt to override a non-singleton bean will result in an exception.
+ *
  * @author Simon Baslé
  * @author Stephane Nicoll
  * @author Sam Brannen

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

@@ -47,6 +47,9 @@
  * registered directly}) will not be found, and a mocked bean will be added to
  * the context alongside the existing dependency.
  *
+ * <p><strong>NOTE</strong>: Only <em>singleton</em> beans can be overridden.
+ * Any attempt to override a non-singleton bean will result in an exception.
+ *
  * @author Simon Baslé
  * @since 6.2
  * @see org.springframework.test.context.bean.override.mockito.MockitoSpyBean @MockitoSpyBean

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

@@ -42,6 +42,9 @@
  * {@link org.springframework.beans.factory.config.ConfigurableListableBeanFactory#registerResolvableDependency(Class, Object)
  * registered directly}) will not be found.
  *
+ * <p><strong>NOTE</strong>: Only <em>singleton</em> beans can be overridden.
+ * Any attempt to override a non-singleton bean will result in an exception.
+ *
  * @author Simon Baslé
  * @since 6.2
  * @see org.springframework.test.context.bean.override.mockito.MockitoBean @MockitoBean

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

@@ -25,7 +25,6 @@
 import org.junit.jupiter.api.Test;
 
 import org.springframework.beans.BeanWrapper;
-import org.springframework.beans.factory.BeanCreationException;
 import org.springframework.beans.factory.FactoryBean;
 import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.beans.factory.config.BeanDefinition;
@@ -43,7 +42,6 @@
 import org.springframework.util.Assert;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.assertj.core.api.Assertions.assertThatNoException;
 import static org.mockito.Mockito.mock;
@@ -228,17 +226,15 @@ void replaceBeanByNameWithMatchingBeanDefinitionForClassBasedSingletonFactoryBea
 	}
 
 	@Test
-	void replaceBeanByNameWithMatchingBeanDefinitionForClassBasedNonSingletonFactoryBean() {
+	void replaceBeanByNameWithMatchingBeanDefinitionForClassBasedNonSingletonFactoryBeanFails() {
 		String beanName = "descriptionBean";
 		AnnotationConfigApplicationContext context = createContext(CaseByName.class);
 		RootBeanDefinition factoryBeanDefinition = new RootBeanDefinition(NonSingletonStringFactoryBean.class);
 		context.registerBeanDefinition(beanName, factoryBeanDefinition);
 
-		assertThatNoException().isThrownBy(context::refresh);
-		// Even though the FactoryBean signals it does not manage a singleton,
-		// the Bean Override support currently replaces it with a singleton.
-		assertThat(context.isSingleton(beanName)).as("isSingleton").isTrue();
-		assertThat(context.getBean(beanName)).isEqualTo("overridden");
+		assertThatIllegalStateException()
+				.isThrownBy(context::refresh)
+				.withMessage("Unable to override bean 'descriptionBean': only singleton beans can be overridden.");
 	}
 
 	@Test
@@ -254,44 +250,33 @@ void replaceBeanByNameWithMatchingBeanDefinitionForInterfaceBasedSingletonFactor
 	}
 
 	@Test
-	void replaceBeanByNameWithMatchingBeanDefinitionForInterfaceBasedNonSingletonFactoryBean() {
+	void replaceBeanByNameWithMatchingBeanDefinitionForInterfaceBasedNonSingletonFactoryBeanFails() {
 		String beanName = "messageServiceBean";
 		AnnotationConfigApplicationContext context = createContext(MessageServiceTestCase.class);
 		RootBeanDefinition factoryBeanDefinition = new RootBeanDefinition(NonSingletonMessageServiceFactoryBean.class);
 		context.registerBeanDefinition(beanName, factoryBeanDefinition);
 
-		assertThatNoException().isThrownBy(context::refresh);
-		// Even though the FactoryBean signals it does not manage a singleton,
-		// the Bean Override support currently replaces it with a singleton.
-		assertThat(context.isSingleton(beanName)).as("isSingleton").isTrue();
-		assertThat(context.getBean(beanName, MessageService.class).getMessage()).isEqualTo("overridden");
+		assertThatIllegalStateException()
+				.isThrownBy(context::refresh)
+				.withMessage("Unable to override bean 'messageServiceBean': only singleton beans can be overridden.");
 	}
 
 	@Test
-	void replaceBeanByNameWithMatchingBeanDefinitionWithPrototypeScope() {
+	void replaceBeanByNameWithMatchingBeanDefinitionWithPrototypeScopeFails() {
 		String beanName = "descriptionBean";
 
 		AnnotationConfigApplicationContext context = createContext(CaseByName.class);
 		RootBeanDefinition definition = new RootBeanDefinition(String.class, () -> "ORIGINAL");
 		definition.setScope(BeanDefinition.SCOPE_PROTOTYPE);
 		context.registerBeanDefinition(beanName, definition);
 
-		assertThatNoException().isThrownBy(context::refresh);
-		// The Bean Override support currently creates a "dummy" BeanDefinition that
-		// retains the prototype scope of the original BeanDefinition.
-		assertThat(context.isSingleton(beanName)).as("isSingleton").isFalse();
-		assertThat(context.isPrototype(beanName)).as("isPrototype").isTrue();
-		// Since the "dummy" BeanDefinition has prototype scope, a manual singleton
-		// is not registered, and the "dummy" BeanDefinition is used to create a
-		// new java.lang.String using the default constructor, which results in an
-		// empty string instead of "overridden". In other words, the bean is not
-		// actually overridden as expected, and no exception is thrown which
-		// silently masks the issue.
-		assertThat(context.getBean(beanName)).isEqualTo("");
+		assertThatIllegalStateException()
+				.isThrownBy(context::refresh)
+				.withMessage("Unable to override bean 'descriptionBean': only singleton beans can be overridden.");
 	}
 
 	@Test
-	void replaceBeanByNameWithMatchingBeanDefinitionWithCustomScope() {
+	void replaceBeanByNameWithMatchingBeanDefinitionWithCustomScopeFails() {
 		String beanName = "descriptionBean";
 		String scope = "customScope";
 
@@ -302,49 +287,28 @@ void replaceBeanByNameWithMatchingBeanDefinitionWithCustomScope() {
 		definition.setScope(scope);
 		context.registerBeanDefinition(beanName, definition);
 
-		assertThatNoException().isThrownBy(context::refresh);
-		// The Bean Override support currently creates a "dummy" BeanDefinition that
-		// retains the custom scope of the original BeanDefinition.
-		assertThat(context.isSingleton(beanName)).as("isSingleton").isFalse();
-		assertThat(context.isPrototype(beanName)).as("isPrototype").isFalse();
-		assertThat(beanFactory.getBeanDefinition(beanName).getScope()).isEqualTo(scope);
-		// Since the "dummy" BeanDefinition has a custom scope, a manual singleton
-		// is not registered, and the "dummy" BeanDefinition is used to create a
-		// new java.lang.String using the default constructor, which results in an
-		// empty string instead of "overridden". In other words, the bean is not
-		// actually overridden as expected, and no exception is thrown which
-		// silently masks the issue.
-		assertThat(context.getBean(beanName)).isEqualTo("");
+		assertThatIllegalStateException()
+				.isThrownBy(context::refresh)
+				.withMessage("Unable to override bean 'descriptionBean': only singleton beans can be overridden.");
 	}
 
 	@Test
-	void replaceBeanByNameWithMatchingBeanDefinitionForPrototypeScopedFactoryBean() {
+	void replaceBeanByNameWithMatchingBeanDefinitionForPrototypeScopedFactoryBeanFails() {
 		String beanName = "messageServiceBean";
 		AnnotationConfigApplicationContext context = createContext(MessageServiceTestCase.class);
 		RootBeanDefinition factoryBeanDefinition = new RootBeanDefinition(SingletonMessageServiceFactoryBean.class);
 		factoryBeanDefinition.setScope(BeanDefinition.SCOPE_PROTOTYPE);
 		context.registerBeanDefinition(beanName, factoryBeanDefinition);
 
-		assertThatNoException().isThrownBy(context::refresh);
-		// The Bean Override support currently creates a "dummy" BeanDefinition that
-		// retains the prototype scope of the original BeanDefinition.
-		assertThat(context.isSingleton(beanName)).as("isSingleton").isFalse();
-		assertThat(context.isPrototype(beanName)).as("isPrototype").isTrue();
-		// Since the "dummy" BeanDefinition has prototype scope, a manual singleton
-		// is not registered, and the "dummy" BeanDefinition is used to create a
-		// new MessageService using the default constructor, which results in an
-		// error since MessageService is an interface.
-		assertThatExceptionOfType(BeanCreationException.class)
-				.isThrownBy(() -> context.getBean(beanName))
-				.withMessageContaining("Specified class is an interface");
+		assertThatIllegalStateException()
+				.isThrownBy(context::refresh)
+				.withMessage("Unable to override bean 'messageServiceBean': only singleton beans can be overridden.");
 	}
 
 	@Test
-	void replaceBeanByNameWithMatchingBeanDefinitionRetainsPrimaryFallbackAndScopeProperties() {
+	void replaceBeanByNameWithMatchingBeanDefinitionRetainsPrimaryAndFallbackFlags() {
 		AnnotationConfigApplicationContext context = createContext(CaseByName.class);
-		context.getBeanFactory().registerScope("customScope", new SimpleThreadScope());
 		RootBeanDefinition definition = new RootBeanDefinition(String.class, () -> "ORIGINAL");
-		definition.setScope("customScope");
 		definition.setPrimary(true);
 		definition.setFallback(true);
 		context.registerBeanDefinition("descriptionBean", definition);
@@ -354,8 +318,8 @@ void replaceBeanByNameWithMatchingBeanDefinitionRetainsPrimaryFallbackAndScopePr
 				.isNotSameAs(definition)
 				.matches(BeanDefinition::isPrimary, "isPrimary")
 				.matches(BeanDefinition::isFallback, "isFallback")
-				.satisfies(d -> assertThat(d.getScope()).isEqualTo("customScope"))
-				.matches(Predicate.not(BeanDefinition::isSingleton), "!isSingleton")
+				.satisfies(d -> assertThat(d.getScope()).isEqualTo(""))
+				.matches(BeanDefinition::isSingleton, "isSingleton")
 				.matches(Predicate.not(BeanDefinition::isPrototype), "!isPrototype");
 	}
 

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

@@ -90,18 +90,11 @@ public TestBean getObject() {
 		public Class<?> getObjectType() {
 			return TestBean.class;
 		}
-
-		@Override
-		public boolean isSingleton() {
-			return false;
-		}
-
 	}
 
 	public interface TestBean {
 
 		String hello();
-
 	}
 
 }