SONARJAVA-6767 Implement new rule S9341: Redundant Spring annotations should be removed - #5930
SONARJAVA-6767 Implement new rule S9341: Redundant Spring annotations should be removed#5930romainbrenguier wants to merge 4 commits into
Conversation
Detect redundant Spring annotations where a more specific composed annotation already implies the parent. Covers stereotype annotations (@component with @Service/@Repository/@Controller/@configuration), @RestController composition, @SpringBootApplication composition, and Spring test annotation redundancies.
…nentscan with filters - Fix @transactional + @DataJpaTest: only flag as redundant when no attributes are set, since custom attributes like readOnly or propagation change runtime behavior - Fix @componentscan + @SpringBootApplication: reject any attribute (not just value/basePackages/basePackageClasses), since attributes like excludeFilters, lazyInit, useDefaultFilters are not exposed by @SpringBootApplication - Add compliant test cases for both fixes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nathsou
left a comment
There was a problem hiding this comment.
Thanks for the implementation. I found four issues that need addressing before this can be merged.
| private static final List<RedundancyRule> REDUNDANCY_RULES = List.of( | ||
| new RedundancyRule(SpringUtils.COMPONENT_ANNOTATION, | ||
| List.of(SpringUtils.SERVICE_ANNOTATION, SpringUtils.REPOSITORY_ANNOTATION, SpringUtils.CONTROLLER_ANNOTATION, SpringUtils.CONFIGURATION_ANNOTATION), null), | ||
| new RedundancyRule(SpringUtils.CONTROLLER_ANNOTATION, |
There was a problem hiding this comment.
[P1] The removal is unsafe when the parent annotation has explicit attributes. For example, @Component("orders") next to @Service supplies the bean name; removing it changes that name. Similarly, @Configuration(proxyBeanMethods = false) alongside @SpringBootApplication changes configuration semantics, and @EnableAutoConfiguration(exclude = Foo.class) loses the exclusion. Only @ComponentScan and @Transactional are guarded today. Report these pairs only when the parent annotation has no explicit attributes, and add compliant regression cases.
| continue; | ||
| } | ||
| for (String impliedByFqn : rule.impliedByFqns) { | ||
| AnnotationTree impliedByAnnotation = annotationsByFqn.get(impliedByFqn); |
There was a problem hiding this comment.
[P1] Collapsing annotations by FQN breaks repeatable annotations. With @ExtendWith(SpringExtension.class), then @ExtendWith(MockitoExtension.class), then @SpringBootTest, this map retains Mockito while valuesForAnnotation examines the first matching semantic annotation. The check can therefore report Mockito as redundant. @ComponentScan has the same risk. Preserve/evaluate each annotation instance rather than one annotation per FQN, and cover repeated-annotation cases.
| List.of(DATA_JPA_TEST), RedundantSpringAnnotationCheck::isTransactionalWithoutCustomAttributes) | ||
| ); | ||
|
|
||
| @Override |
There was a problem hiding this comment.
[P2] Spring stereotype annotations can target records, but the visitor subscribes only to CLASS; @Component @Service record Foo() {} is ignored. Subscribe to Tree.Kind.RECORD as well and add a record test case.
| List.of(DATA_JPA_TEST), RedundantSpringAnnotationCheck::isTransactionalWithoutCustomAttributes) | ||
| ); | ||
|
|
||
| @Override |
There was a problem hiding this comment.
[P2] The linked RSPEC says method-level @ResponseBody in a @RestController should be reported, but this check visits only classes. Either implement that behavior (while resolving the overlap with S6837) or update RSPEC to avoid promising it.
… annotations, and records - Use multimap for annotation collection to properly handle repeatable annotations like multiple @ExtendWith or @componentscan instances - Add attribute guards to prevent unsafe removal of annotations with explicit attributes (@component with bean name, @configuration with proxyBeanMethods, @EnableAutoConfiguration with exclude, @SpringBootConfiguration with attributes) - Add Tree.Kind.RECORD to visited nodes so records are also checked - Evaluate @ExtendWith per annotation instance using AST arguments instead of merged metadata to avoid false positives on non-Spring extensions - Document that method-level @responsebody is handled by S6837 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| private static boolean isExtendWithSpringExtensionOnly(AnnotationTree annotation) { | ||
| var arguments = annotation.arguments(); | ||
| if (arguments.size() != 1) { | ||
| return false; | ||
| } | ||
| ExpressionTree arg = arguments.get(0); | ||
| if (arg.is(Tree.Kind.MEMBER_SELECT)) { | ||
| return isSpringExtensionClassRef((MemberSelectExpressionTree) arg); | ||
| } | ||
| if (arg.is(Tree.Kind.NEW_ARRAY)) { | ||
| var initializers = ((NewArrayTree) arg).initializers(); | ||
| return initializers.size() == 1 | ||
| && initializers.get(0).is(Tree.Kind.MEMBER_SELECT) | ||
| && isSpringExtensionClassRef((MemberSelectExpressionTree) initializers.get(0)); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: @ExtendWith(value = SpringExtension.class) not detected
isExtendWithSpringExtensionOnly only handles a single argument that is a MEMBER_SELECT (SpringExtension.class) or a NEW_ARRAY. When the argument is written in the explicit named form @ExtendWith(value = SpringExtension.class), the argument tree is an ASSIGNMENT, so the method returns false and the redundant annotation is not reported (false negative). Consider unwrapping an ASSIGNMENT whose name is value to its expression before checking, so the named form is treated the same as the shorthand.
Was this helpful? React with 👍 / 👎
The default module uses Spring Boot 2.0.2 which does not have the proxyBeanMethods attribute on @SpringBootConfiguration (added in 2.2). This caused a compilation failure in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 2 resolved / 3 findingsImplements rule S9341 to detect redundant Spring annotations while properly handling custom attributes and record types. Consider updating isExtendWithSpringExtensionOnly to correctly detect 💡 Edge Case:
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|




Detect redundant Spring annotations where a more specific composed annotation already implies the parent. Covers stereotype annotations (@component with @Service/@Repository/@Controller/@configuration), @RestController composition, @SpringBootApplication composition, and Spring test annotation redundancies.
Part of