Using Spring constructor injection with SonarQube

Question

I have a piece of code with field injections I am trying to convert to use constructor injections instead. The initial code looks like this:

@Autowired
private Environment env;

@Autowired
private YYYAdaptor yyyAdaptor;

@Autowired
private JAXBContext jaxbContext;

And this is how I rewrite it:

private Environment env;
private YYYAdaptor yyyAdaptor;
private JAXBContext jaxbContext;

@Autowired
public YYYResource(Environment env, YYYAdaptor yyyAdaptor, 
    @Qualifier("YYYYReq") JAXBContext jaxbContext) {

    this.env = env;
    this.yyyAdaptor = yyyAdaptor;
    this.jaxbContext = jaxbContext;
}

Doing this gives me a critical vulnerability on the sonar scan, with "this member" referring to each of the declared variables:

Annotate this member with "@Autowired", "@Resource", "@Inject", or "@Value", or remove it

What is the best way I can avoid using field injections while avoiding sonar throwing a fit?

declare em `final`!? (..and see what sonar says...) or do you need to re-assign these? — xerx593, Jan 12 '19 at 18:36
`static` will fix the warning :) ...but I am afraid of what happens then (other warnings, does the context start at all) — xerx593, Jan 12 '19 at 18:48
the context would start... making your code smell just to please Sonar sounds like a folly idea, though — Andrew Tobilko, Jan 12 '19 at 18:50
but apparently "autowire" rule and the "constructor injection" rule (or for what reason did you start the effort?) are contradictory/cannot be met both. — xerx593, Jan 12 '19 at 18:51
@xerx593 surprisingly it doesn't cause the code to blow up but just generates different warnings about assigning to static inside constructor instead — Layman, Jan 12 '19 at 18:52
...sure, that's ugly even in my sight! (assigning static in constructor) :), i think you found something, which (can be fixed &) should be reported to sonarqube, and meanwhile you have to stick to only one of the rules/deactivate/ignore the other — xerx593, Jan 12 '19 at 18:57
At PMD they gathered all of "these rules" into "controversial ruleset" and say: "..They are separated out here to allow people to include as they see fit via custom rulesets." — xerx593, Jan 12 '19 at 19:01
i have to say, this rule needs heavy improvement or work. as it is right now a mixture of constructor and field injection. i blindly added those annotations, because in my work environment we only use property injection and in that naive case it made sense. but i never looked at constructor injection. — Simon Schrottner, Jan 14 '19 at 09:45
FYI : rule should be improved to support constructor injection via https://jira.sonarsource.com/browse/SONARJAVA-3153 — benzonico, Jul 12 '19 at 08:07

score 7 · Accepted Answer · answered Jan 12 '19 at 19:15

7

Check-out the SonarQube rule RSPEC-4288: Spring components should use constructor injection. Although it doesn't explain why the final usage is triggered as non-compliant, there is a compliant code sample. Initialize the fields as null to make it SonarQube compliant:

private Environment env = null;
private YYYAdaptor yyyAdaptor = null;
private JAXBContext jaxbContext = null;

However, what SonarQube says is not sacred and is filled with lots of false-positives. These static-analyzers hits the issues that are worth the further introspection, yet not definitive and based on the rules made by people with opinions.

Personally, I'd mark this issue as won't fix and declare the fields as final to make the object immutable:

private final Environment env;
private final YYYAdaptor yyyAdaptor;
private final JAXBContext jaxbContext;

answered Jan 12 '19 at 19:15

Nikolas Charalambidis

40,893
16
117
183

Already tried initializing as null. When you declare the variables get "initialized" as null by the compiler. Sonar recognizes that too so just adds warnings there if you explicitly initialize as null, but does not resolve the initial warning. – Layman Jan 12 '19 at 19:22
2

LOL, I looked over that rule and thought that everything should be fine here: `= null;` can't be the issue, but your answer proves the opposite. +1 – Andrew Tobilko Jan 12 '19 at 19:28
@Andrew That is not the issue there. I think the best thing to do is to mark as won't fix. I think the part about =null; should be removed to avoid misleading – Layman Jan 12 '19 at 19:41
I am aware, going to accept it if you could remove the snippet from the compliant sample. It is a bit misleading – Layman Jan 12 '19 at 20:03
1

My issue is with "Initialize the fields as null to make it SonarQube compliant" since initializing newly declared vars is just redundant. Otherwise thank you for the help. – Layman Jan 14 '19 at 09:48

Tristan · Answer 2 · 2019-10-13T07:05:45.197

The rationale for this SonarQube rule is to avoid NullPointerException like explained for this rule : https://rules.sonarsource.com/java/RSPEC-3306

But when you have @Autowired with required = true (default value), you will never have a NullPointerException with a field injection.

So I think the good practice would be to deactivate and ignore this rule which is deprecated.

In recent SonarQube installations, these rules about constructor injections are disabled by default.

Update : There would be an other use case for constructor injection, it's when you are in a configuration class where each "method" is a bean constructor. You don't want to use field injection when a bean (like RestTemplateBuilder) is only used in some of the constructors, so you use pure constructor injection with no field declaration at all.

Using Spring constructor injection with SonarQube

2 Answers2