SONARJAVA-4685 S6838 Bean methods should not be invoked directly when…

… `proxyBeanMethods` is set to false (#4597)

its/autoscan/src/test/resources/autoscan/diffs/diff_S1874.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S1874",
   "hasTruePositives": true,
-  "falseNegatives": 102,
+  "falseNegatives": 103,
   "falsePositives": 0
-}
+}

its/autoscan/src/test/resources/autoscan/diffs/diff_S6838.json

@@ -0,0 +1,6 @@
+{
+  "ruleKey": "S6838",
+  "hasTruePositives": false,
+  "falseNegatives": 3,
+  "falsePositives": 0
+}

java-checks-test-sources/default/pom.xml

@@ -191,7 +191,7 @@
     <dependency>
       <groupId>org.springframework</groupId>
       <artifactId>spring-context</artifactId>
-      <version>5.0.6.RELEASE</version>
+      <version>5.3.31</version>
       <type>jar</type>
       <scope>provided</scope>
     </dependency>

java-checks-test-sources/default/src/main/java/checks/spring/DirectBeanMethodInvocationWithoutProxyCheckSample.java

@@ -0,0 +1,145 @@
+package checks.spring;
+
+import java.util.Random;
+import org.springframework.beans.factory.config.ConfigurableBeanFactory;
+import org.springframework.context.annotation.Bean;
+import org.springframework.context.annotation.Configuration;
+import org.springframework.context.annotation.Scope;
+import org.springframework.stereotype.Component;
+
+import static java.lang.System.lineSeparator;
+
+public class DirectBeanMethodInvocationWithoutProxyCheckSample {
+  @Configuration(proxyBeanMethods = false)
+  static class NonCompliantConfiguration {
+    @Bean
+    public SimpleBean simpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    public CompositeBean compositeBean() {
+      return new CompositeBean(simpleBean()); // Noncompliant [[sc=32;ec=44]] {{Replace this bean method invocation with a dependency injection.}}
+    }
+
+    @Bean
+    @Scope("Singleton")
+    public SimpleBean anotherSimpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    public CompositeBean anotherCompositeBean() {
+      return new CompositeBean(anotherSimpleBean()); // Noncompliant [[sc=32;ec=51]] {{Replace this bean method invocation with a dependency injection.}}
+    }
+
+    @Bean
+    @Scope(ConfigurableBeanFactory.SCOPE_SINGLETON)
+    public SimpleBean yetAnotherSimpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    public CompositeBean yetAnotherCompositeBean() {
+      return new CompositeBean(yetAnotherSimpleBean()); // Noncompliant [[sc=32;ec=54]] {{Replace this bean method invocation with a dependency injection.}}
+    }
+  }
+
+  @Configuration(proxyBeanMethods = false)
+  static class CompliantConfiguration {
+    @Bean
+    public SimpleBean simpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    @Scope(ConfigurableBeanFactory.SCOPE_PROTOTYPE)
+    public PrototypeBean prototypeBean() {
+      return new PrototypeBean();
+    }
+
+    @Bean
+    public CompositeBean compositeBean(SimpleBean simpleBean) { // Compliant, the simpleBean is injected in the context and used by every compositeBean
+      return new CompositeBean(simpleBean);
+    }
+
+    @Bean
+    public CompositeBean compositeBeanWithPrototypeDependency() {
+      return new CompositeBean(prototypeBean()); // Compliant, beans with a prototype scope are not singletons (ie a new instance is created on each call)
+    }
+    @Bean
+    public NamedBean namedBean() {
+      return new NamedBean(lineSeparator());
+    }
+
+    public NamedBean anotherNamedBean() {
+      return new NamedBean(getAString());
+    }
+  }
+
+  @Configuration(proxyBeanMethods = true)
+  static class ProxyBeanMethodsEnabledExplicitly {
+    @Bean
+    public SimpleBean simpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    public CompositeBean compositeBean() {
+      return new CompositeBean(simpleBean()); // Compliant, call will be proxied and the singleton instance will be returned
+    }
+  }
+
+  @Configuration(value = "nothingToSeeHere")
+  static class ProxyBeanMethodsEnabledImplicitly {
+    @Bean
+    public SimpleBean simpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    public CompositeBean compositeBean() {
+      return new CompositeBean(simpleBean()); // Compliant, call will be proxied and the singleton instance will be returned
+    }
+  }
+
+
+  static class SimpleBean {
+    // ...
+  }
+
+  static class CompositeBean {
+    public CompositeBean(SimpleBean simpleBean) {
+      // ...
+    }
+  }
+  static class PrototypeBean extends SimpleBean {
+    // ...
+  }
+
+  static class NamedBean {
+    private String name;
+
+    public NamedBean(String name) {
+      this.name = name;
+    }
+  }
+
+  @Component
+  abstract class AnnotatedClassWithoutAConfigurationAnnotation {
+    @Bean
+    public SimpleBean simpleBean() {
+      return new SimpleBean();
+    }
+
+    @Bean
+    public CompositeBean compositeBean() {
+      return new CompositeBean(simpleBean()); // Compliant because this is not annotated with Configuration
+    }
+  }
+
+
+  private static String getAString() {
+    return (new Random(42)).nextBoolean() ? "Nothing" : "Something";
+  }
+}

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

