diff --git a/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java b/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java index 1bae480882f..f5bf3a6a683 100644 --- a/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java +++ b/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java @@ -188,7 +188,7 @@ public void javaCheckTestSources() throws Exception { * No differences would mean that we find the same issues with and without the bytecode and libraries */ String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences")); - assertThat(differences).isEqualTo("Issues differences: 3304"); + assertThat(differences).isEqualTo("Issues differences: 3335"); } private static Path pathFor(String path) { diff --git a/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json b/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json index ddb7d2732f4..d17dacf5ad6 100644 --- a/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json +++ b/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json @@ -2878,5 +2878,11 @@ "hasTruePositives": false, "falseNegatives": 5, "falsePositives": 0 + }, + { + "ruleKey": "S6813", + "hasTruePositives": true, + "falseNegatives": 31, + "falsePositives": 0 } ] diff --git a/java-checks-test-sources/src/main/java/checks/spring/FieldDependencyInjectionCheckJakartaSample.java b/java-checks-test-sources/src/main/java/checks/spring/FieldDependencyInjectionCheckJakartaSample.java new file mode 100644 index 00000000000..eb25467e756 --- /dev/null +++ b/java-checks-test-sources/src/main/java/checks/spring/FieldDependencyInjectionCheckJakartaSample.java @@ -0,0 +1,19 @@ +package checks.spring; + +import jakarta.inject.Inject; +import org.springframework.stereotype.Component; + +@Component +public class FieldDependencyInjectionCheckJakartaSample { + @Inject // Noncompliant + private String injected; + + @Inject // Compliant + public FieldDependencyInjectionCheckJakartaSample(String injected) { + } + + @Inject // Compliant + public void setInjected(String injected) { + this.injected = injected; + } +} diff --git a/java-checks-test-sources/src/main/java/checks/spring/FieldDependencyInjectionCheckSample.java b/java-checks-test-sources/src/main/java/checks/spring/FieldDependencyInjectionCheckSample.java new file mode 100644 index 00000000000..2d9018c59bb --- /dev/null +++ b/java-checks-test-sources/src/main/java/checks/spring/FieldDependencyInjectionCheckSample.java @@ -0,0 +1,42 @@ +package checks.spring; + +import javax.annotation.Nullable; +import javax.inject.Inject; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.stereotype.Component; + +@Component +public class FieldDependencyInjectionCheckSample { + @Autowired // Noncompliant [[sc=3;ec=13]] {{Remove this field injection and use constructor injection instead.}} + private String autowired; + + @Inject // Noncompliant + private String injected; + + @Autowired // Noncompliant + @Nullable + private String injectedAndNullable; + + @Autowired // Compliant + public FieldDependencyInjectionCheckSample() { + } + + @Inject // Compliant + public FieldDependencyInjectionCheckSample(String injected) { + } + + @Autowired // Compliant + public void setAutowired(String autowired) { + this.autowired = autowired; + } + + @Inject // Compliant + public void setInjected(String injected) { + this.injected = injected; + } + + private String notInjected; + + @Nullable + private String annotatedButNotInjected; +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java index bf1e5617109..120076d494d 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java +++ b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java @@ -137,6 +137,7 @@ import org.sonar.java.checks.serialization.SerializableSuperConstructorCheck; import org.sonar.java.checks.spring.AsyncMethodsReturnTypeCheck; import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck; +import org.sonar.java.checks.spring.FieldDependencyInjectionCheck; import org.sonar.java.checks.spring.PersistentEntityUsedAsRequestParameterCheck; import org.sonar.java.checks.spring.RequestMappingMethodPublicCheck; import org.sonar.java.checks.spring.SpringAntMatcherOrderCheck; @@ -402,6 +403,7 @@ public final class CheckList { ExcessiveContentRequestCheck.class, ExpressionComplexityCheck.class, ExternalizableClassConstructorCheck.class, + FieldDependencyInjectionCheck.class, FieldModifierCheck.class, FileHeaderCheck.class, FilePermissionsCheck.class, diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/FieldDependencyInjectionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/FieldDependencyInjectionCheck.java new file mode 100644 index 00000000000..e6bc79095d0 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/FieldDependencyInjectionCheck.java @@ -0,0 +1,56 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S6813") +public class FieldDependencyInjectionCheck extends IssuableSubscriptionVisitor { + private static final List INJECTION_ANNOTATIONS = List.of( + "org.springframework.beans.factory.annotation.Autowired", + "javax.inject.Inject", + "jakarta.inject.Inject"); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS); + } + + @Override + public void visitNode(Tree tree) { + var ct = (ClassTree) tree; + ct.members().forEach(member -> { + if (member.is(Tree.Kind.VARIABLE)) { + var vt = (VariableTree) member; + + vt.modifiers().annotations().stream() + .filter(annotationTree -> INJECTION_ANNOTATIONS.stream() + .anyMatch(targetAnnotation -> annotationTree.symbolType().is(targetAnnotation))) + .findFirst() + .ifPresent(annotationTree -> reportIssue(annotationTree, "Remove this field injection and use constructor injection instead.")); + } + }); + } +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.html new file mode 100644 index 00000000000..64700259793 --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.html @@ -0,0 +1,40 @@ +

Why is this an issue?

+

Spring supports dependency injection by using annotations such as @Autowired. This annotation can be used to inject beans via +constructor, setter, and field injection.

+

Generally speaking, field injection is discouraged. It allows the creation of objects in an invalid state and makes testing more difficult. The +dependencies are not explicit when instantiating a class that uses field injection.

+

Apart from that, field injection is not compatible with final fields. Keeping dependencies immutable where possible makes the code easier to +understand, easing development and maintenance.

+

This rule raises an issue when the @Autowired or @Inject annotations are used on a field.

+

How to fix it

+

Use constructor injection instead.

+

By using constructor injection, the dependencies are explicit and must be passed during an object’s construction. This avoids the possibility of +instantiating an object in an invalid state and makes types more testable. Fields can be declared final, which makes the code easier to understand, as +dependencies don’t change after instantiation.

+

Code examples

+

Noncompliant code example

+
+public class SomeService {
+    @Autowired
+    private SomeDependency someDependency;
+}
+
+

Compliant solution

+
+public class SomeService {
+    private final SomeDependency someDependency;
+
+    @Autowired
+    public SomeService(SomeDependency someDependency) {
+        this.someDependency = someDependency;
+    }
+}
+
+

Resources

+

Articles & blog posts

+ + diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json new file mode 100644 index 00000000000..2bcf98f6b5a --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6813.json @@ -0,0 +1,22 @@ +{ + "title": "Avoid field dependency injection", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6813", + "sqKey": "S6813", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM", + "RELIABILITY": "MEDIUM" + }, + "attribute": "CONVENTIONAL" + } +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json index 3ace38ad440..8ba6b71484b 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json @@ -480,6 +480,7 @@ "S6539", "S6541", "S6548", - "S6810" + "S6810", + "S6813" ] } diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/FieldDependencyInjectionCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/FieldDependencyInjectionCheckTest.java new file mode 100644 index 00000000000..c316a2a4241 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/FieldDependencyInjectionCheckTest.java @@ -0,0 +1,43 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class FieldDependencyInjectionCheckTest { + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/spring/FieldDependencyInjectionCheckSample.java")) + .withCheck(new FieldDependencyInjectionCheck()) + .verifyIssues(); + } + + @Test + void testJakarta() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/spring/FieldDependencyInjectionCheckJakartaSample.java")) + .withCheck(new FieldDependencyInjectionCheck()) + .verifyIssues(); + } +}