SONARJAVA-4881 With Spring 6, @transactional and @async annotated methods don't need to be public (#5094)

Since Spring 6, @transactional annotated methods are allowed to be protected and package-private.
This PR updates the rule S2230 to only apply to projects where Spring appears with a version less than 6.

Co-authored-by: Sander Knauff <S.knauff@alfen.com>

Author: romainbrenguier

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S2230",
-  "hasTruePositives": true,
-  "falseNegatives": 15,
+  "hasTruePositives": false,
+  "falseNegatives": 22,
   "falsePositives": 0
-}
+}

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

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

…actionalMethodVisibilityCheckSample.java …MethodVisibilityCheckSample_Spring5.javajava-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodVisibilityCheckSample.java renamed to java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodVisibilityCheckSample_Spring5.java java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodVisibilityCheckSample.java renamed to java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodVisibilityCheckSample_Spring5.java

@@ -4,44 +4,30 @@
 import org.springframework.scheduling.annotation.Async;
 import org.springframework.transaction.annotation.Transactional;
 
-abstract class TransactionalMethodVisibilityCheckSample {
+abstract class TransactionalMethodVisibilityCheckSample_Spring5 {
 
   public interface C {
     @Transactional
     int bar(); // Compliant
   }
 
-  private interface B {
-    @Transactional
-    int bar(); // Compliant
-  }
-
-  private interface A {
-    @Async
-    int bar(); // Compliant
-  }
-
   @Async
   protected abstract Future<String> aMethod(); // Noncompliant
 
-
   @Async
-  public Future<String> asyncMethod(){ // compliant
+  Future<String> defaultVisibilityAsyncMethod(){ // Noncompliant
     return  null;
   }
 
   @Async
-  private  Future<String> asyncMethodPrivate(){ // Noncompliant {{Make this method "public" or remove the "@Async" annotation.}}
+  protected Future<String> protectedVisibilityAsyncMethod(){ // Noncompliant
     return  null;
   }
 
-  @org.springframework.transaction.annotation.Transactional
-  public void publicTransactionalMethod() {} // Compliant
-
   @org.springframework.transaction.annotation.Transactional
   protected void protectedTransactionalMethod() {} // Noncompliant {{Make this method "public" or remove the "@Transactional" annotation.}}
 //               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-  
+
   @org.springframework.transaction.annotation.Transactional
   void defaultVisibilityTransactionalMethod() {} // Noncompliant {{Make this method "public" or remove the "@Transactional" annotation.}}
 }

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

@@ -0,0 +1,30 @@
+package checks.spring;
+
+import java.util.concurrent.Future;
+
+abstract class TransactionalMethodVisibilityCheckSample_Spring6 {
+  
+  public interface C {
+    @org.springframework.transaction.annotation.Transactional
+    int bar(); // Compliant
+  }
+
+  @org.springframework.scheduling.annotation.Async
+  protected Future<String> protectedAsyncMethod(){ // compliant
+    return  null;
+  }
+
+  @org.springframework.scheduling.annotation.Async
+  private Future<String> privateAsyncMethod(){ // Noncompliant {{Make this method non-"private" or remove the "@Async" annotation.}}
+    return  null;
+  }
+
+  @org.springframework.transaction.annotation.Transactional
+  protected void protectedTransactionalMethod() {} // Compliant
+
+
+  @org.springframework.transaction.annotation.Transactional
+  private Future<String> privateTransactionalMethod(){ // Noncompliant {{Make this method non-"private" or remove the "@Transactional" annotation.}}
+    return  null;
+  }
+}

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

@@ -19,23 +19,29 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
+import java.util.function.Function;
 import org.sonar.check.Rule;
+import org.sonar.plugins.java.api.DependencyVersionAware;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.Version;
 import org.sonar.plugins.java.api.tree.AnnotationTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.Tree;
 
 @Rule(key = "S2230")
-public class TransactionalMethodVisibilityCheck extends IssuableSubscriptionVisitor {
+public class TransactionalMethodVisibilityCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware {
 
-  private static final List<String> proxyAnnotations = List.of(
+  private static final List<String> PROXY_ANNOTATIONS = List.of(
     "org.springframework.transaction.annotation.Transactional",
     "org.springframework.scheduling.annotation.Async");
 
-  private static final Map<String, String> annShortName = Map.of(
+  private static final Map<String, String> ANNOTATION_SHORT_NAMES = Map.of(
     "org.springframework.transaction.annotation.Transactional", "@Transactional",
     "org.springframework.scheduling.annotation.Async", "@Async");
 
+  private boolean isSpring6OrLater = false;
+
   @Override
   public List<Tree.Kind> nodesToVisit() {
     return Collections.singletonList(Tree.Kind.METHOD);
@@ -44,15 +50,20 @@ public List<Tree.Kind> nodesToVisit() {
   @Override
   public void visitNode(Tree tree) {
     MethodTree method = (MethodTree) tree;
-    if (!method.symbol().isPublic()) {
-      proxyAnnotations.stream()
+    boolean handledBySpring = isSpring6OrLater ? !method.symbol().isPrivate() : method.symbol().isPublic();
+    if (!handledBySpring) {
+      PROXY_ANNOTATIONS.stream()
         .filter(annSymbol -> hasAnnotation(method, annSymbol))
         .forEach(annSymbol -> reportIssue(
           method.simpleName(),
-          "Make this method \"public\" or remove the \"" + annShortName.get(annSymbol) + "\" annotation."));
+          "Make this method " + requiredVisibilityMessage() + " or remove the \"" + ANNOTATION_SHORT_NAMES.get(annSymbol) + "\" annotation."));
     }
   }
 
+  private String requiredVisibilityMessage() {
+    return isSpring6OrLater ? "non-\"private\"" : "\"public\"";
+  }
+
   private static boolean hasAnnotation(MethodTree method, String annotationSymbol) {
     for (AnnotationTree annotation : method.modifiers().annotations()) {
       if (annotation.symbolType().is(annotationSymbol)) {
@@ -62,4 +73,18 @@ private static boolean hasAnnotation(MethodTree method, String annotationSymbol)
     return false;
   }
 
+  /** Check that Spring transaction artifact is present, and record whether its version is before or after 6.0. */
+  @Override
+  public boolean isCompatibleWithDependencies(Function<String, Optional<Version>> dependencyFinder) {
+    Optional<Version> springContextVersion = dependencyFinder.apply("spring-context");
+    Optional<Version> springTxVersion = dependencyFinder.apply("spring-tx");
+    if (springTxVersion.isEmpty() && springContextVersion.isEmpty()) {
+      return false;
+    }
+    isSpring6OrLater = springContextVersion
+      .or(() -> springTxVersion)
+      .map(v -> v.isGreaterThanOrEqualTo("6.0"))
+      .orElse(false);
+    return true;
+  }
 }

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

@@ -16,6 +16,9 @@
  */
 package org.sonar.java.checks.spring;
 
+import java.io.File;
+import java.util.Collections;
+import java.util.List;
 import org.junit.jupiter.api.Test;
 import org.sonar.java.checks.verifier.CheckVerifier;
 
@@ -25,13 +28,35 @@
 class TransactionalMethodVisibilityCheckTest {
 
   @Test
-  void test() {
+  void test_Spring5() {
     CheckVerifier.newVerifier()
-      .onFile(mainCodeSourcesPath("checks/spring/TransactionalMethodVisibilityCheckSample.java"))
+      .onFile(mainCodeSourcesPath("checks/spring/TransactionalMethodVisibilityCheckSample_Spring5.java"))
       .withCheck(new TransactionalMethodVisibilityCheck())
       .verifyIssues();
   }
 
+  @Test
+  void test_spring6() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath("checks/spring/TransactionalMethodVisibilityCheckSample_Spring6.java"))
+      .withCheck(new TransactionalMethodVisibilityCheck())
+      .withClassPath(List.of(new File("spring-tx-6.0.1.jar"),
+        new File("spring-context-6.0.1.jar")))
+      .verifyIssues();
+  }
+
+  /** Check that when dependencies are not resolved, we do not raise issues. */
+  @Test
+  void test_noDependencies() {
+    CheckVerifier.newVerifier()
+      .onFiles(
+        mainCodeSourcesPath("checks/spring/TransactionalMethodVisibilityCheckSample_Spring5.java"),
+        mainCodeSourcesPath("checks/spring/TransactionalMethodVisibilityCheckSample_Spring6.java"))
+      .withCheck(new TransactionalMethodVisibilityCheck())
+      .withClassPath(Collections.emptyList())
+      .verifyNoIssues();
+  }
+
   @Test
   void test_non_compiling() {
     CheckVerifier.newVerifier()