From 03fe1f0df38090eff97f7ab2e292d881275e860d Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 6 Dec 2024 14:59:49 +0100 Subject: [PATCH 1/2] Improve documentation for BeanOverrideBeanFactoryPostProcessor --- .../BeanOverrideBeanFactoryPostProcessor.java | 41 ++++++++++++------- 1 file changed, 26 insertions(+), 15 deletions(-) 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 4be7503b6f..58e41b0d43 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 @@ -23,7 +23,6 @@ import java.util.Set; import org.springframework.aop.scope.ScopedProxyUtils; import org.springframework.beans.BeansException; -import org.springframework.beans.factory.BeanFactory; import org.springframework.beans.factory.BeanFactoryUtils; import org.springframework.beans.factory.FactoryBean; import org.springframework.beans.factory.NoUniqueBeanDefinitionException; @@ -44,7 +43,7 @@ import org.springframework.util.Assert; /** * A {@link BeanFactoryPostProcessor} implementation that processes identified - * use of {@link BeanOverride @BeanOverride} and adapts the {@link BeanFactory} + * use of {@link BeanOverride @BeanOverride} and adapts the {@code BeanFactory} * accordingly. * *

For each override, the bean factory is prepared according to the chosen @@ -119,9 +118,16 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, // NOTE: This method supports 3 distinct scenarios which must be accounted for. // - // 1) JVM runtime - // 2) AOT processing - // 3) AOT runtime + // - JVM runtime + // - AOT processing + // - AOT runtime + // + // In addition, this method supports 4 distinct use cases. + // + // 1) Override existing bean by-type + // 2) Create bean by-type, with a generated name + // 3) Override existing bean by-name + // 4) Create bean by-name, with a provided name String beanName = handler.getBeanName(); Field field = handler.getField(); @@ -129,7 +135,7 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, if (beanName == null) { beanName = getBeanNameForType(beanFactory, handler, requireExistingBean); if (beanName != null) { - // We are overriding an existing bean by-type. + // 1) We are overriding an existing bean by-type. beanName = BeanFactoryUtils.transformedBeanName(beanName); // If we are overriding a manually registered singleton, we won't find // an existing bean definition. @@ -138,15 +144,16 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, } } else { - // We will later generate a name for the nonexistent bean, but since NullAway - // will reject leaving the beanName set to null, we set it to a placeholder. + // 2) We are creating a bean by-type, with a generated name. + // Since NullAway will reject leaving the beanName set to null, + // we set it to a placeholder that will be replaced later. beanName = PSEUDO_BEAN_NAME_PLACEHOLDER; } } else { Set candidates = getExistingBeanNamesByType(beanFactory, handler, false); if (candidates.contains(beanName)) { - // We are overriding an existing bean by-name. + // 3) We are overriding an existing bean by-name. existingBeanDefinition = beanFactory.getBeanDefinition(beanName); } else if (requireExistingBean) { @@ -156,6 +163,7 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, .formatted(beanName, handler.getBeanType(), field.getDeclaringClass().getSimpleName(), field.getName())); } + // 4) We are creating a bean by-name with the provided beanName. } if (existingBeanDefinition != null) { @@ -214,11 +222,14 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, /** * Check that a bean with the specified {@link BeanOverrideHandler#getBeanName() name} - * and {@link BeanOverrideHandler#getBeanType() type} is registered. - *

If so, put the {@link BeanOverrideHandler} in the early tracking map. - *

The map will later be checked to see if a given bean should be wrapped - * upon creation, during the {@link WrapEarlyBeanPostProcessor#getEarlyBeanReference} - * phase. + * or {@link BeanOverrideHandler#getBeanType() type} has already been registered + * in the {@code BeanFactory}. + *

If so, register the {@link BeanOverrideHandler} and the corresponding bean + * name in the {@link BeanOverrideRegistry}. + *

The registry will later be checked to see if a given bean should be wrapped + * upon creation, during the early bean post-processing phase. + * @see BeanOverrideRegistry#registerBeanOverrideHandler(BeanOverrideHandler, String) + * @see WrapEarlyBeanPostProcessor#getEarlyBeanReference(Object, String) */ private void wrapBean(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler) { String beanName = handler.getBeanName(); @@ -393,7 +404,7 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, * respectively. *

The returned bean definition should not be used to create * a bean instance but rather only for the purpose of having suitable bean - * definition metadata available in the {@link BeanFactory} — for example, + * definition metadata available in the {@code BeanFactory} — for example, * for autowiring candidate resolution. */ private static RootBeanDefinition createPseudoBeanDefinition(BeanOverrideHandler handler) { From aa7b4598031b7633be68cc550da19da79e370f75 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 6 Dec 2024 17:01:31 +0100 Subject: [PATCH 2/2] Fix Phantom Read problem for Bean Overrides in the TestContext framework MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To make an analogy to read phenomena for transactional databases, this commit effectively fixes the "Phantom Read" problem for Bean Overrides. A phantom read occurs when the BeanOverrideBeanFactoryPostProcessor retrieves a set of bean names by-type twice and a new bean definition for a compatible type has been created in the BeanFactory by a BeanOverrideHandler between the first and second retrieval. Continue reading for the details... Prior to this commit, the injection of test Bean Overrides (for example, when using @⁠MockitoBean) could fail in certain scenarios if overrides were created for nonexistent beans "by type" without an explicit name or qualifier. Specifically, if an override for a SubType was created first, and subsequently an attempt was made to create an override for a SuperType (where SubType extends SuperType), the override for the SuperType would "override the override" for the SubType, effectively removing the override for the SubType. Consequently, injection of the override instance into the SubType field would fail with an error message similar to the following. BeanNotOfRequiredTypeException: Bean named 'Subtype#0' is expected to be of type 'Subtype' but was actually of type 'Supertype$Mock$XHb7Aspo' This commit addresses this issue by tracking all generated bean names (in a generatedBeanNames set) and ensuring that a new bean override instance is created for the current BeanOverrideHandler if a previous BeanOverrideHandler already created a bean override instance that now matches the type required by the current BeanOverrideHandler. In other words, if the generatedBeanNames set already contains the beanName that we just found by-type, we cannot "override the override", because we would lose one of the overrides. Instead, we must create a new override for the current handler. In the example given above, we must end up with overrides for both SuperType and SubType. Closes gh-34025 --- .../BeanOverrideBeanFactoryPostProcessor.java | 25 +++++-- .../BeanOverrideContextCustomizerFactory.java | 4 +- ...kitoBeanDuplicateTypeIntegrationTests.java | 56 +++++++++++++++ ...toBeanSuperAndSubtypeIntegrationTests.java | 72 +++++++++++++++++++ 4 files changed, 149 insertions(+), 8 deletions(-) create mode 100644 spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeIntegrationTests.java create mode 100644 spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanSuperAndSubtypeIntegrationTests.java 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 58e41b0d43..4d1f0f0d85 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 @@ -18,6 +18,7 @@ package org.springframework.test.context.bean.override; import java.lang.reflect.Field; import java.util.Arrays; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; @@ -93,12 +94,15 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, @Override public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException { + Set generatedBeanNames = new HashSet<>(); for (BeanOverrideHandler handler : this.beanOverrideHandlers) { - registerBeanOverride(beanFactory, handler); + registerBeanOverride(beanFactory, handler, generatedBeanNames); } } - private void registerBeanOverride(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler) { + private void registerBeanOverride(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler, + Set generatedBeanNames) { + String beanName = handler.getBeanName(); Field field = handler.getField(); Assert.state(!BeanFactoryUtils.isFactoryDereference(beanName),() -> """ @@ -107,14 +111,14 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, beanName, field.getDeclaringClass().getSimpleName(), field.getName())); switch (handler.getStrategy()) { - case REPLACE -> replaceOrCreateBean(beanFactory, handler, true); - case REPLACE_OR_CREATE -> replaceOrCreateBean(beanFactory, handler, false); + case REPLACE -> replaceOrCreateBean(beanFactory, handler, generatedBeanNames, true); + case REPLACE_OR_CREATE -> replaceOrCreateBean(beanFactory, handler, generatedBeanNames, false); case WRAP -> wrapBean(beanFactory, handler); } } private void replaceOrCreateBean(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler, - boolean requireExistingBean) { + Set generatedBeanNames, boolean requireExistingBean) { // NOTE: This method supports 3 distinct scenarios which must be accounted for. // @@ -134,7 +138,15 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, BeanDefinition existingBeanDefinition = null; if (beanName == null) { beanName = getBeanNameForType(beanFactory, handler, requireExistingBean); - if (beanName != null) { + // If the generatedBeanNames set already contains the beanName that we + // just found by-type, that means we are experiencing a "phantom read" + // (i.e., we found a bean that was not previously there). Consequently, + // we cannot "override the override", because we would lose one of the + // overrides. Instead, we must create a new override for the current + // handler. For example, if one handler creates an override for a SubType + // and a subsequent handler creates an override for a SuperType of that + // SubType, we must end up with overrides for both SuperType and SubType. + if (beanName != null && !generatedBeanNames.contains(beanName)) { // 1) We are overriding an existing bean by-type. beanName = BeanFactoryUtils.transformedBeanName(beanName); // If we are overriding a manually registered singleton, we won't find @@ -197,6 +209,7 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, // Generate a name for the nonexistent bean. if (PSEUDO_BEAN_NAME_PLACEHOLDER.equals(beanName)) { beanName = beanNameGenerator.generateBeanName(pseudoBeanDefinition, registry); + generatedBeanNames.add(beanName); } registry.registerBeanDefinition(beanName, pseudoBeanDefinition); diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java index 764f7cedc7..29cc22fc22 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizerFactory.java @@ -16,7 +16,7 @@ package org.springframework.test.context.bean.override; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Set; @@ -42,7 +42,7 @@ class BeanOverrideContextCustomizerFactory implements ContextCustomizerFactory { public BeanOverrideContextCustomizer createContextCustomizer(Class testClass, List configAttributes) { - Set handlers = new HashSet<>(); + Set handlers = new LinkedHashSet<>(); findBeanOverrideHandler(testClass, handlers); if (handlers.isEmpty()) { return null; diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeIntegrationTests.java new file mode 100644 index 0000000000..4ff6f62480 --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeIntegrationTests.java @@ -0,0 +1,56 @@ +/* + * Copyright 2002-2024 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.test.context.bean.override.mockito; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.context.bean.override.example.ExampleService; +import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration tests for {@link MockitoBean @MockitoBean} where duplicate mocks + * are created for the same nonexistent type. + * + * @author Sam Brannen + * @since 6.2.1 + * @see gh-34025 + */ +@SpringJUnitConfig +public class MockitoBeanDuplicateTypeIntegrationTests { + + @MockitoBean + ExampleService service1; + + @MockitoBean + ExampleService service2; + + @Autowired + List services; + + + @Test + void duplicateMocksShouldHaveBeenCreated() { + assertThat(service1).isNotSameAs(service2); + assertThat(services).hasSize(2); + } + +} diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanSuperAndSubtypeIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanSuperAndSubtypeIntegrationTests.java new file mode 100644 index 0000000000..c27854b10d --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanSuperAndSubtypeIntegrationTests.java @@ -0,0 +1,72 @@ +/* + * Copyright 2002-2024 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.test.context.bean.override.mockito; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration tests for {@link MockitoBean @MockitoBean} where mocks are created + * for nonexistent beans for a supertype and subtype of that supertype. + * + *

This test class is designed to reproduce scenarios that previously failed + * along the lines of the following. + * + *

BeanNotOfRequiredTypeException: Bean named 'Subtype#0' is expected to be + * of type 'Subtype' but was actually of type 'Supertype$MockitoMock$XHb7Aspo' + * + * @author Sam Brannen + * @since 6.2.1 + * @see gh-34025 + */ +@SpringJUnitConfig +public class MockitoBeanSuperAndSubtypeIntegrationTests { + + // The declaration order of the following fields is intentional, and prior + // to fixing gh-34025 this test class consistently failed on JDK 17. + + @MockitoBean + Subtype subtype; + + @MockitoBean + Supertype supertype; + + + @Autowired + List supertypes; + + + @Test + void bothMocksShouldHaveBeenCreated() { + assertThat(supertype).isNotSameAs(subtype); + assertThat(supertypes).hasSize(2); + } + + + interface Supertype { + } + + interface Subtype extends Supertype { + } + +}