SONARJAVA-4652 Implement S6809: Async calls should not be done via th…

…is. (#4485)

its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java

@@ -182,15 +182,15 @@ public void javaCheckTestSources() throws Exception {
     softly.assertThat(newTotal).isEqualTo(knownTotal);
     softly.assertThat(rulesCausingFPs).hasSize(6);
     softly.assertThat(rulesNotReporting).hasSize(7);
-    softly.assertThat(rulesSilenced).hasSize(69);
+    softly.assertThat(rulesSilenced).hasSize(70);
 
     /**
      * 4. Check total number of differences (FPs + FNs)
      *
      * 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"));
-    softly.assertThat(differences).isEqualTo("Issues differences: 3335");
+    softly.assertThat(differences).isEqualTo("Issues differences: 3430");
 
     softly.assertAll();
   }

its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json

@@ -2873,6 +2873,12 @@
     "falseNegatives": 0,
     "falsePositives": 0
   },
+  {
+    "ruleKey": "S6809",
+    "hasTruePositives": false,
+    "falseNegatives": 94,
+    "falsePositives": 0
+  },
   {
     "ruleKey": "S6810",
     "hasTruePositives": false,
@@ -2882,7 +2888,7 @@
   {
     "ruleKey": "S6813",
     "hasTruePositives": true,
-    "falseNegatives": 31,
+    "falseNegatives": 32,
     "falsePositives": 0
   }
 ]

java-checks-test-sources/src/main/java/checks/spring/AsyncMethodsCalledViaThisCheckSample.java

@@ -0,0 +1,52 @@
+package checks.spring;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.scheduling.annotation.Async;
+import org.springframework.stereotype.Component;
+import org.springframework.transaction.annotation.Transactional;
+
+@Component
+public class AsyncMethodsCalledViaThisCheckSample {
+
+  @Autowired
+  private AsyncMethodsCalledViaThisCheckSample self;
+
+  @Async
+  void asyncMethod() {
+  }
+
+  @Transactional
+  public void transactionalMethod() {
+  }
+
+  void normal() {
+  }
+
+  void asyncNoncompliant1() {
+    asyncMethod(); // Noncompliant [[sc=5;ec=18]] {{Call async methods via an injected dependency instead of directly via 'this'.}}
+  }
+
+  void asyncNoncompliant2() {
+    this.asyncMethod(); // Noncompliant
+  }
+
+  void asyncCompliant1() {
+    self.asyncMethod();
+  }
+
+  void transactionalNoncompliant1() {
+    transactionalMethod(); // Noncompliant {{Call transactional methods via an injected dependency instead of directly via 'this'.}}
+  }
+
+  void transactionalNoncompliant2() {
+    this.transactionalMethod(); // Noncompliant
+  }
+
+  void transactionalCompliant1() {
+    self.transactionalMethod();
+  }
+
+  void compliant() {
+    normal();
+  }
+}

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

@@ -135,6 +135,7 @@
 import org.sonar.java.checks.serialization.SerializableFieldInSerializableClassCheck;
 import org.sonar.java.checks.serialization.SerializableObjectInSessionCheck;
 import org.sonar.java.checks.serialization.SerializableSuperConstructorCheck;
+import org.sonar.java.checks.spring.AsyncMethodsCalledViaThisCheck;
 import org.sonar.java.checks.spring.AsyncMethodsReturnTypeCheck;
 import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck;
 import org.sonar.java.checks.spring.FieldDependencyInjectionCheck;
@@ -275,6 +276,7 @@ public final class CheckList {
     AssertOnBooleanVariableCheck.class,
     AssertionsInProductionCodeCheck.class,
     AssertsOnParametersOfPublicMethodCheck.class,
+    AsyncMethodsCalledViaThisCheck.class,
     AsyncMethodsReturnTypeCheck.class,
     AtLeastOneConstructorCheck.class,
     AuthorizationsStrongDecisionsCheck.class,

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

@@ -0,0 +1,60 @@
+/*
+ * 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 java.util.Map;
+import org.sonar.check.Rule;
+import org.sonar.java.model.ExpressionUtils;
+import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree;
+import org.sonar.plugins.java.api.tree.MethodInvocationTree;
+import org.sonar.plugins.java.api.tree.Tree;
+
+@Rule(key = "S6809")
+public class AsyncMethodsCalledViaThisCheck extends IssuableSubscriptionVisitor {
+
+  private static final Map<String, String> DISALLOWED_METHOD_ANNOTATIONS = Map.of(
+    "org.springframework.scheduling.annotation.Async", "async",
+    "org.springframework.transaction.annotation.Transactional", "transactional");
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.METHOD_INVOCATION);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    var mit = (MethodInvocationTree) tree;
+
+    if (
+      // If the call is not a member select, it must be an identifier, so it's a call to a local method, implicitly via 'this'
+      !mit.methodSelect().is(Tree.Kind.MEMBER_SELECT) ||
+        // On the other hand, if calls do have a qualifier, an explicit 'this' means we also want to raise an issue.
+        ExpressionUtils.isThis(((MemberSelectExpressionTree) mit.methodSelect()).expression())
+    ) {
+      DISALLOWED_METHOD_ANNOTATIONS.entrySet().stream()
+        .filter(entry -> mit.methodSymbol().metadata().isAnnotatedWith(entry.getKey()))
+        .findFirst()
+        .map(Map.Entry::getValue)
+        .ifPresent(friendlyName -> reportIssue(mit, "Call " + friendlyName + " methods via an injected dependency instead of directly via 'this'."));
+    }
+  }
+}

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

@@ -0,0 +1,62 @@
+<h2>Why is this an issue?</h2>
+<p>A method annotated with Spring’s <code>@Async</code> or <code>@Transactional</code> annotations will not work as expected if invoked directly from
+within its class.</p>
+<p>This is because Spring generates a proxy class with wrapper code to manage the method’s asynchronicity (<code>@Async</code>) or to handle the
+transaction (<code>@Transactional</code>). However, when called using <code>this</code>, the proxy instance is bypassed, and the method is invoked
+directly without the required wrapper code.</p>
+<h2>How to fix it</h2>
+<p>Replace calls to <code>@Async</code> or <code>@Transactional</code> methods via <code>this</code> with calls on an instance that was injected by
+Spring (<code>@Autowired</code>, <code>@Resource</code> or <code>@Inject</code>). The injected instance is a proxy on which the methods can be invoked
+safely.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+@Service
+public class AsyncNotificationProcessor implements NotificationProcessor {
+
+  @Override
+  public void process(Notification notification) {
+    processAsync(notification); // Noncompliant, call bypasses proxy
+  }
+
+  @Async
+  public processAsync(Notification notification) {
+    // ...
+  }
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+@Service
+public class AsyncNotificationProcessor implements NotificationProcessor {
+
+  @Resource
+  private AsyncNotificationProcessor
+
+  @Override
+  public void process(Notification notification) {
+    asyncNotificationProcessor.processAsync(notification); // Compliant, call via injected proxy
+  }
+
+  @Async
+  public processAsync(Notification notification) {
+    // ...
+  }
+}
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> <a href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/scheduling/annotation/Async.html">Spring
+  Framework API - Annotation Interface Async</a> </li>
+  <li> <a href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/transaction/annotation/Transactional.html">Spring
+  Framework API - Annotation Interface Transactional</a> </li>
+</ul>
+<h3>Articles &amp; blog posts</h3>
+<ul>
+  <li> <a href="https://www.baeldung.com/spring-async">Baeldung - How To Do @Async in Spring</a> </li>
+  <li> <a href="https://stackoverflow.com/questions/22561775/spring-async-ignored">Stack Overflow - Spring @Async ignored</a> </li>
+  <li> <a href="https://stackoverflow.com/questions/4396284/does-spring-transactional-attribute-work-on-a-private-method">Stack Overflow - Does Spring
+  @Transactional attribute work on a private method?</a> </li>
+</ul>
+

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

@@ -0,0 +1,23 @@
+{
+  "title": "S6809: Methods with Spring proxy should not be called via \"this\"",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6809",
+  "sqKey": "S6809",
+  "scope": "All",
+  "quickfix": "unknown",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "HIGH",
+      "RELIABILITY": "MEDIUM",
+      "SECURITY": "LOW"
+    },
+    "attribute": "CONVENTIONAL"
+  }
+}

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

@@ -480,6 +480,7 @@
     "S6539",
     "S6541",
     "S6548",
+    "S6809",
     "S6810",
     "S6813"
   ]

java-checks/src/test/java/org/sonar/java/checks/spring/AsyncMethodsCalledViaThisCheckTest.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 static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
+
+class AsyncMethodsCalledViaThisCheckTest {
+  @Test
+  void test() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/spring/AsyncMethodsCalledViaThisCheckSample.java"))
+      .withCheck(new AsyncMethodsCalledViaThisCheck())
+      .verifyIssues();
+  }
+}