From acdd8a00154d34a7fae909e473e600447fa31621 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Mon, 3 Jun 2024 16:57:19 +0200 Subject: [PATCH] Improve exception handling in bean override support - Do not silently abort bean override processing if the ApplicationContext is not a BeanDefinitionRegistry. - Include conflicting bean names in error messages instead of just the number of conflicting beans. - Consistently throw IllegalStateException. - etc. --- .../BeanOverrideBeanFactoryPostProcessor.java | 38 ++++++++++--------- .../BeanOverrideContextCustomizer.java | 6 ++- .../convention/TestBeanOverrideProcessor.java | 7 ++-- ...FailingTestBeanByTypeIntegrationTests.java | 12 +++--- ...lingMockitoBeanByTypeIntegrationTests.java | 12 +++--- ...gMockitoSpyBeanByTypeIntegrationTests.java | 10 +++-- ...FailingMockitoSpyBeanIntegrationTests.java | 2 +- 7 files changed, 48 insertions(+), 39 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 c741c1691b6f..3d1b851c661f 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 @@ -130,15 +130,16 @@ private void registerReplaceDefinition(ConfigurableListableBeanFactory beanFacto String beanName = overrideMetadata.getBeanName(); BeanDefinition existingBeanDefinition = null; if (beanName == null) { - Set candidates = getExistingBeanNamesByType(beanFactory, overrideMetadata, true); - if (candidates.size() != 1) { - Field f = overrideMetadata.getField(); - throw new IllegalStateException("Unable to select a bean definition to override: " + - candidates.size() + " bean definitions found of type " + overrideMetadata.getBeanType() + - " (as required by annotated field '" + f.getDeclaringClass().getSimpleName() + - "." + f.getName() + "')"); + Set candidateNames = getExistingBeanNamesByType(beanFactory, overrideMetadata, true); + int candidateCount = candidateNames.size(); + if (candidateCount != 1) { + Field field = overrideMetadata.getField(); + throw new IllegalStateException("Unable to select a bean definition to override: found " + + candidateCount + " bean definitions of type " + overrideMetadata.getBeanType() + + " (as required by annotated field '" + field.getDeclaringClass().getSimpleName() + + "." + field.getName() + "')" + (candidateCount > 0 ? ": " + candidateNames : "")); } - beanName = candidates.iterator().next(); + beanName = candidateNames.iterator().next(); existingBeanDefinition = beanFactory.getBeanDefinition(beanName); } else { @@ -147,8 +148,8 @@ private void registerReplaceDefinition(ConfigurableListableBeanFactory beanFacto existingBeanDefinition = beanFactory.getBeanDefinition(beanName); } else if (enforceExistingDefinition) { - throw new IllegalStateException("Unable to override bean '" + beanName + "': there is no" + - " bean definition to replace with that name of type " + overrideMetadata.getBeanType()); + throw new IllegalStateException("Unable to override bean '" + beanName + "': there is no " + + "bean definition to replace with that name of type " + overrideMetadata.getBeanType()); } } @@ -181,20 +182,21 @@ private void registerWrapBean(ConfigurableListableBeanFactory beanFactory, Overr String beanName = metadata.getBeanName(); if (beanName == null) { Set candidateNames = getExistingBeanNamesByType(beanFactory, metadata, true); - if (candidateNames.size() != 1) { - Field f = metadata.getField(); - throw new IllegalStateException("Unable to select a bean to override by wrapping: " + - candidateNames.size() + " bean instances found of type " + metadata.getBeanType() + - " (as required by annotated field '" + f.getDeclaringClass().getSimpleName() + - "." + f.getName() + "')"); + int candidateCount = candidateNames.size(); + if (candidateCount != 1) { + Field field = metadata.getField(); + throw new IllegalStateException("Unable to select a bean to override by wrapping: found " + + candidateCount + " bean instances of type " + metadata.getBeanType() + + " (as required by annotated field '" + field.getDeclaringClass().getSimpleName() + + "." + field.getName() + "')" + (candidateCount > 0 ? ": " + candidateNames : "")); } beanName = candidateNames.iterator().next(); } else { Set candidates = getExistingBeanNamesByType(beanFactory, metadata, false); if (!candidates.contains(beanName)) { - throw new IllegalStateException("Unable to override bean '" + beanName + "' by wrapping: there is no" + - " existing bean instance with that name of type " + metadata.getBeanType()); + throw new IllegalStateException("Unable to override bean '" + beanName + "' by wrapping: there is no " + + "existing bean instance with that name of type " + metadata.getBeanType()); } } this.overrideRegistrar.markWrapEarly(metadata, beanName); diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizer.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizer.java index 2f16992b472f..dbc22897f79c 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizer.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/BeanOverrideContextCustomizer.java @@ -81,9 +81,11 @@ private static void addInfrastructureBeanDefinition(BeanDefinitionRegistry regis @Override public void customizeContext(ConfigurableApplicationContext context, MergedContextConfiguration mergedConfig) { - if (context instanceof BeanDefinitionRegistry registry) { - registerInfrastructure(registry, this.detectedClasses); + if (!(context instanceof BeanDefinitionRegistry registry)) { + throw new IllegalStateException("Cannot process bean overrides with an ApplicationContext " + + "that doesn't implement BeanDefinitionRegistry: " + context.getClass()); } + registerInfrastructure(registry, this.detectedClasses); } @Override diff --git a/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessor.java b/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessor.java index 68cecbae8c51..462c4f8a9631 100644 --- a/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessor.java +++ b/spring-test/src/main/java/org/springframework/test/context/bean/override/convention/TestBeanOverrideProcessor.java @@ -111,9 +111,8 @@ static Method findTestBeanFactoryMethod(Class clazz, Class methodReturnTyp @Override public TestBeanOverrideMetadata createMetadata(Annotation overrideAnnotation, Class testClass, Field field) { if (!(overrideAnnotation instanceof TestBean testBeanAnnotation)) { - throw new IllegalStateException(String.format("Invalid annotation passed to %s: expected @TestBean on field %s.%s", - TestBeanOverrideProcessor.class.getSimpleName(), field.getDeclaringClass().getName(), - field.getName())); + throw new IllegalStateException("Invalid annotation passed to %s: expected @TestBean on field %s.%s" + .formatted(getClass().getSimpleName(), field.getDeclaringClass().getName(), field.getName())); } Method overrideMethod; String methodName = testBeanAnnotation.methodName(); @@ -172,7 +171,7 @@ protected Object createOverride(String beanName, @Nullable BeanDefinition existi return this.overrideMethod.invoke(null); } catch (IllegalAccessException | InvocationTargetException ex) { - throw new IllegalArgumentException("Could not invoke bean overriding method " + this.overrideMethod.getName() + + throw new IllegalStateException("Failed to invoke bean overriding method " + this.overrideMethod.getName() + "; a static method with no formal parameters is expected", ex); } } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/FailingTestBeanByTypeIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/FailingTestBeanByTypeIntegrationTests.java index 3c440aaa42c1..c3418ea9a200 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/FailingTestBeanByTypeIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/convention/FailingTestBeanByTypeIntegrationTests.java @@ -16,6 +16,8 @@ package org.springframework.test.context.bean.override.convention; +import java.util.List; + import org.junit.jupiter.api.Test; import org.springframework.context.annotation.Bean; @@ -49,8 +51,8 @@ void zeroCandidates() { cause( instanceOf(IllegalStateException.class), message(""" - Unable to select a bean definition to override: 0 bean definitions \ - found of type %s (as required by annotated field '%s.example')""" + Unable to select a bean definition to override: found 0 bean definitions \ + of type %s (as required by annotated field '%s.example')""" .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); } @@ -62,9 +64,9 @@ void tooManyCandidates() { cause( instanceOf(IllegalStateException.class), message(""" - Unable to select a bean definition to override: 2 bean definitions \ - found of type %s (as required by annotated field '%s.example')""" - .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); + Unable to select a bean definition to override: found 2 bean definitions \ + of type %s (as required by annotated field '%s.example'): %s""" + .formatted(ExampleService.class.getName(), testClass.getSimpleName(), List.of("bean1", "bean2")))))); } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoBeanByTypeIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoBeanByTypeIntegrationTests.java index 34636d51bd46..62a490609ef2 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoBeanByTypeIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoBeanByTypeIntegrationTests.java @@ -16,6 +16,8 @@ package org.springframework.test.context.bean.override.mockito; +import java.util.List; + import org.junit.jupiter.api.Test; import org.springframework.context.annotation.Bean; @@ -49,8 +51,8 @@ void zeroCandidates() { cause( instanceOf(IllegalStateException.class), message(""" - Unable to select a bean definition to override: 0 bean definitions \ - found of type %s (as required by annotated field '%s.example')""" + Unable to select a bean definition to override: found 0 bean definitions \ + of type %s (as required by annotated field '%s.example')""" .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); } @@ -62,9 +64,9 @@ void tooManyCandidates() { cause( instanceOf(IllegalStateException.class), message(""" - Unable to select a bean definition to override: 2 bean definitions \ - found of type %s (as required by annotated field '%s.example')""" - .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); + Unable to select a bean definition to override: found 2 bean definitions \ + of type %s (as required by annotated field '%s.example'): %s""" + .formatted(ExampleService.class.getName(), testClass.getSimpleName(), List.of("bean1", "bean2")))))); } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanByTypeIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanByTypeIntegrationTests.java index 3eac5dbe8dc2..f8ff035883ce 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanByTypeIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanByTypeIntegrationTests.java @@ -16,6 +16,8 @@ package org.springframework.test.context.bean.override.mockito; +import java.util.List; + import org.junit.jupiter.api.Test; import org.springframework.context.annotation.Bean; @@ -48,7 +50,7 @@ void zeroCandidates() { cause( instanceOf(IllegalStateException.class), message(""" - Unable to select a bean to override by wrapping: 0 bean instances found of \ + Unable to select a bean to override by wrapping: found 0 bean instances of \ type %s (as required by annotated field '%s.example')""" .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); } @@ -61,9 +63,9 @@ void tooManyCandidates() { cause( instanceOf(IllegalStateException.class), message(""" - Unable to select a bean to override by wrapping: 2 bean instances found of \ - type %s (as required by annotated field '%s.example')""" - .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); + Unable to select a bean to override by wrapping: found 2 bean instances of \ + type %s (as required by annotated field '%s.example'): %s""" + .formatted(ExampleService.class.getName(), testClass.getSimpleName(), List.of("bean1", "bean2")))))); } diff --git a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanIntegrationTests.java index dbebac956e38..fbe04369eda9 100644 --- a/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/bean/override/mockito/FailingMockitoSpyBeanIntegrationTests.java @@ -44,7 +44,7 @@ void failWhenBeanNotPresentByType() { finishedWithFailure( cause(instanceOf(IllegalStateException.class), message(""" - Unable to select a bean to override by wrapping: 0 bean instances found \ + Unable to select a bean to override by wrapping: found 0 bean instances \ of type %s (as required by annotated field '%s.notPresent')""" .formatted(ExampleService.class.getName(), testClass.getSimpleName()))))); }