From a02adeff6441cc2834f8bb1623b24e2daf0d04bf Mon Sep 17 00:00:00 2001 From: erwan-serandour-sonarsource Date: Wed, 18 Oct 2023 14:28:08 +0200 Subject: [PATCH] SONARJAVA-4651 extend rule S2230 with @Asynch annotations (#4484) --- .../java/org/sonar/java/it/AutoScanTest.java | 4 +-- .../autoscan/autoscan-diff-by-rules.json | 4 +-- .../TransactionalMethodVisibilityCheck.java | 4 +-- .../TransactionalMethodVisibilityCheck.java | 29 ++++++++++++++++--- .../TransactionalMethodVisibilityCheck.java | 21 +++++++++++--- 5 files changed, 47 insertions(+), 15 deletions(-) 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 847a349c3eb..a64b9dec7e2 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 @@ -52,8 +52,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.assertj.core.api.Assertions.assertThat; - public class AutoScanTest { private static final Gson GSON = new GsonBuilder().setPrettyPrinting().create(); @@ -190,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")); - softly.assertThat(differences).isEqualTo("Issues differences: 3430"); + softly.assertThat(differences).isEqualTo("Issues differences: 3446"); softly.assertAll(); } 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 fa859176940..6c1a9ac464d 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 @@ -1112,7 +1112,7 @@ { "ruleKey": "S2230", "hasTruePositives": true, - "falseNegatives": 0, + "falseNegatives": 15, "falsePositives": 0 }, { @@ -2882,7 +2882,7 @@ { "ruleKey": "S6810", "hasTruePositives": false, - "falseNegatives": 5, + "falseNegatives": 6, "falsePositives": 0 }, { diff --git a/java-checks-test-sources/src/main/files/non-compiling/checks/spring/TransactionalMethodVisibilityCheck.java b/java-checks-test-sources/src/main/files/non-compiling/checks/spring/TransactionalMethodVisibilityCheck.java index 116e33c7936..3daec021998 100644 --- a/java-checks-test-sources/src/main/files/non-compiling/checks/spring/TransactionalMethodVisibilityCheck.java +++ b/java-checks-test-sources/src/main/files/non-compiling/checks/spring/TransactionalMethodVisibilityCheck.java @@ -5,11 +5,11 @@ class TransactionalMethodVisibilityCheck { // Cannot compile because a Transactional method should be overridable @org.springframework.transaction.annotation.Transactional - private void privateTransactionalMethod() {} // Noncompliant [[sc=16;ec=42]] {{Make this method "public" or remove the "@Transactional" annotation}} + private void privateTransactionalMethod() {} // Noncompliant [[sc=16;ec=42]] {{Make this method "public" or remove the "@Transactional" annotation.}} // Cannot compile because a Transactional method should be overridable @Transactional - private void privateTransactionalMethodWithImportBasedAnnotation() {} // Noncompliant {{Make this method "public" or remove the "@Transactional" annotation}} + private void privateTransactionalMethodWithImportBasedAnnotation() {} // Noncompliant {{Make this method "public" or remove the "@Transactional" annotation.}} @org.xxx.Transactional private void privateMethodWithNonSpringAnnotation() {} // Compliant diff --git a/java-checks-test-sources/src/main/java/checks/spring/TransactionalMethodVisibilityCheck.java b/java-checks-test-sources/src/main/java/checks/spring/TransactionalMethodVisibilityCheck.java index 152025197b1..cbfc56bc7c2 100644 --- a/java-checks-test-sources/src/main/java/checks/spring/TransactionalMethodVisibilityCheck.java +++ b/java-checks-test-sources/src/main/java/checks/spring/TransactionalMethodVisibilityCheck.java @@ -1,8 +1,10 @@ package checks.spring; +import java.util.concurrent.Future; +import org.springframework.scheduling.annotation.Async; import org.springframework.transaction.annotation.Transactional; -class TransactionalMethodVisibilityCheck { +abstract class TransactionalMethodVisibilityCheck { public interface C { @Transactional @@ -13,13 +15,32 @@ private interface B { @Transactional int bar(); // Compliant } - + + private interface A { + @Async + int bar(); // Compliant + } + + @Async + protected abstract Future aMethod(); // Noncompliant + + + @Async + public Future asyncMethod(){ // compliant + return null; + } + + @Async + private Future asyncMethodPrivate(){ // Noncompliant {{Make this method "public" or remove the "@Async" annotation.}} + 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}} + protected void protectedTransactionalMethod() {} // Noncompliant [[sc=18;ec=46]] {{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}} + void defaultVisibilityTransactionalMethod() {} // Noncompliant {{Make this method "public" or remove the "@Transactional" annotation.}} } diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodVisibilityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodVisibilityCheck.java index 6035b808300..2e18c10c737 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodVisibilityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodVisibilityCheck.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.List; +import java.util.Map; import org.sonar.check.Rule; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.tree.AnnotationTree; @@ -30,6 +31,14 @@ @Rule(key = "S2230") public class TransactionalMethodVisibilityCheck extends IssuableSubscriptionVisitor { + private static final List proxyAnnotations = List.of( + "org.springframework.transaction.annotation.Transactional", + "org.springframework.scheduling.annotation.Async"); + + private static final Map annShortName = Map.of( + "org.springframework.transaction.annotation.Transactional", "@Transactional", + "org.springframework.scheduling.annotation.Async", "@Async"); + @Override public List nodesToVisit() { return Collections.singletonList(Tree.Kind.METHOD); @@ -38,14 +47,18 @@ public List nodesToVisit() { @Override public void visitNode(Tree tree) { MethodTree method = (MethodTree) tree; - if (!method.symbol().isPublic() && hasTransactionalAnnotation(method)) { - reportIssue(method.simpleName(), "Make this method \"public\" or remove the \"@Transactional\" annotation"); + if (!method.symbol().isPublic()) { + proxyAnnotations.stream() + .filter(annSymbol -> hasAnnotation(method, annSymbol)) + .forEach(annSymbol -> reportIssue( + method.simpleName(), + "Make this method \"public\" or remove the \"" + annShortName.get(annSymbol) + "\" annotation.")); } } - private static boolean hasTransactionalAnnotation(MethodTree method) { + private static boolean hasAnnotation(MethodTree method, String annotationSymbol) { for (AnnotationTree annotation : method.modifiers().annotations()) { - if (annotation.symbolType().is("org.springframework.transaction.annotation.Transactional")) { + if (annotation.symbolType().is(annotationSymbol)) { return true; } }