Quick fix action to refactor field injection to constructor injection

[MahatmaFatalError]

In general, field injection comes with some downsides http://olivergierke.de/2013/11/why-field-injection-is-evil/

Unfortunately, refactoring of old, grown code bases towards constructor injection is tedious. So I was looking for a refactoring automation (and apparently I was not the first one with this issue https://stackoverflow.com/questions/44008985/convert-spring-field-injection-to-constructor-injection-intellij-idea)

So in essence, a quick fix action on @Inject or @Autowired fields to create a new or extend an existing constructor (depending on (required = false) flag) would be really nice.

[BoykoAlex]

This will take about 3-5 weeks to implement.
First of we'd need an extension mechanism in JDT LS to contribute to quick fix processors (similarly the way eclipse JDT allowed doing this). We'd need to create a good PR with tests for JDT LS which would take time.
Secondly, contribute a quick fix processor and possibly reconciling if we'd want to mark autowired field with a warning or info to make this finally work.

[martinlippert]

I was hoping that this could be implemented inside of the Spring Boot language server, as the LSP offers something like quick fixes and things like that. Any thoughts around that approach?

[BoykoAlex]

Hmm... I forgot that we already parse java sources and keep them in the cache to provide live hovers... I'll have another look at this.

[BoykoAlex]

@martinlippert do we want to put a "problem" marker in the code (i.e. hint)? Code Action/Quick Fix would be available via the marker in this case...
Alternative is create a Code Action/Quick Fix when user has a cursor on the annotation AST node or field declaration even. Then, VSCode would show a light-bulb i think at the start of the line clicking on which should show the menu where one of the menu items is going to be "Convert to constructor parameter". Eclipse UI wouldn't be as helpful... user would need to drill down into context menu to find the refactoring...
I'm inclined to go with the marker approach... It looks good in Eclipse:

Not so great in VSCode, but i suspect there is some sort of a bug in the client: it does not show the range correctly - only underlines the first symbol:

[martinlippert]

@BoykoAlex In general, both approaches seem to be fine with me. I would aim at some consistency with other quick fixes and refactorings when coding in Java in VSCode and Eclipse. What is the choice for most of the quick Java refactorings in VSCode? If that goes with the lightbulb, then we should follow that example.

If this doesn't look great in Eclipse and doesn't align with the overall Java coding experience in Eclipse, we can spend some additional work on the LSP4E side to improve that experience. We integrated the code completions in a special way to make them work well in the Java editor as well, so that should be possible. But again, I would like to take a look at the existing refactorings in the same area and see how they work for Java in VSCode - and take that as a guiding example.

[martinlippert]

This is available in the latest versions of the Spring Tools across Eclipse and VSCode extensions.

[MahatmaFatalError]

@martinlippert which PRs are related to this issue? I wanted to try out the new Eclipse but could not find the Quick Fix. So I wanted to look at the code...

[martinlippert]

@MahatmaFatalError This belongs to a broader effort that we are working on at the moment to integrate refactorings and code changes using https:/openrewrite, so the code change part of this quick fix is actually implemented in https:/openrewrite/rewrite-spring.

Since this is an early stage and not yet meant for mainstream consumption, you need to enable this in the preferences via Preferences -> Language Servers -> Spring Language Servers -> Spring Boot Language Server and then check Experimental reconciling for Java sources based on Rewrite project. Once you have that setting enabled, the quick fix will appear on code like this:

	@Autowired
	private MyService service;

When you put the cursor on @Autowired or service and hit the shortcut for Quick Fix, you will see a quick fix appearing called Convert @Autowired field into Constructor Parameter.

But keep in mind, this is a very early, experimental feature and not yet optimized or meant for mainstream consumption. It is early work towards enabling a broader set of new Spring-related refactorings, validations, and automated code changes.

Hope this helps. Feedback always welcome!!!

[MahatmaFatalError]

Thanks for providing this background information.
I just tried it out, and it seems I found 2 bugs.

1. The @Autowired annotation is gone after the quick fix (see unused import statement).
2. cmd+z to undo, keep the editor dirty (don't save) and try quick fix again --> something got changed in the background that leads to duplicated constructor

Version: 4.16.0.RELEASE
Build Id: 202209151144

[BoykoAlex]

@MahatmaFatalError Rogue Autowired import (issue 1) i think is fixed just not pushed in yet (upgrade to the latest rewrite). For the second issue i think I'd need some instructions to reproduce... probably has to do with something changing the file outside eclipse...
Do you mind raising these as separate git issues? At least the second issue with dup constructors?

[martinlippert]

One comment around the @Autowired annotation that is gone:

• it is correct that the annotation is not added to the constructor - not having the @Autowired annotation on those constructors is considered a best practice and the tooling can flag that as a warning and provide a quick fix to remove @Autowired from constructors, if found.
• the unused import should maybe be removed as part of the quick fix (in case it is really unused in the file after applying the quick fix)

[MahatmaFatalError]

For the second issue i think I'd need some instructions to reproduce

@BoykoAlex I can't reproduce it either with a real-world class. So maybe it was just a anomaly in the matrix ;)

it is correct that the annotation is not added to the constructor

@martinlippert really? OK official docs https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#using.spring-beans-and-dependency-injection say it is needed when there are multiple constructors. I personally would prefer always having the annotation, but if the best practice is different, OK for me.

Another thing I noticed is that @Autowired(required = false) does not have any effect. I think it would make sense to separate mandatory beans in a required-args-constructor (only required = true) from an all-args-constructor (all beans).

[BoykoAlex]

@MahatmaFatalError A parameter is to be added to a constructor if the constructor meets the following criteria:

1. No constructors. Constructor is created and is not annotated.
2. Only one constructor. Parameter is added.
3. Multiple constructors, only one annotated with @Autowired. This constructor is added a parameter.

Therefore, this refactoring doesn't add @Autowired. @martinlippert was referring to the use case in your screenshots. First screenshot - no constructor case. The new constructor doesn't get the annotation and it is a good practice.