Closed spring-projects-issues closed 7 years ago
spring-projects-issues
commented
8 years ago Sam Brannen commented
Thanks, Juergen! :)
@SpringBean in spring-test-junit5 has now been completely replaced by @Autowired. Very cool indeed!
https://github.com/sbrannen/spring-test-junit5/commit/886c64aeb1c1084cf5a3e33805a903db988956cf
spring-projects-issues
commented
8 years ago 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
commented
8 years ago 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
commented
8 years ago 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) {
// ...
}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
commented
8 years ago 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
commented
8 years ago 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
commented
8 years ago 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
commented
8 years ago 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.
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
commented
8 years ago Sam Brannen commented
FWIW I think adding a new
@SpringBeanannotation would be way worse than the@Autowirechange.
Good! I'm glad we agree on that. ;)
How likely do think it is that people will want to mix
@Autowireparameter injection with injection of other JUnit types? If the answer is "not very" then perhaps@Autowireon 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
commented
8 years ago Sam Brannen commented
I don't think the fact that it's possible to use
@Valuewith SpEL to inject beans is necessarily a good argument to add@Autowireon 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
commented
8 years ago 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:
@Autowired can be declared on parameters.spring-test bundles a custom @Autowired composed annotation (like @SpringBean).@SpringBean.But I would personally never condone the last option.
spring-projects-issues
commented
8 years ago 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
commented
8 years ago 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:
@Autowired on @Bean methodsThe 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?
@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…@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".@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
commented
8 years ago 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) {
// ...
}
Juergen Hoeller commented
Phil Webb, the problem with @Autowired at the method level is pretty tough: Instances of test classes are being passed through the core container, and the container assumes that @Autowired methods are initialization methods to be called during the init phase of the object. As a consequence, for a @Test method annotated with @Autowired at the method level, the container would invoke them during the initializeBean callback! And we can't easily get rid of that, since the container has no understanding of @Test annotations or any other annotations; the plain presence of @Autowired at that level (in any context) triggers the init-time invocation.
The fundamental problem is conceptual though: Those methods are not @Autowired methods within an init-time invocation model; they are rather test methods with individually autowired parameters under a test invocation model. This is the difference that needs to be understood here: @Autowired at the constructor and method level implies invocation semantics in the init phase; @Autowired at the parameter level just implies resolution semantics within some other method invocation model.
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 AutowireCapableBeanFactory.resolveDependency) has always been triggered like that: for each individual parameter. It's just that ConstructorResolver and AutowiredMethodElement use an @Autowired marker at the constructor/method level to trigger that common resolution for each individual parameter.
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 Provider<...> declarations for late resolution purposes. Declaring such dependencies at the test method level is arguably more elegant though, and also more cohesive: Instead of pointing to fields declared way up the same file, a test method would refer to its own parameter types instead. A similar case can be made for MVC handler methods and request-scoped beans, but as I said above, I am not convinced we really need this (and it is not the scope of this JIRA issue).
spring-projects-issues
commented
8 years ago Phil Webb commented
Sorry Juergen Hoeller, I totally missed that Sam Brannen had made exactly the same point about @Autowired on methods above.
spring-projects-issues
commented
8 years ago 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 MethodParameterResolver API is applicable not only to @Test methods but also to callback methods such as @BeforeEach, @AfterEach, @BeforeAll, and @AfterAll methods (analogous to @Before, @After, @BeforeClass, and @AfterClass in JUnit 4).
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 MultipleWebRequestsSpringExtensionTests that I just pushed to spring-test-junit5. Here we eliminate the need to store the WebApplicationContext in an instance field: with DI support for @BeforeEach methods it simply becomes unnecessary to track the instance of the WebApplicationContext. The test methods in the class need the MockMvc instance, not the WebApplicationContext.
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 @Autowired on the WebApplicationContext parameter, but please don't hold that against me. If the parameter is assignable to ApplicationContext, I allow @Autowired to be omitted. ;)
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:
18628 Introduce autowiring support for individual handler method parameters ("is depended on by")
18151 Introduce support for JUnit 5 in the TestContext framework ("is depended on by")
19833 Honor
@Autowired(required=false) at parameter level, as an alternative to java.util.Optional ("is depended on by")18632 Compose
@Autowiredwith@Qualifier18630 Allow
@Qualifierto be used in composed annotations with attribute overridesReferenced from: commits https://github.com/spring-projects/spring-framework/commit/a905412514bfeab250507d81b6926363b0c10973
0 votes, 6 watchers