Implement Spring project detection - #6093
NoemieBenard wants to merge 18 commits into
Conversation
Introduce the springcontext package with BeanDefinitionHolder, BeanDefinitionRegistry, BeanLocation, EntityClassToPropertiesIndex, ProjectPackageScan, SpringContextModel, and TypeToBeanNamesIndex. All classes include Javadoc documentation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…pring (#5662) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…n definitions (#5665) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ies in BeanDefinitionGatherer (#5936) Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…injection (#5950) Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…esolved using "@qualifier" or "@primary" (#6073)
bc76f52 to
c9fb91e
Compare
This comment has been minimized.
This comment has been minimized.
… left by class relocations
c9fb91e to
370de7b
Compare
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| */ | ||
| private final Map<String, Set<String>> collectedPackagesByFile = new HashMap<>(); | ||
|
|
||
| /** |
There was a problem hiding this comment.
⚠️ Bug: @SpringBootApplication own-package fallback keyed on file, not class
collectFromSpringBootApplication passes packagesCollectedAtFileLevel.isEmpty() as useOwnPackageAsFallback, but the comment in targetedPackages states the intent is "only if no packages were already collected via @componentscan on the same class". Because the flag is file-scoped, a file such as @ComponentScan("com.other") class Config {} followed by @SpringBootApplication class App {} (package com.app) drops com.app from ProjectPackageScan entirely, while the same two classes in the reverse order collect both packages — the result depends on declaration order within the file. Track the "@componentscan supplied packages" state per visited class instead of per file.
Make the override decision class-scoped by having collectFromComponentScan report whether it contributed packages for the class being visited.:
SymbolMetadata metadata = classTree.symbol().metadata();
boolean componentScanSuppliedPackages = collectFromComponentScan(metadata);
collectFromSpringBootApplication(classTree.symbol(), metadata, !componentScanSuppliedPackages);
}
/** @return {@code true} if this class's {@code @ComponentScan} contributed at least one package. */
private boolean collectFromComponentScan(SymbolMetadata metadata) {
List<SymbolMetadata.AnnotationValue> componentScanAttributes = metadata.valuesForAnnotation(COMPONENT_SCAN_ANNOTATION);
if (componentScanAttributes == null) {
return false;
}
int before = collectedPackages.size();
componentScanAttributes.stream()
.filter(v -> COMPONENT_SCAN_BASE_ARGUMENTS.contains(v.name()))
.forEach(this::addAnnotationValueToCollectedPackages);
return collectedPackages.size() > before;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| private static boolean hasExactlyOnePrimaryCandidate(Set<String> candidates, BeanDefinitionRegistry registry) { | ||
| return candidates.stream().filter(candidate -> isPrimary(registry, candidate)).count() == 1; | ||
| } | ||
|
|
||
| private static Set<String> excludeCandidatesWithProfile(Set<String> candidates, BeanDefinitionRegistry registry) { | ||
| return candidates.stream().filter(candidate -> !hasProfile(registry, candidate)).collect(Collectors.toUnmodifiableSet()); | ||
| } | ||
|
|
||
| private static boolean isPrimary(BeanDefinitionRegistry registry, String beanName) { | ||
| return registry.getByName(beanName).stream().anyMatch(BeanDefinitionHolder::isPrimary); | ||
| } | ||
|
|
||
| private static boolean hasProfile(BeanDefinitionRegistry registry, String beanName) { | ||
| return registry.getByName(beanName).stream().anyMatch(bean -> bean.getProfiles() != null); | ||
| } |
There was a problem hiding this comment.
💡 Bug: isPrimary/hasProfile resolve bean names globally, ignoring the type
isPrimary and hasProfile call registry.getByName(beanName), which is a project-wide, type-agnostic lookup. When two beans of different types share a name (e.g. two @Bean factory methods both named dataSource in different configurations, or a @Component("service") colliding with a @Bean service() of another type), a @Primary/@Profile flag on the unrelated bean is attributed to the candidate of the type under analysis, silently suppressing (or altering) the ambiguity verdict. Filter the holders returned by getByName to those whose getType() is in the type hierarchy currently being examined, or index @Primary/@Profile per (type, name) pair.
Was this helpful? React with 👍 / 👎
| for (String type : typeToBeanNamesIndex.getKeys()) { | ||
| Set<String> candidates = typeToBeanNamesIndex.getNamesForType(type); | ||
| if (hasUniqueOrPrimaryCandidate(candidates, registry)) { | ||
| continue; | ||
| } | ||
| Set<InjectionPoint> injectionPoints = typeToDependenciesIndex.getDependenciesForType(type); | ||
| Set<String> effectiveCandidates = excludeCandidatesWithProfile(candidates, registry); | ||
| if (!hasUniqueOrPrimaryCandidate(effectiveCandidates, registry)) { | ||
| for (InjectionPoint unresolvedInjectionPoint : findInjectionPointsNotMatchingCandidateByName(candidates, injectionPoints)) { | ||
| issues.add(new SpringContextIssue(unresolvedInjectionPoint.location(), message(effectiveCandidates))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Quality: Issue message omits profiled candidates that caused the ambiguity
The ambiguity decision and the name-match filtering both use the full candidates set, but message(effectiveCandidates) renders only the profile-free subset. For a type with beans {a, b, c(@Profile("prod"))} the reported message is Multiple beans match this dependency (a, b), hiding c even though a @Qualifier towards c is one of the valid fixes the rule accepts (see qualifier_towards_profiled_candidate_is_not_flagged_as_ambiguous). Build the message from candidates so the listed names match the set the user must disambiguate against.
Report the full candidate set in the message.:
for (InjectionPoint unresolvedInjectionPoint : findInjectionPointsNotMatchingCandidateByName(candidates, injectionPoints)) {
issues.add(new SpringContextIssue(unresolvedInjectionPoint.location(), message(candidates)));
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| * | ||
| * @param tree The class tree to visit | ||
| */ | ||
| @Override |
There was a problem hiding this comment.
💡 Edge Case: BeanDefinitionGatherer skips @component records and enums
nodesToVisit() returns only Tree.Kind.CLASS, so a stereotype-annotated record (@Component record Foo(...) {}) or enum is never registered as a bean, and neither its own dependencies nor its contribution to TypeToBeanNamesIndex reach the model. The sibling ComponentScanPackageGatherer already visits CLASS and INTERFACE, so the two gatherers disagree on which declarations they consider. For S9352 this yields missed candidates (a second bean of a type declared as a record is invisible, so a genuinely ambiguous injection point is not reported).
Visit every ClassTree kind that can carry a stereotype annotation.:
@Override
public List<Tree.Kind> nodesToVisit() {
return List.of(Tree.Kind.CLASS, Tree.Kind.RECORD, Tree.Kind.ENUM);
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| static final String SOURCES_DIR = "src/main/java"; | ||
| static final String BINARIES_DIR = "target/classes"; | ||
|
|
||
| private static final long BUILD_TIMEOUT_SECONDS = 45; |
There was a problem hiding this comment.
💡 Quality: 45s Maven timeout will flake on a cold local repository
build() runs mvn clean compile dependency:build-classpath on the copied fixture projects, which declare Spring Boot dependencies. On a CI agent with a cold or partially populated ~/.m2, dependency resolution alone routinely exceeds BUILD_TIMEOUT_SECONDS = 45, and the test then fails with "Maven build timed out" rather than a real analyzer regression. Raise the timeout substantially (or make it configurable via a system property) so a slow download does not read as a test failure.
Make the build timeout generous and overridable.:
private static final long BUILD_TIMEOUT_SECONDS =
Long.getLong("sonar.its.mavenBuildTimeoutSeconds", 600L);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| protected List<FileIssue> analyze(Path projectDir, String... ruleKeys) { | ||
| Path resourceDir = resolveResourceDir(projectDir); | ||
|
|
||
| copyProjectTree(resourceDir); | ||
|
|
||
| MavenBuildHelper mavenHelper = new MavenBuildHelper(projectBaseDir); | ||
| mavenHelper.build(); | ||
|
|
||
| Map<String, List<Path>> moduleFiles = discoverModules(projectBaseDir); | ||
|
|
||
| var activeRules = Arrays.stream(ruleKeys) | ||
| .map(ScannerIntegrationAbstractTest::buildActiveRule) | ||
| .toList(); | ||
|
|
||
| var scannerProperties = buildScannerProperties(moduleFiles, mavenHelper); |
There was a problem hiding this comment.
💡 Quality: Unreachable no-Maven branch and unused module file lists in IT harness
analyze always constructs a MavenBuildHelper and passes it to buildScannerProperties, so the mavenHelper == null branch (and the defaultClasspath() helper it is the only caller of) is dead code that will silently rot. Likewise discoverModules walks every module and collects List<Path> javaFiles, but the returned map's values are never read — only keySet() is used for sonar.modules and the per-module property prefixes. Drop the unreachable branch and reduce discoverModules to returning the ordered set of module names it actually needs.
Was this helpful? React with 👍 / 👎
| classProfiles, | ||
| dependencies, | ||
| typeHierarchy); | ||
| beansCollectedAtFileLevel.add(beanData); |
There was a problem hiding this comment.
💡 Bug: Gathered beans/packages are lost when a sibling check throws
Both gatherers now publish a file's results into the module-level map (beansCollectedByFile / collectedPackagesByFile) only from leaveFile, whereas before this commit visitNode added them directly to collectedBeans / collectedPackages. In VisitorsBridge.IssuableSubscriptionVisitorsRunner.scanFile, visit(tree) throws CheckFailureException (raised by runScanner for any exception from any subscription visitor) before the forEach(..., s -> s.leaveFile(...)) line, and in non-fail-fast mode interruptIfFailFast swallows it and the analysis continues. Trigger: any one of the many IssuableSubscriptionVisitor checks crashes while visiting a Spring-annotated file; outcome: that file's bean definitions and component-scan packages never reach SpringContextModel, so cross-file rules such as AmbiguousDependencyCheck silently lose those candidates — previously they survived. Publishing in visitNode/collectBeanMethod (or accumulating into the per-file map as beans are found) restores the previous robustness.
Publish each bean into the per-file map as it is collected, so the data survives a sibling check failure; keep leaveFile only for the cache write.:
@Override
public void leaveFile(JavaFileScannerContext context) {
if (context.getCacheContext().isCacheEnabled()) {
SpringContextCacheHelper.writeBeanDefinitionsToCache(context, LOG, beansCollectedAtFileLevel);
}
beansCollectedAtFileLevel.clear();
}
// and, in visitNode/collectBeanMethod, after `beansCollectedAtFileLevel.add(beanData)`:
// beansCollectedByFile.put(context.getInputFile(), List.copyOf(beansCollectedAtFileLevel));
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
4 participants
Part of SONARJAVA-6237
Summary by Gitar
BeanDefinitionGathererto collect Spring bean definitions and dependencies@Qualifieror@PrimaryThis will update automatically on new commits.