Skip to content

SONARJAVA-6758: Fix FPs in S5673 for controllers without mappings and redundant annotations #5925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
romainbrenguier wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SONARJAVA-6758: Fix FPs in S5673 for controllers without mappings and redundant annotations #5925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e70fd0f
SONARJAVA-6758: Fix FPs in S5673 for controllers without mappings and…
romainbrenguier Aug 14, 2026
c7196a7
SONARJAVA-6758: Fix Quality Gate issues in S5673
romainbrenguier Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion its/autoscan/src/test/resources/autoscan/diffs/diff_S5673.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"ruleKey": "S5673",
"hasTruePositives": false,
"falseNegatives": 20,
"falseNegatives": 17,
"falsePositives": 0
}
6 changes: 6 additions & 0 deletions java-checks-test-sources/default/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,12 @@
<version>2.5.15</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-actuator</artifactId>
<version>2.0.2.RELEASE</version>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.springframework.security</groupId>
<artifactId>spring-security-crypto</artifactId>
Expand Down
108 changes: 104 additions & 4 deletions ...sources/default/src/main/java/checks/spring/SpringComponentSpecializationCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,17 @@
package checks.spring;

import org.springframework.boot.ApplicationRunner;
import org.springframework.boot.CommandLineRunner;
import org.springframework.boot.actuate.endpoint.annotation.Endpoint;
import org.springframework.boot.actuate.health.HealthIndicator;
import org.springframework.boot.actuate.health.ReactiveHealthIndicator;
import org.springframework.stereotype.Component;
import org.springframework.stereotype.Service;
import org.springframework.stereotype.Repository;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;

