diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6856.json index 0e4003d577c..39ef299c909 100644 --- 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": 59, + "falseNegatives": 61, "falsePositives": 0 } diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java index 667b85aef90..2562d81aaa5 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java +++ b/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,15 @@ 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 + public String getReport(ReportRecord record) { + return "reportDetails"; + } + } } diff --git a/java-checks-test-sources/spring-3.2/src/main/java/checks/ExtractRecordPropertiesTestData.java b/java-checks-test-sources/spring-3.2/src/main/java/checks/ExtractRecordPropertiesTestData.java new file mode 100644 index 00000000000..3691b85e8ad --- /dev/null +++ b/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 + ) { + } +} diff --git a/java-checks-test-sources/spring-3.2/src/main/java/checks/MissingPathVariableAnnotationCheck_classAndRecord.java b/java-checks-test-sources/spring-3.2/src/main/java/checks/MissingPathVariableAnnotationCheck_classAndRecord.java new file mode 100644 index 00000000000..502b97da5ba --- /dev/null +++ b/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(); + } + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java index 0aff085b81d..76d8833f8c9 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheck.java +++ b/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 nodesToVisit() { return List.of(Tree.Kind.CLASS); @@ -191,14 +199,14 @@ private void checkParametersAndPathTemplate(MethodTree method, Set 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 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 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 extractClassAndRecordProperties(MethodTree method) { Set 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,33 @@ private static Set checkForLombokSetters(Symbol.TypeSymbol typeSymbol) { return properties; } + @VisibleForTesting + static Set extractRecordProperties(Type type) { + Set 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 +518,23 @@ private static String substringToCurrentChar(int start) { } } + + @Override + public boolean isCompatibleWithDependencies(Function> dependencyFinder) { + Optional 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; + } } diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java index 60d444a6798..ad6bc0347b2 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/MissingPathVariableAnnotationCheckTest.java @@ -36,14 +36,18 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPathInModule; +import static org.sonar.java.test.classpath.TestClasspathUtils.SPRING_32_MODULE; class MissingPathVariableAnnotationCheckTest { private static final String TEST_SOURCE_FILE = mainCodeSourcesPath("checks/spring/s6856/MissingPathVariableAnnotationCheck_PathVariable.java"); private static final String TEST_SOURCE_FILE_MODEL_ATTRIBUTE = mainCodeSourcesPath("checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java"); private static final String SETTER_PROPERTIES_TEST_FILE = mainCodeSourcesPath("checks/spring/s6856/ExtractSetterPropertiesTestData.java"); + private static final String RECORD_COMPONENTS_TEST_FILE = mainCodeSourcesPathInModule(SPRING_32_MODULE, "checks/ExtractRecordPropertiesTestData.java"); private static final JavaFileScanner check = new MissingPathVariableAnnotationCheck(); private static final Map testTypes = new HashMap<>(); + private static final Map testRecords = new HashMap<>(); @BeforeAll static void scanTestFile() { @@ -67,6 +71,29 @@ public void visitNode(Tree tree) { .verifyNoIssues(); } + @BeforeAll + static void scanRecordTestFile() { + IssuableSubscriptionVisitor recordCollector = new IssuableSubscriptionVisitor() { + @Override + public java.util.List nodesToVisit() { + return java.util.List.of(Tree.Kind.CLASS, Tree.Kind.RECORD); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + Symbol.TypeSymbol symbol = classTree.symbol(); + testRecords.put(symbol.name(), symbol.type()); + } + }; + + CheckVerifier.newVerifier() + .onFile(RECORD_COMPONENTS_TEST_FILE) + .withCheck(recordCollector) + .withClassPath(SPRING_32_MODULE.getClassPath()) + .verifyNoIssues(); + } + @Test void test_compiling() { CheckVerifier.newVerifier() @@ -83,6 +110,15 @@ void test_model_attribute() { .verifyIssues(); } + @Test + void test_classes_and_records() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPathInModule(SPRING_32_MODULE, "checks/MissingPathVariableAnnotationCheck_classAndRecord.java")) + .withCheck(check) + .withClassPath(SPRING_32_MODULE.getClassPath()) + .verifyIssues(); + } + @Test void test_without_semantic() { CheckVerifier.newVerifier() @@ -224,4 +260,38 @@ private static Stream provideExtractSetterPropertiesTestCases() { ); } + @ParameterizedTest(name = "[{index}] {0}") + @MethodSource("provideExtractRecordPropertiesTestCases") + void test_extractRecordProperties(String typeName, Set expectedComponents) { + Type type = testRecords.get(typeName); + Set components = MissingPathVariableAnnotationCheck.extractRecordProperties(type); + + if (!expectedComponents.isEmpty()) { + assertThat(components).containsExactlyInAnyOrderElementsOf(expectedComponents); + } else { + assertThat(components).isEmpty(); + } + } + + private static Stream provideExtractRecordPropertiesTestCases() { + return Stream.of( + Arguments.of( + "RecordWithComponents", + Set.of("project", "year", "month") + ), + Arguments.of( + "EmptyRecord", + Set.of() + ), + Arguments.of( + "RecordWithBindParam", + Set.of("order-name", "details") + ), + Arguments.of( + "RecordMixedBindParam", + Set.of("project-id", "name", "user-id") + ) + ); + } + } diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html index 72a66199423..8351dde38ea 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6856.html @@ -5,8 +5,14 @@

