Skip to content

Commit 4924e89

Browse files
dorian-burihabwa-sonarsourcedorian-burihabwa-sonarsource
authored
SONARJAVA-4653 S6814 Optional REST parameters should have an object type
1 parent f684fc7 commit 4924e89

9 files changed

Lines changed: 260 additions & 3 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,15 +180,15 @@ public void javaCheckTestSources() throws Exception {
180180
softly.assertThat(newTotal).isEqualTo(knownTotal);
181181
softly.assertThat(rulesCausingFPs).hasSize(6);
182182
softly.assertThat(rulesNotReporting).hasSize(7);
183-
softly.assertThat(rulesSilenced).hasSize(70);
183+
softly.assertThat(rulesSilenced).hasSize(71);
184184

185185
/**
186186
* 4. Check total number of differences (FPs + FNs)
187187
*
188188
* No differences would mean that we find the same issues with and without the bytecode and libraries
189189
*/
190190
String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences"));
191-
softly.assertThat(differences).isEqualTo("Issues differences: 3446");
191+
softly.assertThat(differences).isEqualTo("Issues differences: 3451");
192192

193193
softly.assertAll();
194194
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2890,5 +2890,11 @@
28902890
"hasTruePositives": true,
28912891
"falseNegatives": 32,
28922892
"falsePositives": 0
2893+
},
2894+
{
2895+
"ruleKey": "S6814",
2896+
"hasTruePositives": false,
2897+
"falseNegatives": 5,
2898+
"falsePositives": 0
28932899
}
28942900
]

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package checks.spring;
2+
3+
import com.mongodb.lang.Nullable;
4+
import org.springframework.web.bind.annotation.GetMapping;
5+
import org.springframework.web.bind.annotation.PathVariable;
6+
import org.springframework.web.bind.annotation.RequestParam;
7+
8+
class OptionalRestParametersShouldBeObjects {
9+
10+
@GetMapping(value = {"/article", "/article/{id}"})
11+
public Article getArticle(@PathVariable(required = false) int articleId, int unused) { // Noncompliant [[sc=29;ec=75]] {{Convert this optional parameter to an Object type.}}
12+
return new Article(articleId);
13+
}
14+
15+
@GetMapping(value = {"/article", "/article/{id}"})
16+
public Article getArticleWithRequestParam(@RequestParam(required = false) int articleId, int unused) { // Noncompliant [[sc=45;ec=91]] {{Convert this optional parameter to an Object type.}}
17+
return new Article(articleId);
18+
}
19+
20+
@GetMapping(value = {"/article", "/article/{id}"})
21+
public Article getArticle(
22+
@PathVariable(required = false) float articleId, // Noncompliant [[sc=5;ec=53]]
23+
@RequestParam(required = false) int someIssue, // Noncompliant [[sc=5;ec=51]]
24+
@PathVariable(required = false) boolean anotherIssue // Noncompliant [[sc=5;ec=57]]
25+
) {
26+
return new Article((int) (Math.floor(articleId)));
27+
}
28+
29+
@GetMapping(value = {"/article", "/article/{id}"})
30+
public Article getArticle(@PathVariable(required = false) Integer articleId) { // Compliant
31+
return new Article(articleId);
32+
}
33+
34+
@GetMapping(value = {"/article/{id}"})
35+
public Article getArticleButImplictlyRequired(@PathVariable int articleId) { // Compliant
36+
return new Article(articleId);
37+
}
38+
39+
@GetMapping(value = {"/article/{id}"})
40+
public Article getArticleRequestParamButImplictlyRequired(@RequestParam int articleId) { // Compliant
41+
return new Article(articleId);
42+
}
43+
44+
@GetMapping(value = {"/article/{id}"})
45+
public Article getArticleButExplicitlyRequired(@PathVariable(required = true) int articleId) { // Compliant
46+
return new Article(articleId);
47+
}
48+
49+
@GetMapping(value = {"/article/{id}"})
50+
public Article getArticleRequestParamButExplicitlyRequired(@RequestParam(required = true) int articleId) { // Compliant
51+
return new Article(articleId);
52+
}
53+
54+
@GetMapping(value = {"/article/{id}"})
55+
public Article getArticleButSettingADifferentValue(@PathVariable(name = "articleId") int articleId) { // Compliant
56+
return new Article(articleId);
57+
}
58+
59+
@GetMapping(value = {"/article/{id}"})
60+
public Article getArticleButDifferentlyAnnotated(@Nullable int articleId) { // Compliant
61+
return new Article(articleId);
62+
}
63+
64+
record Article(int id) {
65+
}
66+
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@
139139
import org.sonar.java.checks.spring.AsyncMethodsReturnTypeCheck;
140140
import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck;
141141
import org.sonar.java.checks.spring.FieldDependencyInjectionCheck;
142+
import org.sonar.java.checks.spring.OptionalRestParametersShouldBeObjectsCheck;
142143
import org.sonar.java.checks.spring.PersistentEntityUsedAsRequestParameterCheck;
143144
import org.sonar.java.checks.spring.RequestMappingMethodPublicCheck;
144145
import org.sonar.java.checks.spring.SpringAntMatcherOrderCheck;
@@ -520,6 +521,7 @@ public final class CheckList {
520521
OneDeclarationPerLineCheck.class,
521522
OpenSAML2AuthenticationBypassCheck.class,
522523
OptionalAsParameterCheck.class,
524+
OptionalRestParametersShouldBeObjectsCheck.class,
523525
OutputStreamOverrideWriteCheck.class,
524526
OverrideAnnotationCheck.class,
525527
OverwrittenKeyCheck.class,

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

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
/*
2+
* SonarQube Java
3+
* Copyright (C) 2012-2023 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.java.checks.spring;
21+
22+
import java.util.List;
23+
import org.sonar.check.Rule;
24+
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
25+
import org.sonar.plugins.java.api.tree.AnnotationTree;
26+
import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
27+
import org.sonar.plugins.java.api.tree.IdentifierTree;
28+
import org.sonar.plugins.java.api.tree.MethodTree;
29+
import org.sonar.plugins.java.api.tree.Tree;
30+
import org.sonar.plugins.java.api.tree.VariableTree;
31+
32+
@Rule(key = "S6814")
33+
public class OptionalRestParametersShouldBeObjectsCheck extends IssuableSubscriptionVisitor {
34+
35+
private static final List<String> PARAMETER_ANNOTATIONS = List.of(
36+
"org.springframework.web.bind.annotation.PathVariable",
37+
"org.springframework.web.bind.annotation.RequestParam"
38+
);
39+
40+
@Override
41+
public List<Tree.Kind> nodesToVisit() {
42+
return List.of(Tree.Kind.METHOD);
43+
}
44+
45+
@Override
46+
public void visitNode(Tree tree) {
47+
MethodTree method = (MethodTree) tree;
48+
method.parameters().stream()
49+
.filter(OptionalRestParametersShouldBeObjectsCheck::isOptionalPrimitive)
50+
.forEach(parameter -> reportIssue(parameter, "Convert this optional parameter to an Object type."));
51+
}
52+
53+
private static boolean isOptionalPrimitive(VariableTree parameter) {
54+
return parameter.type().symbolType().isPrimitive() &&
55+
parameter.modifiers().annotations().stream()
56+
.anyMatch(OptionalRestParametersShouldBeObjectsCheck::isMarkingAsOptional);
57+
}
58+
59+
private static boolean isMarkingAsOptional(AnnotationTree annotation) {
60+
return PARAMETER_ANNOTATIONS.stream().anyMatch(candidate -> annotation.annotationType().symbolType().is(candidate)) &&
61+
annotation.arguments().stream().anyMatch(expressionTree -> {
62+
// Because all parameters of the supported annotations are assignments, we can cast safely here.
63+
AssignmentExpressionTree assignment = (AssignmentExpressionTree) expressionTree;
64+
IdentifierTree variable = (IdentifierTree) assignment.variable();
65+
Boolean constant = assignment.expression().asConstant(Boolean.class).orElse(Boolean.TRUE);
66+
return "required".equals(variable.name()) && Boolean.FALSE.equals(constant);
67+
});
68+
}
69+
}

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
<h2>Why is this an issue?</h2>
2+
<p>Spring provides two options to mark a REST parameter as optional:</p>
3+
<ol>
4+
<li> Use <code>required = false</code> in the <code>@PathVariable</code> or <code>@RequestParam</code> annotation of the respective method parameter
5+
or </li>
6+
<li> Use type <code>java.util.Optional&lt;T&gt;</code> for the method parameter </li>
7+
</ol>
8+
<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
9+
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
10+
exception occurs because the Spring data mapper cannot map the <code>null</code> value to the parameter.</p>
11+
<h2>How to fix it</h2>
12+
<p>Replace primitive types, such as <code>boolean</code>, <code>char</code>, <code>int</code>, with the corresponding wrapper type, such as
13+
<code>Boolean</code>, <code>Character</code>, <code>Integer</code>.</p>
14+
<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
15+
parameter, such as <code>Optional&lt;Boolean&gt;</code> or <code>Optional&lt;String&gt;</code>, which automatically makes the REST parameter optional.
16+
This is the preferred approach because it enforces the proper handling of <code>null</code> in the method implementation.</p>
17+
<h3>Code examples</h3>
18+
<h4>Noncompliant code example</h4>
19+
<pre data-diff-id="1" data-diff-type="noncompliant">
20+
@RequestMapping(value = {"/article", "/article/{id}"})
21+
public Article getArticle(@PathVariable(required = false) int articleId) { // Noncompliant, null cannot be mapped to int
22+
//...
23+
}
24+
</pre>
25+
<h4>Compliant solution</h4>
26+
<pre data-diff-id="1" data-diff-type="compliant">
27+
@RequestMapping(value = {"/article", "/article/{id}"})
28+
public Article getArticle(@PathVariable(required = false) Integer articleId) { // Compliant
29+
//...
30+
}
31+
</pre>
32+
<h4>Noncompliant code example</h4>
33+
<pre data-diff-id="2" data-diff-type="noncompliant">
34+
@RequestMapping(value = {"/article", "/article/{id}"})
35+
public Article getArticle(@PathVariable(required = false) int articleId) { // Noncompliant, null cannot be mapped to int
36+
//...
37+
}
38+
</pre>
39+
<h4>Compliant solution</h4>
40+
<pre data-diff-id="2" data-diff-type="compliant">
41+
@RequestMapping(value = {"/article", "/article/{id}"})
42+
public Article getArticle(@PathVariable Optional&lt;Integer&gt; articleId) { // Compliant and preferred approach
43+
//...
44+
}
45+
</pre>
46+
<h2>Resources</h2>
47+
<h3>Documentation</h3>
48+
<ul>
49+
<li> <a href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/bind/annotation/PathVariable.html">Spring
50+
Framework API - Annotation Interface PathVariable</a> </li>
51+
</ul>
52+
<h3>Articles &amp; blog posts</h3>
53+
<ul>
54+
<li> <a href="https://www.baeldung.com/spring-optional-path-variables">Baeldung - Spring Optional Path Variables</a> </li>
55+
</ul>
56+

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
{
2+
"title": "Optional REST parameters should have an object type",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [],
10+
"defaultSeverity": "Major",
11+
"ruleSpecification": "RSPEC-6814",
12+
"sqKey": "S6814",
13+
"scope": "All",
14+
"quickfix": "unknown",
15+
"code": {
16+
"impacts": {
17+
"RELIABILITY": "HIGH"
18+
},
19+
"attribute": "CONVENTIONAL"
20+
}
21+
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,7 @@
482482
"S6548",
483483
"S6809",
484484
"S6810",
485-
"S6813"
485+
"S6813",
486+
"S6814"
486487
]
487488
}

‎java-checks/src/test/java/org/sonar/java/checks/spring/OptionalRestParametersShouldBeObjectsCheckTest.java‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
/*
2+
* SonarQube Java
3+
* Copyright (C) 2012-2023 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.java.checks.spring;
21+
22+
import org.junit.jupiter.api.Test;
23+
import org.sonar.java.checks.verifier.CheckVerifier;
24+
import org.sonar.java.checks.verifier.TestUtils;
25+
26+
class OptionalRestParametersShouldBeObjectsCheckTest {
27+
28+
@Test
29+
void test() {
30+
CheckVerifier.newVerifier()
31+
.onFile(TestUtils.mainCodeSourcesPath("checks/spring/OptionalRestParametersShouldBeObjects.java"))
32+
.withCheck(new OptionalRestParametersShouldBeObjectsCheck())
33+
.verifyIssues();
34+
}
35+
36+
}

0 commit comments

Comments
 (0)