Skip to content

SONARJAVA-6491: Implement S8911 - Methods annotated with @Startup should be non-static, non-producer, and parameter-free#5694

Merged
romainbrenguier merged 10 commits into
masterfrom
new-rule/SONARJAVA-6491-S8911
Jun 25, 2026
Merged

SONARJAVA-6491: Implement S8911 - Methods annotated with @Startup should be non-static, non-producer, and parameter-free#5694
romainbrenguier merged 10 commits into
masterfrom
new-rule/SONARJAVA-6491-S8911

Conversation

@romainbrenguier

Copy link
Copy Markdown
Contributor

Summary

This PR implements rule S8911 to detect methods annotated with @Startup (from io.quarkus.runtime.Startup) that violate CDI initialization requirements by being static, having parameters, or being annotated with @Produces.

Rule Behavior

The rule detects methods annotated with @Startup that:

  1. Are declared as static (static methods cannot be managed by CDI lifecycle)
  2. Have one or more parameters (framework cannot provide arguments during automatic invocation)
  3. Are annotated with @Produces (producer methods serve a different purpose than initialization)

Implementation Approach

The implementation follows the pattern established by ScheduledOnlyOnNoArgMethodCheck (S7184):

  1. Extends IssuableSubscriptionVisitor to subscribe to Tree.Kind.METHOD nodes
  2. For each method, checks if it has the @Startup annotation (io.quarkus.runtime.Startup)
  3. If @Startup is present, checks for three violations:
    • Method is static (using methodTree.symbol().isStatic())
    • Method has parameters (using !methodTree.parameters().isEmpty())
    • Method has @Produces annotation (checking for jakarta.enterprise.inject.Produces)
  4. Reports issues with descriptive messages indicating which requirement is violated
  5. Includes secondary locations pointing to the @Startup annotation for context

Test Coverage

The test file covers:

Noncompliant patterns:

  • Static method with @Startup
  • Method with @Startup and parameters
  • Method with both @Startup and @Produces
  • Multiple violations on same method (static + parameters, static + producer, etc.)

Compliant patterns:

  • Non-static, parameter-free method with @Startup (valid usage)
  • Methods using dependency injection (@Inject fields) instead of parameters
  • Methods without @Startup annotation (static, producer, or parameterized methods are OK)
  • Private methods with @Startup (access level doesn't matter for this rule)

Additional Changes

  • Added mock io.quarkus.runtime.Startup annotation in java-checks-test-sources/default/src/main/java/io/quarkus/runtime/Startup.java following the pattern used for other external dependencies (similar to io.realm.RealmConfiguration)
  • Test sample uses existing Jakarta dependencies (jakarta.enterprise.cdi-api and javax.inject) already available in the test sources pom

Resources

🤖 Generated with Claude Code

@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6491

Comment thread its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java
Comment thread its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json
@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6491-S8911 branch from 7474c91 to f20a9ac Compare June 23, 2026 14:42
Comment thread sonar-java-plugin/META-INF/MANIFEST.MF Outdated
@romainbrenguier romainbrenguier force-pushed the new-rule/SONARJAVA-6491-S8911 branch from b6d365b to 93ac858 Compare June 24, 2026 08:22
@romainbrenguier romainbrenguier requested a review from rombirli June 24, 2026 14:55
@romainbrenguier romainbrenguier marked this pull request as ready for review June 24, 2026 14:55

@rombirli rombirli left a comment

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.

LGTM. Please be careful about https://github.com/SonarSource/rspec/pull/7152#discussion_r3472686968 during validation

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.

Rspec PR says it's only in Sonar-Way which seems correct. Could you update rule metadata to exclude it from agentic ai profile

assertThat(actualProfile.isDefault()).isFalse();
assertThat(actualProfile.rules())
.hasSize(465)
.hasSize(466)

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.

Revert

Comment on lines +29 to +30
private static final String STARTUP_FQN = "io.quarkus.runtime.Startup";
private static final String PRODUCES_FQN = "jakarta.enterprise.inject.Produces";

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.

Usually we prefer matchers

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.

I don't know if it's possible, I never did it for annotations. Optional : let's give it a try with an agent

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After exploring the codebase, the current pattern of using symbolType().is(FQN) for annotation checking is actually the standard approach used throughout the java-checks module.

MethodMatchers is specifically designed for matching method invocations by type, name, and parameters - it doesn't support annotation matching. For annotation checks, the codebase consistently uses either:

  1. annotation.annotationType().symbolType().is(FQN) when working with AnnotationTree (syntax tree level)
  2. symbol.metadata().isAnnotatedWith(FQN) for simple presence checks

The current implementation follows pattern #1, which is appropriate here since we need to create secondary locations pointing to the specific AnnotationTree objects. Examples using the same pattern include:

  • TransactionalMethodVisibilityCheck.java:69-74
  • NullableInjectedFieldsHaveDefaultValueCheck.java:161-165

Given that your follow-up comment indicates this is optional and you're uncertain if matchers work for annotations, I believe the current implementation is correct as-is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After searching the codebase, there doesn't appear to be a built-in annotation matcher utility (like MethodMatchers for method calls). Other similar annotation-based checks (e.g., ScheduledOnlyOnNoArgMethodCheck.java:66) use the same pattern: .filter(annotation -> annotation.annotationType().symbolType().is(FQN)).

Creating an annotation matcher would be a larger architectural addition. Since this is marked optional, I'm keeping the current approach which follows the established pattern in the codebase.

Comment on lines +47 to +48
.map(annotation ->
new JavaFileScannerContext.Location("Triggered by this annotation", annotation.annotationType()))

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.

code style suggestion

Suggested change
.map(annotation ->
new JavaFileScannerContext.Location("Triggered by this annotation", annotation.annotationType()))
.map(annotation ->
new JavaFileScannerContext.Location("Triggered by this annotation", annotation.annotationType())
)

Comment on lines +63 to +67
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should not be applied to producer methods",
secondaryLocations,
null);

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.