Why is this an issue?

module and are commonly used to define the routes for different HTTP operations in a RESTful API.

If a method has a path template containing a placeholder, like "/api/resource/{id}", and there’s no @PathVariable annotation on a method parameter to capture the id path variable, Spring will disregard the id variable.

-

This rule will raise an issue if a method has a path template with a placeholder, but no corresponding @PathVariable, or -vice-versa.

+

Path variables can also be automatically bound to class or record properties when used as method parameters (without @PathVariable). +For Spring Web versions prior to 5.3, the @ModelAttribute annotation is required on the parameter to enable binding to class properties. +Starting with Spring Web 5.3, this binding happens automatically without requiring @ModelAttribute. For classes, Spring binds path +variables to properties that have setter methods (including Lombok-generated setters). For records (Spring Web 5.3+), Spring binds path variables to +record components through their accessor methods. The @BindParam annotation can be used on record components to specify custom binding +names.

+

This rule will raise an issue if a method has a path template with a placeholder, but no corresponding @PathVariable, matching class +property, or matching record component, or vice-versa.

How to fix it

Code examples

Noncompliant code example

@@ -33,12 +39,92 @@

Compliant solution

return ResponseEntity.ok("Fetching asset with ID: " + id); } +

Noncompliant code example

+
+// Spring Web < 5.3: Class properties require @ModelAttribute
+@Data
+class UserRequest {
+  private String userId;
+  private String accountId;
+}
+
+@GetMapping("/api/user/{userId}/{accountId}")
+public ResponseEntity<String> getUser(UserRequest request) { // Noncompliant - @ModelAttribute annotation is required for Spring Web < 5.3
+  return ResponseEntity.ok("User: " + request.getUserId());
+}
+
+

Compliant solution

+
+// Spring Web < 5.3: Class properties require @ModelAttribute
+@Data
+class UserRequest {
+  private String userId;
+  private String accountId;
+}
+
+@GetMapping("/api/user/{userId}/{accountId}")
+public ResponseEntity<String> getUser(@ModelAttribute UserRequest request) { // Compliant - @ModelAttribute enables binding to class properties
+  return ResponseEntity.ok("User: " + request.getUserId());
+}
+
+

Noncompliant code example

+
+// Spring Web 5.3+: Class properties
+@Data
+class UserRequest {
+  private String userId;
+  private String accountId;
+}
+
+@GetMapping("/api/user/{userId}/{companyId}")
+public ResponseEntity<String> getUser(UserRequest request) { // Noncompliant - 'companyId' path variable doesn't match any property with a setter
+  return ResponseEntity.ok("User: " + request.getUserId());
+}
+
+

Compliant solution

+
+// Spring Web 5.3+: Class properties
+@Data
+class UserRequest {
+  private String userId;
+  private String accountId;
+}
+
+@GetMapping("/api/user/{userId}/{accountId}")
+public ResponseEntity<String> getUser(UserRequest request) { // Compliant - path variables match properties with setters
+  return ResponseEntity.ok("User: " + request.getUserId());
+}
+
+

Noncompliant code example

+
+// Spring Web 5.3+: Record components
+record OrderRequest(String orderId, String customerId) {}
+
+@GetMapping("/api/order/{order-id}/{customer-id}")
+public ResponseEntity<String> getOrder(OrderRequest request) { // Noncompliant - Record component names don't match path variables
+  return ResponseEntity.ok("Order: " + request.orderId());
+}
+
+

Compliant solution

+
+// Spring Web 5.3+: Record components
+record OrderRequest(@BindParam("order-id") String orderId, @BindParam("customer-id") String customerId) {}
+
+@GetMapping("/api/order/{order-id}/{customer-id}")
+public ResponseEntity<String> getOrder(OrderRequest request) { // Compliant - @BindParam maps component names to path variables
+  return ResponseEntity.ok("Order: " + request.orderId());
+}
+

Resources

Documentation