SONARJAVA-2753 Rule S3305: No autowired fields in "@configuration" cl…

…ass (#2067)

java-checks/pom.xml

@@ -227,6 +227,11 @@
                   <version>2.1</version>
                   <type>jar</type>
                 </artifactItem>
+                <artifactItem>
+                  <groupId>javax.inject</groupId>
+                  <artifactId>javax.inject</artifactId>
+                  <version>1</version>
+                </artifactItem>
                 <artifactItem>
                   <groupId>com.google.guava</groupId>
                   <artifactId>guava</artifactId>

java-checks/src/main/java/org/sonar/java/checks/CheckList.java

@@ -67,6 +67,7 @@
 import org.sonar.java.checks.spring.SpringComponentWithNonAutowiredMembersCheck;
 import org.sonar.java.checks.spring.SpringComponentWithWrongScopeCheck;
 import org.sonar.java.checks.spring.SpringComposedRequestMappingCheck;
+import org.sonar.java.checks.spring.SpringConfigurationWithAutowiredFieldsCheck;
 import org.sonar.java.checks.spring.SpringRequestMappingMethodCheck;
 import org.sonar.java.checks.spring.SpringScanDefaultPackageCheck;
 import org.sonar.java.checks.spring.SpringSecurityDebugModeCheck;
@@ -530,6 +531,7 @@ public static List<Class<? extends JavaCheck>> getJavaChecks() {
       .add(StaticFieldUpdateInConstructorCheck.class)
       .add(NestedTernaryOperatorsCheck.class)
       .add(SpringComponentWithNonAutowiredMembersCheck.class)
+      .add(SpringConfigurationWithAutowiredFieldsCheck.class)
       .add(SpringComponentWithWrongScopeCheck.class)
       .add(SpringComposedRequestMappingCheck.class)
       .add(SpringRequestMappingMethodCheck.class)

java-checks/src/main/java/org/sonar/java/checks/spring/SpringConfigurationWithAutowiredFieldsCheck.java

@@ -0,0 +1,112 @@
+/*
+ * SonarQube Java
+ * Copyright (C) 2012-2018 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.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.sonar.check.Rule;
+import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.semantic.Symbol;
+import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
+import org.sonar.plugins.java.api.tree.ClassTree;
+import org.sonar.plugins.java.api.tree.IdentifierTree;
+import org.sonar.plugins.java.api.tree.MethodTree;
+import org.sonar.plugins.java.api.tree.Tree;
+import org.sonar.plugins.java.api.tree.VariableTree;
+
+@Rule(key = "S3305")
+public class SpringConfigurationWithAutowiredFieldsCheck extends IssuableSubscriptionVisitor {
+
+  private static final String MESSAGE_FORMAT = "Inject this field value directly into \"%s\", the only method that uses it.";
+
+  private static final String CONFIGURATION_ANNOTATION = "org.springframework.context.annotation.Configuration";
+  private static final String BEAN_ANNOTATION = "org.springframework.context.annotation.Bean";
+  private static final List<String> AUTOWIRED_ANNOTATIONS = Arrays.asList(
+    "org.springframework.beans.factory.annotation.Autowired",
+    "javax.inject.Inject");
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return Collections.singletonList(Tree.Kind.CLASS);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    ClassTree classTree = (ClassTree) tree;
+    if (classTree.symbol().metadata().isAnnotatedWith(CONFIGURATION_ANNOTATION)) {
+      Map<Symbol, VariableTree> autowiredFields = new HashMap<>();
+      classTree.members().forEach(m -> collectAutowiredFields(m, autowiredFields));
+
+      Map<Symbol, List<MethodTree>> methodsThatUseAutowiredFields = new HashMap<>();
+      autowiredFields.keySet().forEach(f -> methodsThatUseAutowiredFields.put(f, new ArrayList<>()));
+      classTree.members().forEach(m -> collectMethodsThatUseAutowiredFields(m, methodsThatUseAutowiredFields));
+
+      // report autowired fields that are used by a single method, if that method is @Bean
+      methodsThatUseAutowiredFields.entrySet().stream()
+        .filter(methodsForField -> methodsForField.getValue().size() == 1 &&
+          methodsForField.getValue().get(0).symbol().metadata().isAnnotatedWith(BEAN_ANNOTATION))
+        .forEach(methodsForField -> reportIssue(
+          autowiredFields.get(methodsForField.getKey()).simpleName(),
+          String.format(MESSAGE_FORMAT, methodsForField.getValue().get(0).simpleName().name())));
+    }
+  }
+
+  private static void collectAutowiredFields(Tree tree, Map<Symbol, VariableTree> autowiredFields) {
+    if (!tree.is(Tree.Kind.VARIABLE)) {
+      return;
+    }
+    VariableTree variable = (VariableTree) tree;
+    Symbol variableSymbol = variable.symbol();
+    if (AUTOWIRED_ANNOTATIONS.stream().anyMatch(a -> variableSymbol.metadata().isAnnotatedWith(a))) {
+      autowiredFields.put(variableSymbol, variable);
+    }
+  }
+
+  private static void collectMethodsThatUseAutowiredFields(Tree tree, Map<Symbol, List<MethodTree>> methodsThatUseAutowiredFields) {
+    if (!tree.is(Tree.Kind.METHOD)) {
+      return;
+    }
+    IdentifiersVisitor identifiersVisitor = new IdentifiersVisitor(methodsThatUseAutowiredFields.keySet());
+    tree.accept(identifiersVisitor);
+    // for each autowired field that is referenced in this method, add the current method name to the list
+    identifiersVisitor.isFieldReferenced.entrySet().stream()
+      .filter(Map.Entry::getValue)
+      .map(Map.Entry::getKey)
+      .forEach(field -> methodsThatUseAutowiredFields.get(field).add((MethodTree) tree));
+  }
+
+  private static class IdentifiersVisitor extends BaseTreeVisitor {
+    private final Map<Symbol, Boolean> isFieldReferenced = new HashMap<>();
+
+    IdentifiersVisitor(Set<Symbol> autowiredFields) {
+      autowiredFields.forEach(f -> isFieldReferenced.put(f, false));
+    }
+
+    @Override
+    public void visitIdentifier(IdentifierTree identifierTree) {
+      isFieldReferenced.computeIfPresent(identifierTree.symbol(), (fieldSym, isPresent) -> true);
+    }
+  }
+}

java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3305_java.html

@@ -0,0 +1,32 @@
+<p>When <code>@Autowired</code> is used, dependencies need to be resolved when the class is instantiated, which may cause early initialization of
+beans or lead the context to look in places it shouldn't to find the bean. To avoid this tricky issue and optimize the way the context loads,
+dependencies should be requested as late as possible. That means using parameter injection instead of field injection for dependencies that are only
+used in a single <code>@Bean</code> method.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+@Configuration
+public class ​FooConfiguration {
+
+  @Autowired private ​DataSource dataSource​;  // Noncompliant
+
+  @Bean
+  public ​MyService myService() {
+    return new ​MyService(this​.dataSource​);
+  }
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+@Configuration
+public class ​FooConfiguration {
+
+ @Bean
+  public ​MyService myService(DataSource dataSource) {
+    return new ​MyService(dataSource);
+  }
+}
+</pre>
+<h2>Exceptions</h2>
+<p>Fields used in methods that are called directly by other methods in the application (as opposed to being invoked automatically by the Spring
+framework) are ignored by this rule so that direct callers don't have to provide the dependencies themselves.</p>
+

java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S3305_java.json

@@ -0,0 +1,17 @@
+{
+  "title": "Factory method injection should be used in \"@Configuration\" classes",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "spring",
+    "performance"
+  ],
+  "defaultSeverity": "Critical",
+  "ruleSpecification": "RSPEC-3305",
+  "sqKey": "S3305",
+  "scope": "Main"
+}

java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/Sonar_way_profile.json

@@ -257,6 +257,7 @@
     "S3066",
     "S3067",
     "S3281",
+    "S3305",
     "S3346",
     "S3355",
     "S3358",

java-checks/src/test/files/checks/spring/SpringConfigurationWithAutowiredFieldsCheck.java

@@ -0,0 +1,118 @@
+package src.test.files.checks.spring;
+
+import javax.inject.Inject;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+
+class Bar { }
+
+class Foo {
+  private final Bar bar;
+  public Foo(Bar bar) { this.bar = bar; }
+}
+
+@Configuration
+class A {
+
+  @Autowired private Bar singleUsage; // Noncompliant [[sc=26;ec=37]] {{Inject this field value directly into "method", the only method that uses it.}}
+  @Inject private Bar jsr330; // Noncompliant [[sc=23;ec=29]] {{Inject this field value directly into "jsr330", the only method that uses it.}}
+  @Autowired private Bar multipleUsage;
+  @Autowired private Bar notUsedInBeanMethod;
+  @Autowired private Bar notUsed;
+  private Bar notAutowired;
+
+  @Bean
+  public Foo method() {
+    return new Foo(this.singleUsage);
+  }
+
+  @Bean
+  public Foo jsr330() {
+    return new Foo(this.jsr330);
+  }
+
+  @Bean
+  public Foo method2() {
+    return new Foo(this.multipleUsage);
+  }
+
+  @Bean
+  public Foo method3() {
+    return new Foo(this.multipleUsage);
+  }
+
+  public Foo method4() {
+    return new Foo(this.notUsedInBeanMethod);
+  }
+
+  @Bean
+  public Foo method5() {
+    return new Foo(this.notAutowired);
+  }
+}
+
+@Configuration
+class B {
+  @Autowired private Bar multipleUsage;
+  @Bean
+  public Foo method() {
+    return indirectMethod();
+  }
+  private Foo indirectMethod() {
+    return new Foo(this.multipleUsage);
+  }
+  @Bean Foo method2() {
+    return new Foo(this.multipleUsage);
+  }
+}
+
+@Configuration
+class FalseNegative {
+
+  private Bar bar; // FN
+
+  @Autowired
+  public void setBar(Bar bar) {
+    this.bar = bar;
+  }
+
+  @Bean
+  public Foo method() {
+    return new Foo(this.bar);
+  }
+}
+
+@Configuration
+class Ok {
+
+  @Bean
+  public Foo method(Bar bar) {
+    return new Foo(bar);
+  }
+}
+
+@Configuration
+public class NestedConfig {
+
+  @Configuration
+  public class InnerConfig {
+    @Autowired private Bar x; // Noncompliant
+    @Bean
+    public Foo method() {
+      return new Foo(this.x);
+    }
+  }
+}
+
+public class StaticConfig {
+  @Configuration
+  public static class InnerStaticConfig {
+    @Autowired private Bar x; // Noncompliant
+
+    @Bean
+    public Foo method() {
+      return new Foo(this.x);
+    }
+  }
+}

java-checks/src/test/java/org/sonar/java/checks/spring/SpringConfigurationWithAutowiredFieldsCheckTest.java

@@ -0,0 +1,33 @@
+/*
+ * SonarQube Java
+ * Copyright (C) 2012-2018 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.Test;
+import org.sonar.java.checks.verifier.JavaCheckVerifier;
+
+public class SpringConfigurationWithAutowiredFieldsCheckTest {
+
+  @Test
+  public void test() {
+    JavaCheckVerifier.verify("src/test/files/checks/spring/SpringConfigurationWithAutowiredFieldsCheck.java", new SpringConfigurationWithAutowiredFieldsCheck());
+    JavaCheckVerifier.verifyNoIssueWithoutSemantic("src/test/files/checks/spring/SpringConfigurationWithAutowiredFieldsCheck.java", new SpringConfigurationWithAutowiredFieldsCheck());
+  }
+
+}