SONARJAVA-4723 implement rule S6856 : PathVariableAnnotationShouldBeP…

…resent (#4595)

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

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S1172",
   "hasTruePositives": true,
-  "falseNegatives": 13,
+  "falseNegatives": 14,
   "falsePositives": 0
 }

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

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

java-checks-test-sources/default/src/main/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java

@@ -0,0 +1,190 @@
+package checks;
+
+import java.util.Map;
+import java.util.Optional;
+import org.springframework.web.bind.annotation.DeleteMapping;
+import org.springframework.web.bind.annotation.GetMapping;
+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 String get(String id) {
+    return "Hello World";
+  }
+
+  @PostMapping(value = "/{name}") // Noncompliant {{Bind path variable "name" to a method parameter.}}
+  public String post(String id) {
+    return "Hello World";
+  }
+
+  @PutMapping(path = "/{id}") // Noncompliant
+  public String put(String id) {
+    return "Hello World";
+  }
+
+  @DeleteMapping("/{id}") // Noncompliant
+  public String delete(String id) {
+    return "Hello World";
+  }
+
+  @PutMapping("/id")
+  @DeleteMapping("/{id}") // Noncompliant
+  public String deletePut(String id) {
+    return "Hello World";
+  }
+
+
+  @GetMapping("/{id}")
+  public String getCompliant(@PathVariable  String id) { // compliant
+    return "Hello World";
+  }
+
+  @PostMapping("/{id}")
+  public String postCompliant(@PathVariable  String id) { // compliant
+    return "Hello World";
+  }
+
+  @PutMapping("/{id}")
+  public String putCompliant(@PathVariable  String id) { // compliant
+    return "Hello World";
+  }
+
+
+  @DeleteMapping("/{id}")
+  public String deleteCompliant(@PathVariable String id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}")
+  @DeleteMapping({"/{id}"})
+  public String deleteGetCompliant(@PathVariable String id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}")
+  public String getOtherThanString(@PathVariable Integer id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping()
+  public String getEmpty() { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}")
+  public String getFullyQualified(@org.springframework.web.bind.annotation.PathVariable String id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}/{name}")
+  public String get2PathVariables(@PathVariable String id, @PathVariable String name) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}") // Noncompliant
+  public String getBadName(@PathVariable String a) {
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}/{name}/{age}") // Noncompliant
+  public String get2SameName(@PathVariable("name") String a, @PathVariable(name = "name") String b, @PathVariable(value = "id", required=false) String c) {
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}/{name}/{age}")
+  public String get3Name(@PathVariable("name") String a, @PathVariable(name = "age") String b, @PathVariable(value = "id", required=false) String c) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}")
+  public String getMap(@PathVariable Map<String, String> map) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}/{name}")
+  public String getMap2(@PathVariable Map<String, String> map) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}/{name}/{age}")
+  public String getMapMixed(@PathVariable Map<String, String> map, @PathVariable String age) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping(value = {"/a/{id}", "/b/{id}", "/c"})
+  public String getSeveralPaths(@PathVariable Optional<String> id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping({"/a/{id}", "/b/{id}", "/c"})
+  public String getSeveralPathsDefault(@PathVariable Optional<String> id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/a/{id:.+}")
+  public String getRegex(@PathVariable String id) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping("/a/{id:.+}/{name:.+}")
+  public String getRegex2(@PathVariable String id, @PathVariable String name) { // compliant
+    return "Hello World";
+  }
+
+  public String withoutAnnotation(String id) { // compliant
+    return "Hello World";
+  }
+
+  public String withoutRequestMappingAnnotation(@PathVariable  String id) { // compliant
+    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"},
+    headers={"aHeader=aValue", "anotherHeader=anotherValue"},
+    params={"aPara", "anotherParam=anotherValue"},
+    name="aName",
+    path={"/{id}", "/{name}"}
+  )
+  public String getFullExample(@PathVariable Map<String,String> x) { // compliant
+    return "Hello World";
+  }
+
+  @GetMapping( // Noncompliant
+    produces={"application/json", "application/xml"},
+    consumes={"application/json", "application/xml"},
+    headers={"aHeader=aValue", "anotherHeader=anotherValue"},
+    params={"aPara", "anotherParam=anotherValue"},
+    name="aName",
+    path={"/{id}", "/name"}
+  )
+  public String getFullExampleNonCompliant(Map<String,String> x) {
+    return "Hello World";
+  }
+
+  @GetMapping("/id-{id:.+}")
+  public String getCrazyPath(@PathVariable String id) { // compliant
+    return "Hello World";
+  } // compliant
+
+  @GetMapping("/id-{id:.+}") // Noncompliant
+  public String getCrazyPathNonCompliant(String id) {
+    return "Hello World";
+  }
+
+  @GetMapping("/{id}/{xxx${placeHolder}xxxx}/{${{placeHolder}}}")
+  public String getPlaceHolder(String id) { // compliant, we don't consider this case
+    return "Hello World";
+  }
+
+}

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

@@ -0,0 +1,155 @@
+/*
+ * 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;
+
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Predicate;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.sonar.check.Rule;
+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.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 {
+  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 List<String> MAPPING_ANNOTATIONS = List.of(
+    "org.springframework.web.bind.annotation.GetMapping",
+    "org.springframework.web.bind.annotation.PostMapping",
+    "org.springframework.web.bind.annotation.PutMapping",
+    "org.springframework.web.bind.annotation.DeleteMapping");
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.METHOD);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    MethodTree method = (MethodTree) tree;
+
+    MAPPING_ANNOTATIONS
+      .forEach(annotation -> reportIssueOnParameters(method, annotation));
+  }
+
+  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;
+      });
+
+    if (containsMap) {
+      /*
+       * 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))
+      .flatMap(Optional::stream)
+      .collect(Collectors.toSet());
+
+    extractPathArgumentFromMappingAnnotations(method, annotation)
+      .map(path -> extractPathVariables(path))
+      .map(pathVariables -> {
+        pathVariables.removeAll(pathVariablesNames);
+        return pathVariables;
+      })
+      .filter(pathVariables -> !pathVariables.isEmpty())
+      .forEach(pathVariables -> reportIssue(
+        annotation(method, annotation),
+        "Bind path variable \"" + String.join("\", \"", pathVariables) + "\" to a method parameter."));
+  }
+
+  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);
+  }
+
+  private static Set<String> extractPathVariables(String path) {
+    if (CONTAINS_PLACEHOLDER.test(path)) {
+      return new HashSet<>();
+    }
+
+    return Stream.of(path.split("\\{"))
+      .map(EXTRACT_PATH_VARIABLE::matcher)
+      .filter(Matcher::matches)
+      .map(matcher -> matcher.group(1))
+      .collect(Collectors.toSet());
+  }
+
+  private static Optional<String> pathVariableName(VariableTree parameter) {
+    SymbolMetadata metadata = parameter.symbol().metadata();
+
+    return Optional.ofNullable(metadata.valuesForAnnotation(PATH_VARIABLE_ANNOTATION)).flatMap(arguments -> {
+      Map<String, Object> nameToValue = arguments.stream().collect(
+        Collectors.toMap(SymbolMetadata.AnnotationValue::name, SymbolMetadata.AnnotationValue::value));
+
+      return Optional.ofNullable((String) nameToValue.get("value"))
+        .or(() -> Optional.ofNullable((String) nameToValue.get("name")))
+        .or(() -> Optional.of(parameter.simpleName().name()));
+    });
+
+  }
+
+  private static Stream<String> extractPathArgumentFromMappingAnnotations(MethodTree method, String annotation) {
+    SymbolMetadata metadata = method.symbol().metadata();
+    return Optional.ofNullable(metadata.valuesForAnnotation(annotation)).flatMap(arguments -> {
+      Map<String, Object> nameToValue = arguments.stream().collect(
+        Collectors.toMap(SymbolMetadata.AnnotationValue::name, SymbolMetadata.AnnotationValue::value));
+
+      return arrayOrString(nameToValue.get("path"))
+        .or(() -> arrayOrString(nameToValue.get("value")));
+    }).orElseGet(Stream::empty);
+  }
+
+  private static Optional<Stream<String>> arrayOrString(Object value) {
+    if (value == null) {
+      return Optional.empty();
+    }
+
+    Object[] array = (Object[]) value;
+    return Optional.of(Stream.of(array)
+      .map(x -> (String) x));
+  }
+
+}

java-checks/src/test/java/org/sonar/java/checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest.java

@@ -0,0 +1,36 @@
+/*
+ * 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;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.java.checks.verifier.CheckVerifier;
+import org.sonar.java.checks.verifier.TestUtils;
+
+class PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckTest {
+
+  @Test
+  void test() {
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheckSample.java"))
+      .withCheck(new PathVariableAnnotationShouldBePresentIfPathVariableIsUsedCheck())
+      .verifyIssues();
+  }
+
+}