Skip to content

Commit c3ae6b5

Browse files
erwan-serandourerwan-serandour
authored
SONARJAVA-4748 S6833 don't raise an issue if a method has a mapping annotation and no responseBody annotation (#5118)
1 parent f45c429 commit c3ae6b5

5 files changed

Lines changed: 202 additions & 100 deletions

File tree

‎its/autoscan/src/test/resources/autoscan/diffs/diff_S3752.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"ruleKey": "S3752",
33
"hasTruePositives": true,
4-
"falseNegatives": 25,
4+
"falseNegatives": 26,
55
"falsePositives": 0
66
}

‎java-checks-test-sources/default/src/main/java/checks/spring/ControllerWithRestControllerReplacementCheck.java‎

Lines changed: 0 additions & 88 deletions
This file was deleted.

‎java-checks-test-sources/default/src/main/java/checks/spring/ControllerWithRestControllerReplacementCheckSample.java‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
package checks.spring;
2+
3+
import javax.annotation.Nullable;
4+
import org.springframework.stereotype.Controller;
5+
import org.springframework.stereotype.Service;
6+
import org.springframework.web.bind.annotation.DeleteMapping;
7+
import org.springframework.web.bind.annotation.GetMapping;
8+
import org.springframework.web.bind.annotation.PatchMapping;
9+
import org.springframework.web.bind.annotation.PostMapping;
10+
import org.springframework.web.bind.annotation.PutMapping;
11+
import org.springframework.web.bind.annotation.RequestMapping;
12+
import org.springframework.web.bind.annotation.ResponseBody;
13+
14+
public class ControllerWithRestControllerReplacementCheckSample {
15+
16+
@Controller // Noncompliant [[quickfixes=qf1,qf2]]
17+
//^^^^^^^^^^^
18+
// "@ResponseBody" annotations.}}
19+
// fix@qf2 {{Replace "@Controller" by "@RestController".}}
20+
// edit@qf2 [[sl=16;el=16;sc=3;ec=14]] {{@RestController}}
21+
class ClassOne {
22+
23+
@ResponseBody
24+
// ^^^^^^^^^^^^^<
25+
// fix@qf1 {{Remove "@ResponseBody" annotations.}}
26+
// edit@qf1 [[sl=23;el=23;sc=5;ec=18]] {{}}
27+
public void m() {
28+
}
29+
}
30+
31+
@Controller // Noncompliant
32+
@ResponseBody
33+
class ClassTwo {
34+
35+
public void m() {
36+
}
37+
}
38+
39+
@Controller // Noncompliant
40+
class ClassThree {
41+
42+
@ResponseBody
43+
public void m() {
44+
}
45+
46+
public void m2() {
47+
}
48+
}
49+
50+
@Controller // Compliant
51+
class ClassFour {
52+
53+
public void m() {
54+
}
55+
}
56+
57+
@Controller // Compliant
58+
class ClassFive {
59+
60+
@Nullable
61+
public void m() {
62+
}
63+
}
64+
65+
@Service // Compliant
66+
class ClassSix {
67+
68+
@ResponseBody
69+
public void m() {
70+
}
71+
}
72+
73+
@Controller // Noncompliant [[quickfixes=qf3,qf4]]
74+
//^^^^^^^^^^^
75+
// "@ResponseBody" annotations.}}
76+
// fix@qf4 {{Replace "@Controller" by "@RestController".}}
77+
// edit@qf4 [[sl=73;el=73;sc=3;ec=14]] {{@RestController}}
78+
class ClassSeven {
79+
80+
@ResponseBody
81+
// ^^^^^^^^^^^^^<
82+
// fix@qf3 {{Remove "@ResponseBody" annotations.}}
83+
// edit@qf3 [[sl=80;el=80;sc=5;ec=18]] {{}}
84+
public void m() {
85+
}
86+
87+
@ResponseBody
88+
// ^^^^^^^^^^^^^<
89+
// fix@qf3 {{Remove "@ResponseBody" annotations.}}
90+
// edit@qf3 [[sl=87;el=87;sc=5;ec=18]] {{}}
91+
public void m2() {
92+
}
93+
}
94+
95+
@Controller // Noncompliant
96+
class GetAndResponseBody {
97+
String foo = "hello";
98+
99+
@ResponseBody
100+
@GetMapping
101+
public void m() {}
102+
103+
public void n() {}
104+
}
105+
106+
@Controller // compliant
107+
class MixedResponseBodyAndGet {
108+
@GetMapping
109+
public void m() {}
110+
@ResponseBody
111+
public void n() {}
112+
}
113+
114+
@Controller // compliant
115+
class MixedResponseBodyAndPost {
116+
@PostMapping
117+
public void m() {}
118+
@PostMapping
119+
@ResponseBody
120+
public void n() {}
121+
}
122+
123+
@Controller // compliant
124+
class MixedResponseBodyAndRequest {
125+
@RequestMapping
126+
public void m() {}
127+
@ResponseBody
128+
public void n() {}
129+
}
130+
131+
@Controller // compliant
132+
class MixedResponseBodyAndPatch {
133+
@PatchMapping
134+
public void m() {}
135+
@ResponseBody
136+
public void n() {}
137+
@ResponseBody
138+
public void k() {}
139+
}
140+
141+
@Controller // compliant
142+
class MixedResponseBodyAndDelete {
143+
@DeleteMapping
144+
public void m() {}
145+
@ResponseBody
146+
public void n() {}
147+
}
148+
149+
@Controller // compliant
150+
class MixedResponseBodyAndPut {
151+
@PutMapping
152+
public void m() {}
153+
@ResponseBody
154+
public void n() {}
155+
}
156+
157+
}

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

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,28 @@
1818

