diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5673.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5673.json index 559fd7787c9..07775157976 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S5673.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S5673.json @@ -1,6 +1,6 @@ { "ruleKey": "S5673", "hasTruePositives": false, - "falseNegatives": 20, + "falseNegatives": 17, "falsePositives": 0 } \ No newline at end of file diff --git a/java-checks-test-sources/default/pom.xml b/java-checks-test-sources/default/pom.xml index 4a13f6cb24b..ae5a0c951ce 100644 --- a/java-checks-test-sources/default/pom.xml +++ b/java-checks-test-sources/default/pom.xml @@ -356,6 +356,12 @@ 2.5.15 provided + + org.springframework.boot + spring-boot-actuator + 2.0.2.RELEASE + provided + org.springframework.security spring-security-crypto diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/SpringComponentSpecializationCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/SpringComponentSpecializationCheckSample.java index 8672679f351..bd981231a65 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/SpringComponentSpecializationCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/spring/SpringComponentSpecializationCheckSample.java @@ -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 { @@ -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 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 @@ -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 diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/SpringComponentSpecializationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/SpringComponentSpecializationCheck.java index a91e398f42b..dea306b2fbc 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/SpringComponentSpecializationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/SpringComponentSpecializationCheck.java @@ -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 SPECIALIZED_STEREOTYPE_ANNOTATIONS = Set.of( + SpringUtils.CONTROLLER_ANNOTATION, + SpringUtils.REST_CONTROLLER_ANNOTATION, + SpringUtils.SERVICE_ANNOTATION, + SpringUtils.REPOSITORY_ANNOTATION); + + private static final List 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 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 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 nodesToVisit() { return List.of(Tree.Kind.CLASS, Tree.Kind.INTERFACE); @@ -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; + } + + 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") ||