SONARJAVA-6421 Implement S9352: Bean autowiring ambiguity should be resolved using "@Qualifier" or "@Primary" - #6044
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| private static boolean isResolved(Set<String> candidates, Set<String> injectionPointNames, BeanDefinitionRegistry registry) { | ||
| // injectionPointNames merges every injection point of this type declared on the bean: it is only resolved | ||
| // if EVERY one of them names a candidate (by bean name or by a @Qualifier declared on that candidate bean | ||
| // itself), otherwise at least one injection point remains ambiguous. | ||
| return candidates.size() <= 1 | ||
| || injectionPointNames.stream().allMatch(name -> matchesCandidate(name, candidates, registry)) | ||
| || candidates.stream().anyMatch(candidate -> isPrimary(registry, candidate)); | ||
| } | ||
|
|
||
| private static boolean matchesCandidate(String injectionPointName, Set<String> candidates, BeanDefinitionRegistry registry) { | ||
| return candidates.contains(injectionPointName) | ||
| || candidates.stream().anyMatch(candidate -> injectionPointName.equals(qualifierOf(registry, candidate))); | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
⚠️ Bug: Field name matched against a bean's own @qualifier hides ambiguity
dependencyKey collapses an injection point's @qualifier value and its field/parameter name into the same string, so matchesCandidate cannot tell them apart and now accepts a plain field name as a match against a candidate bean's own declared @qualifier. Trigger: @Component @Qualifier("mainService") class A implements Svc, @Component class B implements Svc, and a consumer with @Autowired private Svc mainService; (no @qualifier at the injection point). Spring's by-name fallback (DefaultListableBeanFactory.determineAutowireCandidate -> matchesBeanName) only compares bean names and aliases, never a bean's @qualifier value, so Spring fails with NoUniqueBeanDefinitionException while the rule now stays silent (false negative). Fix by marking qualifier-derived names in the dependency model so the qualifier-vs-qualifier comparison is only applied to injection points that actually declare @qualifier.
Distinguish qualifier-derived injection point names from field/parameter names (serialization/deserialization keeps working since the marker is inside the Base64-encoded name).:
// BeanDefinitionGatherer: tag qualifier-derived names so consumers can distinguish them
private static final String QUALIFIER_MARKER = "@";
private static String dependencyKey(String fieldOrParamName, @Nullable String qualifier) {
return qualifier != null ? QUALIFIER_MARKER + qualifier : fieldOrParamName;
}
// AmbiguousDependencyCheck: only match against a candidate's own @Qualifier for
// injection points that declared one
private static boolean matchesCandidate(String injectionPointName, Set<String> candidates, BeanDefinitionRegistry registry) {
if (injectionPointName.startsWith("@")) {
String qualifier = injectionPointName.substring(1);
return candidates.contains(qualifier)
|| candidates.stream().anyMatch(c -> qualifier.equals(qualifierOf(registry, c)));
}
return candidates.contains(injectionPointName);
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| private static boolean matchesCandidate(String injectionPointName, Set<String> candidates, BeanDefinitionRegistry registry) { | ||
| return candidates.contains(injectionPointName) | ||
| || candidates.stream().anyMatch(candidate -> injectionPointName.equals(qualifierOf(registry, candidate))); | ||
| } | ||
|
|
||
| @Nullable | ||
| private static String qualifierOf(BeanDefinitionRegistry registry, String beanName) { | ||
| return registry.getByName(beanName).stream() | ||
| .map(BeanDefinitionHolder::getQualifier) | ||
| .filter(Objects::nonNull) | ||
| .findFirst() | ||
| .orElse(null); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Several candidates sharing one @qualifier value counted as resolved
matchesCandidate uses anyMatch, so an injection point is considered resolved as soon as at least one candidate carries the matching qualifier. Trigger: two beans of the same type both annotated @Qualifier("main") plus a consumer with @Qualifier("main") — Spring still throws NoUniqueBeanDefinitionException, but the rule reports nothing. The same holds when one candidate's bean name and another candidate's @qualifier value both equal the injection point name. Require exactly one matching candidate instead of at least one.
Treat the injection point as resolved only when a single candidate matches by name or by its own @qualifier.:
private static boolean matchesCandidate(String injectionPointName, Set<String> candidates, BeanDefinitionRegistry registry) {
return candidates.stream()
.filter(candidate -> candidate.equals(injectionPointName)
|| injectionPointName.equals(qualifierOf(registry, candidate)))
.count() == 1;
}
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| var encodedQualifier = bean.qualifier() != null | ||
| ? Base64.getEncoder().encodeToString(bean.qualifier().getBytes(StandardCharsets.UTF_8)) | ||
| : ""; |
There was a problem hiding this comment.
💡 Quality: Bean-level qualifier cache round-trip is never exercised by tests
The new bean-level qualifier field was added to the cache format, but every updated test literal uses the empty (||) form, and no test scans a class annotated with a class-level @Qualifier to assert BeanDefinitionHolder.getQualifier() or a non-empty serialize/deserialize round-trip. A regression in that field (wrong index, missing Base64 encoding) would silently drop qualifiers on cache hits during incremental analysis, turning the new resolution logic back into false positives, and no test would fail. Add a gatherer test with a @Qualifier-annotated component asserting getQualifier() after a normal scan and after scanWithoutParsing restores it from cache.
Was this helpful? React with 👍 / 👎
This comment has been minimized.
This comment has been minimized.
| TypeToBeanNamesIndex typeToBeanNamesIndex = model.getTypeToBeanNamesIndex(); | ||
|
|
||
| List<AmbiguousDependency> ambiguousDependencies = new ArrayList<>(); | ||
| for (BeanDefinitionHolder bean : registry.getAll()) { |
There was a problem hiding this comment.
Here makes sense to iterate in outer loop through TypeToBeanNamesIndex map to inspect types, then for each type iterate through specific beans using BeanDefinitionRegistry.
| // itself), otherwise at least one injection point remains ambiguous. | ||
| return candidates.size() <= 1 | ||
| || injectionPointNames.stream().allMatch(name -> matchesCandidate(name, candidates, registry)) | ||
| || hasExactlyOnePrimaryCandidate(candidates, registry); |
There was a problem hiding this comment.
Here it's very important to check for having Profile set for the candidates. Candidates having configured profile should be excluded from consideration as possibly mutually exclusive.
| if (!isResolved(candidates, dependency.getValue(), registry)) { | ||
| // A @Fallback candidate is only a real contender when it is the sole remaining one; otherwise it is | ||
| // ignored by Spring, so the effective candidates are whichever bean(s) are not marked @Fallback. | ||
| Set<String> effectiveCandidates = excludeFallbackCandidates(candidates, registry); |
There was a problem hiding this comment.
Fallback annotation is modern (introduced in Spring Framework 6.2) and thus currently low used. We can add it later in follow-up ticket.
| // ignored by Spring, so the effective candidates are whichever bean(s) are not marked @Fallback. | ||
| Set<String> effectiveCandidates = excludeFallbackCandidates(candidates, registry); | ||
| if (effectiveCandidates.size() > 1) { | ||
| ambiguousDependencies.add(new AmbiguousDependency(bean.getLocation(), message(requiredType, effectiveCandidates))); |
There was a problem hiding this comment.
As we've discussed, another alternative is to raise issues on project level instead. Drawback is that we won't see it in SonarLint. So the current approach looks appropriate.
| @Override | ||
| public void execute(SensorContext context) { | ||
| // Nothing to do for now | ||
| reportAmbiguousDependencies(context); |
There was a problem hiding this comment.
Here we have in mind that we'll implement multiple checks which will be called here. So I'd recommend to use Strategy design pattern.
For it we need to create some common interface with some execute method which all the needed checks will implement, and common record with information needed to create an issue (generalize AmbiguousDependency).
Then we can inject all of them into this sensor (injection mechanism is on you) and run execute method for all of them in a loop.
asya-vorobeva
left a comment
asya-vorobeva
left a comment
There was a problem hiding this comment.
To properly test such checks, please use scanner-integration-framework capabilities provided in this ticket.
CI failed: CI workflow completed successfully with all tests passing and a diff report artifact generated.OverviewAll 12 analyzed logs indicate that the CI workflow completed successfully. The autoscan tests and diff report generation executed as expected without any blocking errors or test failures. FailuresNo Failures Detected (confidence: high)
Summary
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
2 participants
Summary by Gitar
AmbiguousDependencyCheckto detect Spring bean autowiring ambiguity and support@FallbackconfigurationThis will update automatically on new commits.