From 9107f7b5928ea46cfcd2ff16f3df6bfe464ff653 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 7 Feb 2025 17:59:08 +0100 Subject: [PATCH] =?UTF-8?q?Honor=20@=E2=81=A0Primary=20before=20fallback?= =?UTF-8?q?=20qualifier=20for=20Bean=20Overrides?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prior to this commit, test bean overrides (for example, @⁠MockitoBean, @⁠TestBean, etc.) eagerly honored the name of the annotated field as a fallback qualifier, effectively ignoring @⁠Primary and @⁠Fallback semantics for certain use cases. This led to situations where a bean override for a test would select a different bean than the core container would for the same autowiring metadata. To address that, this commit revises the implementation of BeanOverrideBeanFactoryPostProcessor so that @⁠Primary and @⁠Fallback semantics are consistently honored before attempting to use the annotated field's name as a fallback qualifier. Closes gh-34374 --- .../BeanOverrideBeanFactoryPostProcessor.java | 93 ++++++++++------ ...eConflictingQualifierIntegrationTests.java | 100 ++++++++++++++++++ ...ingBeansAndOnePrimaryIntegrationTests.java | 3 +- .../test/mockito/MockitoAssertions.java | 10 +- 4 files changed, 169 insertions(+), 37 deletions(-) create mode 100644 spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryAndOneConflictingQualifierIntegrationTests.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 c9224bc70b3..1a01295a17e 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 @@ -249,26 +249,21 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, if (beanName == null) { // We are wrapping an existing bean by-type. Set candidateNames = getExistingBeanNamesByType(beanFactory, handler, true); - int candidateCount = candidateNames.size(); - if (candidateCount == 1) { - beanName = candidateNames.iterator().next(); + String uniqueCandidate = determineUniqueCandidate(beanFactory, candidateNames, beanType, field); + if (uniqueCandidate != null) { + beanName = uniqueCandidate; } else { - String primaryCandidate = determinePrimaryCandidate(beanFactory, candidateNames, beanType.toClass()); - if (primaryCandidate != null) { - beanName = primaryCandidate; + String message = "Unable to select a bean to wrap: "; + int candidateCount = candidateNames.size(); + if (candidateCount == 0) { + message += "there are no beans of type %s%s.".formatted(beanType, requiredByField(field)); } else { - String message = "Unable to select a bean to wrap: "; - if (candidateCount == 0) { - message += "there are no beans of type %s%s.".formatted(beanType, requiredByField(field)); - } - else { - message += "found %d beans of type %s%s: %s" - .formatted(candidateCount, beanType, requiredByField(field), candidateNames); - } - throw new IllegalStateException(message); + message += "found %d beans of type %s%s: %s" + .formatted(candidateCount, beanType, requiredByField(field), candidateNames); } + throw new IllegalStateException(message); } beanName = BeanFactoryUtils.transformedBeanName(beanName); } @@ -287,18 +282,20 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, } @Nullable - private String getBeanNameForType(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler, + private static String getBeanNameForType(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler, boolean requireExistingBean) { Field field = handler.getField(); ResolvableType beanType = handler.getBeanType(); Set candidateNames = getExistingBeanNamesByType(beanFactory, handler, true); - int candidateCount = candidateNames.size(); - if (candidateCount == 1) { - return candidateNames.iterator().next(); + String uniqueCandidate = determineUniqueCandidate(beanFactory, candidateNames, beanType, field); + if (uniqueCandidate != null) { + return uniqueCandidate; } - else if (candidateCount == 0) { + + int candidateCount = candidateNames.size(); + if (candidateCount == 0) { if (requireExistingBean) { throw new IllegalStateException( "Unable to override bean: there are no beans of type %s%s." @@ -307,18 +304,13 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, return null; } - String primaryCandidate = determinePrimaryCandidate(beanFactory, candidateNames, beanType.toClass()); - if (primaryCandidate != null) { - return primaryCandidate; - } - throw new IllegalStateException( "Unable to select a bean to override: found %d beans of type %s%s: %s" .formatted(candidateCount, beanType, requiredByField(field), candidateNames)); } - private Set getExistingBeanNamesByType(ConfigurableListableBeanFactory beanFactory, BeanOverrideHandler handler, - boolean checkAutowiredCandidate) { + private static Set getExistingBeanNamesByType(ConfigurableListableBeanFactory beanFactory, + BeanOverrideHandler handler, boolean checkAutowiredCandidate) { Field field = handler.getField(); ResolvableType resolvableType = handler.getBeanType(); @@ -345,25 +337,56 @@ class BeanOverrideBeanFactoryPostProcessor implements BeanFactoryPostProcessor, // Filter out scoped proxy targets. beanNames.removeIf(ScopedProxyUtils::isScopedTarget); - // In case of multiple matches, fall back on the field's name as a last resort. - if (field != null && beanNames.size() > 1) { + return beanNames; + } + + /** + * Determine the unique candidate in the given set of bean names. + *

Honors both primary and fallback semantics, and + * otherwise matches against the field name as a fallback qualifier. + * @return the name of the unique candidate, or {@code null} if none found + * @since 6.2.3 + * @see org.springframework.beans.factory.support.DefaultListableBeanFactory#determineAutowireCandidate + */ + @Nullable + private static String determineUniqueCandidate(ConfigurableListableBeanFactory beanFactory, + Set candidateNames, ResolvableType beanType, @Nullable Field field) { + + // Step 0: none or only one + int candidateCount = candidateNames.size(); + if (candidateCount == 0) { + return null; + } + if (candidateCount == 1) { + return candidateNames.iterator().next(); + } + + // Step 1: check primary candidate + String primaryCandidate = determinePrimaryCandidate(beanFactory, candidateNames, beanType.toClass()); + if (primaryCandidate != null) { + return primaryCandidate; + } + + // Step 2: use the field name as a fallback qualifier + if (field != null) { String fieldName = field.getName(); - if (beanNames.contains(fieldName)) { - return Set.of(fieldName); + if (candidateNames.contains(fieldName)) { + return fieldName; } } - return beanNames; + + return null; } /** * Determine the primary candidate in the given set of bean names. *

Honors both primary and fallback semantics. * @return the name of the primary candidate, or {@code null} if none found - * @see org.springframework.beans.factory.support.DefaultListableBeanFactory#determinePrimaryCandidate(Map, Class) + * @see org.springframework.beans.factory.support.DefaultListableBeanFactory#determinePrimaryCandidate */ @Nullable - private static String determinePrimaryCandidate( - ConfigurableListableBeanFactory beanFactory, Set candidateBeanNames, Class beanType) { + private static String determinePrimaryCandidate(ConfigurableListableBeanFactory beanFactory, + Set candidateBeanNames, Class beanType) { if (candidateBeanNames.isEmpty()) { return null; diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryAndOneConflictingQualifierIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryAndOneConflictingQualifierIntegrationTests.java new file mode 100644 index 00000000000..d29657a351e --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryAndOneConflictingQualifierIntegrationTests.java @@ -0,0 +1,100 @@ +/* + * Copyright 2002-2025 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.integration; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.ApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; +import org.springframework.context.annotation.Primary; +import org.springframework.stereotype.Component; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import static org.mockito.BDDMockito.then; +import static org.springframework.test.mockito.MockitoAssertions.assertIsMock; +import static org.springframework.test.mockito.MockitoAssertions.assertIsNotMock; + +/** + * Tests that {@link MockitoBean @MockitoBean} can be used to mock a bean when + * there are multiple candidates; one is primary; and the field name matches + * the name of a candidate which is not the primary candidate. + * + * @author Sam Brannen + * @since 6.2.3 + * @see MockitoBeanWithMultipleExistingBeansAndOnePrimaryIntegrationTests + * @see MockitoBeanWithMultipleExistingBeansAndExplicitBeanNameIntegrationTests + * @see MockitoBeanWithMultipleExistingBeansAndExplicitQualifierIntegrationTests + */ +@ExtendWith(SpringExtension.class) +class MockitoBeanWithMultipleExistingBeansAndOnePrimaryAndOneConflictingQualifierIntegrationTests { + + // The name of this field must be "baseService" to match the name of the non-primary candidate. + @MockitoBean + BaseService baseService; + + @Autowired + Client client; + + + @Test // gh-34374 + void test(ApplicationContext context) { + assertIsMock(baseService, "baseService field"); + assertIsMock(context.getBean("extendedService"), "extendedService bean"); + assertIsNotMock(context.getBean("baseService"), "baseService bean"); + + client.callService(); + + then(baseService).should().doSomething(); + } + + + @Configuration(proxyBeanMethods = false) + @Import({ BaseService.class, ExtendedService.class, Client.class }) + static class Config { + } + + @Component("baseService") + static class BaseService { + + public void doSomething() { + } + } + + @Primary + @Component("extendedService") + static class ExtendedService extends BaseService { + } + + @Component("client") + static class Client { + + private final BaseService baseService; + + public Client(BaseService baseService) { + this.baseService = baseService; + } + + public void callService() { + this.baseService.doSomething(); + } + } + +} diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryIntegrationTests.java index 25d48d64145..b4c8a245b3d 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/integration/MockitoBeanWithMultipleExistingBeansAndOnePrimaryIntegrationTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -43,6 +43,7 @@ import static org.springframework.test.mockito.MockitoAssertions.assertMockName; * @author Sam Brannen * @author Phillip Webb * @since 6.2 + * @see MockitoBeanWithMultipleExistingBeansAndOnePrimaryAndOneConflictingQualifierIntegrationTests * @see MockitoBeanWithMultipleExistingBeansAndExplicitBeanNameIntegrationTests * @see MockitoBeanWithMultipleExistingBeansAndExplicitQualifierIntegrationTests */ diff --git a/spring-test/src/test/java/org/springframework/test/mockito/MockitoAssertions.java b/spring-test/src/test/java/org/springframework/test/mockito/MockitoAssertions.java index b4a19e11994..5427fa6dc36 100644 --- a/spring-test/src/test/java/org/springframework/test/mockito/MockitoAssertions.java +++ b/spring-test/src/test/java/org/springframework/test/mockito/MockitoAssertions.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -37,6 +37,14 @@ public abstract class MockitoAssertions { assertThat(isMock(obj)).as("%s is a Mockito mock", message).isTrue(); } + public static void assertIsNotMock(Object obj) { + assertThat(isMock(obj)).as("is a Mockito mock").isFalse(); + } + + public static void assertIsNotMock(Object obj, String message) { + assertThat(isSpy(obj)).as("%s is a Mockito mock", message).isFalse(); + } + public static void assertIsSpy(Object obj) { assertThat(isSpy(obj)).as("is a Mockito spy").isTrue(); }