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 69e95e2ec73..45a742f61b7 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 @@ -178,7 +178,7 @@ public void javaCheckTestSources() throws Exception { assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs); assertThat(newTotal).isEqualTo(knownTotal); - assertThat(rulesCausingFPs).hasSize(4); + assertThat(rulesCausingFPs).hasSize(5); assertThat(rulesNotReporting).hasSize(7); assertThat(rulesSilenced).hasSize(69); @@ -188,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")); - assertThat(differences).isEqualTo("Issues differences: 3266"); + assertThat(differences).isEqualTo("Issues differences: 3268"); } private static Path pathFor(String path) { 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 7fddf8ad7b4..e821487782c 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 @@ -3,7 +3,7 @@ "ruleKey": "S100", "hasTruePositives": true, "falseNegatives": 0, - "falsePositives": 0 + "falsePositives": 2 }, { "ruleKey": "S101", diff --git a/java-checks-test-sources/src/main/files/non-compiling/filters/SpringFilter.java b/java-checks-test-sources/src/main/files/non-compiling/filters/SpringFilter.java index 04e320ec7bb..deb4a07329f 100644 --- a/java-checks-test-sources/src/main/files/non-compiling/filters/SpringFilter.java +++ b/java-checks-test-sources/src/main/files/non-compiling/filters/SpringFilter.java @@ -1,7 +1,21 @@ package filters; +import java.util.List; + public class SpringFilter { + static class S100 implements Repository { + static class S100_1 { + // FP in case of missing semantic - 'Repository' is not resolved + List findByAddress_ZipCode(ZipCode zipCode) { // WithIssue + return List.of(); + } + } + + interface Person {} + interface ZipCode {} + } + static class S1185 { abstract static class S1185_1 { void bar() {} diff --git a/java-checks-test-sources/src/main/java/filters/SpringFilter.java b/java-checks-test-sources/src/main/java/filters/SpringFilter.java index 27e3b5dbe6d..bf9688d0e4b 100644 --- a/java-checks-test-sources/src/main/java/filters/SpringFilter.java +++ b/java-checks-test-sources/src/main/java/filters/SpringFilter.java @@ -1,9 +1,11 @@ package filters; +import java.util.List; import javax.servlet.http.HttpServlet; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import org.springframework.data.mongodb.repository.Query; import org.springframework.stereotype.Component; import org.springframework.stereotype.Repository; import org.springframework.stereotype.Service; @@ -17,6 +19,39 @@ public class SpringFilter { + static class S100 { + static class S100_1 implements org.springframework.data.repository.Repository { + List findByAddress_ZipCode(ZipCode zipCode) { // NoIssue + return List.of(); + } + + public void invalidName_() {} // WithIssue + public void _invalidName() {} // WithIssue + } + + static class S100_2 { + List findByAddress_ZipCode(ZipCode zipCode) { // WithIssue + return List.of(); + } + } + + static class S100_3 implements PersonRepository { + @Override + public List findByAddress_ZipCode(Address address, ZipCode zipCode) { + return List.of(); + } + } + + interface PersonRepository extends org.springframework.data.repository.Repository { + @Query("select p from Person where p.address=?1 and p.zipcode=?2") + List findByAddress_ZipCode(Address address, ZipCode zipCode); // NoIssue + } + + interface Address {} + interface Person {} + interface ZipCode {} + } + static class S107 { S107(String p1, String p2, String p3, String p4) {} // NoIssue diff --git a/java-checks/src/main/java/org/sonar/java/filters/SpringFilter.java b/java-checks/src/main/java/org/sonar/java/filters/SpringFilter.java index 2406dd28cc6..1a0c7f40be3 100644 --- a/java-checks/src/main/java/org/sonar/java/filters/SpringFilter.java +++ b/java-checks/src/main/java/org/sonar/java/filters/SpringFilter.java @@ -26,8 +26,9 @@ import org.sonar.java.checks.ServletInstanceFieldCheck; import org.sonar.java.checks.TooManyParametersCheck; import org.sonar.java.checks.helpers.ExpressionsHelper; +import org.sonar.java.checks.naming.BadMethodNameCheck; import org.sonar.plugins.java.api.JavaCheck; -import org.sonar.plugins.java.api.semantic.Symbol.MethodSymbol; +import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.SymbolMetadata; import org.sonar.plugins.java.api.tree.AnnotationTree; import org.sonar.plugins.java.api.tree.ClassTree; @@ -60,6 +61,7 @@ public class SpringFilter extends BaseTreeVisitorIssueFilter { @Override public Set> filteredRules() { return Set.of( + /* S100_ */ BadMethodNameCheck.class, /* S107_ */ TooManyParametersCheck.class, /* S1185 */ MethodOnlyCallsSuperCheck.class, /* S1258 */ AtLeastOneConstructorCheck.class, @@ -81,17 +83,36 @@ public void visitVariable(VariableTree tree) { @Override public void visitMethod(MethodTree tree) { - MethodSymbol symbol = tree.symbol(); + Symbol.MethodSymbol symbol = tree.symbol(); + Tree reportTree = tree.simpleName(); if (tree.is(Tree.Kind.CONSTRUCTOR)) { SymbolMetadata ownerMetadata = symbol.owner().metadata(); - excludeLinesIfTrue(S107_CLASS_ANNOTATION_EXCEPTIONS.stream().anyMatch(ownerMetadata::isAnnotatedWith), tree.simpleName(), TooManyParametersCheck.class); + excludeLinesIfTrue(S107_CLASS_ANNOTATION_EXCEPTIONS.stream().anyMatch(ownerMetadata::isAnnotatedWith), reportTree, TooManyParametersCheck.class); } else { SymbolMetadata methodMetadata = symbol.metadata(); - excludeLinesIfTrue(S107_METHOD_ANNOTATION_EXCEPTIONS.stream().anyMatch(methodMetadata::isAnnotatedWith), tree.simpleName(), TooManyParametersCheck.class); + excludeLinesIfTrue(S107_METHOD_ANNOTATION_EXCEPTIONS.stream().anyMatch(methodMetadata::isAnnotatedWith), reportTree, TooManyParametersCheck.class); + excludeLinesIfTrue(isRepositoryPropertyExpression(symbol), reportTree, BadMethodNameCheck.class); } super.visitMethod(tree); } + /** + * This methods requires semantic information to take a decision and filter out issues. + * The knowledge of being in a SpringData context can not be inferred from only tokens; it needs to understand the interfaces that the owning class implements. + * + * As a consequence, in case of missing semantic (degraded environment), S100 (BadMethodNameCheck) will still raise issues on SpringData methods. + * These issues will be considered FPs by the user, but can not be eliminated without introducing way too many FN for the rule. + * + * @param symbol the symbol of the method under analysis + * @return true if the method is understood as being a repository property expression, containing an underscore character in the middle of its name. Returns false otherwise. + */ + private static boolean isRepositoryPropertyExpression(Symbol.MethodSymbol symbol) { + String name = symbol.name(); + int underscorePosition = name.indexOf('_'); + boolean isSeparatorInMethodName = underscorePosition > 0 && underscorePosition < name.length() - 1; + return isSeparatorInMethodName && symbol.owner().type().isSubtypeOf("org.springframework.data.repository.Repository"); + } + private static boolean hasAutowiredField(ClassTree tree) { return tree.members().stream() .filter(member -> member.is(Tree.Kind.VARIABLE))