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}
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.
64fade8 to
c946cf0
Compare
| @Configuration | ||
| public class CacheConfig { | ||
|
|
||
| @Bean |
There was a problem hiding this comment.
Let's configure these beans in different modules / files to be sure that it recognizes such cases.
| + " disambiguate it with \"@Qualifier\" or mark one bean as \"@Primary\"."; | ||
|
|
||
| @Override | ||
| public List<SpringContextIssue> execute(SpringContextModel model) { |
There was a problem hiding this comment.
This method is super-hard to read. Let's add Javadoc comment describing how it acts. If you will fill that it's not enough, add additional internal comments in the method.
asya-vorobeva
left a comment
asya-vorobeva
left a comment
There was a problem hiding this comment.
This PR became too huge. Would be great to separate it into several ones: one with SpringModel refactoring, another with rule implementation (including tests). Or at least to re-group-squash commits to have clear history and when merging not do squash.
| } | ||
|
|
||
| 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.
I couldn't find a place where we add profiles information to the bean holder. Probably we missed it? Should be done in BeanDefinitionGatherer.
| import org.sonar.java.test.classpath.TestClasspathUtils; | ||
|
|
||
| @Execution(ExecutionMode.CONCURRENT) | ||
| @Execution(ExecutionMode.SAME_THREAD) |
There was a problem hiding this comment.
💡 Quality: junit-platform.properties still documents/enables removed CONCURRENT mode
ScannerIntegrationAbstractTest is now @Execution(ExecutionMode.SAME_THREAD), so no test class in its/scanner-integration-tests runs concurrently, yet junit-platform.properties still enables parallel execution and its comment explicitly states "Subclasses of ScannerIntegrationAbstractTest are annotated with @execution(CONCURRENT); these properties activate it" — the only two subclasses (AmbiguousDependencyCrossModuleTest, SpringBeansShouldBeAccessibleCrossModuleTest) inherit SAME_THREAD. A maintainer reading the properties file will believe these ITs run in parallel and may "restore" concurrency, re-introducing whatever flakiness this commit removed. Update the comment (and drop the now-inert parallel properties, or note why they are kept) so config and code agree.
Align the properties file with the SAME_THREAD annotation and explain why.:
# Parallel execution is intentionally disabled: ScannerIntegrationAbstractTest is
# annotated with @Execution(SAME_THREAD) because the scanner runs share static state
# (plugin location, runner config) and must not overlap.
junit.jupiter.execution.parallel.enabled=false
Was this helpful? React with 👍 / 👎
CI failed: All CI workflows completed successfully with no actual build or test errors detected.OverviewAll 25 analyzed CI job logs show successful execution of the workflow, including the autoscan diff report generation and artifact upload. Only standard runner deprecation warnings and informational log messages were present. FailuresNone Detected (confidence: high)
Summary
Code Review 👍 Approved with suggestions 13 resolved / 14 findingsImplements Spring rule S9352 to detect ambiguous bean autowiring without Consider updating 💡 Quality: junit-platform.properties still documents/enables removed CONCURRENT mode📄 its/scanner-integration-tests/src/test/java/org/sonar/java/it/ScannerIntegrationAbstractTest.java:56
Align the properties file with the SAME_THREAD annotation and explain why.✅ 13 resolved✅ Bug: Rule flags its own documented @fallback compliant example
✅ Bug: One resolved injection point hides other ambiguous ones
✅ Bug: False positive when @qualifier is declared on the bean itself
✅ Edge Case: Two @primary candidates silently treated as resolved
✅ Quality: Two-candidate fallback test passes even if exclusion is broken
...and 8 more resolved from earlier reviews 🤖 Prompt for agentsTip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
2 participants
Summary by Gitar
S9352to detect ambiguous bean autowiring without@Qualifieror@PrimaryTypeToDependenciesIndexand integrated it intoBeanDefinitionGathererto track injection pointsThis will update automatically on new commits.