SONARJAVA-6489: Implement rule S8909 - Quarkus CacheKeyGenerator should be instantiable

[romainbrenguier]

Summary

Implements rule S8909 to detect Quarkus CacheKeyGenerator implementations that cannot be instantiated by the CDI container.

Changes

• Implemented CacheKeyGeneratorInstantiableCheck to detect classes implementing CacheKeyGenerator that:
  • Have no public no-args constructor AND
  • Are not CDI beans (missing scope annotations like @ApplicationScoped, @Dependent, etc.)
• Added comprehensive test coverage with both compliant and non-compliant cases
• Created mock-up CacheKeyGenerator interface for testing (avoiding external dependency)

Test Coverage

The rule detects:

• ✅ Classes with constructor dependencies but no CDI scope annotation
• ✅ Classes with multiple constructors (none no-args) and no CDI scope
• ✅ Classes with private constructors

The rule correctly ignores:

• ✅ Classes with CDI scope annotations (even with dependencies)
• ✅ Classes with public no-args constructors
• ✅ Abstract classes and interfaces
• ✅ Non-CacheKeyGenerator classes

🤖 Generated with Claude Code

[hashicorp-vault-sonar-prod]

SONARJAVA-6489

[joke1196]

This looks good to me. As I am not familiar with the analyzer I am not fully sure what the pitfalls could be, but here the implementation and the test make sense

[rombirli]

Why? I never had to bump this one so im curious to know the reason

[romainbrenguier]

The new rule S8909 introduced in this PR causes 3 false positives in AutoScan mode (analysis without full bytecode/dependencies), as documented in its/autoscan/src/test/resources/autoscan/diffs/diff_S8909.json. This makes it the 12th rule that produces false positives in AutoScan mode, so the assertion needed to be updated from 11 to 12.

The AutoScan test compares issues found with complete semantic analysis (Maven build with bytecode) versus limited semantic analysis (AutoScan mode), and tracks which rules cause false positives without full bytecode information.

[romainbrenguier]

Apparently the new rule has some false positives in auto-scan mode. I'll try to fix them.

[rombirli]

List.of

[rombirli]

List.of

[rombirli]

Why dont we merge those ifs

[rombirli]

A bit verbose, I propose

Suggested change

	private static boolean hasPublicNoArgsConstructor(ClassTree classTree) {
	Collection<Symbol> constructors = classTree.symbol().lookupSymbols("<init>");
	var noArgConstructor = constructors.stream()
	.filter(CacheKeyGeneratorInstantiableCheck::isNoArgConstructor)
	.findFirst();
	if (noArgConstructor.isEmpty()) {
	return false;
	}
	Symbol constructor = noArgConstructor.get();
	if (constructor.declaration() == null) {
	return classTree.symbol().isPublic();
	}
	return constructor.isPublic();
	}
	private static boolean isNoArgConstructor(Symbol constructor) {
	return ((Symbol.MethodSymbol) constructor).parameterTypes().isEmpty();
	}
	private static boolean hasPublicNoArgsConstructor(ClassTree classTree) {
	Collection<Symbol> constructors = classTree.symbol().lookupSymbols("<init>");
	return constructors.stream()
	.map(Symbol.MethodSymbol.class::cast)
	.filter(CacheKeyGeneratorInstantiableCheck::isNoArgConstructor)
	.findFirst()
	.map(Symbol::isPublic)
	.orElse(classTree.symbol().isPublic());
	}
	private static boolean isNoArgConstructor(Symbol.MethodSymbol constructor) {
	return constructor.parameterTypes().isEmpty();
	}

[gitar-bot]

💡 Edge Case: Simple-name annotation match may suppress issues for unrelated annotations

matchesAnnotation falls back to matching only the annotation's simple name for the IDENTIFIER case (and as one OR branch for MEMBER_SELECT). Because all entries in CDI_SCOPE_ANNOTATIONS share common simple names (e.g. ApplicationScoped, Dependent, RequestScoped), any annotation with one of those simple names — regardless of its actual package — will be treated as a CDI scope annotation and suppress the issue. This is a reasonable tradeoff to avoid autoscan false positives when semantics are unresolved, but in fully-resolved compilations it could produce a false negative if a class uses an unrelated @Dependent/@RequestScoped from a different library and still lacks a real CDI scope. Consider preferring the resolved symbolType().is(fqn) check and only relying on the simple-name match when the type cannot be resolved (unknown type), to keep the fully-resolved path precise.

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

[gitar-bot]

Code Review 👍 Approved with suggestions 3 resolved / 4 findings

Implements rule S8909 to detect non-instantiable Quarkus CacheKeyGenerator implementations, resolving metadata and dependency issues. Ensure the simple-name annotation matcher is narrowed to prevent false suppressions from unrelated annotations.

💡 Edge Case: Simple-name annotation match may suppress issues for unrelated annotations

📄 java-checks/src/main/java/org/sonar/java/checks/quarkus/CacheKeyGeneratorInstantiableCheck.java:85-96

