SONARJAVA-4723 in S6856, filter path variables used by Model Attribut…

…e when we extracting path variable from the url (#4606)
SonarSource · Dec 7, 2023 · ebac5e9 · ebac5e9
1 parent fd7d4e1
commit ebac5e9
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 46 deletions.
diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json
@@ -1,6 +1,6 @@
 {
   "ruleKey": "S6856",
   "hasTruePositives": false,
-  "falseNegatives": 21,
+  "falseNegatives": 22,
   "falsePositives": 0
 }
diff --git a/...esentIfPathVariableIsUsedCheckSample.java → ...ingPathVariableAnnotationCheckSample.java b/...esentIfPathVariableIsUsedCheckSample.java → ...ingPathVariableAnnotationCheckSample.java
@@ -4,12 +4,13 @@
 import java.util.Optional;
 import org.springframework.web.bind.annotation.DeleteMapping;
 import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.ModelAttribute;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.PostMapping;
 import org.springframework.web.bind.annotation.PutMapping;
 
-public class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample {
-  @GetMapping("/{id}") // Noncompliant {{Bind path variable "id" to a method parameter.}}
+public class MissingPathVariableAnnotationCheckSample {
+  @GetMapping("/{id}") // Noncompliant [[sc=3;ec=23]] {{Bind path variable "id" to a method parameter.}}
   public String get(String id) {
     return "Hello World";
   }
@@ -141,13 +142,6 @@ public String withoutRequestMappingAnnotation(@PathVariable  String id) { // com
     return "Hello World";
   }
 
-
-  @GetMapping("/{id}/{name}") // Noncompliant
-  public String mapStringToInt(@PathVariable Map<String,Integer> map) {
-    return "Hello World";
-  }
-
-
   @GetMapping(
     produces={"application/json", "application/xml"},
     consumes={"application/json", "application/xml"},
@@ -187,4 +181,54 @@ public String getPlaceHolder(String id) { // compliant, we don't consider this c
     return "Hello World";
   }
 
+  static class ModelA {
+    @ModelAttribute("user")
+    public String getUser(@PathVariable String id, @PathVariable String name) {
+      return "user";
+    }
+
+    @GetMapping("/{id}/{name}")
+    public String get() { // compliant, @ModelAttribute is always called before @GetMapping to generate the model. In our case model attribute
+      // consume the id and name path variables
+      return "Hello World";
+    }
+
+    @GetMapping("/{id}/{name}/{age}") // Compliant
+    public String get2(@PathVariable String age) { // compliant
+      return "Hello World";
+    }
+
+    @GetMapping("/{id}/{name}/{age}") // Noncompliant  {{Bind path variable "age" to a method parameter.}}
+    public String get3() {
+      return "Hello World";
+    }
+  }
+
+  static class ModelB {
+    @ModelAttribute("user")
+    public String getUser(@PathVariable String id) {
+      return "user";
+    }
+
+    @ModelAttribute("id")
+    public String getId(@PathVariable String name) {
+      return "id";
+    }
+
+    @GetMapping("/{id}/{name}")
+    public String get() { // compliant
+      return "Hello World";
+    }
+
+    @GetMapping("/{id}/{name}/{age}")
+    public String get2(@PathVariable String age) { // compliant
+      return "Hello World";
+    }
+
+    @GetMapping("/{id}/{name}/{age}") // Noncompliant
+    public String get3() {
+      return "Hello World";
+    }
+  }
+
 }
diff --git a/...ldBePresentIfPathVariableIsUsedCheck.java → ...s/MissingPathVariableAnnotationCheck.java b/...ldBePresentIfPathVariableIsUsedCheck.java → ...s/MissingPathVariableAnnotationCheck.java
@@ -33,16 +33,18 @@
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.semantic.SymbolMetadata;
 import org.sonar.plugins.java.api.semantic.Type;
+import org.sonar.plugins.java.api.tree.ClassTree;
 import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.VariableTree;
 
 @Rule(key = "S6856")
-public class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck extends IssuableSubscriptionVisitor {
+public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor {
   private static final String PATH_VARIABLE_ANNOTATION = "org.springframework.web.bind.annotation.PathVariable";
-  private static final Pattern EXTRACT_PATH_VARIABLE = Pattern.compile("([^:}/]*)(:.*)?\\}.*");
-  private static final Predicate<String> CONTAINS_PLACEHOLDER = Pattern.compile("\\$\\{.*\\}").asPredicate();
+  private static final String MODEL_ATTRIBUTE_ANNOTATION = "org.springframework.web.bind.annotation.ModelAttribute";
+  private static final Pattern EXTRACT_PATH_VARIABLE = Pattern.compile("([^:}/]*)(:.*)?}.*");
+  private static final Predicate<String> CONTAINS_PLACEHOLDER = Pattern.compile("\\$\\{.*}").asPredicate();
   private static final List<String> MAPPING_ANNOTATIONS = List.of(
     "org.springframework.web.bind.annotation.GetMapping",
     "org.springframework.web.bind.annotation.PostMapping",
@@ -51,58 +53,77 @@ public class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck exte
 
   @Override
   public List<Tree.Kind> nodesToVisit() {
-    return List.of(Tree.Kind.METHOD);
+    return List.of(Tree.Kind.CLASS);
   }
 
   @Override
   public void visitNode(Tree tree) {
-    MethodTree method = (MethodTree) tree;
+    ClassTree clazzTree = (ClassTree) tree;
 
-    MAPPING_ANNOTATIONS
-      .forEach(annotation -> reportIssueOnParameters(method, annotation));
-  }
+    List<MethodTree> methods = clazzTree.members().stream()
+      .filter(member -> member.is(Tree.Kind.METHOD))
+      .map(MethodTree.class::cast)
+      .collect(Collectors.toList());
 
-  private void reportIssueOnParameters(MethodTree method, String annotation) {
-    boolean containsMap = method.parameters().stream()
-      .filter(parameter -> parameter.symbol().metadata().isAnnotatedWith(PATH_VARIABLE_ANNOTATION))
-      .anyMatch(parameter -> {
-        Type type = parameter.type().symbolType();
-        // if the type is not Map<String,String>, Spring will throw a ClassCastException exception at runtime
-        boolean stringToString = type.typeArguments().stream().allMatch(typeArgument -> typeArgument.is("java.lang.String"));
-        return type.isSubtypeOf("java.util.Map") && stringToString;
-      });
+    Set<String> modelAttributePathVariable = methods.stream()
+      .filter(method -> method.symbol().metadata().isAnnotatedWith(MODEL_ATTRIBUTE_ANNOTATION))
+      .flatMap(method -> method.parameters().stream())
+      .map(MissingPathVariableAnnotationCheck::pathVariableName)
+      .flatMap(Optional::stream)
+      .collect(Collectors.toSet());
+
+    methods.forEach(method -> MAPPING_ANNOTATIONS
+      .forEach(annotation -> checkParameters(method, annotation, modelAttributePathVariable)));
+  }
 
-    if (containsMap) {
+  private void checkParameters(MethodTree method, String annotation, Set<String> modelAttributePathVariable) {
+    if (containsMap(method)) {
       /*
        * If any of the method parameters is a map, we assume all path variables are captured
        * and there is no mismatch with path variables in the request mapping.
        */
       return;
     }
 
-    Set<String> pathVariablesNames = method.parameters().stream()
-      .map(variable -> pathVariableName(variable))
+    Set<String> unusedPathVariables = findUnusedPathVariables(method, annotation, modelAttributePathVariable);
+    if (!unusedPathVariables.isEmpty()) {
+      reportIssue(
+        annotation(method, annotation),
+        "Bind path variable \"" + String.join("\", \"", unusedPathVariables) + "\" to a method parameter.");
+    }
+  }
+
+  private static Set<String> findUnusedPathVariables(MethodTree method, String annotation, Set<String> modelAttributePathVariable) {
+    Set<String> pathVariablesUsedInArguments = method.parameters().stream()
+      .map(MissingPathVariableAnnotationCheck::pathVariableName)
       .flatMap(Optional::stream)
       .collect(Collectors.toSet());
 
-    extractPathArgumentFromMappingAnnotations(method, annotation)
-      .map(path -> extractPathVariables(path))
-      .map(pathVariables -> {
-        pathVariables.removeAll(pathVariablesNames);
-        return pathVariables;
+    return extractPathArgumentFromMappingAnnotations(method, annotation)
+      .map(MissingPathVariableAnnotationCheck::extractPathVariables)
+      .flatMap(pathVariables -> {
+        pathVariables.removeAll(pathVariablesUsedInArguments);
+        pathVariables.removeAll(modelAttributePathVariable);
+        return pathVariables.stream();
       })
-      .filter(pathVariables -> !pathVariables.isEmpty())
-      .forEach(pathVariables -> reportIssue(
-        annotation(method, annotation),
-        "Bind path variable \"" + String.join("\", \"", pathVariables) + "\" to a method parameter."));
+      .collect(Collectors.toSet());
+  }
+
+  private static boolean containsMap(MethodTree method) {
+    return method.parameters().stream()
+      .filter(parameter -> parameter.symbol().metadata().isAnnotatedWith(PATH_VARIABLE_ANNOTATION))
+      .anyMatch(parameter -> {
+        Type type = parameter.type().symbolType();
+        return type.isSubtypeOf("java.util.Map");
+      });
   }
 
   private static ExpressionTree annotation(MethodTree method, String name) {
     return method.modifiers().annotations().stream()
       .filter(annotation -> annotation.symbolType().is(name))
       .findFirst()
-      // it will never be null because we are filtering on the annotation before.
-      .orElse(null);
+      // it will never be empty because we are filtering on the annotation before.
+      .orElseThrow();
   }
 
   private static Set<String> extractPathVariables(String path) {

diff --git a/...PresentIfPathVariableIsUsedCheckTest.java → ...ssingPathVariableAnnotationCheckTest.java b/...PresentIfPathVariableIsUsedCheckTest.java → ...ssingPathVariableAnnotationCheckTest.java
@@ -23,13 +23,13 @@
 import org.sonar.java.checks.verifier.CheckVerifier;
 import org.sonar.java.checks.verifier.TestUtils;
 
-class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest {
+class MissingPathVariableAnnotationCheckTest {
 
   @Test
   void test() {
     CheckVerifier.newVerifier()
-      .onFile(TestUtils.mainCodeSourcesPath("checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java"))
-      .withCheck(new PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck())
+      .onFile(TestUtils.mainCodeSourcesPath("checks/MissingPathVariableAnnotationCheckSample.java"))
+      .withCheck(new MissingPathVariableAnnotationCheck())
       .verifyIssues();
   }
 

diff --git a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java
@@ -279,7 +279,7 @@
 import org.sonar.java.checks.OverwrittenKeyCheck;
 import org.sonar.java.checks.ParameterReassignedToCheck;
 import org.sonar.java.checks.ParsingErrorCheck;
-import org.sonar.java.checks.PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck;
+import org.sonar.java.checks.MissingPathVariableAnnotationCheck;
 import org.sonar.java.checks.PopulateBeansCheck;
 import org.sonar.java.checks.PredictableSeedCheck;
 import org.sonar.java.checks.PreferStreamAnyMatchCheck;
@@ -969,7 +969,7 @@ public final class CheckList {
     OverrideAnnotationCheck.class,
     OverwrittenKeyCheck.class,
     PasswordEncoderCheck.class,
-    PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck.class,
+    MissingPathVariableAnnotationCheck.class,
     PersistentEntityUsedAsRequestParameterCheck.class,
     PopulateBeansCheck.class,
     PossessiveQuantifierContinuationCheck.class,