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 40b90603eda..0e4003d577c 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": 47, + "falseNegatives": 59, "falsePositives": 0 } diff --git a/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java b/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java index f7d5cdbe0c2..e4cb27cfd16 100644 --- a/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java +++ b/java-checks-common/src/main/java/org/sonar/java/checks/helpers/ExpressionsHelper.java @@ -51,6 +51,16 @@ public class ExpressionsHelper { + private static final Set SIMPLE_TYPES = Set.of( + "java.lang.String", + "java.lang.Integer", + "java.lang.Long", + "java.lang.Double", + "java.lang.Float", + "java.lang.Boolean", + "java.util.Optional" + ); + private ExpressionsHelper() { } @@ -277,4 +287,13 @@ public static List getIdentifierAssignments(IdentifierTree ident return assignments; } + public static boolean isStandardDataType(Type type) { + if (type.isPrimitive()) { + return true; + } + + String fqn = type.fullyQualifiedName(); + return SIMPLE_TYPES.contains(fqn); + } + } diff --git a/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java b/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java index c2b1fed85e6..a92f21c85c7 100644 --- a/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java +++ b/java-checks-common/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java @@ -21,7 +21,9 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.CompilationUnitTree; import org.sonar.plugins.java.api.tree.ExpressionStatementTree; import org.sonar.plugins.java.api.tree.ExpressionTree; @@ -29,6 +31,7 @@ import org.sonar.plugins.java.api.tree.ImportTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.VariableTree; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; @@ -232,4 +235,35 @@ private static ExpressionTree getCallArgument(String code) { var methodInvocationTree = (MethodInvocationTree) exprStmtTree.expression(); return methodInvocationTree.arguments().get(0); } + + @ParameterizedTest + @ValueSource(strings = {"byte", "short", "int", "long", "float", "double", "boolean", "char", + "String", "Integer", "Long", "Double", "Float", "Boolean", "java.util.Optional"}) + void isStandardDataType_basicTypes(String type) { + String code = newCode( + "void f() {", + " " + type + " x;", + "}" + ); + MethodTree method = methodTree(code); + VariableTree variable = (VariableTree) method.block().body().get(0); + Type variableType = variable.type().symbolType(); + assertThat(ExpressionsHelper.isStandardDataType(variableType)).isTrue(); + } + + @Test + void isStandardDataType_customClass() { + String code = newCode( + "static class CustomClass {}", + "void f() {", + " CustomClass x;", + "}" + ); + // The method is at index 1 (after the CustomClass at index 0) + MethodTree method = (MethodTree) classTree(code).members().get(1); + VariableTree variable = (VariableTree) method.block().body().get(0); + Type type = variable.type().symbolType(); + assertThat(ExpressionsHelper.isStandardDataType(type)).isFalse(); + } + } diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/s6856/ExtractSetterPropertiesTestData.java b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/ExtractSetterPropertiesTestData.java new file mode 100644 index 00000000000..969e894f9f5 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/ExtractSetterPropertiesTestData.java @@ -0,0 +1,114 @@ +package checks.spring.s6856; + +import lombok.Data; +import lombok.Setter; + +class ExtractSetterPropertiesTestData { + + // Class with explicit setters + static class ExplicitSetters { + private String name; + private int age; + private boolean active; + + public void setName(String name) { + this.name = name; + } + + public void setAge(int age) { + this.age = age; + } + + public void setActive(boolean active) { + this.active = active; + } + + // Not a setter - has wrong return type + public String setInvalid(String value) { + return value; + } + + // Not a setter - has no parameters + public void setEmpty() { + } + + // Not a setter - has multiple parameters + public void setMultiple(String a, String b) { + } + + // Not a setter - is private + private void setPrivate(String value) { + } + + // Not a setter - is static + public static void setStatic(String value) { + } + } + + // Class with Lombok @Data (generates setters for all non-final fields) + @Data + static class LombokData { + private String project; + private int year; + private String month; + private final String constant = "CONST"; // Should be excluded (final) + private static String staticField; // Should be excluded (static) + } + + // Class with Lombok @Setter at class level + @Setter + static class LombokClassLevelSetter { + private String firstName; + private String lastName; + private final String id = "ID"; // Should be excluded (final) + private static int count; // Should be excluded (static) + } + + // Class with Lombok @Setter at field level + static class LombokFieldLevelSetter { + @Setter + private String email; + @Setter + private int score; + private String noSetter; // No @Setter, should be excluded + private static String staticField; // Should be excluded (static) + } + + // Mixed: explicit setters + Lombok field-level @Setter + static class MixedSetters { + @Setter + private String lombokField; + private String explicitField; + + public void setExplicitField(String value) { + this.explicitField = value; + } + } + + // Class with no setters + static class NoSetters { + private String field; + + public String getField() { + return field; + } + } + + // Empty class + static class EmptyClass { + } + + // Class with only getters + static class OnlyGetters { + private String name; + private int age; + + public String getName() { + return name; + } + + public int getAge() { + return age; + } + } +} 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 new file mode 100644 index 00000000000..667b85aef90 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_ModelAttribute.java @@ -0,0 +1,213 @@ +package checks.spring.s6856; + +import java.util.Map; +import java.util.Optional; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.ModelAttribute; +import org.springframework.web.bind.annotation.PathVariable; + +public class MissingPathVariableAnnotationCheck_ModelAttribute { + + class ParentController { + @ModelAttribute("viewCfg") + 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){ + 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){ + return ""; + } + } + + static class ModelA { + @ModelAttribute("user") + public String getUser(@PathVariable String id, @PathVariable String name) { // always compliant when method annotated with @ModelAttribute + return "user"; // because the case is too complex to handle + } + + @ModelAttribute("empty") + public String emptyModel(String notPathVariable){ + return ""; + } + + @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 template 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"; + } + } + + 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; + } + } + + static class ModelAttributeBindToClass { + @GetMapping("/reports/{project}/{year}/{month}") + public String getReport(@ModelAttribute ReportPeriod period) { + // Spring sees {project} in the URL and calls period.setProject() + // Spring sees {year} in the URL and calls period.setYear() + return "reportDetails"; + } + } + + // Test case: Parameter WITHOUT @ModelAttribute annotation should NOT extract properties + static class WithoutModelAttributeAnnotation { + @GetMapping("/api/{id}/{name}") // Noncompliant {{Bind template variable "name", "id" to a method parameter.}} + public String process(ReportPeriod period) { + return "result"; + } + } + + // Test case: @ModelAttribute with STANDARD DATA TYPES should be skipped (no property extraction) + static class ModelAttributeWithStandardDataTypes { + @GetMapping("/string/{value}") // Noncompliant {{Bind template variable "value" to a method parameter.}} + public String processString(@ModelAttribute String value) { + return "result"; + } + + @GetMapping("/int/{count}") // Noncompliant {{Bind template variable "count" to a method parameter.}} + public String processInt(@ModelAttribute int count) { + return "result"; + } + + @GetMapping("/integer/{num}") // Noncompliant {{Bind template variable "num" to a method parameter.}} + public String processInteger(@ModelAttribute Integer num) { + return "result"; + } + + @GetMapping("/long/{id}") // Noncompliant {{Bind template variable "id" to a method parameter.}} + public String processLong(@ModelAttribute Long id) { + return "result"; + } + + @GetMapping("/double/{price}") // Noncompliant {{Bind template variable "price" to a method parameter.}} + public String processDouble(@ModelAttribute Double price) { + return "result"; + } + + @GetMapping("/float/{value}") // Noncompliant {{Bind template variable "value" to a method parameter.}} + public String processFloat(@ModelAttribute Float value) { + return "result"; + } + + @GetMapping("/boolean/{flag}") // Noncompliant {{Bind template variable "flag" to a method parameter.}} + public String processBoolean(@ModelAttribute Boolean flag) { + return "result"; + } + + @GetMapping("/optional/{id}") // Noncompliant {{Bind template variable "id" to a method parameter.}} + public String processOptional(@ModelAttribute Optional id) { + return "result"; + } + + @GetMapping("/map/{key}") // Noncompliant {{Bind template variable "key" to a method parameter.}} + public String processMap(@ModelAttribute Map params) { + // Map is a standard data type - no property extraction + // Note: @ModelAttribute Map is different from @PathVariable Map + // @PathVariable Map captures all path variables, but @ModelAttribute Map does not + return "result"; + } + } + + // Test case: Mixed scenario - complex type with standard type parameters + static class MixedParameterTypes { + @GetMapping("/data/{id}/{name}/{age}") // Noncompliant {{Bind template variable "name", "id", "age" to a method parameter.}} + public String process( + @ModelAttribute ReportPeriod period, // Complex type - extracts project, year, month + String regularParam // Not @ModelAttribute - ignored + ) { + return "result"; + } + + @GetMapping("/user/{project}/{year}") // Compliant + public String processPartial( + @ModelAttribute ReportPeriod period, // Extracts project, year, month + @PathVariable String year // Explicitly bound + ) { + return "result"; + } + } + + // Test case: Multiple @ModelAttribute parameters + static class MultipleModelAttributes { + @GetMapping("/multi/{project}/{id}") // Noncompliant {{Bind template variable "id" to a method parameter.}} + public String process( + @ModelAttribute ReportPeriod period, // Extracts project, year, month + @ModelAttribute String name // Standard type - no extraction + ) { + return "result"; + } + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/MissingPathVariableAnnotationCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_PathVariable.java similarity index 78% rename from java-checks-test-sources/default/src/main/java/checks/spring/MissingPathVariableAnnotationCheckSample.java rename to java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_PathVariable.java index 61dd7162266..1c391e64899 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/MissingPathVariableAnnotationCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableAnnotationCheck_PathVariable.java @@ -1,37 +1,16 @@ -package checks.spring; +package checks.spring.s6856; 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.ModelAttribute; import org.springframework.web.bind.annotation.PatchMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.PutMapping; import org.springframework.web.bind.annotation.RequestMapping; -public class MissingPathVariableAnnotationCheckSample { - - class ParentController { - @ModelAttribute("viewCfg") - 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){ - 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){ - return ""; - } - } +public class MissingPathVariableAnnotationCheck_PathVariable { @GetMapping("/{name:[a-z-]+}-{version:\\d\\.\\d\\.\\d}{ext:\\.[a-z]+}") // Noncompliant public void handleWithoutExt(@PathVariable String name, @PathVariable String version) {} @@ -243,61 +222,6 @@ public String getPlaceHolder(@PathVariable String id, @PathVariable String a, @P return "Hello World"; } - static class ModelA { - @ModelAttribute("user") - public String getUser(@PathVariable String id, @PathVariable String name) { // always compliant when method annotated with @ModelAttribute - return "user"; // because the case is too complex to handle - } - - @ModelAttribute("empty") - public String emptyModel(String notPathVariable){ - return ""; - } - - @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 template 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"; - } - } - @GetMapping("/a/path") public String pathVariableWithoutParameter(@PathVariable String aVar){ // Noncompliant {{Bind method parameter "aVar" to a template variable.}} diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/MissingPathVariableParentInDifferentSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableParentInDifferentSample.java similarity index 91% rename from java-checks-test-sources/default/src/main/java/checks/spring/MissingPathVariableParentInDifferentSample.java rename to java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableParentInDifferentSample.java index bb58814a5fb..213b55807f8 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/MissingPathVariableParentInDifferentSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/spring/s6856/MissingPathVariableParentInDifferentSample.java @@ -1,4 +1,4 @@ -package checks.spring; +package checks.spring.s6856; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.annotation.PathVariable; diff --git a/java-checks/src/main/java/org/sonar/java/checks/GettersSettersOnRightFieldCheck.java b/java-checks/src/main/java/org/sonar/java/checks/GettersSettersOnRightFieldCheck.java index 959225256c0..9b8e6b44b80 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/GettersSettersOnRightFieldCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/GettersSettersOnRightFieldCheck.java @@ -33,6 +33,9 @@ import org.sonar.plugins.java.api.tree.StatementTree; import org.sonar.plugins.java.api.tree.Tree; +import static org.sonar.java.checks.helpers.MethodTreeUtils.isGetterLike; +import static org.sonar.java.checks.helpers.MethodTreeUtils.isSetterLike; + @Rule(key = "S4275") public class GettersSettersOnRightFieldCheck extends IssuableSubscriptionVisitor { @@ -48,39 +51,6 @@ public void visitNode(Tree tree) { isSetterLike(methodTree.symbol()).ifPresent(fieldName -> checkSetter(fieldName, methodTree)); } - private static Optional isGetterLike(Symbol.MethodSymbol methodSymbol) { - if (!methodSymbol.parameterTypes().isEmpty() || isPrivateStaticOrAbstract(methodSymbol)) { - return Optional.empty(); - } - String methodName = methodSymbol.name(); - if (methodName.length() > 3 && methodName.startsWith("get")) { - return Optional.of(lowerCaseFirstLetter(methodName.substring(3))); - } - if (methodName.length() > 2 && methodName.startsWith("is")) { - return Optional.of(lowerCaseFirstLetter(methodName.substring(2))); - } - return Optional.empty(); - } - - private static Optional isSetterLike(Symbol.MethodSymbol methodSymbol) { - if (methodSymbol.parameterTypes().size() != 1 || isPrivateStaticOrAbstract(methodSymbol)) { - return Optional.empty(); - } - String methodName = methodSymbol.name(); - if (methodName.length() > 3 && methodName.startsWith("set") && methodSymbol.returnType().type().isVoid()) { - return Optional.of(lowerCaseFirstLetter(methodName.substring(3))); - } - return Optional.empty(); - } - - private static boolean isPrivateStaticOrAbstract(Symbol.MethodSymbol methodSymbol) { - return methodSymbol.isPrivate() || methodSymbol.isStatic() || methodSymbol.isAbstract(); - } - - private static String lowerCaseFirstLetter(String methodName) { - return Character.toLowerCase(methodName.charAt(0)) + methodName.substring(1); - } - private void checkGetter(String fieldName, MethodTree methodTree) { Symbol.TypeSymbol getterOwner = ((Symbol.TypeSymbol) methodTree.symbol().owner()); if (hasNoPrivateFieldMatchingNameAndType(fieldName, methodTree.symbol().returnType().type(), getterOwner)) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java index 50c9e682241..f095860ca12 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java @@ -259,4 +259,37 @@ public void visitLambdaExpression(LambdaExpressionTree lambdaExpressionTree) { } } + public static Optional isGetterLike(Symbol.MethodSymbol methodSymbol) { + if (!methodSymbol.parameterTypes().isEmpty() || isPrivateStaticOrAbstract(methodSymbol)) { + return Optional.empty(); + } + String methodName = methodSymbol.name(); + if (methodName.length() > 3 && methodName.startsWith("get")) { + return Optional.of(lowerCaseFirstLetter(methodName.substring(3))); + } + if (methodName.length() > 2 && methodName.startsWith("is")) { + return Optional.of(lowerCaseFirstLetter(methodName.substring(2))); + } + return Optional.empty(); + } + + public static Optional isSetterLike(Symbol.MethodSymbol methodSymbol) { + if (methodSymbol.parameterTypes().size() != 1 || isPrivateStaticOrAbstract(methodSymbol)) { + return Optional.empty(); + } + String methodName = methodSymbol.name(); + if (methodName.length() > 3 && methodName.startsWith("set") && methodSymbol.returnType().type().isVoid()) { + return Optional.of(lowerCaseFirstLetter(methodName.substring(3))); + } + return Optional.empty(); + } + + private static boolean isPrivateStaticOrAbstract(Symbol.MethodSymbol methodSymbol) { + return methodSymbol.isPrivate() || methodSymbol.isStatic() || methodSymbol.isAbstract(); + } + + private static String lowerCaseFirstLetter(String methodName) { + return Character.toLowerCase(methodName.charAt(0)) + methodName.substring(1); + } + } 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 dbee3cbb5d2..0aff085b81d 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 @@ -26,7 +26,9 @@ 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.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.SymbolMetadata; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AnnotationTree; @@ -35,6 +37,9 @@ import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; +import static org.sonar.java.checks.helpers.ExpressionsHelper.isStandardDataType; +import static org.sonar.java.checks.helpers.MethodTreeUtils.isSetterLike; + @Rule(key = "S6856") public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisitor { private static final String PATH_VARIABLE_ANNOTATION = "org.springframework.web.bind.annotation.PathVariable"; @@ -51,6 +56,10 @@ public class MissingPathVariableAnnotationCheck extends IssuableSubscriptionVisi "org.springframework.web.bind.annotation.DeleteMapping", "org.springframework.web.bind.annotation.PatchMapping"); + private static final Set LOMBOK_SETTER_ANNOTATIONS = Set.of( + "lombok.Data", + "lombok.Setter"); + @Override public List nodesToVisit() { return List.of(Tree.Kind.CLASS); @@ -163,7 +172,7 @@ private void checkParametersAndPathTemplate(MethodTree method, Set model templateVariables.add(new UriInfo<>(ann, templateVariablesFromMapping(values))); } - // we handle the case where a path variable doesn't match to an uri parameter (/{aParam}/) + // we handle the case where a path variable doesn't match to uri parameter (/{aParam}/) Set allTemplateVariables = templateVariables.stream() .flatMap(uri -> uri.value().stream()) .collect(Collectors.toSet()); @@ -182,10 +191,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 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)); templateVariables.stream() .filter(uri -> !allPathVariables.containsAll(uri.value())) @@ -265,6 +278,73 @@ private static String removePropertyPlaceholder(String path){ return path.replaceAll(PROPERTY_PLACEHOLDER_PATTERN, ""); } + private static Set extractModelAttributeClassProperties(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)) { + continue; + } + + // Extract setter properties from the class + properties.addAll(extractSetterProperties(parameterType)); + } + + return properties; + } + + @VisibleForTesting + static Set extractSetterProperties(Type type) { + Symbol.TypeSymbol typeSymbol = type.symbol(); + + // Extract properties from Lombok-generated setters + Set properties = new HashSet<>(checkForLombokSetters(typeSymbol)); + + // Extract properties from explicit setter methods + for (Symbol member : typeSymbol.memberSymbols()) { + if (!member.isMethodSymbol()) { + continue; + } + + Symbol.MethodSymbol method = (Symbol.MethodSymbol) member; + + // Check if it's a setter and extract a property name + isSetterLike(method).ifPresent(properties::add); + } + + return properties; + } + + private static Set checkForLombokSetters(Symbol.TypeSymbol typeSymbol) { + Set properties = new HashSet<>(); + + // Check if the class has Lombok annotations that generate setters + boolean hasLombokSetters = typeSymbol.metadata().annotations().stream() + .anyMatch(annotation -> LOMBOK_SETTER_ANNOTATIONS.contains(annotation.symbol().type().fullyQualifiedName())); + + // Extract properties from fields if Lombok generates setters + for (Symbol.VariableSymbol field : typeSymbol.memberSymbols().stream().filter(Symbol::isVariableSymbol).map(Symbol.VariableSymbol.class::cast).toList()) { + if (field.isStatic() || field.isFinal()) { + continue; + } + + // Check if field has @Setter annotation at field level + boolean hasFieldLevelSetter = field.metadata().annotations().stream() + .anyMatch(annotation -> "lombok.Setter".equals(annotation.symbol().type().fullyQualifiedName())); + + // Add property if class-level or field-level Lombok setter exists + if (hasLombokSetters || hasFieldLevelSetter) { + properties.add(field.name()); + } + } + + return properties; + } + static class PathPatternParser { private PathPatternParser() { } diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java deleted file mode 100644 index fbac1c4fc0d..00000000000 --- a/java-checks/src/test/java/org/sonar/java/checks/helpers/ExpressionsHelperTest.java +++ /dev/null @@ -1,132 +0,0 @@ -/* - * SonarQube Java - * Copyright (C) 2012-2025 SonarSource Sàrl - * mailto:info AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA. - * - * 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 Sonar Source-Available License for more details. - * - * You should have received a copy of the Sonar Source-Available License - * along with this program; if not, see https://sonarsource.com/license/ssal/ - */ -package org.sonar.java.checks.helpers; - -import java.lang.reflect.Constructor; -import javax.annotation.Nullable; -import org.junit.jupiter.api.Test; -import org.sonar.plugins.java.api.tree.IdentifierTree; -import org.sonar.plugins.java.api.tree.MethodTree; - -import static org.assertj.core.api.Assertions.assertThat; - -class ExpressionsHelperTest extends JParserTestUtils { - - @Test - void private_constructor() throws Exception { - Constructor constructor = ExpressionsHelper.class.getDeclaredConstructor(); - assertThat(constructor.isAccessible()).isFalse(); - constructor.setAccessible(true); - constructor.newInstance(); - } - - @Test - void simpleAssignment() { - String code = newCode( "int foo() {", - "boolean a;", - "a = true;", - "return a;", - "}"); - assertValueResolution(code, true); - } - - @Test - void initializerAndAssignment() { - String code = newCode( "int foo() {", - "boolean a = false;", - "a = true;", - "return a;", - "}"); - assertValueResolution(code, null); - } - - @Test - void simpleInitializer() { - String code = newCode( "int foo() {", - "boolean a = true;", - "return a;", - "}"); - assertValueResolution(code, true); - } - - @Test - void andAssignment() { - String code = newCode( "int foo() {", - "boolean a;", - "a &= false;", - "return a;", - "}"); - assertValueResolution(code, null); - } - - @Test - void selfAssigned() { - String code = newCode( "int foo() {", - "boolean a = a;", - "return a;", - "}"); - assertValueResolution(code, null); - } - - @Test - void unknownValue() { - String code = newCode( "int foo(boolean a) {", - "return a;", - "}"); - assertValueResolution(code, null); - } - - @Test - void notAnIdentifier() { - String code = newCode( "int foo() {", - "boolean a = bar();", - "return a;", - "}", - "boolean bar() {", - "return true;", - "}"); - assertValueResolution(code, null); - } - - @Test - void moreThanOneAssignment() { - String code = newCode( "int foo() {", - "boolean a;", - "a = true;", - "a = false;", - "return a;", - "}"); - assertValueResolution(code, null); - } - - @Test - void variableSwapSOE() { - String code = newCode("String foo(String a, String b) {", - "String c = a;", - "a = b;", - "b = c;", - "return a;}"); - assertValueResolution(code, null); - } - - private void assertValueResolution(String code, @Nullable T target) { - MethodTree method = methodTree(code); - IdentifierTree a = variableFromLastReturnStatement(method.block().body()); - Boolean value = ExpressionsHelper.getConstantValueAsBoolean(a).value(); - assertThat(value).isEqualTo(target); - } -} 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 9bf584afef6..60d444a6798 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 @@ -16,17 +16,56 @@ */ package org.sonar.java.checks.spring; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; +import java.util.stream.Stream; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.sonar.java.checks.verifier.CheckVerifier; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScanner; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.Tree; 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; class MissingPathVariableAnnotationCheckTest { - private static final String TEST_SOURCE_FILE = mainCodeSourcesPath("checks/spring/MissingPathVariableAnnotationCheckSample.java"); + 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 JavaFileScanner check = new MissingPathVariableAnnotationCheck(); + private static final Map testTypes = new HashMap<>(); + + @BeforeAll + static void scanTestFile() { + IssuableSubscriptionVisitor typeCollector = new IssuableSubscriptionVisitor() { + @Override + public java.util.List nodesToVisit() { + return java.util.List.of(Tree.Kind.CLASS); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + Symbol.TypeSymbol symbol = classTree.symbol(); + testTypes.put(symbol.name(), symbol.type()); + } + }; + + CheckVerifier.newVerifier() + .onFile(SETTER_PROPERTIES_TEST_FILE) + .withCheck(typeCollector) + .verifyNoIssues(); + } @Test void test_compiling() { @@ -36,6 +75,14 @@ void test_compiling() { .verifyIssues(); } + @Test + void test_model_attribute() { + CheckVerifier.newVerifier() + .onFile(TEST_SOURCE_FILE_MODEL_ATTRIBUTE) + .withCheck(check) + .verifyIssues(); + } + @Test void test_without_semantic() { CheckVerifier.newVerifier() @@ -46,7 +93,7 @@ void test_without_semantic() { } @Test - void expr() { + void test_composed_request_mapping() { CheckVerifier.newVerifier() .onFile("src/test/files/checks/spring/SpringComposedRequestMappingCheck.java") .withCheck(check) @@ -115,4 +162,66 @@ void test_pattern_parser_errors() { .isInstanceOf(MissingPathVariableAnnotationCheck.DoNotReport.class); } + @ParameterizedTest(name = "[{index}] {0}") + @MethodSource("provideExtractSetterPropertiesTestCases") + void test_extractSetterProperties(String typeName, Set expectedProperties, Set unexpectedProperties) { + Type type = testTypes.get(typeName); + Set properties = MissingPathVariableAnnotationCheck.extractSetterProperties(type); + + if (!expectedProperties.isEmpty()) { + assertThat(properties).containsExactlyInAnyOrderElementsOf(expectedProperties); + } else { + assertThat(properties).isEmpty(); + } + + if (!unexpectedProperties.isEmpty()) { + assertThat(properties).doesNotContainAnyElementsOf(unexpectedProperties); + } + } + + private static Stream provideExtractSetterPropertiesTestCases() { + return Stream.of( + Arguments.of( + "ExplicitSetters", + Set.of("name", "age", "active"), + Set.of("invalid", "empty", "multiple", "private", "static") + ), + Arguments.of( + "LombokData", + Set.of("project", "year", "month"), + Set.of("constant", "staticField") + ), + Arguments.of( + "LombokClassLevelSetter", + Set.of("firstName", "lastName"), + Set.of("id", "count") + ), + Arguments.of( + "LombokFieldLevelSetter", + Set.of("email", "score"), + Set.of("noSetter", "staticField") + ), + Arguments.of( + "MixedSetters", + Set.of("lombokField", "explicitField"), + Set.of() + ), + Arguments.of( + "NoSetters", + Set.of(), + Set.of("field") + ), + Arguments.of( + "EmptyClass", + Set.of(), + Set.of() + ), + Arguments.of( + "OnlyGetters", + Set.of(), + Set.of("name", "age") + ) + ); + } + }