SONARJAVA-6413 Implement BeanDefinitionGatherer to collect Spring bean definitions by asya-vorobeva · Pull Request #5665 · SonarSource/sonar-java

asya-vorobeva · 2026-06-10T08:37:14Z

Add BeanDefinitionGatherer that discovers beans from stereotype annotations (@component, @service, @repository, @controller, @RestController, @configuration) and @bean methods inside configuration/component classes
Capture @primary flag and dependencies (@Autowired fields, constructors, setters; @bean method parameters)
Register BeanDefinitionGatherer in SpringContextModelGatherers

What's not done here:

Didn't implement caching layer as it was done in ComponentScanPackageGatherer, needs to be added in following PRs
Ignored @qualifier annotations (most important) and @dependsOn / @lookup annotations (less important)
Didn't fill TypeToBeanNamesIndex, should be done when working on SONARJAVA-6412
To properly fill dependantBeans we need to do the second pass when beans from all modules will be collected. It should be done in aggregation step in SpringContextModelSensor, before to create and raise issues..

hashicorp-vault-sonar-prod · 2026-06-10T08:37:32Z

gitar-bot · 2026-06-10T09:07:22Z

+  private static List<String> collectAutowiredDependencies(ClassTree classTree) {
+    List<String> deps = new ArrayList<>();
+    for (Tree member : classTree.members()) {
+      if (member.is(Tree.Kind.VARIABLE)) {
+        VariableTree field = (VariableTree) member;
+        if (field.symbol().metadata().isAnnotatedWith(SpringUtils.AUTOWIRED_ANNOTATION)) {
+          deps.add(field.symbol().type().fullyQualifiedName());
+        }
+      } else if (member.is(Tree.Kind.CONSTRUCTOR, Tree.Kind.METHOD)) {
+        MethodTree method = (MethodTree) member;
+        if (method.symbol().metadata().isAnnotatedWith(SpringUtils.AUTOWIRED_ANNOTATION)) {
+          method.parameters().stream()
+            .map(p -> p.symbol().type().fullyQualifiedName())
+            .forEach(deps::add);
+        }


💡 Edge Case: Implicit single-constructor injection not captured as dependencies

collectAutowiredDependencies only records dependencies for fields/methods/constructors explicitly annotated with @Autowired (BeanDefinitionGatherer.java:206-224). Since Spring 4.3, a bean class with a single constructor is autowired implicitly without requiring @Autowired. Such beans will be registered with an empty dependingBeans list, so downstream checks relying on the dependency graph (e.g. unused/unsatisfied bean detection) will see incomplete data. Consider also collecting parameters of a class's sole constructor when no member is @Autowired-annotated.

_{Was this helpful? React with 👍 / 👎}

gitar-bot · 2026-06-10T09:48:49Z

@@ -43,16 +49,23 @@ public static String packageName(@Nullable PackageDeclarationTree packageDeclara
      pieces.push(separator);
      expr = mse.expression();
    }
-    if (expr.is(Tree.Kind.IDENTIFIER)) {
-      IdentifierTree idt = (IdentifierTree) expr;
-      pieces.push(idt.name());
-    }
-
+    pieces.push(((IdentifierTree) expr).name());
    StringBuilder sb = new StringBuilder();
-    for (String piece: pieces) {
+    for (String piece : pieces) {


💡 Edge Case: packageName removes guard, casts expr to IdentifierTree unconditionally

The refactored PackageUtils.packageName (PackageUtils.java:52) replaces the previous defensive block

if (expr.is(Tree.Kind.IDENTIFIER)) { ... }

with an unconditional cast pieces.push(((IdentifierTree) expr).name());. For normal qualified package declarations the leftmost expression is always an identifier, so this works for the cases covered by the new tests. However, the previous code tolerated a non-identifier leftmost expression by simply skipping it, whereas the new code will throw a ClassCastException. The new PackageUtilsTest does not exercise any non-identifier path (the old branch is now untested). This is low risk in practice but reduces robustness compared to the original; if you intend the cast to always be safe, a comment or an assert documenting the invariant would help, otherwise restore the guard.

_{Was this helpful? React with 👍 / 👎}

@service

…n definitions - Add BeanDefinitionGatherer that discovers beans from stereotype annotations (@component, @service, @repository, @controller, @RestController, @configuration) and @bean methods inside configuration/component classes - Capture @primary flag and dependencies (@Autowired fields, constructors, setters; @bean method parameters) - Register BeanDefinitionGatherer in SpringContextModelGatherers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

aurelien-coet-sonarsource

Overall LGTM, just a few small comments. I think that the first of the two remaining suggestions by the Gitar bot is also correct ?

aurelien-coet-sonarsource · 2026-06-10T14:08:15Z

+      for (MethodTree method : methodsOf(classTree)) {
+        if (method.symbol().metadata().isAnnotatedWith(SpringUtils.BEAN_ANNOTATION)) {
+          collectBeanMethod(method, pkg);
+        }
+      }


Suggestion: the call to methodsOf needs to iterate over the class members to filter them and only keep methods, then instantiates a list, then this list is iterated over again. It would probably be more efficient to directly operate on the stream:

Suggested change

for (MethodTree method : methodsOf(classTree)) {

if (method.symbol().metadata().isAnnotatedWith(SpringUtils.BEAN_ANNOTATION)) {

collectBeanMethod(method, pkg);

}

}

classTree.members().stream()

.filter(m -> m.is(Tree.Kind.METHOD))

.map(MethodTree.class::cast)

.filter(m -> m.symbol().metadata().isAnnotatedWith(SpringUtils.BEAN_ANNOTATION))

.forEach(method -> collectBeanMethod(method, pkg));

I extract the method already used in another visitor to SpringUtils.

aurelien-coet-sonarsource · 2026-06-10T14:12:13Z

+    return false;
+  }
+
+  private static boolean isSpringBeanDefinitionsClass(SymbolMetadata meta) {


Would it make sense to move this method in SpringUtils ?

Better solution is to remove this method :) I do it in following commit

aurelien-coet-sonarsource · 2026-06-11T12:01:02Z

-  // ---- ComponentScanPackageGatherer -----------------------------------------
-
-  @Test
-  void componentScanPackageGatherer_collects_package_from_springBootApplication() {


Why was this test deleted ?

Because we provide the tests for this gatherer in ComponentScanPackageGathererTest.

Ah ok, got it!

asya-vorobeva · 2026-06-11T12:19:29Z

Implicit single-constructor injection not captured as dependencies

This one as added as TODO in Jira ticket. Feel free to do it in follow-up

packageName removes guard, casts expr to IdentifierTree unconditionally

This is really super-low risk, I don't think that such situations will happen in real-world apps.

sonarqube-next · 2026-06-11T12:31:38Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
92.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

gitar-bot · 2026-06-11T12:49:57Z

Code Review 👍 Approved with suggestions 4 resolved / 6 findings

Implements BeanDefinitionGatherer to discover Spring bean definitions and their dependencies. Consider explicitly handling implicit constructor injection and adding null-safety guards to the package identifier casting logic.

💡 Edge Case: Implicit single-constructor injection not captured as dependencies

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:206-220

collectAutowiredDependencies only records dependencies for fields/methods/constructors explicitly annotated with @Autowired (BeanDefinitionGatherer.java:206-224). Since Spring 4.3, a bean class with a single constructor is autowired implicitly without requiring @Autowired. Such beans will be registered with an empty dependingBeans list, so downstream checks relying on the dependency graph (e.g. unused/unsatisfied bean detection) will see incomplete data. Consider also collecting parameters of a class's sole constructor when no member is @Autowired-annotated.

💡 Edge Case: packageName removes guard, casts expr to IdentifierTree unconditionally

📄 java-frontend/src/main/java/org/sonar/java/utils/PackageUtils.java:40-54

The refactored PackageUtils.packageName (PackageUtils.java:52) replaces the previous defensive block

if (expr.is(Tree.Kind.IDENTIFIER)) { ... }

with an unconditional cast pieces.push(((IdentifierTree) expr).name());. For normal qualified package declarations the leftmost expression is always an identifier, so this works for the cases covered by the new tests. However, the previous code tolerated a non-identifier leftmost expression by simply skipping it, whereas the new code will throw a ClassCastException. The new PackageUtilsTest does not exercise any non-identifier path (the old branch is now untested). This is low risk in practice but reduces robustness compared to the original; if you intend the cast to always be safe, a comment or an assert documenting the invariant would help, otherwise restore the guard.

✅ 4 resolved

✅ Bug: BeanDefinitionGatherer loses beans on incremental (cached) analysis

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:76-90 📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/ComponentScanPackageGatherer.java:78-92
BeanDefinitionGatherer collects beans only in visitNode, which runs only when a file is parsed. It does not override scanWithoutParsing/leaveFile, so it inherits the default scanWithoutParsing that returns true (JavaFileScanner.java:42-43). In VisitorsBridge/IssuableSubscriptionVisitorsRunner the per-scanner results are AND-combined: a file is parsed only if some scanner returns false, otherwise it is skipped entirely with no visitNode call.

During an incremental analysis where a Spring source file is unchanged, ComponentScanPackageGatherer returns true from its cache-backed scanWithoutParsing (it persists per-file data via leaveFile and restores it from cache), and BeanDefinitionGatherer also returns true by default. With all gatherers returning true, the file is skipped, visitNode never runs, and the beans for that file are neither recollected nor restored from any cache. The resulting BeanDefinitionRegistry is incomplete on cached runs, unlike ComponentScanPackageGatherer which explicitly handles this case. The base class Javadoc even calls out this caching requirement.

Fix options: (a) implement the same cache read/write pattern as ComponentScanPackageGatherer (preferred — keeps incremental performance), or (b) at minimum override scanWithoutParsing to return false so the file is always parsed (correct but defeats incremental skipping for the whole module).

✅ Quality: isConfigurationOrComponentClass duplicates isSpringBeanClass set

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:130-141 📄 java-frontend/src/main/java/org/sonar/java/utils/SpringUtils.java:46-53
isConfigurationOrComponentClass(meta) (BeanDefinitionGatherer.java:134-141) checks exactly the same six annotations that make up SpringUtils.STEREOTYPE_ANNOTATIONS used by isSpringBeanClass(meta) (line 130-132): CONFIGURATION, COMPONENT, SERVICE, REPOSITORY, CONTROLLER, REST_CONTROLLER. The two predicates are therefore functionally identical, but one is expressed as a stream over the shared constant and the other as a hand-maintained chain of ||. This duplication is a maintenance hazard: if STEREOTYPE_ANNOTATIONS is later extended (or trimmed), the two methods will silently diverge and @Bean-method scanning will no longer match class-level bean detection. Consider having isConfigurationOrComponentClass delegate to isSpringBeanClass, or removing it entirely and reusing the same check.

✅ Quality: Redundant duplicate isSpringBeanClass(meta) check in visitNode

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:85-99
In BeanDefinitionGatherer.visitNode, isSpringBeanClass(meta) is evaluated twice — once at line 86 to register the class-level bean and again at line 99 to gate @Bean method collection. Both blocks are guarded by the identical condition with no intervening mutation of meta, so the second evaluation is redundant. Merging the two blocks into a single if (isSpringBeanClass(meta)) { ... } improves clarity and avoids the duplicate stream traversal over STEREOTYPE_ANNOTATIONS.

✅ Edge Case: defaultBeanName diverges from Spring's decapitalization rule

📄 java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:160-162
defaultBeanName lowercases the first character unconditionally. Spring's AnnotationBeanNameGenerator uses java.beans.Introspector.decapitalize, which leaves the name unchanged when the first two characters are both uppercase (e.g. URLParser -> URLParser, ABCService -> ABCService). For such class names the gatherer will register a bean under a name (uRLParser) that does not match the actual Spring bean name, causing later lookups by name to miss. Consider using Introspector.decapitalize(simpleName) to mirror Spring's behavior exactly.

🤖 Prompt for agents

Code Review: Implements BeanDefinitionGatherer to discover Spring bean definitions and their dependencies. Consider explicitly handling implicit constructor injection and adding null-safety guards to the package identifier casting logic.

1. 💡 Edge Case: Implicit single-constructor injection not captured as dependencies
   Files: java-frontend/src/main/java/org/sonar/java/model/springcontext/BeanDefinitionGatherer.java:206-220

   `collectAutowiredDependencies` only records dependencies for fields/methods/constructors explicitly annotated with `@Autowired` (BeanDefinitionGatherer.java:206-224). Since Spring 4.3, a bean class with a single constructor is autowired implicitly without requiring `@Autowired`. Such beans will be registered with an empty `dependingBeans` list, so downstream checks relying on the dependency graph (e.g. unused/unsatisfied bean detection) will see incomplete data. Consider also collecting parameters of a class's sole constructor when no member is `@Autowired`-annotated.

2. 💡 Edge Case: packageName removes guard, casts expr to IdentifierTree unconditionally
   Files: java-frontend/src/main/java/org/sonar/java/utils/PackageUtils.java:40-54

   The refactored `PackageUtils.packageName` (PackageUtils.java:52) replaces the previous defensive block
   
     if (expr.is(Tree.Kind.IDENTIFIER)) { ... }
   
   with an unconditional cast `pieces.push(((IdentifierTree) expr).name());`. For normal qualified package declarations the leftmost expression is always an identifier, so this works for the cases covered by the new tests. However, the previous code tolerated a non-identifier leftmost expression by simply skipping it, whereas the new code will throw a ClassCastException. The new PackageUtilsTest does not exercise any non-identifier path (the old branch is now untested). This is low risk in practice but reduces robustness compared to the original; if you intend the cast to always be safe, a comment or an assert documenting the invariant would help, otherwise restore the guard.

Options

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

Comment with these commands to change:

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

_{Was this helpful? React with 👍 / 👎 | Gitar}

asya-vorobeva marked this pull request as draft June 10, 2026 08:37