Fix: type matching for request-scope generic beans

[currenjin]

This PR fixes an issue where @MockBean fails to work with request-scoped Supplier<T> without explicit bean name.

Issue

When using @MockBean with a request-scoped generic bean (especially Supplier<T>), Spring fails to match the bean by type unless the bean name is explicitly specified. This happens because the type matching algorithm doesn't properly handle the generic type information for scoped proxy beans.

Solution

The solution enhances AbstractBeanFactory.isTypeMatch method to check for beans with a scope and look up their corresponding target bean definitions. When a scoped proxy bean is found, the method tries to match the type against the target bean's resolvable type.

Testing

Added a test case that verifies type matching works correctly for scoped proxy beans with generic types.

Fixes gh-30043

[pankratz76]

nicely done

[pankratz76]

The two if statements at the same level look suspicious at first glance, making me think twice and question the logic. Moving the second if inside the first one would make it clear that it is an intended recheck of the first operation returning null. Nesting it this way makes the intent clearer—at least to me.

Alternatively, using the defaultIfNull method—although nested in this case as it’s a double-check method—would achieve the same result in just three lines. This keeps the logic concise, leaving only the essential parts without any redundancy.

Suggested change

	ResolvableType targetResolvableType = targetMbd.targetType;
	if (targetResolvableType == null) {
	targetResolvableType = targetMbd.factoryMethodReturnType;
	}
	if (targetResolvableType == null) {
	targetResolvableType = ResolvableType.forClass(targetMbd.getBeanClass());
	}
	if (typeToMatch.isAssignableFrom(targetResolvableType)) {
	ObjectUtils.defaultIfNull(targetMbd.targetType,
	ObjectUtils.defaultIfNull(targetMbd.factoryMethodReturnType, ResolvableType.forClass(targetMbd.getBeanClass())))
	if (typeToMatch.isAssignableFrom(targetResolvableType)) {

[currenjin]

Thanks for the feedback on the nested if statements! I've refactored the code to make the intent clearer, as you suggested.

Instead of having two if statements at the same level, I've now properly nested them to show the fallback logic more explicitly:

ResolvableType targetResolvableType = targetMbd.targetType;
if (targetResolvableType == null) {
    targetResolvableType = targetMbd.factoryMethodReturnType;
    if (targetResolvableType == null) {
       targetResolvableType = ResolvableType.forClass(targetMbd.getBeanClass());
    }
}

[pankratz76]

considering randomized test data as String targetBeanName = "scopedTarget.wordBean"; would pass the test but fail production.

[currenjin]

Thank you for the feedback! I've addressed your concerns in two commits:

1. I've improved the resolvable type extraction logic in isTypeMatch method by using properly nested if statements for better clarity of intent. This makes the fallback logic more explicit while maintaining the same functionality. Unfortunately, I couldn't use the defaultIfNull method as it's not available in this version of Spring.

2. I've refactored the test to use variables for bean names instead of hardcoded strings. This makes the test more flexible and better simulates real-world scenarios where bean names might vary, addressing your concern about potential production issues. The variables maintain the "scopedTarget." prefix convention that Spring uses, while allowing the actual names to be dynamic.

Let me know if you have any other suggestions!

[pankratz76]

considering

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/ObjectUtils.html#defaultIfNull(T,T)

#34587

we should be able to reduce all redundancy by using the internal ternary operator hidden inside ObjectUtils.defaultIfNull

Suggested change

	ResolvableType targetResolvableType = targetMbd.targetType;
	if (targetResolvableType == null) {
	targetResolvableType = targetMbd.factoryMethodReturnType;
	if (targetResolvableType == null) {
	targetResolvableType = ResolvableType.forClass(targetMbd.getBeanClass());
	}
	}
	if (typeToMatch.isAssignableFrom(targetResolvableType)) {
	if (typeToMatch.isAssignableFrom(ObjectUtils.getIfNull(targetResolvableType, ObjectUtils.getIfNull(targetMbd.factoryMethodReturnType, ResolvableType.forClass(targetMbd.getBeanClass()))))) {

[pankratz76]

but of course this can be done afterward or not at all, as its working an acceptable anyways.

[currenjin]

Thank you for the suggestion and for pointing me to the ObjectUtils.defaultIfNull from Apache Commons Lang!

I initially wasn't able to use any defaultIfNull method because I was working with org.springframework.util.ObjectUtils, which doesn't have this method in our current version.

Your approach using nested ternary operators looks more concise and elegant. I agree this would be a great improvement, but I think we should leave it for a separate refactoring PR in the future. This way, we can keep the current PR focused on fixing the original issue with clean, minimal changes.

Thanks again for your approval and the valuable feedback throughout this PR!

[currenjin]

Actually, I just noticed you've added the getIfNull method to ObjectUtils.java - this is fantastic!

Could you guide me on how I can best incorporate this change into my implementation? Should I update my PR to use this new method with the nested ternary approach you suggested, or would you prefer to handle that separately after merging?

I'm excited to use this more elegant solution if that's the right approach at this stage.

[pankratz76]

I would suggest merging it as it is and incorporating the refactoring into the Util PR (#34587) so we don’t add empty code and can deliver a practical example right away.

If it’s merged before, then you can, of course, try it here. I would not suggest mixing the PRs now.

Thanks for your heartwarming feedback—teamwork makes the dream work!

[pankratz76]

just some personal inline flavor one considering RSPEC-S1488 and https://docs.openrewrite.org/recipes/staticanalysis/inlinevariable. Now it looks very well structured overall. Good increment. 👍

[pankratz76]

Suggested change

	boolean isMatch = factory.isTypeMatch(proxyBeanName, supplierType);
	assertThat(isMatch).isTrue();
	assertThat(factory.isTypeMatch(proxyBeanName, supplierType)).isTrue();

[pankratz76]

Suggested change

	String[] names = factory.getBeanNamesForType(supplierType);
	assertThat(names).contains(proxyBeanName);
	assertThat(factory.getBeanNamesForType(supplierType)).contains(proxyBeanName);

[pankratz76]

Suggested change

	String targetBeanName = "scopedTarget.wordBean";
	String proxyBeanName = "wordBean";
	String proxyBeanName = "wordBean-" + UUID.randomUUID();
	String targetBeanName = "scopedTarget." + proxyBeanName;

this should still work, right?

[pankratz76]

Is it better to include the Apache library to obtain this syntactic sugar? This utility would reduce a lot of boilerplate code. Or do you prefer the 10 lines of boilerplate code instead of the 1 line that's actually needed?

There is obviously a need for this, so when we can't copy it, can we rely on other open-source frameworks providing the feature already?

Thanks for the advisory. @bclozel

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/ObjectUtils.html#firstNonNull(T...)