Allow @Autowired to be declared on parameters [SPR-14057]

[spring-projects-issues]

Sam Brannen opened SPR-14057 and commented

Status Quo

@Autowired currently cannot be declared on a parameter.

Impetus

This feature is needed by #18151 and #18628.

Deliverables

• Allow @Autowired to be declared on parameters in order to support dependency injection for individual method or constructor parameters.

---

Issue Links:

• Introduce autowiring support for individual handler method parameters [SPR-14056] #18628 Introduce autowiring support for individual handler method parameters ("is depended on by")
• Introduce support for JUnit 5 in the TestContext framework [SPR-13575] #18151 Introduce support for JUnit 5 in the TestContext framework ("is depended on by")
• Honor @Autowired(required=false) at parameter level, as an alternative to java.util.Optional [SPR-15268] #19833 Honor @Autowired(required=false) at parameter level, as an alternative to java.util.Optional ("is depended on by")
• Compose @Autowired with @Qualifier [SPR-14060] #18632 Compose @Autowired with @Qualifier
• Allow @Qualifier to be used in composed annotations with attribute overrides [SPR-14058] #18630 Allow @Qualifier to be used in composed annotations with attribute overrides

Referenced from: commits a905412

0 votes, 6 watchers

[spring-projects-issues]

Sam Brannen commented

Thanks, Juergen! :)

@SpringBean in spring-test-junit5 has now been completely replaced by @Autowired. Very cool indeed!

sbrannen/spring-test-junit5@886c64a

[spring-projects-issues]

Phil Webb commented

Oliver Drotbohm pointed out on in a discussion that this could add quite a bit of confusion to @Autowired. I agree, and I'm not totally convinced we should allow the annotation to be used on parameters, especially it it's not clear when it will work and when it will fail.

Perhaps there could be another way to solve the JUnit 5 requirement (which seems to be the biggest driver for the issue). Looking at the existing sample, I can't help thinking that @Autowired on parameters adds a lot of noise. I wonder if attempting injection and ignoring failures might be enough? If nothing has injected the bean then the test would probably fail anyway.

I'm also not really convinced that we should add support for MVC methods. I quite like the current distinction between elements that are injected from the HTTP request, and components that are injected from the framework.

Perhaps there are really compelling use-cases that I've not thought about, but currently it feels like we might be making this change because we can, rather than because we should.

[spring-projects-issues]

Sam Brannen commented

Phil Webb, thanks for chiming in!

I wonder if attempting injection and ignoring failures might be enough? If nothing has injected the bean then the test would probably fail anyway.

That's unfortunately not possible. Just like in Spring MVC, JUnit 5's support for method injection queries a set of registered providers to find one (and only one) that supports the parameter. Thus, you cannot have multiple providers claiming to support the same parameter; in fact, such a scenario results in an exception in JUnit 5.

In general, one should not focus completely on Spring beans as the sole type of dependency being resolved.

Consider that some objects come from JUnit, some from Spring, and some from other third parties (e.g., mocks from Mockito). Each provider must be able to confidently decide that it and only it can support a given parameter. So although Spring could certainly preemptively attempt to resolve a dependency when it's being asked if the parameter is supported by Spring, that would lead to potential conflicts with other providers registered for the same test.

I'm also not really convinced that we should add support for MVC methods. I quite like the current distinction between elements that are injected from the HTTP request, and components that are injected from the framework.

That's actually the focus of #18628. So feel free to chime in on that topic there. ;)

Anyway, for what it's worth, it is already possible to inject components via @Value (and a SpEL expression) into MVC methods.

Perhaps there are really compelling use-cases that I've not thought about, but currently it feels like we might be making this change because we can, rather than because we should.

I personally don't see any harm in allowing @Autowired to be declared on parameters. But if the team decides to revert this support, that still does not prevent Spring's JUnit 5 integration from supporting it via a custom annotation like @SpringBean (which I just deleted from the spring-test-junit5 project today); it just has the downside that it introduces a special autowired annotation that is only applicable in tests.

[spring-projects-issues]

Phil Webb commented

