SONARJAVA-6758: Fix FPs in S5673 for controllers without mappings and redundant annotations - #5925
SONARJAVA-6758: Fix FPs in S5673 for controllers without mappings and redundant annotations#5925romainbrenguier wants to merge 2 commits into
Conversation
… redundant annotations - Require request mapping annotations before suggesting @Controller/@RestController - Skip raising when a specialized stereotype annotation is already present alongside @component - Exclude classes implementing non-web framework interfaces (ApplicationRunner, CommandLineRunner, HealthIndicator) - Exclude classes annotated with actuator endpoint annotations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| private static boolean hasRequestMappingMethod(ClassTree classTree) { | ||
| for (Tree member : classTree.members()) { | ||
| if (member instanceof MethodTree method) { | ||
| for (AnnotationTree annotation : method.modifiers().annotations()) { | ||
| if (REQUEST_MAPPING_ANNOTATIONS.contains(annotation.annotationType().symbolType().fullyQualifiedName())) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Class-level @RequestMapping not detected as a mapping method
hasRequestMappingMethod only inspects method-level annotations, so a controller that declares its request mapping at the class level (e.g. @component @RequestMapping("/api") with handler methods that carry no mapping annotation, or methods inherited from a base class) will no longer be flagged. This trades the fixed FPs for potential false negatives on legitimate controllers. Consider also checking for a class-level @RequestMapping / mapping annotation before concluding the class handles no HTTP requests.
Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
Class-level @RequestMapping alone is not a handler in Spring MVC (it only sets a path prefix), so I would not treat that as a required change.
Inherited mappings are a real gap: hasRequestMappingMethod only looks at classTree.members(), so a @Component subclass of a base controller with @GetMapping methods would now be an FN. Please walk superTypes() and use isAnnotatedWith — I left a review comment on that method.
There was a problem hiding this comment.
Agreed on both points. I'm updating hasRequestMappingMethod to also walk superclasses (via the semantic model, checking each ancestor's method symbols for a mapping annotation) so inherited handler methods are still detected, without treating a class-level @RequestMapping as a handler by itself.
Extract duplicated "Controller" and "RestController" string literals into constants to fix S1192 issues. Add test cases for HealthIndicator, ReactiveHealthIndicator, and @endpoint to improve coverage on new code above 90% threshold. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
RSPEC is now inconsistent with the analyzer
The ticket also asks to improve S5673 RSPEC (it was judged not actionable). There is no RSPEC PR, and the published examples are now wrong.
rules/S5673/java/rule.adoc still shows an empty FooBarRestController as noncompliant. After this change that class would not raise, which is why the unit test had to add @GetMapping to keep it noncompliant.
Please open a sibling RSPEC PR that:
- puts a mapping method on the Controller/RestController noncompliant example
- documents the new exceptions (no mapping methods, already-specialized stereotypes, actuator/startup types)
- aligns the issue message with the implementation (“or rename this type if
@Componentis intentional”)
| private static boolean hasRequestMappingMethod(ClassTree classTree) { | ||
| for (Tree member : classTree.members()) { | ||
| if (member instanceof MethodTree method) { | ||
| for (AnnotationTree annotation : method.modifiers().annotations()) { | ||
| if (REQUEST_MAPPING_ANNOTATIONS.contains(annotation.annotationType().symbolType().fullyQualifiedName())) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This only inspects classTree.members(), so mappings declared on a superclass or implemented interface are missed. That is a common Spring pattern and a new FN:
public abstract class BaseController {
@GetMapping("/status")
public String status() { return "ok"; }
}
@Component // FN after this PR; previously raised
public class StatusController extends BaseController {
}memberSymbols() is also declaration-only. Walking classTree.symbol().superTypes() (as SpringRequestMappingMethodCheck already does) and checking method.metadata().isAnnotatedWith(...) would also catch inherited and composed mapping annotations.
| private static final List<String> NON_WEB_FRAMEWORK_ANNOTATIONS = List.of( | ||
| "org.springframework.boot.actuate.endpoint.annotation.Endpoint", | ||
| "org.springframework.boot.actuate.endpoint.web.annotation.RestControllerEndpoint", | ||
| "org.springframework.boot.actuate.endpoint.web.annotation.ControllerEndpoint"); | ||
|
|
There was a problem hiding this comment.
@RestControllerEndpoint and @ControllerEndpoint are in the exclusion list but untested. Those are the cases where this extra exclusion actually matters: they can carry @GetMapping and must not be rewritten to @Controller / @RestController.
Please add sample classes for both. The @Endpoint / HealthIndicator / ApplicationRunner cases are already covered, and those exclusions are mostly redundant with the mapping-method guard (the original FPs had no mapping annotations).
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsRefines S5673 rule logic to prevent false positives on unmapped controllers and redundant annotations. Consider updating hasRequestMappingMethod to also inspect class-level 💡 Edge Case: Class-level
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
2 participants
Summary
@Controlleror@RestController, the rule now verifies the class has at least one method with a request mapping annotation (@RequestMapping,@GetMapping,@PostMapping,@PutMapping,@DeleteMapping,@PatchMapping). Classes named "Controller" that don't handle HTTP requests no longer trigger the rule.ApplicationRunner,CommandLineRunner,HealthIndicator, orReactiveHealthIndicator, or annotated with@Endpoint,@RestControllerEndpoint, or@ControllerEndpointare excluded from Controller/RestController suggestions.@Controller,@RestController,@Service,@Repository) alongside@Component, the rule no longer raises.Test plan
🤖 Generated with Claude Code