Allow @Autowired to be declared on parameters [SPR-14057] #18629
Comments
|
Sam Brannen commented Thanks, Juergen! :)
|
|
Phil Webb commented Oliver Drotbohm pointed out on in a discussion that this could add quite a bit of confusion to 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 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. |
|
Sam Brannen commented Phil Webb, thanks for chiming in!
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.
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
I personally don't see any harm in allowing |
|
Phil Webb commented FWIW I think adding a new How likely do think it is that people will want to mix 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) {
// ...
} |
|
Phil Webb commented I don't think the fact that it's possible to use |
|
Juergen Hoeller commented I'm not concerned about formally allowing In the end, I don't see the confusion part with this lenient declaration step: Hardly anybody will notice that the The only core framework case that I'm considering for 4.3 still is actually evaluating |
|
Juergen Hoeller commented Actually, looking at it, I certainly prefer In any case, the readability of |
|
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 |
|
Juergen Hoeller commented To be clear, this particular change here in #18629 is literally just about redeclaring the #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: |
|
Sam Brannen commented
Good! I'm glad we agree on that. ;)
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.
Both of these are decidedly a failure: annotating a method in a test class with So this approach is unfortunately broken from the start.
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? |
|
Sam Brannen commented
I didn't mean to imply that it was a good idea, rather only that it is already possible. ;) |
|
Sam Brannen commented
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 Keep in mind that JUnit 5 supports method parameter injection, and
But I would personally never condone the last option. |
|
Sam Brannen commented
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 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. |
|
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
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?
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. |
|
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
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) {
// ...
} |
|
Juergen Hoeller commented Phil Webb, the problem with The fundamental problem is conceptual though: Those methods are not Related to this, Oliver Drotbohm, the actual injection point for a constructor or method is each individual parameter, not the constructor/method itself. Our resolution algorithm (the above-mentioned As for the use case in test classes specifically, an advantage of dependency resolution per test method is that one can specifically retrieve what's actually needed for that particular test method. Injecting all potential entry points at the field/constructor level enforces early initialization for every test method on that class, even if not needed by all the tests, and even if those dependencies are declared as prototype beans... which is pretty common in test setups. Declaring such dependencies at the level of each test method allows for late resolution (i.e. at the actual time of invoking each method), and in particular for the resolution of the actually needed dependencies only. Of course, one could use scoped proxies or |
|
Phil Webb commented Sorry Juergen Hoeller, I totally missed that Sam Brannen had made exactly the same point about |
|
Sam Brannen commented Hi everybody, Since people seem to be following this issue rather than #18628, I will share some additional food for thought here. Until now people have been focusing too heavily on the notion of parameter injection into test methods. However, there are in fact other compelling (perhaps more compelling) use cases for having dependencies from Spring injected into other types of methods. For example, JUnit 5's And this is quite an important differentiating factor... dare I say a game changer! I concede that injecting Spring components into test methods will not always be a best practice; however, injecting components, application contexts, configuration parameters, etc. into other callback methods might in fact become the best practice. Consider the following class MultipleWebRequestsSpringExtensionTests {
MockMvc mockMvc;
@BeforeEach
void setUpMockMvc(WebApplicationContext wac) {
this.mockMvc = webAppContextSetup(wac)
.alwaysExpect(status().isOk())
.alwaysExpect(content().contentTypeCompatibleWith(APPLICATION_JSON))//
.build();
}
@Test
void getPerson42() throws Exception {
this.mockMvc.perform(get("/person/42"))
.andExpect(jsonPath("$.name", is("Dilbert")));
}
@Test
void getPerson99() throws Exception {
this.mockMvc.perform(get("/person/99"))
.andExpect(jsonPath("$.name", is("Wally")));
}
}I don't know what everybody else thinks, but I personally find such use cases very compelling! And I am eager to be able to benefit from parameter injection on a daily basis (even if it's limited to testing scenarios). p.s. Yes, I realize that the above example does not even use |
spring-projects-issues
added
type: enhancement
Sam Brannen opened SPR-14057 and commented
Status Quo
@Autowiredcurrently cannot be declared on a parameter.Impetus
This feature is needed by #18151 and #18628.
Deliverables
@Autowiredto be declared on parameters in order to support dependency injection for individual method or constructor parameters.Issue Links:
@Autowired(required=false) at parameter level, as an alternative to java.util.Optional ("is depended on by")@Autowiredwith@Qualifier@Qualifierto be used in composed annotations with attribute overridesReferenced from: commits a905412
0 votes, 6 watchers
The text was updated successfully, but these errors were encountered: