Skip to content

SONARJAVA-6791 Handle @Qualifier annotations on autowired dependencies in BeanDefinitionGatherer - #5936

Open
NoemieBenard wants to merge 8 commits into
epic-SONARJAVA-6237from
nb/sonarjava-6791-qualifier-handling
Open

SONARJAVA-6791 Handle @Qualifier annotations on autowired dependencies in BeanDefinitionGatherer#5936
NoemieBenard wants to merge 8 commits into
epic-SONARJAVA-6237from
nb/sonarjava-6791-qualifier-handling

Conversation

@NoemieBenard

@NoemieBenard NoemieBenard commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary by Gitar

  • Spring context model updates:
    • Introduced BeanDependency record to capture dependency type and optional @Qualifier value.
    • Updated BeanDefinitionGatherer to extract @Qualifier annotations on fields, constructors, and @Bean methods.

This will update automatically on new commits.

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6791

@NoemieBenard
NoemieBenard marked this pull request as ready for review August 18, 2026 12:11
@NoemieBenard
NoemieBenard force-pushed the nb/sonarjava-6791-qualifier-handling branch from cf0c655 to ef6cdf8 Compare August 18, 2026 15:42
NoemieBenard and others added 2 commits August 18, 2026 17:49
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
/** Names of other beans this bean depends on. */
private List<String> dependingBeans;
/** Dependencies this bean requires, each capturing the required type and an optional {@code @Qualifier} name. */
private List<BeanDependency> dependingBeans;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list can be rather large, and we need to have fast look-up to be able to define needed dependency. I'd suggest to think about using Map / Set, the key should be the bean's name. In Spring framework this should be qualifier or type.

@gitar-bot

gitar-bot Bot commented Aug 21, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 5 resolved / 5 findings

Adds @Qualifier annotation handling on autowired dependencies in BeanDefinitionGatherer and refactors depending beans to a map, addressing all import ordering, serialization, and collision findings.

✅ 5 resolved
Quality: Redundant same-package import breaks import ordering

📄 java-frontend/src/test/java/org/sonar/java/model/springcontext/BeanDefinitionGathererTest.java:20
import org.sonar.java.model.springcontext.BeanDependency; is redundant because the test class is in the same package (org.sonar.java.model.springcontext), and it is inserted between the two java.util.* imports, breaking alphabetical ordering. Remove this import line.

Quality: Duplicated BeanData construction into two parallel lists

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:116-127 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:304-315
In both visitNode and collectBeanMethod an identical new BeanData(...) is built and added to collectedBeans and again to beansCollectedAtFileLevel. The two lists must stay in sync; a future edit to one construction site that forgets the other would silently make the cache write diverge from the in-memory module data. Build the BeanData once into a local variable and add the same instance to both lists to remove the duplication and the sync hazard.

Bug: Qualifier values written raw to cache break serialization

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:158-171 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:231-237 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:322-336
The removed serializeDependency/deserializeDependency Base64-encoded the qualifier before writing it to the cache. The new format writes key:value with the key (a @Qualifier value, i.e. an arbitrary annotation string) unencoded. If a qualifier contains : (DEP_KEY_VALUE_SEPARATOR), , (DEP_SEPARATOR), | (FIELD_SEPARATOR) or a newline (BEAN_SEPARATOR), serialization/deserialization corrupts: e.g. @Qualifier("a:b") deserializes with key a and a truncated value, and @Qualifier("a,b") makes entry.indexOf(...) return -1 and throws in substring, discarding the whole file's cache. Restore Base64 encoding for the key (and value if needed) so arbitrary qualifier strings round-trip safely.

Quality: Doc comment says decapitalized class name, code uses field name

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionHolder.java:54-58 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:301 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:317 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:322-324
The new Javadoc states the key is "the default Spring bean name (simple class name, decapitalized)" when no @Qualifier is present, but dependencyKey actually uses field.simpleName().name() / parameter name. These differ whenever a field/param is named differently from its decapitalized type (e.g. ApplicationContext ctx). Align the comment with the implementation (or vice versa) to avoid confusion for downstream consumers.

Edge Case: Map keyed by field/param name silently drops colliding dependencies

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:295-309 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:322-324
Switching dependingBeans from a List to a Map keyed by qualifier-or-field/param-name means two distinct dependencies that share a key collapse into one, silently losing a dependency the old list preserved. Collisions are reachable: e.g. an @Autowired field named service plus an @Autowired setter/constructor param also named service, or a @Qualifier("x") dependency alongside another dependency whose field name is x (putAll/put overwrite). Consider keying by a guaranteed-unique value or keeping a collection of dependencies per key if multiple injection points must be retained.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqube-next

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants