Improve exception handling in bean override support

sbrannen · sbrannen · commit acdd8a00154d · 2024-06-03T16:57:19.000+02:00
- 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.
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
@@ -130,15 +130,16 @@ private void registerReplaceDefinition(ConfigurableListableBeanFactory beanFacto
 		String beanName = overrideMetadata.getBeanName();
 		BeanDefinition existingBeanDefinition = null;
 		if (beanName == null) {
-			Set<String> 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<String> 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<String> 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<String> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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())))));
 	}

Original file line number	Diff line number	Diff line change
`@@ -81,9 +81,11 @@ private static void addInfrastructureBeanDefinition(BeanDefinitionRegistry regis`
`81`	`81`
`82`	`82`	`@Override`
`83`	`83`	`public void customizeContext(ConfigurableApplicationContext context, MergedContextConfiguration mergedConfig) {`
`84`		`- if (context instanceof BeanDefinitionRegistry registry) {`
`85`		`- registerInfrastructure(registry, this.detectedClasses);`
	`84`	`+ if (!(context instanceof BeanDefinitionRegistry registry)) {`
	`85`	`+ throw new IllegalStateException("Cannot process bean overrides with an ApplicationContext " +`
	`86`	`+ "that doesn't implement BeanDefinitionRegistry: " + context.getClass());`
`86`	`87`	`}`
	`88`	`+ registerInfrastructure(registry, this.detectedClasses);`
`87`	`89`	`}`
`88`	`90`
`89`	`91`	`@Override`
Original file line number	Diff line number	Diff line change
`@@ -111,9 +111,8 @@ static Method findTestBeanFactoryMethod(Class<?> clazz, Class<?> methodReturnTyp`
`111`	`111`	`@Override`
`112`	`112`	`public TestBeanOverrideMetadata createMetadata(Annotation overrideAnnotation, Class<?> testClass, Field field) {`
`113`	`113`	`if (!(overrideAnnotation instanceof TestBean testBeanAnnotation)) {`
`114`		`- throw new IllegalStateException(String.format("Invalid annotation passed to %s: expected @TestBean on field %s.%s",`
`115`		`- TestBeanOverrideProcessor.class.getSimpleName(), field.getDeclaringClass().getName(),`
`116`		`- field.getName()));`
	`114`	`+ throw new IllegalStateException("Invalid annotation passed to %s: expected @TestBean on field %s.%s"`
	`115`	`+ .formatted(getClass().getSimpleName(), field.getDeclaringClass().getName(), field.getName()));`
`117`	`116`	`}`
`118`	`117`	`Method overrideMethod;`
`119`	`118`	`String methodName = testBeanAnnotation.methodName();`
`@@ -172,7 +171,7 @@ protected Object createOverride(String beanName, @Nullable BeanDefinition existi`
`172`	`171`	`return this.overrideMethod.invoke(null);`
`173`	`172`	`}`
`174`	`173`	`catch (IllegalAccessException \| InvocationTargetException ex) {`
`175`		`- throw new IllegalArgumentException("Could not invoke bean overriding method " + this.overrideMethod.getName() +`
	`174`	`+ throw new IllegalStateException("Failed to invoke bean overriding method " + this.overrideMethod.getName() +`
`176`	`175`	`"; a static method with no formal parameters is expected", ex);`
`177`	`176`	`}`
`178`	`177`	`}`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ void failWhenBeanNotPresentByType() {`
`44`	`44`	`finishedWithFailure(`
`45`	`45`	`cause(instanceOf(IllegalStateException.class),`
`46`	`46`	`message("""`
`47`		`- Unable to select a bean to override by wrapping: 0 bean instances found \`
	`47`	`+ Unable to select a bean to override by wrapping: found 0 bean instances \`
`48`	`48`	`of type %s (as required by annotated field '%s.notPresent')"""`
`49`	`49`	`.formatted(ExampleService.class.getName(), testClass.getSimpleName())))));`
`50`	`50`	`}`