public class SpringComponentSpecializationCheckSample {
Expand Down Expand Up @@ -40,28 +48,118 @@ public class OrderDao {
public class CustomerDao {
}

// RestController patterns
// RestController patterns - with request mapping methods

@Component // Noncompliant {{Use @RestController instead of @Component, or rename this type if the @Component annotation is intentional}}
public class FooBarRestController {
@GetMapping("/foo")
public String foo() { return "foo"; }
}

@Component // Noncompliant {{Use @RestController instead of @Component, or rename this type if the @Component annotation is intentional}}
public class ApiRestController {
@RequestMapping("/api")
public String api() { return "api"; }
}

@Component // Noncompliant {{Use @RestController instead of @Component, or rename this type if the @Component annotation is intentional}}
public class UserRestControllerImpl {
@PostMapping("/users")
public void createUser() { }
}

// Controller patterns
// Controller patterns - with request mapping methods

@Component // Noncompliant {{Use @Controller instead of @Component, or rename this type if the @Component annotation is intentional}}
public class HomeController {
@GetMapping("/home")
public String home() { return "home"; }
}

@Component // Noncompliant {{Use @Controller instead of @Component, or rename this type if the @Component annotation is intentional}}
public class LoginControllerImpl {
@PostMapping("/login")
public String login() { return "login"; }
}

// Compliant - Controllers without request mapping methods (FP fix)

@Component
public class BatchController {
}

@Component
public class DataProcessingController {
public void process() { }
}

@Component
public class SchedulerRestController {
public void runTask() { }
}

// Compliant - Controllers implementing non-web framework interfaces

@Component
public class StartupController implements ApplicationRunner {
@Override
public void run(org.springframework.boot.ApplicationArguments args) { }
}

@Component
public class InitController implements CommandLineRunner {
@Override
public void run(String... args) { }
}

// Compliant - Controllers with request mappings but implementing HealthIndicator
@Component
public class HealthCheckController implements HealthIndicator {
@GetMapping("/health")
public String healthStatus() { return "UP"; }

@Override
public org.springframework.boot.actuate.health.Health health() { return null; }
}

// Compliant - Controllers with request mappings but implementing ReactiveHealthIndicator
@Component
public class ReactiveHealthCheckController implements ReactiveHealthIndicator {
@GetMapping("/health/reactive")
public String reactiveHealthStatus() { return "UP"; }

@Override
public reactor.core.publisher.Mono<org.springframework.boot.actuate.health.Health> health() { return null; }
}

// Compliant - Controllers with request mappings but annotated with @Endpoint
@Component
@Endpoint(id = "custom")
public class CustomEndpointController {
@GetMapping("/custom")
public String custom() { return "custom"; }
}

// Compliant - Redundant annotation: @Component alongside a specialized stereotype

@Component
@Service
public class RedundantServiceAnnotation {
}

@Component
@Controller
public class RedundantControllerAnnotation {
}

@Component
@RestController
public class RedundantRestControllerAnnotation {
}

@Component
@Repository
public class RedundantRepositoryAnnotation {
}

// Compliant - Correct annotations used
Expand Down Expand Up @@ -115,12 +213,14 @@ public class userservice {
public class USERREPOSITORY {
}

@Component // Noncompliant {{Use @Controller instead of @Component, or rename this type if the @Component annotation is intentional}}
@Component
public class maincontroller {
// Compliant - no request mapping methods
}

@Component // Noncompliant {{Use @RestController instead of @Component, or rename this type if the @Component annotation is intentional}}
@Component
public class apirestcontroller {
// Compliant - no request mapping methods
}

// Interface patterns
Expand Down
88 changes: 83 additions & 5 deletions ...checks/src/main/java/org/sonar/java/checks/spring/SpringComponentSpecializationCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,48 @@

import java.util.List;
import java.util.Optional;
import java.util.Set;
import javax.annotation.CheckForNull;
import org.sonar.check.Rule;
import org.sonar.java.checks.helpers.SpringUtils;
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
import org.sonar.plugins.java.api.semantic.Type;
import org.sonar.plugins.java.api.tree.AnnotationTree;
import org.sonar.plugins.java.api.tree.ClassTree;
import org.sonar.plugins.java.api.tree.MethodTree;
import org.sonar.plugins.java.api.tree.Tree;

@Rule(key = "S5673")
public class SpringComponentSpecializationCheck extends IssuableSubscriptionVisitor {

private static final Set<String> SPECIALIZED_STEREOTYPE_ANNOTATIONS = Set.of(
SpringUtils.CONTROLLER_ANNOTATION,
SpringUtils.REST_CONTROLLER_ANNOTATION,
SpringUtils.SERVICE_ANNOTATION,
SpringUtils.REPOSITORY_ANNOTATION);

private static final List<String> REQUEST_MAPPING_ANNOTATIONS = List.of(
"org.springframework.web.bind.annotation.RequestMapping",
"org.springframework.web.bind.annotation.GetMapping",
"org.springframework.web.bind.annotation.PostMapping",
"org.springframework.web.bind.annotation.PutMapping",
"org.springframework.web.bind.annotation.DeleteMapping",
"org.springframework.web.bind.annotation.PatchMapping");

private static final List<String> NON_WEB_FRAMEWORK_INTERFACES = List.of(
"org.springframework.boot.ApplicationRunner",
"org.springframework.boot.CommandLineRunner",
"org.springframework.boot.actuate.health.HealthIndicator",
"org.springframework.boot.actuate.health.ReactiveHealthIndicator");

private static final String CONTROLLER = "Controller";
private static final String REST_CONTROLLER = "RestController";

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");

Comment on lines +58 to +62

@nathsou nathsou Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

@Override
public List<Tree.Kind> nodesToVisit() {
return List.of(Tree.Kind.CLASS, Tree.Kind.INTERFACE);
Expand All @@ -46,23 +77,70 @@ public void visitNode(Tree tree) {
return;
}

if (hasSpecializedStereotypeAnnotation(classTree)) {
return;
}

String className = classTree.simpleName().name();
String suggestedAnnotation = getSuggestedAnnotation(className);

if (suggestedAnnotation != null) {
if (suggestedAnnotation != null && shouldRaise(suggestedAnnotation, classTree)) {
reportIssue(componentAnnotation.get(), String.format("Use @%s instead of @Component, or rename this type if the @Component annotation is intentional", suggestedAnnotation));
}
}

private static boolean hasSpecializedStereotypeAnnotation(ClassTree classTree) {
return classTree.modifiers().annotations().stream()
.anyMatch(a -> SPECIALIZED_STEREOTYPE_ANNOTATIONS.contains(a.annotationType().symbolType().fullyQualifiedName()));
}

private static boolean shouldRaise(String suggestedAnnotation, ClassTree classTree) {
if (CONTROLLER.equals(suggestedAnnotation) || REST_CONTROLLER.equals(suggestedAnnotation)) {
return hasRequestMappingMethod(classTree) && !implementsNonWebFrameworkInterface(classTree) && !hasNonWebFrameworkAnnotation(classTree);
}
return true;
}

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;
}
Comment on lines +104 to +115

@gitar-bot gitar-bot Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 👍 / 👎

@nathsou nathsou Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gitar-bot gitar-bot Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +104 to +115

@nathsou nathsou Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 boolean implementsNonWebFrameworkInterface(ClassTree classTree) {
Type classType = classTree.symbol().type();
if (classType == null) {
return false;
}
for (String interfaceFqn : NON_WEB_FRAMEWORK_INTERFACES) {
if (classType.isSubtypeOf(interfaceFqn)) {
return true;
}
}
return false;
}

private static boolean hasNonWebFrameworkAnnotation(ClassTree classTree) {
return classTree.modifiers().annotations().stream()
.anyMatch(a -> NON_WEB_FRAMEWORK_ANNOTATIONS.contains(a.annotationType().symbolType().fullyQualifiedName()));
}

@CheckForNull
private static String getSuggestedAnnotation(String className) {
// Check RestController first to avoid false matches with Controller
if (endsWithIgnoreCase(className, "RestController") || endsWithIgnoreCase(className, "RestControllerImpl")) {
return "RestController";
if (endsWithIgnoreCase(className, REST_CONTROLLER) || endsWithIgnoreCase(className, REST_CONTROLLER + "Impl")) {
return REST_CONTROLLER;
}

if (endsWithIgnoreCase(className, "Controller") || endsWithIgnoreCase(className, "ControllerImpl")) {
return "Controller";
if (endsWithIgnoreCase(className, CONTROLLER) || endsWithIgnoreCase(className, CONTROLLER + "Impl")) {
return CONTROLLER;
}

if (endsWithIgnoreCase(className, "Service") ||
Expand Down
Loading