From 54948a4e88e14885fdb2e22d65a71ed7195699fc Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Tue, 10 Dec 2024 12:48:24 +0100 Subject: [PATCH] Log warning when one Bean Override overrides another Bean Override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is currently possible for one Bean Override to override another logically equivalent Bean Override. For example, a @⁠TestBean can override a @⁠MockitoBean, and vice versa. In fact, it's also possible for a @⁠MockitoBean to override another @⁠MockitoBean, for a @⁠TestBean to override a @⁠TestBean, etc. However, there may be viable use cases for one override overriding another override. For example, one may have a need to spy on a bean created by a @⁠TestBean factory method. In light of that, we do not prohibit one Bean Override from overriding another Bean Override; however, with this commit we now log a warning to help developers diagnose issues in case such an override is unintentional. For example, given the following test class, where TestConfig registers a single bean of type MyService named "myService"... @⁠SpringJUnitConfig(TestConfig.class) class MyTests { @⁠TestBean(methodName = "example.TestUtils#createMyService") MyService testService; @⁠MockitoBean MyService mockService; @⁠Test void test() { // ... } } ... running that test class results in a log message similar to the following, which has been formatted for readability. WARN - Bean with name 'myService' was overridden by multiple handlers: [ [TestBeanOverrideHandler@44b21f9f field = example.MyService example.MyTests.testService, beanType = example.MyService, beanName = [null], strategy = REPLACE_OR_CREATE ], [MockitoBeanOverrideHandler@7ee8130e field = example.MyService example.MyTests.mockService, beanType = example.MyService, beanName = [null], strategy = REPLACE_OR_CREATE, reset = AFTER, extraInterfaces = set[[empty]], answers = RETURNS_DEFAULTS, serializable = false ] ] NOTE: The last registered BeanOverrideHandler wins. In the above example, that means that @⁠MockitoBean overrides @⁠TestBean, resulting in a Mockito mock for the MyService bean in the test's ApplicationContext. Closes gh-34056 --- .../bean/override/BeanOverrideRegistry.java | 32 +++++- ...uplicateTypeCreationIntegrationTests.java} | 17 +-- ...licateTypeReplacementIntegrationTests.java | 97 +++++++++++++++++ ...BeanOverridesTestBeanIntegrationTests.java | 103 ++++++++++++++++++ ...oSpyBeanDuplicateTypeIntegrationTests.java | 20 ++-- 5 files changed, 252 insertions(+), 17 deletions(-) rename spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/{MockitoBeanDuplicateTypeIntegrationTests.java => MockitoBeanDuplicateTypeCreationIntegrationTests.java} (76%) create mode 100644 spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeReplacementIntegrationTests.java create mode 100644 spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanOverridesTestBeanIntegrationTests.java diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideRegistry.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideRegistry.java index aead53904d9b..fa465e509151 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideRegistry.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideRegistry.java @@ -17,8 +17,13 @@ package org.springframework.test.context.bean.override; import java.lang.reflect.Field; -import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.Map.Entry; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.BeanCreationException; import org.springframework.beans.factory.config.ConfigurableBeanFactory; @@ -37,9 +42,12 @@ */ class BeanOverrideRegistry { - private final Map handlerToBeanNameMap = new HashMap<>(); + private static final Log logger = LogFactory.getLog(BeanOverrideRegistry.class); + - private final Map wrappingBeanOverrideHandlers = new HashMap<>(); + private final Map handlerToBeanNameMap = new LinkedHashMap<>(); + + private final Map wrappingBeanOverrideHandlers = new LinkedHashMap<>(); private final ConfigurableBeanFactory beanFactory; @@ -57,7 +65,25 @@ class BeanOverrideRegistry { * bean via {@link #wrapBeanIfNecessary(Object, String)}. */ void registerBeanOverrideHandler(BeanOverrideHandler handler, String beanName) { + Assert.state(!this.handlerToBeanNameMap.containsKey(handler), () -> + "Cannot register BeanOverrideHandler for bean with name '%s'; detected multiple registrations for %s" + .formatted(beanName, handler)); + + // Check if beanName was already registered, before adding the new mapping. + boolean beanNameAlreadyRegistered = this.handlerToBeanNameMap.containsValue(beanName); + // Add new mapping before potentially logging a warning, to ensure that + // the current handler is logged as well. this.handlerToBeanNameMap.put(handler, beanName); + + if (beanNameAlreadyRegistered && logger.isWarnEnabled()) { + List competingHandlers = this.handlerToBeanNameMap.entrySet().stream() + .filter(entry -> entry.getValue().equals(beanName)) + .map(Entry::getKey) + .toList(); + logger.warn("Bean with name '%s' was overridden by multiple handlers: %s" + .formatted(beanName, competingHandlers)); + } + if (handler.getStrategy() == BeanOverrideStrategy.WRAP) { this.wrappingBeanOverrideHandlers.put(beanName, handler); } 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/MockitoBeanDuplicateTypeCreationIntegrationTests.java similarity index 76% rename from spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeIntegrationTests.java rename to spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeCreationIntegrationTests.java index d00ead41963e..695e10a5b801 100644 --- 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/MockitoBeanDuplicateTypeCreationIntegrationTests.java @@ -25,25 +25,26 @@ import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.test.mockito.MockitoAssertions.assertIsMock; /** * Integration tests for {@link MockitoBean @MockitoBean} where duplicate mocks - * are created for the same nonexistent type. + * are created for the same nonexistent type, selected by-type. * * @author Sam Brannen * @since 6.2.1 * @see gh-34025 + * @see MockitoBeanDuplicateTypeReplacementIntegrationTests * @see MockitoSpyBeanDuplicateTypeIntegrationTests - * @see MockitoSpyBeanDuplicateTypeAndNameIntegrationTests */ @SpringJUnitConfig -public class MockitoBeanDuplicateTypeIntegrationTests { +public class MockitoBeanDuplicateTypeCreationIntegrationTests { @MockitoBean - ExampleService service1; + ExampleService mock1; @MockitoBean - ExampleService service2; + ExampleService mock2; @Autowired List services; @@ -51,8 +52,10 @@ public class MockitoBeanDuplicateTypeIntegrationTests { @Test void duplicateMocksShouldHaveBeenCreated() { - assertThat(service1).isNotSameAs(service2); - assertThat(services).hasSize(2); + assertThat(services).containsExactly(mock1, mock2); + assertThat(mock1).isNotSameAs(mock2); + assertIsMock(mock1); + assertIsMock(mock2); } } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeReplacementIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeReplacementIntegrationTests.java new file mode 100644 index 000000000000..76124c07383a --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanDuplicateTypeReplacementIntegrationTests.java @@ -0,0 +1,97 @@ +/* + * 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.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +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; +import static org.mockito.BDDMockito.given; +import static org.springframework.test.context.bean.override.mockito.MockReset.AFTER; +import static org.springframework.test.context.bean.override.mockito.MockReset.BEFORE; +import static org.springframework.test.mockito.MockitoAssertions.assertIsMock; + +/** + * Integration tests for {@link MockitoBean @MockitoBean} where duplicate mocks + * are created to replace the same existing bean, selected by-type. + * + *

In other words, this test class demonstrates how one {@code @MockitoBean} + * can silently override another {@code @MockitoBean}. + * + * @author Sam Brannen + * @since 6.2.1 + * @see gh-34056 + * @see MockitoBeanDuplicateTypeCreationIntegrationTests + * @see MockitoSpyBeanDuplicateTypeIntegrationTests + */ +@SpringJUnitConfig +public class MockitoBeanDuplicateTypeReplacementIntegrationTests { + + @MockitoBean(reset = AFTER) + ExampleService mock1; + + @MockitoBean(reset = BEFORE) + ExampleService mock2; + + @Autowired + List services; + + /** + * One could argue that we would ideally expect an exception to be thrown when + * two competing mocks are created to replace the same existing bean; however, + * we currently only log a warning in such cases. + *

This method therefore asserts the status quo in terms of behavior. + *

And the log can be manually checked to verify that an appropriate + * warning was logged. + */ + @Test + void onlyOneMockShouldHaveBeenCreated() { + // Currently logs something similar to the following. + // + // WARN - Bean with name 'exampleService' was overridden by multiple handlers: + // [MockitoBeanOverrideHandler@5478ce1e ..., MockitoBeanOverrideHandler@5edc70ed ...] + + // Last one wins: there's only one physical mock + assertThat(services).containsExactly(mock2); + assertThat(mock1).isSameAs(mock2); + + assertIsMock(mock2); + assertThat(MockReset.get(mock2)).as("MockReset").isEqualTo(BEFORE); + + assertThat(mock2.greeting()).isNull(); + given(mock2.greeting()).willReturn("mocked"); + assertThat(mock2.greeting()).isEqualTo("mocked"); + } + + + @Configuration + static class Config { + + @Bean + ExampleService exampleService() { + return () -> "@Bean"; + } + } + +} diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanOverridesTestBeanIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanOverridesTestBeanIntegrationTests.java new file mode 100644 index 000000000000..9a52589f0ef2 --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoBeanOverridesTestBeanIntegrationTests.java @@ -0,0 +1,103 @@ +/* + * 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.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.test.context.bean.override.convention.TestBean; +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; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.BDDMockito.given; +import static org.springframework.test.mockito.MockitoAssertions.assertIsMock; + +/** + * Integration tests for Bean Overrides where a {@link MockitoBean @MockitoBean} + * overrides a {@link TestBean @TestBean} when trying to replace the same existing + * bean, selected by-type. + * + *

In other words, this test class demonstrates how one Bean Override can + * silently override another Bean Override. + * + * @author Sam Brannen + * @since 6.2.1 + * @see gh-34056 + * @see MockitoBeanDuplicateTypeCreationIntegrationTests + * @see MockitoBeanDuplicateTypeReplacementIntegrationTests + */ +@SpringJUnitConfig +public class MockitoBeanOverridesTestBeanIntegrationTests { + + @TestBean + ExampleService testService; + + @MockitoBean + ExampleService mockService; + + @Autowired + List services; + + + static ExampleService testService() { + return new RealExampleService("@TestBean"); + } + + + /** + * One could argue that we would ideally expect an exception to be thrown when + * two competing overrides are created to replace the same existing bean; however, + * we currently only log a warning in such cases. + *

This method therefore asserts the status quo in terms of behavior. + *

And the log can be manually checked to verify that an appropriate + * warning was logged. + */ + @Test + void mockitoBeanShouldOverrideTestBean() { + // Currently logs something similar to the following. + // + // WARN - Bean with name 'exampleService' was overridden by multiple handlers: + // [TestBeanOverrideHandler@770beef5 ..., MockitoBeanOverrideHandler@6dd1f638 ...] + + // Last override wins... + assertThat(services).containsExactly(mockService); + assertThat(testService).isSameAs(mockService); + + assertIsMock(mockService); + + assertThat(mockService.greeting()).isNull(); + given(mockService.greeting()).willReturn("mocked"); + assertThat(mockService.greeting()).isEqualTo("mocked"); + } + + + @Configuration + static class Config { + + @Bean + ExampleService exampleService() { + return () -> "@Bean"; + } + } + +} diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeIntegrationTests.java index 35a813847466..772fab5e2c45 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/MockitoSpyBeanDuplicateTypeIntegrationTests.java @@ -36,28 +36,34 @@ * * @author Sam Brannen * @since 6.2.1 - * @see MockitoBeanDuplicateTypeIntegrationTests + * @see gh-34056 + * @see MockitoBeanDuplicateTypeCreationIntegrationTests * @see MockitoSpyBeanDuplicateTypeAndNameIntegrationTests */ @SpringJUnitConfig public class MockitoSpyBeanDuplicateTypeIntegrationTests { @MockitoSpyBean - ExampleService service1; + ExampleService spy1; @MockitoSpyBean - ExampleService service2; + ExampleService spy2; @Autowired List services; @Test - void test() { - assertThat(service1).isSameAs(service2); - assertThat(services).containsExactly(service1); + void onlyOneSpyShouldHaveBeenCreated() { + // Currently logs something similar to the following. + // + // WARN - Bean with name 'exampleService' was overridden by multiple handlers: + // [MockitoSpyBeanOverrideHandler@1d269ed7 ..., MockitoSpyBeanOverrideHandler@437ebf59 ...] - assertIsSpy(service1, "service1"); + assertThat(services).containsExactly(spy2); + assertThat(spy1).isSameAs(spy2); + + assertIsSpy(spy2); }