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] 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 { + } + +}