1919
import java.util.ArrayList;
2020
import java.util.List;
21+
import java.util.Optional;
2122
import org.sonar.check.Rule;
2223
import org.sonar.java.checks.helpers.QuickFixHelper;
2324
import org.sonar.java.reporting.JavaQuickFix;
2425
import org.sonar.java.reporting.JavaTextEdit;
2526
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
2627
import org.sonar.plugins.java.api.JavaFileScannerContext;
28+
import org.sonar.plugins.java.api.tree.AnnotationTree;
2729
import org.sonar.plugins.java.api.tree.ClassTree;
2830
import org.sonar.plugins.java.api.tree.MethodTree;
2931
import org.sonar.plugins.java.api.tree.Tree;
3032

3133
@Rule(key = "S6833")
3234
public class ControllerWithRestControllerReplacementCheck extends IssuableSubscriptionVisitor {
35+
private static final String RESPONSE_BODY = "org.springframework.web.bind.annotation.ResponseBody";
36+
private static final List<String> MAPPING_ANNOTATIONS = List.of(
37+
"org.springframework.web.bind.annotation.RequestMapping",
38+
"org.springframework.web.bind.annotation.GetMapping",
39+
"org.springframework.web.bind.annotation.PostMapping",
40+
"org.springframework.web.bind.annotation.PutMapping",
41+
"org.springframework.web.bind.annotation.PatchMapping",
42+
"org.springframework.web.bind.annotation.DeleteMapping");
3343

3444
@Override
3545
public List<Tree.Kind> nodesToVisit() {
@@ -48,22 +58,31 @@ public void visitNode(Tree tree) {
4858
return;
4959
}
5060

61+
List<AnnotationTree> responseBodyOnMethods = new ArrayList<>();
62+
63+
for (Tree member : classTree.members()) {
64+
if (member instanceof MethodTree method) {
65+
66+
var response = firstAnnotation(method, List.of(RESPONSE_BODY));
67+
if (response.isPresent()) {
68+
responseBodyOnMethods.add(response.get());
69+
} else if (firstAnnotation(method, MAPPING_ANNOTATIONS).isPresent()) {
70+
return;
71+
}
72+
}
73+
}
74+
5175
var secondaryLocations = new ArrayList<JavaFileScannerContext.Location>();
5276
List<JavaTextEdit> edits = new ArrayList<>();
5377

54-
classTree.members().stream()
55-
.filter(member -> member.is(Tree.Kind.METHOD))
56-
.map(MethodTree.class::cast)
57-
.forEach(method -> {
58-
var methodAnnotation = method.modifiers().annotations().stream()
59-
.filter(a -> "org.springframework.web.bind.annotation.ResponseBody".equals(a.annotationType().symbolType().fullyQualifiedName()))
60-
.findFirst();
61-
methodAnnotation.ifPresent(annotationTree -> secondaryLocations.add(new JavaFileScannerContext.Location("Remove this \"@ResponseBody\" annotation.", annotationTree)));
62-
methodAnnotation.ifPresent(annotationTree -> edits.add(JavaTextEdit.removeTree(annotationTree)));
78+
responseBodyOnMethods
79+
.forEach(ann -> {
80+
secondaryLocations.add(new JavaFileScannerContext.Location("Remove this \"@ResponseBody\" annotation.", ann));
81+
edits.add(JavaTextEdit.removeTree(ann));
6382
});
6483

6584
classTree.modifiers().annotations().stream()
66-
.filter(a -> "org.springframework.web.bind.annotation.ResponseBody".equals(a.annotationType().symbolType().fullyQualifiedName()))
85+
.filter(ControllerWithRestControllerReplacementCheck::isResponseBody)
6786
.forEach(annotationTree -> secondaryLocations.add(new JavaFileScannerContext.Location("Remove this \"@ResponseBody\" annotation.", annotationTree)));
6887

6988
if (secondaryLocations.isEmpty()) {
@@ -81,4 +100,18 @@ public void visitNode(Tree tree) {
81100

82101
}
83102

103+
private static boolean isResponseBody(AnnotationTree a) {
104+
return RESPONSE_BODY.equals(a.symbolType().fullyQualifiedName());
105+
}
106+
107+
private static Optional<AnnotationTree> firstAnnotation(MethodTree method, List<String> annFullyQualifiedNames) {
108+
for (AnnotationTree annotation : method.modifiers().annotations()) {
109+
String fqn = annotation.symbolType().fullyQualifiedName();
110+
if (annFullyQualifiedNames.contains(fqn)) {
111+
return Optional.of(annotation);
112+
}
113+
}
114+
return Optional.empty();
115+
}
116+
84117
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ class ControllerWithRestControllerReplacementCheckTest {
2626
@Test
2727
void test() {
2828
CheckVerifier.newVerifier()
29-
.onFile(mainCodeSourcesPath("checks/spring/ControllerWithRestControllerReplacementCheck.java"))
29+
.onFile(mainCodeSourcesPath("checks/spring/ControllerWithRestControllerReplacementCheckSample.java"))
3030
.withCheck(new ControllerWithRestControllerReplacementCheck())
3131
.verifyIssues();
3232
}

0 commit comments

Comments
 (0)