FWIW I think adding a new @SpringBean annotation would be way worse than the @Autowire change.

How likely do think it is that people will want to mix @Autowire parameter injection with injection of other JUnit types? If the answer is "not very" then perhaps @Autowire on the method would be enough to indicate that Spring is handling injection of all parameters. For me this would be a little more logical since it works in the same way as the existing constructor and setter based system.

So a test like this would be fine:

@Autowired
void example(Person person, Dog dog) {
// ... 
}

A test like this would fail:

@Autowired
void example(Person person, Dog dog, TestInfo testInfo) {
// ... 
}

but could always be rewritten:

@Autowired
private Person person;

@Autowired
private Dog dog;

void example(TestInfo testInfo) {
// ... 
}

[spring-projects-issues]

Phil Webb commented

I don't think the fact that it's possible to use @Value with SpEL to inject beans is necessarily a good argument to add @Autowire on MVC controllers. I've not seen many people do that in the wild.

[spring-projects-issues]

Juergen Hoeller commented

I'm not concerned about formally allowing @Autowired declarations on parameters. The question of where it is actually supported applies quite generally: Even for constructors, fields and methods, we only support in on actual Spring-managed beans; that distinction might not be so obvious to everyone, e.g. compared to entity objects or form objects. And as Sam pointed out, @Value already has broader use than just core injection points... and also many places where it can be declared but doesn't work at all.

