SONARJAVA-4653 S6814 Optional REST parameters should have an object type

its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java

@@ -180,15 +180,15 @@ public void javaCheckTestSources() throws Exception {
     softly.assertThat(newTotal).isEqualTo(knownTotal);
     softly.assertThat(rulesCausingFPs).hasSize(6);
     softly.assertThat(rulesNotReporting).hasSize(7);
-    softly.assertThat(rulesSilenced).hasSize(70);
+    softly.assertThat(rulesSilenced).hasSize(71);
 
     /**
      * 4. Check total number of differences (FPs + FNs)
      *
      * No differences would mean that we find the same issues with and without the bytecode and libraries
      */
     String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences"));
-    softly.assertThat(differences).isEqualTo("Issues differences: 3446");
+    softly.assertThat(differences).isEqualTo("Issues differences: 3451");
 
     softly.assertAll();
   }

its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json

@@ -2890,5 +2890,11 @@
     "hasTruePositives": true,
     "falseNegatives": 32,
     "falsePositives": 0
+  },
+  {
+    "ruleKey": "S6814",
+    "hasTruePositives": false,
+    "falseNegatives": 5,
+    "falsePositives": 0
   }
 ]

java-checks-test-sources/src/main/java/checks/spring/OptionalRestParametersShouldBeObjects.java