@@ -0,0 +1,113 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Optional;
+import org.sonar.check.Rule;
+import org.sonar.java.checks.helpers.ExpressionsHelper;
+import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.semantic.SymbolMetadata;
+import org.sonar.plugins.java.api.tree.AnnotationTree;
+import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
+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.MethodInvocationTree;
+import org.sonar.plugins.java.api.tree.MethodTree;
+import org.sonar.plugins.java.api.tree.Tree;
+
+
+@Rule(key = "S6838")
+public class DirectBeanMethodInvocationWithoutProxyCheck extends IssuableSubscriptionVisitor {
+  private static final String CONFIGURATION_ANNOTATION = "org.springframework.context.annotation.Configuration";
+  private static final String SCOPE_ANNOTATION = "org.springframework.context.annotation.Scope";
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.CLASS);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    Optional<AnnotationTree> configurationAnnotation = getConfigurationAnnotation((ClassTree) tree);
+    if (configurationAnnotation.isEmpty() || !hasProxyBeanMethodsDisabled(configurationAnnotation.get())) {
+      return;
+    }
+    var visitor = new NonProxiedMethodInvocationVisitor((ClassTree) tree);
+    tree.accept(visitor);
+    visitor.locations.forEach(invocation -> reportIssue(invocation, "Replace this bean method invocation with a dependency injection."));
+  }
+
+  private static Optional<AnnotationTree> getConfigurationAnnotation(ClassTree tree) {
+    return tree.modifiers().annotations().stream()
+      .filter(annotationInstance -> annotationInstance.symbolType().is(CONFIGURATION_ANNOTATION))
+      .findFirst();
+  }
+
+
+  private static boolean hasProxyBeanMethodsDisabled(AnnotationTree annotation) {
+    return annotation.arguments().stream()
+      .filter(argument -> argument.is(Tree.Kind.ASSIGNMENT))
+      .map(AssignmentExpressionTree.class::cast)
+      .anyMatch(DirectBeanMethodInvocationWithoutProxyCheck::setsProxyBeanMethodsToFalse);
+  }
+
+  private static boolean setsProxyBeanMethodsToFalse(AssignmentExpressionTree assignment) {
+    return "proxyBeanMethods".equals(((IdentifierTree) assignment.variable()).name()) &&
+      Boolean.FALSE.equals(ExpressionsHelper.getConstantValueAsBoolean(assignment.expression()).value());
+  }
+
+  private static class NonProxiedMethodInvocationVisitor extends BaseTreeVisitor {
+    private final ClassTree parentClass;
+    private final List<MethodInvocationTree> locations = new ArrayList<>();
+
+    public NonProxiedMethodInvocationVisitor(ClassTree parentClass) {
+      this.parentClass = parentClass;
+    }
+
+    @Override
+    public void visitMethodInvocation(MethodInvocationTree tree) {
+      super.visitMethodInvocation(tree);
+      MethodTree declaration = tree.methodSymbol().declaration();
+      if (declaration == null || hasPrototypeScope(declaration)) {
+        return;
+      }
+      Tree parent = declaration.parent();
+      if (parent == parentClass) {
+        locations.add(tree);
+      }
+    }
+
+    /*
+     * A method with the prototype scope is meant to return a new instance on every call.
+     */
+    private static boolean hasPrototypeScope(MethodTree method) {
+      List<SymbolMetadata.AnnotationValue> annotationValues = method.symbol().metadata().valuesForAnnotation(SCOPE_ANNOTATION);
+      return annotationValues != null && annotationValues.stream()
+        .filter(argument -> List.of("value", "scopeName").contains(argument.name()))
+        .map(SymbolMetadata.AnnotationValue::value)
+        .map(String.class::cast)
+        .anyMatch("prototype"::equalsIgnoreCase);
+    }
+  }
+
+}

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

@@ -0,0 +1,35 @@
+/*
+ * 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 org.sonar.java.checks.verifier.TestUtils;
+
+class DirectBeanMethodInvocationWithoutProxyCheckTest {
+
+  @Test
+  void test() {
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/spring/DirectBeanMethodInvocationWithoutProxyCheckSample.java"))
+      .withCheck(new DirectBeanMethodInvocationWithoutProxyCheck())
+      .verifyIssues();
+  }
+}

sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java

@@ -564,6 +564,7 @@
 import org.sonar.java.checks.spring.AvoidQualifierOnBeanMethodsCheck;
 import org.sonar.java.checks.spring.ControllerWithRestControllerReplacementCheck;
 import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck;
+import org.sonar.java.checks.spring.DirectBeanMethodInvocationWithoutProxyCheck;
 import org.sonar.java.checks.spring.FieldDependencyInjectionCheck;
 import org.sonar.java.checks.spring.ModelAttributeNamingConventionForSpELCheck;
 import org.sonar.java.checks.spring.NonSingletonAutowiredInSingletonCheck;
@@ -809,6 +810,7 @@ public final class CheckList {
     DefaultInitializedFieldCheck.class,
     DeprecatedTagPresenceCheck.class,
     DiamondOperatorCheck.class,
+    DirectBeanMethodInvocationWithoutProxyCheck.class,
     DisableAutoEscapingCheck.class,
     DisallowedConstructorCheck.class,
     DisallowedMethodCheck.class,