From d7ee801f2c61d5be547056280662eea4e5188d3f Mon Sep 17 00:00:00 2001 From: Dorian Burihabwa Date: Fri, 20 Oct 2023 10:33:26 +0200 Subject: [PATCH 1/2] SONARJAVA-4653 Prevent FPs when `@RequestParam` provides a default value --- .../OptionalRestParametersShouldBeObjects.java | 5 +++++ ...onalRestParametersShouldBeObjectsCheck.java | 18 ++++++++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java b/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java index fe67c849265..70b1715bf19 100644 --- a/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java +++ b/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java @@ -56,6 +56,11 @@ public Article getArticleButSettingADifferentValue(@PathVariable(name = "article return new Article(articleId); } + @GetMapping(value = "{/article/id") + public Article getArticleRequestParamWithDefaultValue(@RequestParam(required = false, defaultValue = "1") int articleId) { // Compliant + return new Article(articleId); + } + @GetMapping(value = {"/article/{id}"}) public Article getArticleButDifferentlyAnnotated(@Nullable int articleId) { // Compliant return new Article(articleId); diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java index 278c500f130..dc5bea76ba8 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java @@ -31,10 +31,11 @@ @Rule(key = "S6814") public class OptionalRestParametersShouldBeObjectsCheck extends IssuableSubscriptionVisitor { - + private static final String PATH_VARIABLE_ANNOTATION = "org.springframework.web.bind.annotation.PathVariable"; + private static final String REQUEST_PARAM_ANNOTATION = "org.springframework.web.bind.annotation.RequestParam"; private static final List PARAMETER_ANNOTATIONS = List.of( - "org.springframework.web.bind.annotation.PathVariable", - "org.springframework.web.bind.annotation.RequestParam" + PATH_VARIABLE_ANNOTATION, + REQUEST_PARAM_ANNOTATION ); @Override @@ -53,7 +54,7 @@ public void visitNode(Tree tree) { private static boolean isOptionalPrimitive(VariableTree parameter) { return parameter.type().symbolType().isPrimitive() && parameter.modifiers().annotations().stream() - .anyMatch(OptionalRestParametersShouldBeObjectsCheck::isMarkingAsOptional); + .anyMatch(annotation -> isMarkingAsOptional(annotation) && !hasDefaultValue(annotation)); } private static boolean isMarkingAsOptional(AnnotationTree annotation) { @@ -66,4 +67,13 @@ private static boolean isMarkingAsOptional(AnnotationTree annotation) { return "required".equals(variable.name()) && Boolean.FALSE.equals(constant); }); } + + private static boolean hasDefaultValue(AnnotationTree annotation) { + return annotation.annotationType().symbolType().is(REQUEST_PARAM_ANNOTATION) && + annotation.arguments().stream().anyMatch(expressionTree -> { + AssignmentExpressionTree assignment = (AssignmentExpressionTree) expressionTree; + IdentifierTree variable = (IdentifierTree) assignment.variable(); + return "defaultValue".equals(variable.name()); + }); + } } From 9d88b881feb1f3eaa36fbbc231cfa99395d5bb13 Mon Sep 17 00:00:00 2001 From: Dorian Burihabwa Date: Fri, 20 Oct 2023 10:47:17 +0200 Subject: [PATCH 2/2] SONARJAVA-4653 S6814: Prevent crashes on annotation parameter casting --- .../OptionalRestParametersShouldBeObjects.java | 10 ++++++++++ ...OptionalRestParametersShouldBeObjectsCheck.java | 14 +++++++++----- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java b/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java index 70b1715bf19..9e92b8aa737 100644 --- a/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java +++ b/java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java @@ -66,6 +66,16 @@ public Article getArticleButDifferentlyAnnotated(@Nullable int articleId) { // C return new Article(articleId); } + @GetMapping(value = {"/article/{id}"}) + public Article getArticlePathVariableWithValue(@PathVariable("id") int articleId) { // Compliant + return new Article(articleId); + } + + @GetMapping(value = {"/article/{id}"}) + public Article getArticleRequestParamWithValue(@RequestParam("id") int articleId) { // Compliant + return new Article(articleId); + } + record Article(int id) { } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java index dc5bea76ba8..048d2477cdd 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheck.java @@ -20,6 +20,7 @@ package org.sonar.java.checks.spring; import java.util.List; +import java.util.stream.Stream; import org.sonar.check.Rule; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.tree.AnnotationTree; @@ -59,9 +60,7 @@ private static boolean isOptionalPrimitive(VariableTree parameter) { private static boolean isMarkingAsOptional(AnnotationTree annotation) { return PARAMETER_ANNOTATIONS.stream().anyMatch(candidate -> annotation.annotationType().symbolType().is(candidate)) && - annotation.arguments().stream().anyMatch(expressionTree -> { - // Because all parameters of the supported annotations are assignments, we can cast safely here. - AssignmentExpressionTree assignment = (AssignmentExpressionTree) expressionTree; + streamAllNamedArguments(annotation).anyMatch(assignment -> { IdentifierTree variable = (IdentifierTree) assignment.variable(); Boolean constant = assignment.expression().asConstant(Boolean.class).orElse(Boolean.TRUE); return "required".equals(variable.name()) && Boolean.FALSE.equals(constant); @@ -70,10 +69,15 @@ private static boolean isMarkingAsOptional(AnnotationTree annotation) { private static boolean hasDefaultValue(AnnotationTree annotation) { return annotation.annotationType().symbolType().is(REQUEST_PARAM_ANNOTATION) && - annotation.arguments().stream().anyMatch(expressionTree -> { - AssignmentExpressionTree assignment = (AssignmentExpressionTree) expressionTree; + streamAllNamedArguments(annotation).anyMatch(assignment -> { IdentifierTree variable = (IdentifierTree) assignment.variable(); return "defaultValue".equals(variable.name()); }); } + + private static Stream streamAllNamedArguments(AnnotationTree annotation) { + return annotation.arguments().stream() + .filter(expression -> expression.is(Tree.Kind.ASSIGNMENT)) + .map(AssignmentExpressionTree.class::cast); + } }