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

its/autoscan/src/test/resources/autoscan/diffs/diff_S5673.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S5673",
   "hasTruePositives": false,
-  "falseNegatives": 20,
+  "falseNegatives": 17,
   "falsePositives": 0
 }

java-checks-test-sources/default/pom.xml

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

java-checks-test-sources/default/src/main/java/checks/spring/SpringComponentSpecializationCheckSample.java

@@ -1,9 +1,19 @@
 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.endpoint.web.annotation.ControllerEndpoint;
+import org.springframework.boot.actuate.endpoint.web.annotation.RestControllerEndpoint;
+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 {
@@ -40,28 +50,164 @@ 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 - Controllers with request mappings but annotated with @RestControllerEndpoint
+  @Component
+  @RestControllerEndpoint(id = "restEndpoint")
+  public class ActuatorRestController {
+    @GetMapping("/actuator/rest")
+    public String restEndpoint() { return "rest"; }
+  }
+
+  // Compliant - Controllers with request mappings but annotated with @ControllerEndpoint
+  @Component
+  @ControllerEndpoint(id = "controllerEndpoint")
+  public class ActuatorController {
+    @GetMapping("/actuator/controller")
+    public String controllerEndpoint() { return "controller"; }
+  }
+
+  // Controllers with inherited request mapping methods
+
+  public abstract class BaseRestController {
+    @GetMapping("/status")
+    public String status() { return "ok"; }
+  }
+
+  @Component // Noncompliant {{Use @RestController instead of @Component, or rename this type if the @Component annotation is intentional}}
+  public class StatusRestController extends BaseRestController {
+  }
+
+  public abstract class BaseController {
+    @PostMapping("/submit")
+    public String submit() { return "submitted"; }
+  }
+
+  @Component // Noncompliant {{Use @Controller instead of @Component, or rename this type if the @Component annotation is intentional}}
+  public class FormController extends BaseController {
+  }
+
+  // Compliant - Controller subclass without inherited mapping methods
+
+  public abstract class BaseProcessController {
+    public void process() { }
+  }
+
+  @Component
+  public class TaskController extends BaseProcessController {
+  }
+
+  // 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
@@ -115,12 +261,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

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

@@ -18,17 +18,49 @@
 
 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.Symbol;
+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");
+
   @Override
   public List<Tree.Kind> nodesToVisit() {
     return List.of(Tree.Kind.CLASS, Tree.Kind.INTERFACE);
@@ -46,23 +78,81 @@ 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;
+          }
+        }
+      }
+    }
+    for (Type superType : classTree.symbol().superTypes()) {
+      if (hasRequestMappingMethodInSymbol(superType.symbol())) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  private static boolean hasRequestMappingMethodInSymbol(Symbol.TypeSymbol typeSymbol) {
+    return typeSymbol.memberSymbols().stream()
+      .filter(Symbol::isMethodSymbol)
+      .anyMatch(method -> REQUEST_MAPPING_ANNOTATIONS.stream().anyMatch(method.metadata()::isAnnotatedWith));
+  }
+
+  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") ||