From d79258ac7349d6ce9fb2574bb399b1d51c172def Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 27 Sep 2024 17:36:22 +0200 Subject: [PATCH] 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 --- .../annotation-mockitobean.adoc | 3 + .../annotation-testbean.adoc | 3 + .../bean-overriding.adoc | 3 + .../BeanOverrideBeanFactoryPostProcessor.java | 43 ++++++---- .../bean/override/OverrideMetadata.java | 3 + .../bean/override/convention/TestBean.java | 3 + .../bean/override/mockito/MockitoBean.java | 3 + .../bean/override/mockito/MockitoSpyBean.java | 3 + ...OverrideBeanFactoryPostProcessorTests.java | 82 ++++++------------- ...itoBeanForBeanFactoryIntegrationTests.java | 7 -- 10 files changed, 70 insertions(+), 83 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-mockitobean.adoc b/framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-mockitobean.adoc index 1cc24ea7d4..62b5b89ecb 100644 --- a/framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-mockitobean.adoc +++ b/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] diff --git a/framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-testbean.adoc b/framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-testbean.adoc index a2165c2730..fc4c2c63d7 100644 --- a/framework-docs/modules/ROOT/pages/testing/annotations/integration-spring/annotation-testbean.adoc +++ b/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 `#` – 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. diff --git a/framework-docs/modules/ROOT/pages/testing/testcontext-framework/bean-overriding.adoc b/framework-docs/modules/ROOT/pages/testing/testcontext-framework/bean-overriding.adoc index 243de7e3bd..23502063e9 100644 --- a/framework-docs/modules/ROOT/pages/testing/testcontext-framework/bean-overriding.adoc +++ b/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. diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessor.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessor.java index ba38bce1b3..141be2976b 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessor.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessor.java @@ -40,6 +40,7 @@ import org.springframework.beans.factory.support.RootBeanDefinition; 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 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, 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 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, // 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 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, .formatted(beanName, overrideMetadata.getBeanType())); } } + validateBeanDefinition(beanFactory, beanName); this.overrideRegistrar.markWrapEarly(overrideMetadata, beanName); this.overrideRegistrar.registerNameForMetadata(overrideMetadata, beanName); } @@ -276,23 +278,30 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, * definition metadata available in the {@link BeanFactory} — 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()); } diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/OverrideMetadata.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/OverrideMetadata.java index 0ab99f6f0b..2c68fef410 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/OverrideMetadata.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/OverrideMetadata.java @@ -49,6 +49,9 @@ import static org.springframework.core.annotation.MergedAnnotations.SearchStrate * instance creation} — for example, based on further processing of the * annotation or the annotated field. * + *

NOTE: Only singleton 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 diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBean.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBean.java index 0ff1c0f54b..c141b53d20 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBean.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBean.java @@ -93,6 +93,9 @@ import org.springframework.test.context.bean.override.BeanOverride; * } * } * + *

NOTE: Only singleton 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 diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoBean.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoBean.java index 9d0737392f..90a2e0a452 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoBean.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoBean.java @@ -47,6 +47,9 @@ import org.springframework.test.context.bean.override.BeanOverride; * registered directly}) will not be found, and a mocked bean will be added to * the context alongside the existing dependency. * + *

NOTE: Only singleton 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 diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBean.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBean.java index db2b8fb3ce..c0664206c1 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBean.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBean.java @@ -42,6 +42,9 @@ import org.springframework.test.context.bean.override.BeanOverride; * {@link org.springframework.beans.factory.config.ConfigurableListableBeanFactory#registerResolvableDependency(Class, Object) * registered directly}) will not be found. * + *

NOTE: Only singleton 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 diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessorTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessorTests.java index abbd216bce..864d5f2f90 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessorTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/BeanOverrideBeanFactoryPostProcessorTests.java @@ -25,7 +25,6 @@ import java.util.function.Predicate; 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.test.util.ReflectionTestUtils; 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 @@ class BeanOverrideBeanFactoryPostProcessorTests { } @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,21 +250,19 @@ class BeanOverrideBeanFactoryPostProcessorTests { } @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); @@ -276,22 +270,13 @@ class BeanOverrideBeanFactoryPostProcessorTests { 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 @@ class BeanOverrideBeanFactoryPostProcessorTests { 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 @@ class BeanOverrideBeanFactoryPostProcessorTests { .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"); } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForBeanFactoryIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForBeanFactoryIntegrationTests.java index 1100788799..842b2d3741 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForBeanFactoryIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanForBeanFactoryIntegrationTests.java @@ -90,18 +90,11 @@ public class MockitoBeanForBeanFactoryIntegrationTests { public Class getObjectType() { return TestBean.class; } - - @Override - public boolean isSingleton() { - return false; - } - } public interface TestBean { String hello(); - } }