Require Construction injection for member variables in a Spring Component

[Sud Ramasamy]

Hi,

I'd like to have a rule that says all Spring components should have their member variables injected via the constructor.

There appears to be logic in the following two rules that could be used as references to create a new rule to enforce this.

SpringComponentWithNonAutowiredMembersCheck (S3749 - https://github.com/aepfli/sonar-java/blob/master/java-checks/src/main/java/org/sonar/java/checks/spring/SpringComponentWithNonAutowiredMembersCheck.java)

 (non-Spring) ConstructorInjectionCheck (S3306 - https://github.com/aepfli/sonar-java/blob/master/java-checks/src/main/java/org/sonar/java/checks/ConstructorInjectionCheck.java)

If this rule is acceptable to be added to the sonar-java plugin what are the next steps? I’d be glad to submit a PR to iterate on the implementation.

-sud

[G. Ann Campbell]

Hi sud,

I think this rule covers it, doesn't it?

Ann

[Sud Ramasamy]

The one you linked to is the S3749 rule that I referenced. It only checks to see if the member variables themselves have the injection annotation on them (ie. field injection). But we use constructor injection where the injection annotation is on the constructor. 

The other rule I referenced is S3306 which enforces that constructor injection should be used instead of field injection. So looks like what I need is a slight tweak on this to say if there are non-static class members they should be constructor injected. What's the best way to accomplish. And we might need two implementations of this...one for the non-spring injection annotations (ie @Inject) and the other for the Spring injection annotations (@Autowired).

Please let me know if there are additional questions. And thanks for your consideration.

-sud

[Michael Gumowski]

Hey Sud,

We specified the following rule, based on the fact that we were indeed missing the case of injection done using constructor: RSPEC-4288

I think it match what you would like to have. Can you maybe second-checking it?

Cheers,

Michael

--
You received this message because you are subscribed to the Google Groups "SonarQube" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sonarqube+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/sonarqube/a19315f4-3ff1-4038-969c-06e92af5340e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Michael Gumowski | SonarSource

Software Developer, Language Team
https://www.sonarsource.com

[Sud Ramasamy]

This would be the Spring counterpart to S3306 which certainly would be useful. It's checking if there are autowired member variables then they should instead be constructor injected.

We may need one more rule to cover the scenario I described. The rspec you've written only kicks in if the member variable is autowired. I need a "constructor injected member variable check" to kick in even when the field is not autowired. And like I mentioned this should likely honor both Spring and javax.inject annotations.

Thanks.

-sud

[Michael Gumowski]

Hey Sud,

I slightly modified the description of the rule to kick in also when the constructor is not annotated.

Regarding support of javax.inject annotations, I have some doubts. Identifying spring component is straight-forward (check for class annotations). 

Regarding "simple" usage of @Inject, it's a bit trickier. In such case, I guess that we could only trigger the rule if there is at least one field annotated with @Inject, because we certainly do not want to trigger the rule on any class. Do you see any pattern which would allow us to identify class which would require constructor injection otherwise?

Michael

[Sud Ramasamy]

Thanks. The updated version of the rspec reflects the language that I was trying to convey.

Regarding a similar rule for non Spring managed beans that are using javax.inject you maybe right. I don't have enough experience to comment on this.

Regarding javax.inject though, if it is a Spring managed bean but is using javax.inject for injection will this proposed rule account for it?

Also, recent versions of Spring have made the autowire annotation optional when there is only one default constructor.  Should this rule allow for this? I'm split on this because it's very rare to have multiple constructors for our Spring beans. But enforcing this rule does make the developer's intent clear. So for those who don't agree with that they would simply disable this rule.  Thoughts?

-sud