matchesAnnotation falls back to matching only the annotation's simple name for the IDENTIFIER case (and as one OR branch for MEMBER_SELECT). Because all entries in CDI_SCOPE_ANNOTATIONS share common simple names (e.g. ApplicationScoped, Dependent, RequestScoped), any annotation with one of those simple names — regardless of its actual package — will be treated as a CDI scope annotation and suppress the issue. This is a reasonable tradeoff to avoid autoscan false positives when semantics are unresolved, but in fully-resolved compilations it could produce a false negative if a class uses an unrelated @Dependent/@RequestScoped from a different library and still lacks a real CDI scope. Consider preferring the resolved symbolType().is(fqn) check and only relying on the simple-name match when the type cannot be resolved (unknown type), to keep the fully-resolved path precise.

✅ 3 resolved

✅ Quality: Missing S8909 rule metadata (HTML + JSON) breaks build

📄 java-checks/src/main/java/org/sonar/java/checks/quarkus/CacheKeyGeneratorInstantiableCheck.java:30
The new rule CacheKeyGeneratorInstantiableCheck is annotated with @Rule(key = "S8909") and is auto-registered via CheckListGenerator, but no metadata files exist for it. GeneratedCheckListTest asserts that both org/sonar/l10n/java/rules/java/S8909.html and S8909.json exist for every registered rule (see GeneratedCheckListTest.java:137-142), and a Glob confirms neither file is present. Without these the rule has no description, no severity/type/profile assignment, and the test suite (hence the build) will fail. Add S8909.html (rule description) and S8909.json (metadata: title, type, status, tags, remediation, default severity, and SonarWay profile membership) under java-checks/src/main/resources/org/sonar/l10n/java/rules/java/.

✅ Quality: Redundant quarkus-cache dependency conflicts with local mock interface

📄 java-checks-test-sources/default/pom.xml:189-194 📄 java-checks-test-sources/default/src/main/java/io/quarkus/cache/CacheKeyGenerator.java:8-9
The PR adds the real io.quarkus:quarkus-cache:3.2.0.Final dependency in pom.xml AND also adds a hand-written mock io/quarkus/cache/CacheKeyGenerator.java with the same fully-qualified name. The PR description states the mock was created "avoiding external dependency", which directly contradicts also adding the dependency. Having both a local source file and a jar that define io.quarkus.cache.CacheKeyGenerator on the same compile path is at best redundant and can cause type-resolution ambiguity / duplicate-type issues in the test-sources module. Pick one approach: either rely on the real dependency and delete the mock, or keep the mock and drop the new dependency from pom.xml (consistent with the stated intent). Note the real Quarkus CacheKeyGenerator also declares a default boolean generationGroup() method, so the mock is not a faithful copy.

✅ Edge Case: Implicit constructor of package-private class treated as public

📄 java-checks/src/main/java/org/sonar/java/checks/quarkus/CacheKeyGeneratorInstantiableCheck.java:86-98 📄 java-checks/src/main/java/org/sonar/java/checks/quarkus/CacheKeyGeneratorInstantiableCheck.java:49-51
hasPublicNoArgsConstructor returns true when the no-arg constructor's declaration() == null (implicit default constructor), regardless of visibility. An implicit default constructor inherits the class's access level, so for a package-private (or private nested) class implementing CacheKeyGenerator the implicit constructor is NOT public. The issue message explicitly tells users to add a "public no-args constructor", yet such a class is reported compliant — an inconsistency that can produce a false negative if Quarkus requires a public constructor for non-CDI instantiation. Consider also checking the effective visibility of the implicit constructor (e.g. require the class itself to be public), or align the message with the actual check. Also note records/enums implementing the interface are not covered since nodesToVisit() only registers Tree.Kind.CLASS.

🤖 Prompt for agents

Code Review: Implements rule S8909 to detect non-instantiable Quarkus CacheKeyGenerator implementations, resolving metadata and dependency issues. Ensure the simple-name annotation matcher is narrowed to prevent false suppressions from unrelated annotations.

1. 💡 Edge Case: Simple-name annotation match may suppress issues for unrelated annotations
   Files: java-checks/src/main/java/org/sonar/java/checks/quarkus/CacheKeyGeneratorInstantiableCheck.java:85-96

   `matchesAnnotation` falls back to matching only the annotation's simple name for the IDENTIFIER case (and as one OR branch for MEMBER_SELECT). Because all entries in `CDI_SCOPE_ANNOTATIONS` share common simple names (e.g. `ApplicationScoped`, `Dependent`, `RequestScoped`), any annotation with one of those simple names — regardless of its actual package — will be treated as a CDI scope annotation and suppress the issue. This is a reasonable tradeoff to avoid autoscan false positives when semantics are unresolved, but in fully-resolved compilations it could produce a false negative if a class uses an unrelated `@Dependent`/`@RequestScoped` from a different library and still lacks a real CDI scope. Consider preferring the resolved `symbolType().is(fqn)` check and only relying on the simple-name match when the type cannot be resolved (unknown type), to keep the fully-resolved path precise.

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}

[sonarqube-next]

Quality Gate failed

Failed conditions
78.0% Coverage on New Code (required ≥ 90%)

See analysis details on SonarQube