code style suggestion

Suggested change
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should not be applied to producer methods",
secondaryLocations,
null);
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should not be applied to producer methods",
secondaryLocations,
null
);

Comment on lines +72 to +76
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should only be applied to no-arg methods",
secondaryLocations,
null);

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.

Suggested change
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should only be applied to no-arg methods",
secondaryLocations,
null);
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should only be applied to no-arg methods",
secondaryLocations,
null
);

Comment on lines +53 to +57
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should not be applied to static methods",
secondaryLocations,
null);

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.

Suggested change
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should not be applied to static methods",
secondaryLocations,
null);
reportIssue(
methodTree.simpleName(),
"\"@Startup\" annotation should not be applied to static methods",
secondaryLocations,
null
);

Comment thread its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json
…sources/org/sonar/l10n/java/rules/java/Sonar_agentic_AI_profile.json

Comment: [Rspec PR](https://github.com/SonarSource/rspec/pull/7152/changes#diff-de722320e2e762504eb80a52f56b8891ffe8c3076d3524802ac0208edd9e33adR19) says it's only in Sonar-Way which seems correct. Could you update rule metadata to exclude it from agentic ai profile
…/sonar/java/checks/StartupAnnotationCheck.java

Comment: code style suggestion

```suggestion
      .map(annotation ->
        new JavaFileScannerContext.Location("Triggered by this annotation", annotation.annotationType())
      )
```
…/sonar/java/checks/StartupAnnotationCheck.java

Comment: code style suggestion

```suggestion
      reportIssue(
        methodTree.simpleName(),
        "\"@startup\" annotation should not be applied to producer methods",
        secondaryLocations,
        null
      );
```
…/sonar/java/checks/StartupAnnotationCheck.java

Comment: ```suggestion
      reportIssue(
        methodTree.simpleName(),
        "\"@startup\" annotation should not be applied to static methods",
        secondaryLocations,
        null
      );
```
@sonarqube-next

Copy link
Copy Markdown

@romainbrenguier romainbrenguier enabled auto-merge (squash) June 25, 2026 11:58
@romainbrenguier romainbrenguier merged commit 3570287 into master Jun 25, 2026
15 checks passed
@romainbrenguier romainbrenguier deleted the new-rule/SONARJAVA-6491-S8911 branch June 25, 2026 12:02
@gitar-bot

gitar-bot Bot commented Jun 25, 2026

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

Implements rule S8911 to detect non-compliant Quarkus @Startup methods while resolving previous issues regarding profile registration, inconsistent test cleanup, and accidental inclusion of build artifacts.

✅ 4 resolved
Bug: S8911 not added to Sonar_way_profile.json

📄 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8911.json:3-15
The new rule S8911 has "status": "ready" and "defaultSeverity": "Critical" in S8911.json, but the key "S8911" was never added to the checked-in Sonar_way_profile.json rule list. As a result the rule will be shipped but will not be activated in the default "Sonar way" quality profile, so it will never fire for users relying on that profile. The existing profile test (JavaSonarWayProfileTest) only asserts a minimum rule count, so this omission is not caught by tests. Add "S8911" to the ruleKeys array in Sonar_way_profile.json (in the correct sorted position) so the new rule is enabled by default like other ready/critical rules.

Quality: activateLicense() removed only from AutoScanTest, inconsistent with other ITs

📄 its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java:66
The delta removes .activateLicense() from the OrchestratorRule builder in AutoScanTest, while keeping .setEdition(ENTERPRISE_LW). Other integration-test suites that target the same ENTERPRISE_LW edition still call .activateLicense() (e.g. its/plugin/tests/.../JavaTestSuite.java:68, CacheEnabledTest.java:69, JspTest.java:57,62, its/ruling/.../JavaRulingTest.java:110).

If activateLicense() is genuinely no longer required (e.g. the license is now injected via environment/CI), the same removal should be applied consistently across all ENTERPRISE_LW suites; otherwise this divergence is confusing and may indicate the AutoScanTest will silently fail to obtain enterprise features in some environments. Please confirm the removal is intentional and align the other suites, or restore the call.

Quality: Build artifacts (MANIFEST.MF, .class files) committed to source control

📄 sonar-java-plugin/META-INF/MANIFEST.MF:1-8
This commit accidentally adds generated build output to the repository instead of source files:

  • sonar-java-plugin/META-INF/MANIFEST.MF (contains a placeholder Build-Time: _, a hardcoded Implementation-Build commit hash, and a snapshot version)
  • sonar-java-plugin/org/sonar/java/CheckListGenerator.class
  • sonar-java-plugin/org/sonar/java/CheckListGenerator$Metadata.class
  • sonar-java-plugin/org/sonar/java/GeneratedCheckList.class
  • sonar-java-plugin/org/sonar/java/package-info.class

I verified these are now tracked by git (git ls-files) and are not covered by .gitignore (git check-ignore returns non-zero). These files are produced by the Maven build (JAR plugin / compilation) and land in non-standard locations (sonar-java-plugin/META-INF/ and sonar-java-plugin/org/, not under target/), which strongly suggests they were committed by mistake while fixing CI locally. Committing build output bloats the repo, will cause spurious diffs/merge conflicts on every build, and the embedded commit hash/version will quickly go stale.

Suggested fix: remove these files from the commit (git rm --cached) and add the offending paths to .gitignore so they are not re-added. None of them are needed for the S8911 rule implementation.

Quality: S6813 false-negative baseline increased (65->66) without explanation

📄 its/autoscan/src/test/resources/autoscan/diffs/diff_S6813.json:4 📄 its/autoscan/src/test/resources/autoscan/diffs/diff_S8899.json:1-6 📄 its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java:201
diff_S6813.json records an increase in falseNegatives from 65 to 66, and a new diff_S8899.json plus the bump of rulesNotReporting from 19 to 20 are added. These are autoscan baseline changes for rules (S6813, S8899) that are unrelated to the S8911 feature this PR claims to implement.

The +1 false negative for S6813 likely stems from the new test sources added for S8911 (e.g. the mock io.quarkus.runtime.Startup / Jakarta test sample) introducing a Spring/CDI construct that S6813 no longer detects in no-binaries (autoscan) mode. This is plausibly a legitimate baseline update, but the commit message only mentions "updated autoscan test baselines" without explaining the root cause. Please confirm the new false negative is expected behavior (a true autoscan limitation) and not an actual regression introduced by the added test code, and document the reason in the PR.

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

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