Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kotlin examples for setter injection incorrectly use field injection #28596

Closed
sbrannen opened this issue Jun 9, 2022 · 5 comments
Closed

Kotlin examples for setter injection incorrectly use field injection #28596

sbrannen opened this issue Jun 9, 2022 · 5 comments
Assignees
Labels
theme: kotlin An issue related to Kotlin support type: documentation A documentation task
Milestone

Comments

@sbrannen
Copy link
Member

sbrannen commented Jun 9, 2022

While analyzing #28590, I noticed that many of the examples involving setter injection and Kotlin incorrectly use field injection.

Searching for lateinit var movieFinder: MovieFinder will reveal several of those -- for example, those using @Resource, @Autowired, @Inject.

There may well be other cases in the reference manual.


Regarding the "fix", #28590 proposed the @Required set shorthand syntax for annotating a setter method in Kotlin. I'm not a Kotlin expert and therefore assume that works, but I'm wondering if it wouldn't be better to demonstrate annotated setter methods with a complete setXyz(...) method.

@sbrannen sbrannen added type: documentation A documentation task theme: kotlin An issue related to Kotlin support labels Jun 9, 2022
@sbrannen
Copy link
Member Author

sbrannen commented Jun 9, 2022

I'm wondering if it wouldn't be better to demonstrate annotated setter methods with a complete setXyz(...) method.

@sdeleuze, what are your thoughts on this?

@sbrannen
Copy link
Member Author

sbrannen commented Jun 9, 2022

@SchroedingersGitHub, since you raised #28590, would you be interested in submitting a PR to address additional issues with annotated setter methods in Kotlin?

@sbrannen sbrannen added this to the 5.3.x milestone Jun 9, 2022
@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Jun 9, 2022
@sdeleuze
Copy link
Contributor

sdeleuze commented Jun 15, 2022

In Kotlin those are properties not fields. For most use cases, I think what is documented is fine (it is the idiomatic way to do injection in Kotlin with Spring) unless I am mistaken. When you need to specify that the annotation should be applied on a getter or setter, the recommended way to do this is to use annotation use site targets.

So I think #28590 should be refined to use @set:Required lateinit var movieFinder: MovieFinder (I will create a related issue), and this issue can probably be closed without further commit unless we find specific cases not working as expected (usually validation related).

cc @poutsma

@sbrannen
Copy link
Member Author

When you need to specify that the annotation should be applied on a getter or setter, the recommended way to do this is to use annotation use site targets.

Thanks for enlightening me. That syntax looks much clearer.

So I think #28590 should be refined to use @set:Required lateinit var movieFinder: MovieFinder (I will create a related issue),

Sounds good. Thanks.

In Kotlin those are properties not fields. For most use cases, I think what is documented is fine (it is the idiomatic way to do injection in Kotlin with Spring) unless I am mistaken.

I can well imagine that what's demonstrated in the examples is the idiomatic way to do things in Kotlin. That would make sense to keep it simple.

But what's documented is a different story, and that is the main reason I created this issue.

For example, we show the reader the following for both setter and field injection, which is incorrect. I hope it is not in fact performing both setter and field injection. Rather, I hope it's resulting in only one form of DI.

@Autowired
lateinit var movieFinder: MovieFinder

My point is that we should either change the examples or change the documentation.

If you think nobody actually uses true setter injection in Kotlin (which I imagine is the case), then let's stick to the idiomatic way people use this but improve the documentation to point out that it's technically field injection (or "property injection") instead of setter injection.

@sdeleuze
Copy link
Contributor

Maybe a pragmatic outcome could be : in the places where we document specifically and explicitly setter injection, we can maybe specify @set:Autowired or similar for correctness. But what I want to avoid is to see those @set:Autowired and similar everywhere in the doc. For other places where setter inject is not explicitly documented, I think we should keep @Autowired to provide the idiomatic version.

Side note : the most idiomatic version in Kotlin is Constructor injection of val properties.

@sdeleuze sdeleuze modified the milestones: 5.3.x, 5.3.22 Jun 15, 2022
@sdeleuze sdeleuze removed the status: waiting-for-feedback We need additional information before we can continue label Jun 15, 2022
@sdeleuze sdeleuze self-assigned this Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: kotlin An issue related to Kotlin support type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

2 participants