SONARJAVA-5975 S6856: Add support for record component extraction

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

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

java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java

@@ -10,20 +10,22 @@ public class MissingPathVariableAnnotationCheck_ModelAttribute {
 
   class ParentController {
     @ModelAttribute("viewCfg")
-    public String getView(@PathVariable("view") final String view){
+    public String getView(@PathVariable("view") final String view) {
       return "";
     }
   }
+
   class ChildController extends ParentController {
     @GetMapping("/model/{view}") //Compliant, parent class defines 'view' path var in the model attribute
-    public String list(@ModelAttribute("viewCfg") final String viewConfig){
+    public String list(@ModelAttribute("viewCfg") final String viewConfig) {
       return "";
     }
   }
+
   class MissingParentChildController extends MissingPathVariableParentInDifferentSample {
     @GetMapping("/model/{view}") // Noncompliant
     // FP: parent class in different file, cannot collect the model attribute
-    public String list(@ModelAttribute("parentView") final String viewConfig){
+    public String list(@ModelAttribute("parentView") final String viewConfig) {
       return "";
     }
   }
@@ -35,7 +37,7 @@ public String getUser(@PathVariable String id, @PathVariable String name) { // a
     }
 
     @ModelAttribute("empty")
-    public String emptyModel(String notPathVariable){
+    public String emptyModel(String notPathVariable) {
       return "";
     }
 
@@ -210,4 +212,16 @@ public String process(
       return "result";
     }
   }
+
+  // Test case: Records: must be noncompliant for spring-web < 5.3
+  record ReportRecord(String project, int year, String month) {
+  }
+
+  static class RecordBinding {
+    @GetMapping("/reports/{project}/{year}/{month}") // Noncompliant
+    // Compliant
+    public String getReport(ReportRecord record) {
+      return "reportDetails";
+    }
+  }
 }

java-checks-test-sources/spring-3.2/src/main/java/checks/ExtractRecordPropertiesTestData.java

@@ -0,0 +1,25 @@
+package checks;
+
+import org.springframework.web.bind.annotation.BindParam;
+
+public class ExtractRecordPropertiesTestData {
+  // Record with components
+  record RecordWithComponents(String project, int year, String month) {
+  }
+
+  // Empty record
+  record EmptyRecord() {
+  }
+
+  // Record with @BindParam annotation
+  record RecordWithBindParam(@BindParam("order-name") String orderName, String details) {
+  }
+
+  // Record with mixed @BindParam and regular components
+  record RecordMixedBindParam(
+    @BindParam("project-id") String projectId,
+    String name,
+    @BindParam("user-id") String userId
+  ) {
+  }
+}

java-checks-test-sources/spring-3.2/src/main/java/checks/MissingPathVariableAnnotationCheck_classAndRecord.java

@@ -0,0 +1,70 @@
+package checks;
+
+import org.springframework.web.bind.annotation.BindParam;
+import org.springframework.web.bind.annotation.GetMapping;
+
+public class MissingPathVariableAnnotationCheck_classAndRecord {
+  static class ReportPeriod {
+    private String project;
+    private int year;
+    private String month;
+
+    public String getProject() {
+      return project;
+    }
+
+    public int getYear() {
+      return year;
+    }
+
+    public String getMonth() {
+      return month;
+    }
+
+    public void setProject(String project) {
+      this.project = project;
+    }
+
+    public void setYear(int year) {
+      this.year = year;
+    }
+
+    public void setMonth(String month) {
+      this.month = month;
+    }
+  }
+
+  record ReportPeriodRecord(String project, int year, String month) {
+  }
+
+  static class ReportPeriodBind {
+    @GetMapping("/reports/{project}/{year}/{month}")
+    public String getReport(ReportPeriod period) {
+      // Spring sees {project} in the URL and calls period.setProject()
+      // Spring sees {year} in the URL and calls period.setYear()
+      return "reportDetails";
+    }
+
+    @GetMapping("/reports/{project}/{year}/{month}")
+    public String getAnotherReport(ReportPeriodRecord period) {
+      // Spring sees {project} in the URL and calls period.project()
+      // Spring sees {year} in the URL and calls period.year()
+      return "reportDetails";
+    }
+
+    public record Order(@BindParam("order-name") String orderName, String details){}
+
+    @GetMapping("/{order-name}/details")
+    public String getOrderDetails(Order order){
+      // Spring sees {order-name} in the URL and calls order.orderName()
+      return order.details();
+    }
+
+    @GetMapping("/{orderName}/details") // Noncompliant {{Bind template variable "orderName" to a method parameter.}}
+    public String getOrderDetailsWrongParameterName(Order order){
+      // Spring sees {orderName} in the URL and can't find order's orderName because of the wrong binding
+      return order.details();
+    }
+  }
+
+}

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

@@ -21,13 +21,17 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
 import org.sonar.check.Rule;
 import org.sonar.java.annotations.VisibleForTesting;
+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.semantic.Symbol;
 import org.sonar.plugins.java.api.semantic.SymbolMetadata;
 import org.sonar.plugins.java.api.semantic.Type;
@@ -41,7 +45,7 @@
 import static org.sonar.java.checks.helpers.MethodTreeUtils.isSetterLike;
 
 @Rule(key = "S6856")
-public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor {
+public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware {
   private static final String PATH_VARIABLE_ANNOTATION = "org.springframework.web.bind.annotation.PathVariable";
   private static final String MAP = "java.util.Map";
   private static final String MODEL_ATTRIBUTE_ANNOTATION = "org.springframework.web.bind.annotation.ModelAttribute";
@@ -60,6 +64,10 @@ public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisi
     "lombok.Data",
     "lombok.Setter");
 
+  private static final String BIND_PARAM_ANNOTATION = "org.springframework.web.bind.annotation.BindParam";
+
+  private SpringWebVersion springWebVersion;
+
   @Override
   public List<Tree.Kind> nodesToVisit() {
     return List.of(Tree.Kind.CLASS);
@@ -191,14 +199,14 @@ private void checkParametersAndPathTemplate(MethodTree method, Set<String> model
       return;
     }
 
-    // finally, we handle the case where a uri parameter (/{aParam}/) doesn't match to path- or ModelAttribute- inherited variables
+    // finally, we handle the case where a uri parameter (/{aParam}/) doesn't match to path-, ModelAttribute-, or class / record inherited variables
     Set<String> allPathVariables = methodParameters.stream()
       .map(ParameterInfo::value)
       .collect(Collectors.toSet());
     // Add properties inherited from @ModelAttribute methods
     allPathVariables.addAll(modelAttributeMethodParameters);
-    // Add properties inherited from @ModelAttribute class parameters
-    allPathVariables.addAll(extractModelAttributeClassProperties(method));
+    // Add properties inherited from class and record parameters
+    allPathVariables.addAll(extractClassAndRecordProperties(method));
 
     templateVariables.stream()
       .filter(uri -> !allPathVariables.containsAll(uri.value()))
@@ -278,20 +286,29 @@ private static String removePropertyPlaceholder(String path){
     return path.replaceAll(PROPERTY_PLACEHOLDER_PATTERN, "");
   }
 
-  private static Set<String> extractModelAttributeClassProperties(MethodTree method) {
+  private boolean requiresModelAttributeAnnotation(SymbolMetadata metadata) {
+    // for spring-web < 5.3 we need to use ModelAttribute annotation to extract properties from classes / records
+    return springWebVersion == SpringWebVersion.LESS_THAN_5_3 && !metadata.isAnnotatedWith(MODEL_ATTRIBUTE_ANNOTATION);
+  }
+
+  private Set<String> extractClassAndRecordProperties(MethodTree method) {
     Set<String> properties = new HashSet<>();
 
     for (var parameter : method.parameters()) {
-      SymbolMetadata metadata = parameter.symbol().metadata();
       Type parameterType = parameter.type().symbolType();
-
-      if (!metadata.isAnnotatedWith(MODEL_ATTRIBUTE_ANNOTATION) || parameterType.isUnknown()
-        || isStandardDataType(parameterType) || parameterType.isSubtypeOf(MAP)) {
+      if (parameterType.isUnknown()
+        || isStandardDataType(parameterType) || parameterType.isSubtypeOf(MAP)
+        || requiresModelAttributeAnnotation(parameter.symbol().metadata())) {
         continue;
       }
 
-      // Extract setter properties from the class
-      properties.addAll(extractSetterProperties(parameterType));
+      if (parameterType.isSubtypeOf("java.lang.Record") && springWebVersion != SpringWebVersion.LESS_THAN_5_3) {
+        // Extract record's components
+        properties.addAll(extractRecordProperties(parameterType));
+      } else if (parameterType.isClass()) {
+        // Extract setter properties from the class
+        properties.addAll(extractSetterProperties(parameterType));
+      }
     }
 
     return properties;
@@ -345,6 +362,32 @@ private static Set<String> checkForLombokSetters(Symbol.TypeSymbol typeSymbol) {
     return properties;
   }
 
+  @VisibleForTesting
+  static Set<String> extractRecordProperties(Type type) {
+    Set<String> properties = new HashSet<>();
+    // For records, extract component names from the record components
+    // Records automatically generate accessor methods for their components
+    type.symbol().memberSymbols().stream()
+      .filter(Symbol::isVariableSymbol)
+      .map(Symbol.VariableSymbol.class::cast)
+      .filter(f -> !f.isStatic()).forEach(field -> properties.add(getComponentName(field)));
+
+    return properties;
+  }
+
+  private static String getComponentName(Symbol.VariableSymbol field) {
+    // Check if the component has @BindParam annotation for custom binding name
+    String componentName = field.name();
+    var bindParamValues = field.metadata().valuesForAnnotation(BIND_PARAM_ANNOTATION);
+    if (bindParamValues != null) {
+      Object value = bindParamValues.get(0).value();
+      if (value instanceof String bindParamName && !bindParamName.isEmpty()) {
+        componentName = bindParamName;
+      }
+    }
+    return componentName;
+  }
+
   static class PathPatternParser {
     private PathPatternParser() {
     }
@@ -474,4 +517,23 @@ private static String substringToCurrentChar(int start) {
     }
 
   }
+
+  @Override
+  public boolean isCompatibleWithDependencies(Function<String, Optional<Version>> dependencyFinder) {
+    Optional<Version> springWebCurrentVersion = dependencyFinder.apply("spring-web");
+    if (springWebCurrentVersion.isEmpty()) {
+      return false;
+    }
+    springWebVersion = getSpringWebVersion(springWebCurrentVersion.get());
+    return true;
+  }
+
+  private static SpringWebVersion getSpringWebVersion(Version springWebVersion) {
+    return  (springWebVersion.isLowerThan("5.3") ? SpringWebVersion.LESS_THAN_5_3 : SpringWebVersion.START_FROM_5_3);
+  }
+
+  private enum SpringWebVersion {
+    LESS_THAN_5_3,
+    START_FROM_5_3;
+  }
 }