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

Replacing field injection with constructor injection #557

Open
crankydillo opened this issue Jul 30, 2024 · 13 comments
Open

Replacing field injection with constructor injection #557

crankydillo opened this issue Jul 30, 2024 · 13 comments
Labels
recipe Recipe requested

Comments

@crankydillo
Copy link

What problem are you trying to solve?

We want to convert a bunch of classes that used Spring field injection to constructor injection. We are willing to live with some caveats (e.g. only works if no constructors, select annotations, etc).

Describe the situation before applying the recipe

class A {
   @Autowired
   @Qualifier("B")
    private B b;
}

Describe the situation after applying the recipe

class A {
    private B;

    public A(@Qualifier("B") b) {
        this.b = b;
    }
}

Any additional context

We could likely get value from something that didn't hit all the corner cases. However, if there are a bunch of corner cases that ultimately means an unending feature request on this, it's not like we really need this.

Are you interested in contributing this recipe to OpenRewrite?

Yes.

@timtebeek
Copy link
Contributor

Hi @crankydillo ; Thanks for the offer to help! Agree that this would be a nice addition, and best to start with a limited set of known cases, and reject anything else (at first) that might make it more complicated.

On the known cases to reject I'd include

  • has existing explicit constructor
    • could lead to ambiguity
  • has implicit Lombok provided constructor
  • has fields annotated with more than just @Autowired, like validation or jackson annotations
    • as not all annotations are equally valid on constructor arguments as they are on fields
  • class is extends or is extended by another class
    • could lead to failure calling super()
  • class has more than say ~10 autowired fields
    • arbitrary cut off, but very large constructors won't work as well

What we're left with then is

  • introduce constructor with arguments in same order as fields
  • remove autowired annotations from fields
  • move any qualifier annotations to arguments

Anything you'd add to this list?

@timtebeek timtebeek moved this to Recipes Wanted in OpenRewrite Jul 30, 2024
@timtebeek timtebeek added the recipe Recipe requested label Jul 30, 2024
@crankydillo
Copy link
Author

crankydillo commented Jul 30, 2024

RE field->constructor annotations, I was thinking we could move @Autowired and @Autowired + @Qualifier. I know for us that would handle quite a few cases.

I might still do large constructors or maybe make that part configurable. The idea is to create an obvious code smell, which is currently hidden by 'Spring magic'.

@punkratz312
Copy link

When is this coming? Quick fixes applied by this recipe would be very helpful in reducing the boilerplate.

@timtebeek
Copy link
Contributor

hi @punkratz312 ; we don't have a fixed schedule, but you're welcome to push up a draft PR with tests if you'd like to help out. :)

@timtebeek
Copy link
Contributor

Adding a note here that this is also necessary when you want to move away from @org.springframework.beans.factory.annotation.Required, which was removed in Spring Framework 6+.

@martinlippert
Copy link

We have this as a quick fix in the Spring Tools for VSCod and Eclipse: https://x.com/springtools4/status/1843922035824787896

It uses org.openrewrite.java.spring.AutowiredFieldIntoConstructorParameterVisitor from rewrite-spring , so improvements to this (e.g. handling the @Qualifier annotation nicely) should be made there, I think.

@martinlippert
Copy link

@crankydillo Interested in contributing some improvements to the org.openrewrite.java.spring.AutowiredFieldIntoConstructorParameterVisitor, for example to cover @Qualifier annotations in a nice way?

@crankydillo
Copy link
Author

@martinlippert If I can squeeze out some time. Honestly, I didn't know that class you mentioned existed. Wouldn't you say that meets at least the title of this issue? I can change the title to specify we are asking for additional things like @Qualifier. WDYT?

@martinlippert
Copy link

Yeah, probably changing the title to something around taking @Qualifier annotations into account makes sense to me.

We could also take a look, but if you plan to contribute this, we would not go down this path to avoid duplicate work (and vice versa). Just let us know whether you plan to work on this or not - both is totally fine.

@martinlippert
Copy link

Also adding @BoykoAlex for awareness.

@martinlippert
Copy link

The reconciler is very specific for the Spring Tools language servers reconciling mechanism and based on ASTs from JDT, so I don't see any value in this for the rewrite-spring repo - as far as I understand.

A recipe itself might make sense for rewrite-spring, but I let @BoykoAlex fill in the details here.

@BoykoAlex
Copy link
Contributor

The first link would be useful for the recipe implementation. Essentially it is a recipe taking two parameters: owner class fully qualified name and the name of the field in the class to convert into the constructor parameter.
A recipe converting all autowired fields into constructor parameters would have to be written differently. Scan for autowired fields and then call doAfterVisit(new AutowiredFieldIntoConstructorParameterVisitor(classFqName, fieldName)); for each found field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Status: Recipes Wanted
Development

No branches or pull requests

5 participants