XML bean definition with factory-method does not always determine correct target type

[anschnapp]

Affects: 6.3.1 and latest 5.x.x)
Tested on 6.3.1 (but latest 5.x.x is also involved because original project where i face it was on a 5.x.x version)

I had an issue with my xml bean definition when i updated to newest mockito version. It seemed like now the order of beans matter and mocking beans have to be defined before the implementation beans. Otherwise the bean was simply not available as a candidate for injections.

I figured out the root cause of it and i will describe it via a minimal example (so not the mockito case).

It seems like getTypeForFactoryMethod inside AbstractAutowireCapableBeanFactory is used to figure out the return type of a factory bean.

The method is quite complex and it seems like it is not capable of finding the correct and unique method declaration (which would be used for calling it and creating the bean at the end). Instead if finds multiple method candidates. All method candidates which fits a specific "schema" will be taken into account. All potential result classes are determined and in the end reduced to one common type which is the closest super type of all found types.

In my case where the mock method in mockito is highly overloaded. Most determined return types (inside the getTypeForFactoryMethod processing) has the correct class type, some are from type java.lang.Object. Which ends up by choosing the java.lang.Object type as the final determined type. Which of course is not compatible with the concrete service i tried to mock.

If the mock definition was at the beginning then the bean could be created before other bean dependencies would be checked. And then it just worked (because the bean type is clear on creation and the hole complex determine logic isn't called at all).

It seems like the issue occurs if the static factory method is overloaded in a way that the generic parameter is not there or structured differently. Order of parameter seems not to be relevant.

I have put it into a minimal example here:
https://github.com/anschnapp/spring-potential-factory-method-issue-ticket-32091

If you check it out and run it, the test will fail. Please take a closer look the the xml bean definition of the tests and also of the factory method class (which has some javadoc on it to point you on my current assumptions)

In my opinion this behaviour is quite bad. The type for injection type changes now based on the order of bean declaration.
Also using the most close superclass of multiple method candidates is rarely the right thing IMO, and could cause hard to find issues (at least a warning should be logged here).

For me the perfect solution would be if it's somehow possible to add a "hint" for the return type of a method-factory method. Then the logic should simply assume that the type is correct for dependency management. And then later check if this is really correct on creation of the bean. And then all of the very complex and still not correct logic to determine the class type could be avoided.

Edited: as suggested in the comments i have removed the example here and instead link to a runnable minimal example project.

[snicoll]

Let's get to the example.

Thanks but please edit your description to remove all that code in text into an application we can actually run. You can attach a zip to this issue or push the code to a GitHub repository.

[anschnapp]

@snicoll thanks for the fast answer. I have adjusted the description and add a link to a runnable github project, just for the case here is also the link to it: https://github.com/anschnapp/spring-potential-factory-method-issue-ticket-32091

[jhoeller]

This is a long-standing consequence of Spring's factory method overloading in combination with generic return types. Factory methods are primarily meant to refer to a specific component, potentially overloaded for different arguments needed to construct the component - but otherwise always building that specific component type or subclasses thereof. That's where the common return type algorithm comes from, e.g. considering overloaded @Bean methods for the same bean type.

A type hint can be declared on a registered RootBeanDefinition via setTargetType. That's primarily for programmatic registrations in namespace handlers and the like, not exposed in the XML bean definition model.

So for standard XML bean definitions against free-form factory method overloads that are inconsistently based on generics (with no common return type determinable), the type is not going to be available for matching before the bean is actually constructed. You could declare that bean first so that it gets constructed first, or you could add a depends-on declaration from other beans to the generic bean so that the latter gets constructed first in any declaration order:

	<bean class="demo.ServiceWithDependency" autowire="constructor" depends-on="innerService"/>

	<bean id="innerService" class="demo.StaticFactoryMethodUtil" factory-method="create">
		<constructor-arg type="java.lang.Class" value="demo.InnerService"/>
	</bean>

In the most direct fashion, you could express a specific reference to that bean through <constructor-arg ref="...">, resolving the bean by name rather than by type - no depends-on attribute or declaration order necessary then:

	<bean class="demo.ServiceWithDependency">
		<constructor-arg ref="innerService"/>
	</bean>

	<bean id="innerService" class="demo.StaticFactoryMethodUtil" factory-method="create">
		<constructor-arg type="java.lang.Class" value="demo.InnerService"/>
	</bean>

Last but not least, you could create a custom factory class for your specific components. Even for test setups, I would actually recommend that: e.g. a MyFactory class with a static createInnerService method that declares the concrete return type, and the XML bean definition pointing to it. The implementation of that method could call the mock factory then. Alternatively, you could create a @Configuration class for the same purpose, with those concrete factory methods declared as @Bean.

All in all, I'm afraid this works as designed. Given the late stages of the XML bean definition model, we are not going to revise its declaration capability in that respect. The @Configuration model - or a custom factory class as mentioned above - can nicely accommodate such scenarios already and can easily be combined with XML-based component definitions.

You may even use overloaded factory methods, just make sure that each set of overloaded methods (sharing a distinct factory method name) provide a constructor-like arrangement for a specific bean. A generics-based factory method should ideally not be overloaded at all or have consistent generics across its overloads.

[anschnapp]

@jhoeller I work in a old project which is based on many modules and have thousands of tests.

By using dependsOn I would have to know many many dependency trees which are normally resolved automatically in spring. Bringing the mocks to the top also looks quite odd and is hard to argue for other / new developers on the same code base. Switching to java based settings (like i would use for all new projects) is too much effort here.

I currently use a workaround where i created a simpler factory which delegates to mockito. But also this solution is far from being ideal. I had to introduce a new library which i had to include in many modules. Also all developers have to understand why we have this additional indirection and don't simple use mockito as it is. Sure it's nothing critical but still inconvenient.

More important then the inconvenience is that it's not obvious from the documentation that the bean method factory is "primarily meant to refer to a specific component".

Which could cause to hard to find issues for spring users.

There is also a (very old and RC related) spring documentation which described that mock method could be used like this:
https://spring.io/blog/2012/11/07/spring-framework-3-2-rc1-new-testing-features

With the interesting cite:

In Spring 3.2, generic return types for factory methods are now properly inferred, and autowiring by type for mocks should work as expected. As a result, custom work-arounds such as a MockitoFactoryBean, EasyMockFactoryBean, or Springockito are likely no longer necessary.

A quick github search with 2.2k results based on finding only variants which exactly matches:

class="org.mockito.Mockito" factory-method="mock">

(https://github.com/search?q=class%3D%22org.mockito.Mockito%22+factory-method%3D%22mock%22%3E&type=code)

... is also an indicator for me that this is not a totally rare edge case scenario.

If people faces this issue they might need to spend a lot of time to find the issue (takes me a hole day of debugging)

I would suggest that at least the documentation for the xml factory method should be adjusted to give a hint that generic method with many overload candidates could be an issue.

Also a warning if there was a generic method which could not uniquely resolved (or maybe only if the fallback is to java.lang.Object) would be reasonable. (and could safe many time and effort on people searching on this issue)

I could help to adjust such comments / logging if such a change/pr from my side would be welcome.

[sbrannen]

There is also a (very old and RC related) spring documentation which described that mock method could be used like this:
spring.io/blog/2012/11/07/spring-framework-3-2-rc1-new-testing-features

That's a blast from the past! 😉

I had an issue with my xml bean definition when i updated to newest mockito version.

FWIW, we also ran into that issue due to the introduction of the mock(T...) method in Mockito 4.10, and we introduced a workaround by creating a dedicated factory method that delegates to Mockito.mock(Class<T>), which you can see in this commit ad5c636.

spring-framework/spring-beans/src/test/java/org/springframework/beans/factory/annotation/AutowiredAnnotationBeanPostProcessorTests.java

Lines 2182 to 2189 in 8d60138

	/**
	* Mimics and delegates to {@link Mockito#mock(Class)} -- created here to avoid factory
	* method resolution issues caused by the introduction of {@code Mockito.mock(T...)}
	* in Mockito 4.10.
	*/
	public static <T> T createMockitoMock(Class<T> classToMock) {
	return Mockito.mock(classToMock);
	}

[anschnapp]

@jhoeller, if the spring core team run itself into this issue as @sbrannen told. And also internally a workaround was needed to solve it. Wouldn't this be a strong indicator that this behaviour deserves at least a warning and some java docs improvements? To prevent others to run into the same issues, which could be quite hard to find and understand.

I could understand that this old xml bean definition is not that important for modern development and that this issue don't exists for the programmatic bean definition style. Also that this code has proven for many other things over a long time and therefore a bigger refactoring in this area is "not planned".

[jhoeller]

A documentation note certainly makes sense here, warning against use of factory method name resolution against an inconsistent set of target methods, and naming Mockito as an example. Let's reopen this issue for that purpose.

[contivero]

In case it's of any help to anybody bumping into this issue due to Mockito 4.10+ and Spring, apart from the previous proposed solutions, another option is calling the mock method that takes a name, to avoid the ambiguity of the other two. Basically:

<bean id="myService" class="org.mockito.Mockito" factory-method="mock">
	<constructor-arg type="java.lang.Class" value="com.example.MyService" />
	<constructor-arg type="java.lang.String" value="myService" />
</bean>

This is also useful in the case you cannot place the XML bean definition earlier because you want to override some previous definition.

[jhoeller]

@contivero good point, I've added a documentation note with a suggestion along those lines.

[marcosemiao]

The original bug appeared when Mockito introduced in version 4.10 the following method

public static <T> T mock(T... reified)

When Spring analyzes this method it takes all the mock methods regardless of the type with at least the number of parameters defined in the bean
For info class AbstractAutowireCapableBeanFactory method getTypeForFactoryMethod variable minNrOfArgs

When it arrives at this new method that contains a generic type, it returns a java.lang.Object and this is not good.

To avoid the problem @contivero suggests adding a String parameter, this allows not to analyze this new method:

public static <T> T mock(T... reified)

It worked but it does not work again since version Mockito 5.1.0. From this version Mockito introduces 3 new methods that contain 2 input parameters and the second parameter is an array:

The method described allowed to add a second String parameter that allows not to match with the Mockito method:

public static <T> T mock(@SuppressWarnings("rawtypes") Answer defaultAnswer, T... reified)
public static <T> T mock(String name, T... reified)
public static <T> T mock(MockSettings settings, T... reified)

As before, these 3 methods return the java.lang.Object type, so the method proposed by @contivero no longer works. It would be good to update the note in the doc to not give a bad lead.

It's a shame because if this happens to Mockito, it can happen with another library.

Please @jhoeller re-open this issue to at least update the documentation, thank you.