@@ -0,0 +1,66 @@
+package checks.spring;
+
+import com.mongodb.lang.Nullable;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.PathVariable;
+import org.springframework.web.bind.annotation.RequestParam;
+
+class OptionalRestParametersShouldBeObjects {
+
+  @GetMapping(value = {"/article", "/article/{id}"})
+  public Article getArticle(@PathVariable(required = false) int articleId, int unused) { // Noncompliant [[sc=29;ec=75]] {{Convert this optional parameter to an Object type.}}
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article", "/article/{id}"})
+  public Article getArticleWithRequestParam(@RequestParam(required = false) int articleId, int unused) { // Noncompliant [[sc=45;ec=91]] {{Convert this optional parameter to an Object type.}}
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article", "/article/{id}"})
+  public Article getArticle(
+    @PathVariable(required = false) float articleId, // Noncompliant [[sc=5;ec=53]]
+    @RequestParam(required = false) int someIssue, // Noncompliant [[sc=5;ec=51]]
+    @PathVariable(required = false) boolean anotherIssue // Noncompliant [[sc=5;ec=57]]
+  ) {
+    return new Article((int) (Math.floor(articleId)));
+  }
+
+  @GetMapping(value = {"/article", "/article/{id}"})
+  public Article getArticle(@PathVariable(required = false) Integer articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article/{id}"})
+  public Article getArticleButImplictlyRequired(@PathVariable int articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article/{id}"})
+  public Article getArticleRequestParamButImplictlyRequired(@RequestParam int articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article/{id}"})
+  public Article getArticleButExplicitlyRequired(@PathVariable(required = true) int articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article/{id}"})
+  public Article getArticleRequestParamButExplicitlyRequired(@RequestParam(required = true) int articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article/{id}"})
+  public Article getArticleButSettingADifferentValue(@PathVariable(name = "articleId") int articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  @GetMapping(value = {"/article/{id}"})
+  public Article getArticleButDifferentlyAnnotated(@Nullable int articleId) { // Compliant
+    return new Article(articleId);
+  }
+
+  record Article(int id) {
+  }
+}

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

@@ -139,6 +139,7 @@
 import org.sonar.java.checks.spring.AsyncMethodsReturnTypeCheck;
 import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck;
 import org.sonar.java.checks.spring.FieldDependencyInjectionCheck;
+import org.sonar.java.checks.spring.OptionalRestParametersShouldBeObjectsCheck;
 import org.sonar.java.checks.spring.PersistentEntityUsedAsRequestParameterCheck;
 import org.sonar.java.checks.spring.RequestMappingMethodPublicCheck;
 import org.sonar.java.checks.spring.SpringAntMatcherOrderCheck;
@@ -520,6 +521,7 @@ public final class CheckList {
     OneDeclarationPerLineCheck.class,
     OpenSAML2AuthenticationBypassCheck.class,
     OptionalAsParameterCheck.class,
+    OptionalRestParametersShouldBeObjectsCheck.class,
     OutputStreamOverrideWriteCheck.class,
     OverrideAnnotationCheck.class,
     OverwrittenKeyCheck.class,

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

@@ -0,0 +1,69 @@
+/*
+ * 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.spring;
+
+import java.util.List;
+import org.sonar.check.Rule;
+import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.tree.AnnotationTree;
+import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
+import org.sonar.plugins.java.api.tree.IdentifierTree;
+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 = "S6814")
+public class OptionalRestParametersShouldBeObjectsCheck extends IssuableSubscriptionVisitor {
+
+  private static final List<String> PARAMETER_ANNOTATIONS = List.of(
+    "org.springframework.web.bind.annotation.PathVariable",
+    "org.springframework.web.bind.annotation.RequestParam"
+  );
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.METHOD);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    MethodTree method = (MethodTree) tree;
+    method.parameters().stream()
+      .filter(OptionalRestParametersShouldBeObjectsCheck::isOptionalPrimitive)
+      .forEach(parameter -> reportIssue(parameter, "Convert this optional parameter to an Object type."));
+  }
+
+  private static boolean isOptionalPrimitive(VariableTree parameter) {
+    return parameter.type().symbolType().isPrimitive() &&
+      parameter.modifiers().annotations().stream()
+        .anyMatch(OptionalRestParametersShouldBeObjectsCheck::isMarkingAsOptional);
+  }
+
+  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;
+        IdentifierTree variable = (IdentifierTree) assignment.variable();
+        Boolean constant = assignment.expression().asConstant(Boolean.class).orElse(Boolean.TRUE);
+        return "required".equals(variable.name()) && Boolean.FALSE.equals(constant);
+      });
+  }
+}

java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6814.html

@@ -0,0 +1,56 @@
+<h2>Why is this an issue?</h2>
+<p>Spring provides two options to mark a REST parameter as optional:</p>
+<ol>
+  <li> Use <code>required = false</code> in the <code>@PathVariable</code> or <code>@RequestParam</code> annotation of the respective method parameter
+  or </li>
+  <li> Use type <code>java.util.Optional&lt;T&gt;</code> for the method parameter </li>
+</ol>
+<p>When using 1., the absence of the parameter, when the REST function is called, is encoded by <code>null</code>, which can only be used for object
+types. If <code>required = false</code> is used for a parameter with a primitive and the REST function is called without the parameter, a runtime
+exception occurs because the Spring data mapper cannot map the <code>null</code> value to the parameter.</p>
+<h2>How to fix it</h2>
+<p>Replace primitive types, such as <code>boolean</code>, <code>char</code>, <code>int</code>, with the corresponding wrapper type, such as
+<code>Boolean</code>, <code>Character</code>, <code>Integer</code>.</p>
+<p>Alternatively, you might choose to remove <code>required = false</code> from the annotation and use an <code>Optional&lt;T&gt;</code> type for the
+parameter, such as <code>Optional&lt;Boolean&gt;</code> or <code>Optional&lt;String&gt;</code>, which automatically makes the REST parameter optional.
+This is the preferred approach because it enforces the proper handling of <code>null</code> in the method implementation.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+@RequestMapping(value = {"/article", "/article/{id}"})
+public Article getArticle(@PathVariable(required = false) int articleId) { // Noncompliant, null cannot be mapped to int
+   //...
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+@RequestMapping(value = {"/article", "/article/{id}"})
+public Article getArticle(@PathVariable(required = false) Integer articleId) { // Compliant
+   //...
+}
+</pre>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="2" data-diff-type="noncompliant">
+@RequestMapping(value = {"/article", "/article/{id}"})
+public Article getArticle(@PathVariable(required = false) int articleId) { // Noncompliant, null cannot be mapped to int
+   //...
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="2" data-diff-type="compliant">
+@RequestMapping(value = {"/article", "/article/{id}"})
+public Article getArticle(@PathVariable Optional&lt;Integer&gt; articleId) { // Compliant and preferred approach
+   //...
+}
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> <a href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/bind/annotation/PathVariable.html">Spring
+  Framework API - Annotation Interface PathVariable</a> </li>
+</ul>
+<h3>Articles &amp; blog posts</h3>
+<ul>
+  <li> <a href="https://www.baeldung.com/spring-optional-path-variables">Baeldung - Spring Optional Path Variables</a> </li>
+</ul>
+

java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6814.json

@@ -0,0 +1,21 @@
+{
+  "title": "Optional REST parameters should have an object type",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6814",
+  "sqKey": "S6814",
+  "scope": "All",
+  "quickfix": "unknown",
+  "code": {
+    "impacts": {
+      "RELIABILITY": "HIGH"
+    },
+    "attribute": "CONVENTIONAL"
+  }
+}

java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json

@@ -482,6 +482,7 @@
     "S6548",
     "S6809",
     "S6810",
-    "S6813"
+    "S6813",
+    "S6814"
   ]
 }

java-checks/src/test/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheckTest.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.spring;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.java.checks.verifier.CheckVerifier;
+import org.sonar.java.checks.verifier.TestUtils;
+
+class OptionalRestParametersShouldBeObjectsCheckTest {
+
+  @Test
+  void test() {
+    CheckVerifier.newVerifier()
+      .onFile(TestUtils.mainCodeSourcesPath("checks/spring/OptionalRestParametersShouldBeObjects.java"))
+      .withCheck(new OptionalRestParametersShouldBeObjectsCheck())
+      .verifyIssues();
+  }
+
+}