[Misc] Record the deliberate SonarQube java:S1214 false positive of IWemConstants in the code - #438
claude[bot] wants to merge 2 commits into
Conversation
…the code * The flagged interfaces are deliberate constant holders kept for backward compatibility; one of them already carries @SuppressWarnings("checkstyle:InterfaceIsType") for the very same finding. Co-Authored-By: Vincent Massol <vincent@massol.net> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji1e2t2yZv5rU4MTYhDoQr
| */ | ||
| // Old interface not describing a type, hard to remove for backward-compatibility reasons. | ||
| @SuppressWarnings("checkstyle:InterfaceIsType") | ||
| @SuppressWarnings({"checkstyle:InterfaceIsType", "java:S1214"}) |
There was a problem hiding this comment.
XWikiWikiModelHandler is in the internal package. Why would it be a problem to move the constants? Do they surface somewhere publicly?
Same question below for InternalWikiScannerContext (which is in the impl package).
There was a problem hiding this comment.
You're right on both, and I've dropped both suppressions in 1dbc6df — the backward-compatibility
sentence did not hold for either of them. I had copied it from the files where the code base had
already written it (the @SuppressWarnings("checkstyle:InterfaceIsType") ones), and extended it to
two sites where nobody had, which is exactly the thing that comment must not do.
What I checked:
| Package | Referenced from xwiki-platform | Used as a type | Verdict | |
|---|---|---|---|---|
XWikiWikiModelHandler |
…parser.xhtml.wikimodel (internal, so Revapi-excluded) |
none | never | suppression dropped |
InternalWikiScannerContext.IBlockTypes |
…wikimodel.impl, protected nested |
none | only inside its own enclosing class | suppression dropped |
IWemConstants |
org.xwiki.rendering.wikimodel (published) |
— | never, but referenced as IWemConstants.X from 15 files |
kept |
So the constants do not surface publicly in either case you asked about:
XWikiWikiModelHandleris not a marker either — nothing ever declares a variable, parameter or
return of that type. Its 11implementsclauses (9 insyntax-xhtml, 2 insyntax-xhtml5) exist
purely to inherit the constants, which is precisely the shape the rule is about. Making it a
constants class and qualifying the uses in those 11 handlers is a real (and, I think, worthwhile)
refactor — happy to do it as its own change, but it is more than a Sonar cleanup so I did not
widen this PR with it.IBlockTypesis even more local: every reference to it is insideInternalWikiScannerContext
itself and there is no subclass in xwiki-rendering or xwiki-platform, so the constants could move
into the enclosing class. The one thing worth a moment there is thatimplis notinternal,
so unlike the first case Revapi does watch it and removing aprotectednested type of a public
class is a visible removal — worth checking rather than assuming.
IWemConstants keeps its suppression and I've made the reason specific instead of generic, since
the generic wording is what invited this question:
// Constant holder rather than a type, and a published one: the styles are referenced as
// IWemConstants.X throughout the wikimodel parsers, so moving them out would be a breaking change.Verified after the change: mvn clean install -Plegacy,quality -pl xwiki-rendering-wikimodel →
199 tests green, Checkstyle green, "API checks completed without failures".
Let me know if you'd like the XWikiWikiModelHandler and IBlockTypes moves as a follow-up PR.
Generated by Claude Code
…tually holds * XWikiWikiModelHandler sits in an internal package (Revapi-excluded), is referenced nowhere in xwiki-platform and is never used as a type, so the backward-compatibility reason did not apply. * InternalWikiScannerContext.IBlockTypes is referenced only inside its own enclosing class and has no subclass in either repo, so it did not apply there either. * IWemConstants keeps the suppression, with the reason made specific and checkable. Co-Authored-By: Vincent Massol <vincent@massol.net> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ji1e2t2yZv5rU4MTYhDoQr
Jira URL
None — this is a
[Misc]SonarQube cleanup commit.Changes
Description
One SonarCloud
java:S1214issue — "Move constants defined in this interface to another class or enum" — recorded as the
false positive it is, the way our Sonar conventions prescribe:
@SuppressWarnings("java:S1214")plus a
//comment stating why, in the code, rather than Accepted in SonarCloud, which hidesthe reasoning from the next developer who comes along to "fix" it again.
No executable statement is changed: the diff is one comment and one annotation.
IWemConstants(org.xwiki.rendering.wikimodel) is a published constant holder, not a type:nothing ever declares a variable, parameter or return of that type, but its styles are referenced as
IWemConstants.Xfrom 15 files across the wikimodel parsers. Moving the constants out wouldtherefore be a breaking change for every caller, so the rule's own remediation is not available —
which is an objection to the remediation, not to the observation, hence a suppression with the
reason rather than a change.
Clarifications
suppressed the rule on
XWikiWikiModelHandlerand onInternalWikiScannerContext.IBlockTypes,reusing the code base's existing sentence "Old interface not describing a type, hard to remove
for backward-compatibility reasons". That reason does not hold for either:
XWikiWikiModelHandlersits in an internal package (Revapi-excluded), is referenced nowherein xwiki-platform, and is never used as a type — its 11
implementsclauses exist purely toinherit the constants;
IBlockTypesis referenced only inside its own enclosing class and has nosubclass in either repo. Both are genuine findings whose fix is a refactor rather than a Sonar
cleanup, so they are left open here; see the review thread for the analysis and the offer to do
those moves as a follow-up.
issue — the change is a pure insert above the declaration, which cannot inherit a pre-existing
finding.
Screenshots & Video
N/A
Executed Tests
BUILD SUCCESS — 199 tests, 0 failures, 0 errors, Checkstyle green, "API checks completed
without failures" (Revapi).
The original three-site version was verified in a wider chained reactor together with its siblings:
rendering 2 modules / 381 tests, xwiki-commons 356 tests / 4 modules, xwiki-platform 2042
tests / 16 modules (oldcore included), all
BUILD SUCCESS.Expected merging strategy
Squash and merge, no backport needed.
Related
Same sweep, sibling PRs: xwiki/xwiki-platform#6379 and xwiki/xwiki-commons#1976 — one change, three
repos, verified in a single chained reactor.
Generated by Claude Code