Open parameter autowiring utility for external use

[ledoyen]

Background

This PR is to open utility methods used by the SpringExtension for JUnit Jupiter in spring-test.

These mechanisms can be used in other test frameworks, and are already very well documented and tested.

Proposal

Open autowiring utility methods in ParameterAutowireUtils, so they can be used by other frameworks.

Deliverables

• Make AutowireUtils public and declare its current methods as package-private.
• Move functionality currently in ParameterAutowireUtils to AutowireUtils and make the isAutowirable() and resolveDependency() methods public.
• Refactor the SpringExtension to use AutowireUtils instead of ParameterAutowireUtils.
• Remove ParameterAutowireUtils from the spring-test module.
• Introduce unit tests in AutowireUtilsTests for isAutowirable() and resolveDependency().

[sbrannen]

Hi @ledoyen,

Would you mind opening a JIRA ticket to request this feature so that we can better track it and discuss it?

For example, if we open up this API we would need to consider where this utility class should live. In other words, if we make it public, it is rather unlikely that it would remain in the Jupiter-specific package in spring-test.

Also, I would like to know how you intend to use this in "other test frameworks".

Thanks,

Sam

[ledoyen]

Sure thing, I thought this kind of change fall under the scope of 'trivial' cases.

Here it is : #19926.

I intent to use this in a test framework I am building to support specification by example workshops: Glacio. I think that the extension way of JUnit5 is very inspiring as it allow composition of plugins and I would like to have that same easiness of composition in Glacio. This is for example different from cucumber-jvm where test classes lifecycle is handled by the extension (the ObjectFactory) rather than by the framework itself.

[ledoyen]

Let me know if you need me to change the PR to change package, add specific tests, etc.

[sbrannen]

Sure thing, I thought this kind of change fall under the scope of 'trivial' cases.

Well, if we only change the visibility of the class and methods, then yes it is rather trivial, but we may do more than that. 😉

Here it is : SPR-17627.

Thanks

I intent to use this in a test framework I am building to support specification by example workshops: Glacio.

Thanks for the link. I'll take a look.

I think that the extension way of JUnit5 is very inspiring as it allow composition of plugins

I'm glad you like it!

and I would like to have that same easiness of composition in Glacio.

Understood.

[sbrannen]

@ledoyen, have you considered providing a link to Glacio here?

[ledoyen]

Yes, but for now it is merely a proof of concept.
Once I have valid feedbacks from the battleground, I will propose it for the Third-party-Extensions page.
Thanks for asking 😃

[sbrannen]

Updated status to "pending-design-work"

[sbrannen]

FYI: this issue is closely related to #18628.

[sbrannen]

If org.springframework.beans.factory.support.AutowireUtils were a public utility class, I would likely recommend that we move the functionality in ParameterAutowireUtils into AutowireUtils.

However, since AutowireUtils is not public, I propose that we simply relocate ParameterAutowireUtils to the org.springframework.beans.factory.support package alongside AutowireUtils and make the ParameterAutowireUtils class and its isAutowirable() and resolveDependency() methods public.

@jhoeller, are you OK with the above proposal?

[sbrannen]

Team Decision:

• make AutowireUtils public and declare its current methods package-private.
• move the functionality of ParameterAutowireUtils to AutowireUtils and make the isAutowirable() and resolveDependency() methods public.

I'll update the Deliverables for this issue accordingly.

[sbrannen]

OK, @ledoyen, I've updated the Deliverables for this issue.

Do you have time to rework the PR to cover those tasks?

If so, please let us know approximately when you think you can do that.

If you don't have time to do that within the coming week or so, I may go ahead and complete the tasks.

So, thanks in advance for feedback!

Sam

[ledoyen]

Yes I have time to work on what have been decided.

However, as the two public methods of ParameterAutowireUtils are not directly tested, but tested through the tests of SpringExtension, do they need specific tests ?

Furthermore, methods of ParameterAutowireUtils use ApplicationContext, which is not defined in the scope of the spring-beans project.
I would appreciate some hint on this topic, as the only solution I see is to keep ParameterAutowireUtils in spring-test or move it to spring-context, without changing AutowireUtils which is needed in spring-beans.

[sbrannen]

Yes I have time to work on what have been decided.

Great!

However, as the two public methods of ParameterAutowireUtils are not directly tested, but tested through the tests of SpringExtension, do they need specific tests ?

I think they do. When a class is opened up for direct use by the public, I prefer to have local unit tests in place.

But if you don't have time for that, don't worry about it: I can pick up that task after the merge.

Furthermore, methods of ParameterAutowireUtils use ApplicationContext, which is not defined in the scope of the spring-beans project.

Oops! You're totally right.

I would appreciate some hint on this topic, as the only solution I see is to keep ParameterAutowireUtils in spring-test or move it to spring-context, without changing AutowireUtils which is needed in spring-beans.

Technically speaking, ParameterAutowireUtils only needs to know about the ApplicationContext type in isAutowirable(). For resolveDependency() we could change the type of the last parameter from ApplicationContext to AutowireCapableBeanFactory, and the caller would then simply have to invoke ApplicationContext#getAutowireCapableBeanFactory() to supply the bean factory instead of the application context.

I guess the main question is "Do we want to have the 'instance of ApplicationContext' check in isAutowirable()?"

1. If so, we would then have to move ParameterAutowireUtils to spring-context.
2. If not, we could move the 'instance of ApplicationContext' check to SpringExtension#supportsParameter().

The downside of the latter choice is that third-party clients of ParameterAutowireUtils would not benefit from a built-in 'instance of ApplicationContext' check.

I am therefore leaning toward moving ParameterAutowireUtils to spring-context — for example, in the org.springframework.context.support package.

@jhoeller, thoughts?

[sbrannen]

@ledoyen, in a7770e7 I removed the dependency on ApplicationContext from ParameterAutowireUtils.

This allows you to implement the Deliverables as currently stated.

Please let me know if you need any further assistance.

[ledoyen]

Sounds good to me. I will ge back to you with some code tomorrow evening if that is ok to you.

[ledoyen]

Here is the proposal, hoping Deliverables have the required quality.

[sbrannen]

Here is the proposal, hoping Deliverables have the required quality.

Yep, I think looks pretty good.

Thanks!

[sbrannen]

This has been merged into master.

[sbrannen]

Further refinements were pushed in 6f6be27.

[ledoyen]

Thanks for pointing that out !

[sbrannen]

Thanks for pointing that out !

If you're referring to the "refinements", sure... no problem!

By the way, the build was also broken due to CheckStyle violations, but I fixed that in f2a5415.

Those CheckStyle violations catch me by surprise every time.

[ledoyen]

Checkstyle may run in PR checks (I think I have seen that of the PMD project), so that eventually external commiters can correct that by themselves.

[sbrannen]

Checkstyle may run in PR checks (I think I have seen that of the PMD project), so that eventually external commiters can correct that by themselves.

Would you mind opening a new GitHub issue to address that?

[ledoyen]

Done : #22487

[sbrannen]

Done : #22487

Thanks

[jhoeller]

@sbrannen I'm afraid we have to take a step back and move those two new methods to a utility in beans.factory.annotation since the present solution has a cycle between beans.factory.support and beans.factory.annotation (due to the references to our annotation types)...

[jhoeller]

Maybe a ParameterResolutionDelegate class in beans.factory.annotation? That's where I've locally moved it to, seems to work/read fine so far.

[jhoeller]

On review, this looks like an ideal location indeed, right next to the annotation types that it actually resolves (also right next to the analogous AutowiredAnnotationBeanPostProcessor) and well-placed for its beans.factory.annotation/config dependencies (since the parameter resolution code doesn't have any beans.factory.support dependency itself).

If there are no objections to the name ParameterResolutionDelegate, I'll commit this revision along with Phil's MergedAnnotations PR later today.

[sbrannen]

Thanks for catching the cycle, @jhoeller! 😱

It sounds like moving it to beans.factory.annotation is indeed the best solution.

As for the name, just go with your ParameterResolutionDelegate proposal for now, and we can change the name before GA if we so desire.

Cheers