In the end, I don't see the confusion part with this lenient declaration step: Hardly anybody will notice that the @Autowired annotation can even be declared on parameters to begin with. And if it enables third parties to reuse our autowired marker for triggering our dependency resolution algorithm in a custom method parameter scenario, that doesn't seem so bad to me. I prefer this over those parties having to invent their own annotations for the same purpose. Whether we have to support it for MVC handler methods (#18628) is a quite different matter, and I'm not convinced about that one yet; the main driver there for me is parity with @Value and also JUnit 5.

The only core framework case that I'm considering for 4.3 still is actually evaluating @Autowired when present at the constructor parameter level, along the lines of how @Value can be used there already. With making the constructor-level @Autowired marker optional in 4.3 (for single constructor scenarios), the readability of @Autowired at the parameter level is actually quite nice, in particular next to @Value usage in a multi-parameter scenario. And there's a benefit that I'm keen on: parameter-specific required=false markers, which otherwise can only be expressed using Java 8's java.util.Optional... with no Java 6/7 equivalent yet.

[spring-projects-issues]

Juergen Hoeller commented

Actually, looking at it, I certainly prefer Optional for marking individual parameters as non-required... if just it wasn't Java 8 specific. Maybe that's not a big deal. And the semantics of 'required' for a multi-constructor scenario are somewhat special, so maybe better left as-is.

In any case, the readability of @Autowired at the parameter level is quite nice... simply for self-documenting specific autowired parameters next to @Value parameters. @Autowired is therefore conceptually not at odds with being declared at the parameter level, is it?

[spring-projects-issues]

Stéphane Nicoll commented

For the record, I am +1 with everything Phil said. It looks like a very JUnit driven change (i.e. a change that we wouldn't have done otherwise). It feels to me that such a core container change should have other drivers and I honestly don't want to see users injecting collaborators in MVC methods.

Why can't we just specify the dependency or Optional<X> for @Autowired(required=false) and be done with it?

[spring-projects-issues]

Juergen Hoeller commented

To be clear, this particular change here in #18629 is literally just about redeclaring the @Autowired annotation to allow for being applied to parameters; no actual support anywhere in the core framework.

#18628 is meant to track autowired parameter support for our common handler methods. I had a strong suspicion that this might not reach consensus; it just waits in the 5.0 bucket for some further discussion.

The key question for 4.3 is: Do we have strong opinions against even allowing such declarations in custom code? It might force some parties (not just JUnit) to invent their own annotation for the same purpose...

Concepturally, my main concern is just: @Autowired needs to trigger AutowireCapableBeanFactory.resolveDependency for actual resolution at runtime, preserving the same resolution rules for all such injection points. As long as this is being honored, individual parameter resolution is conceptually within our core model intentions.

[spring-projects-issues]

Sam Brannen commented

FWIW I think adding a new @SpringBean annotation would be way worse than the @Autowire change.

Good! I'm glad we agree on that. ;)

How likely do think it is that people will want to mix @Autowire parameter injection with injection of other JUnit types? If the answer is "not very" then perhaps @Autowire on the method would be enough to indicate that Spring is handling injection of all parameters. For me this would be a little more logical since it works in the same way as the existing constructor and setter based system.

I think it's very likely that people will write tests in JUnit 5 that have parameters injected from various sources: their own custom resolvers as well as those from JUnit, Spring, and other third parties.

So a test like this would be fine:

@Autowired
void example(Person person, Dog dog) {
// ... 
}

A test like this would fail:

@Autowired
void example(Person person, Dog dog, TestInfo testInfo) {
// ... 
}

Both of these are decidedly a failure: annotating a method in a test class with @Autowired will instruct Spring to perform DI on the method when the test instance is prepared. That means that such test methods will be invoked twice: once by Spring for autowiring of the test instance and again by JUnit when actually executing the test. Furthermore, when Spring invokes the method for the purpose of DI, that invocation may throw exceptions (due to failed assertions, etc.).

So this approach is unfortunately broken from the start.

but could always be rewritten:

@Autowired
private Person person;

@Autowired
private Dog dog;

void example(TestInfo testInfo) {
// ... 
}

One could rewrite the test like that, but IMHO that somewhat defeats the purpose of method injection in JUnit 5. It's meant to be flexible, to allow for new styles of use cases (including use cases we can't yet foresee). So why would Spring want to limit developers in certain scenarios?

[spring-projects-issues]

Sam Brannen commented

I don't think the fact that it's possible to use @Value with SpEL to inject beans is necessarily a good argument to add @Autowire on MVC controllers. I've not seen many people do that in the wild.

I didn't mean to imply that it was a good idea, rather only that it is already possible. ;)

[spring-projects-issues]

Sam Brannen commented

Perhaps there are really compelling use-cases that I've not thought about, but currently it feels like we might be making this change because we can, rather than because we should.

One thing Juergen has mentioned elsewhere is that injecting a request-scoped bean into an MVC handler method could completely avoid the use of a scoped proxy.

So that's at least one new use case. But again... those discussions are better suited for #18628.

As Juergen pointed out, this issue is really only about whether or not Spring should allow @Autowired to be declared directly on a parameter.

Keep in mind that JUnit 5 supports method parameter injection, and spring-test will eventually include the support already available in spring-test-junit5. Once this functionality gets incorporated into spring-test proper, I only foresee three options:

1. @Autowired can be declared on parameters.
2. spring-test bundles a custom @Autowired composed annotation (like @SpringBean).
3. Require every development team in the world to invent its own equivalent of @SpringBean.

But I would personally never condone the last option.

[spring-projects-issues]

Sam Brannen commented

The key question for 4.3 is: Do we have strong opinions against even allowing such declarations in custom code? It might force some parties (not just JUnit) to invent their own annotation for the same purpose...

Just to be perfectly clear on this...

JUnit has zero dependencies on Spring and will therefore not be inventing any such annotation for Spring.

On the contrary, it is Spring itself (in the spring-test module) that will be inventing such an annotation (like @SpringBean).

So the question is not only whether Spring wants to force third parties to invent such annotations but also whether Spring wants to invent such annotations itself.

[spring-projects-issues]

Oliver Drotbohm commented

I don't think it's about "forcing" someone to do something as I think there are conceptual problems to the approach. And it feels like we're already discussing implementation aspects before it's even clear what problem we solve. So far the only problem I've seen solved is "JUnit has new API, we need to do something with it".

As probably already obvious from Twitter I don't think that making @Autowired applicable to parameters is a good ideas as it's one more way to use @Autowired with very limited applicability. Let's take a step back and have a look at how the annotation can be used right now:

1. On constructors (all parameters are subject to injection)
2. On fields (field subject to injection)
3. On methods (all parameters subject to injection)
4. There's even an implicit @Autowired on @Bean methods

The first three sort of work everwhere Spring controls the injection, 4 is already a special thing but can be argued as being part of the JavaConfig support overall.

When it comes to the teams recommendation we have settle for 1 as the primary injection mechanism to users. Support for that has evolved over various generations of Spring: 4.0 introduced Objenesis based proxy creation so that you don't have to fall back to field injection for proxied types without interfaces. In 4.3 support for constructor injection is coming in configuration classes. So the only part of the framework that forces you to use a particular injection style different from out usual recommendation is the test parts. I'd really love to see the time and resources we spend on JUnit to actually make sure, it will allow the test context framework to process injection analogous to all other parts of the framework. Currently the only reason you have to use field injection in test classes is: because JUnit requires a default constructor. We should fix that, not create more inconsistencies.

Support for injection into methods really comes with a lot of downsides and the only benefit being "it integrates with a feature that JUnit 5" introduced, which basically resembles a "because we can" or "because we feel we have to" sentiment. What are the downsides I see?

1. We create yet another way to inject things. — To be honest, I've never been approached by anyone missing injection of parameters. If we introduce support for using @Autowired at parameters and the test context framework makes use of that, how are we going to argue it's not going to be supported in MVC (or will it be?), or any other Spring component. So that particular way of using @Autowired would only be supported in one part of the framework but not in others. A fact, that we're currently actively working on to remove (see support for constructor injection support in configuration classes). Why would we want to create yet another exception? Which leads me to…
2. Semantics can't be defined in a self contained manner. — All other injection mechanisms above share one common thing. The define an injection point and define semantics for it completely. With @Autowired supported on parameter, you'll *always' have to answer the question: what about the other parameters? The problem here is, we can't even define those semantics ourselves as they might be defined by third party frameworks. That's unprecedented actually as even in the current test context framework injection is controlled by Spring and the injection semantics can basically be described as "works as in the core container".
3. It shares the same incentives to write bad code as field injection. — Being able to inject components into test methods over injecting them into fields only makes sense if the test methods use very different dependencies. This in turn raises the question about the cohesion of the test class. It makes it even easier to create test-god classes that just get test methods added to them at will even without that huge list of @Autowired fields (as it would currently reflect) causing raised eyebrows.

One thing I really like about the way Spring MVC currently works (and what's actually a very distinguishing factor from the CDI world) is a very simple rule when it comes to the question: what get's into the fields and what gets into method parameters. Fields is other application components (Spring DI), method parameters a framework (MVC) specific components that usually change from request to request. I really think there's nothing wrong with applying the very same model to the test context framework: inject application components into the test class, have everything framework (JUnit) specific (TestContext etc.) at the test method level, methods that get injected by the particular framework, just as with MVC (and to some degree even the Java configuration classes). I really think we should invest into making sure JUnit lets our users write code that is consistent with the general recommendations, not change Spring Framework because of new hooks in JUnit. No offense but there's over a decade of experience in designing application components and recommendations arrived from that in Spring Framework, so it feels a bit like the tail is waving with the dog. If parameter injection of components would really solve a real problem, why hasn't any user been asking for that in the context of MVC. I think the answer is pretty simple: because our current responsibility model (components into final fields, framework specifics via method invocations) is sound.

I'd argue that's a pretty simple and consistent and we should strive for those values, not adding more stuff because we can or feel we have to because of new API in JUnit.

[spring-projects-issues]

Phil Webb commented

Regardless of the final outcome I'd like to suggest that if parameter injection happens with JUnit 5 we should allow @Autowired to be used on test methods to indicate all parameters are handled by Spring. This will at least allow a programming model consistent with other areas of the framework:

@Autowired
public MyConfiguration(Some bean, SomeOther bean) {
  // ...
}

@Autowired
public MyComponent(Some bean, SomeOther bean) {
  // ...
}

@Autowired
public void setSomeBean(Some bean {
  // ...
}



@Autowired
public void testSomething(Some bean) {
   // ... 
}