SONARJAVA-4647 Improve S120 to report one project-level issue per bad package name

[asya-vorobeva]

Previously, the rule raised an issue on every file within a non-compliant package, producing n issues for n classes in the same package. The rule now collects unique bad package names across all files and reports a single project-level issue per package via endOfAnalysis().

[hashicorp-vault-sonar-prod]

SONARJAVA-4647

[gitar-bot]

Code Review ✅ Approved 4 resolved / 4 findings

Updates rule S120 to report a single project-level issue per bad package name instead of raising redundant issues per file, addressing incremental analysis issues and caching bugs. No remaining findings.

✅ 4 resolved

✅ Bug: Project issue lost on incremental analysis (unchanged files skipped)

📄 java-checks/src/main/java/org/sonar/java/checks/naming/BadPackageNameCheck.java:33 📄 java-checks/src/main/java/org/sonar/java/checks/naming/BadPackageNameCheck.java:46-58
Because the check now implements EndOfAnalysis, VisitorsBridge.canVisitorBeSkippedOnUnchangedFiles marks it unskippable and calls scanWithoutParsing(...) for every unchanged file; the inherited default returns true (JavaFileScanner:41), so the file is considered handled without ever being parsed and no package name is collected. In an incremental/cached analysis (canSkipUnchangedFiles + cache) a package whose files are all unchanged is therefore never added to badPackageNames, and since the issue is now project-level there is no per-file issue for the platform to carry over — the issue silently disappears and reappears on the next full analysis. Follow the pattern of AbstractPackageInfoChecker: override scanWithoutParsing to read the package name from the read cache (returning false when absent) and write it to the write cache from scanFile.

✅ Quality: badPackageNames is never cleared after reporting

📄 java-checks/src/main/java/org/sonar/java/checks/naming/BadPackageNameCheck.java:44 📄 java-checks/src/main/java/org/sonar/java/checks/naming/BadPackageNameCheck.java:60-65
endOfAnalysis iterates badPackageNames but never clears it, unlike the other EndOfAnalysis checks in the repo (e.g. ExcessiveContentRequestCheck resets its accumulators). If the same check instance is ever driven through more than one analysis/module (VisitorsBridge.endOfAnalysis is invoked once per scanner run), stale package names from the previous run are re-reported as duplicate project-level issues. Clear the set after reporting.

✅ Performance: Cache entry not propagated on scan-without-parsing hit

📄 java-checks/src/main/java/org/sonar/java/checks/naming/BadPackageNameCheck.java:50-57 📄 java-checks/src/test/java/org/sonar/java/checks/naming/BadPackageNameCheckTest.java:91-101
On a cache hit scanWithoutParsing reads the package name but never writes it to the write cache (no copyFromPrevious, and scanFile is not executed in that run), so the entry disappears from the cache produced by that analysis. In the following analysis of the same unchanged file readBytes returns null, scanWithoutParsing returns false, and because this check is unskippable (EndOfAnalysis, see VisitorsBridge.canVisitorBeSkippedOnUnchangedFiles) VisitorsBridge reports allScansSucceeded=false, forcing a full parse of that file for every scanner — so incremental skipping only works on every other analysis. Sibling caching checks (SpringBeansShouldBeAccessibleCheck.readFromCache, AbstractPackageInfoChecker.processFileAndCacheIfApplicable) deliberately re-populate the write cache on a hit; the new caching test only asserts scan counts, never that writeCache2 still holds the key, which is why this slipped through.

✅ Performance: Default-package files get no cache entry, blocking file skipping

📄 java-checks/src/main/java/org/sonar/java/checks/naming/BadPackageNameCheck.java:50-64
scanFile writes to the cache only when packageDeclaration != null, so a file in the default package never produces an entry; on every subsequent analysis scanWithoutParsing returns false for that unchanged file and, since the check is unskippable, the whole file is parsed again for all scanners. AbstractPackageInfoChecker handles exactly this case by caching an empty string and treating it as the default package. Note the fix must keep the empty name out of badPackageNames, otherwise the default package would be reported as a bad name.

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]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube