Skip to content

Fix and clean up constant comparisons flagged by CodeQL java/constant-comparison#226

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/codeql-constant-comparison
Open

Fix and clean up constant comparisons flagged by CodeQL java/constant-comparison#226
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:features/codeql-constant-comparison

Conversation

@vharseko

@vharseko vharseko commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

Resolves the 19 open java/constant-comparison CodeQL alerts: 2 real defects
fixed
, 12 provably-constant (dead) comparisons removed in code
(behaviour-identical, so the alerts are cleared at the source), and 5 dismissed
as won't fix (intentional defensive/readable code).

Real bugs (2)

Module File Defect
persistit/ui ManagementTableModel Column-removal loop for (i = _displayedColumnCount; --i >= count;) inside if (count > _displayedColumnCount) — body never executed, so surplus table columns were never removed. Bounds were swapped.
json-schema/cli Main Verbose validation tested valid.length == 0 (a boolean[1], always false) and never set the flag true → "OK - Object is valid!" never printed. Init flag to true, test valid[0].

Dead / redundant comparisons removed (12, behaviour-identical)

Module File Dead comparison
persistit/core TransactionPlayer Replay loop re-checked type != TX.TYPE without re-reading type. Now re-reads type per continuation record so the check is live and consistent with the recordSize check.
persistit/core LongRecordHelper if (count > MAX_LONG_RECORD_CHAIN) nested inside an identical one — flattened.
persistit/core KeyParser else-branch re-tested if (c == '\\'), unreachable there (backslash handled above).
persistit/core Key return i < _size ? i + 1 : -1i < _size always true → return i + 1.
persistit/core Key Removed if (index < 0) break; inside a for (…; index >= 0; …) loop.
commons/util JsonValue Dropped && index >= 0 after the preceding if (index < 0) throw.
commons/json-fluent JsonValue Same && index >= 0 (legacy copy).
persistit/ui AdminUIBufferPanel Dropped _bufferPoolIndex >= 0 && after the index is normalised to 0 two lines above.
persistit/core IOMeter Dropped op >= 0 && after if (op == -1) break.
persistit/core FastIndex Removed vestigial faultAt var and its if (faultAt < 0) guards (assigned then immediately returned, never read).
persistit/core Buffer Removed a dead diagnostic-dump clamp if (keep < TAILBLOCK_HDR_SIZE_DATA && keep > tbSize) (never holds; fill loop already handles keep > tbSize).

Behaviour

Only two behaviours change, both to fix the bugs above (columns now removed; verbose
success now printed). Everything else is behaviour-identical removal of
provably-constant sub-expressions. TransactionPlayer now re-reads type on each
continuation record — a live re-validation that was previously dead; every record is
already type-validated in the first pass, so this only tightens defence-in-depth.

Not in this PR (dismissed as won't fix, 5)

Intentional code where the constant sub-expression is deliberate:
LongRecordHelper and Key.maxStorableKeySize defensive assertions/clamps,
Buffer page-integrity bounds and a provable invariant on the hot key-search path,
and the explicit HTTP status-family range ladder in Status.

Testing

mvn -o -pl persistit/core,persistit/ui,commons/json-schema/cli,commons/util/util,commons/json-fluent compile — all modules compile.
commons/util JsonValueTest + JsonValueFunctionsTest (96) and json-fluent JsonValueTest (5) pass, covering the get(int) change. (persistit unit tests are skipped in the offline build environment.)

@vharseko vharseko added codeql CodeQL static-analysis findings bug java refactoring Code cleanup / refactoring, no behavior change labels Jul 8, 2026
@vharseko vharseko requested a review from maximthomas July 8, 2026 10:07
…-comparison

Fix 2 real defects and remove 12 provably-constant (dead) comparisons across
commons and persistit. The dead-comparison removals are behaviour-identical and
clear the alerts at the source rather than by annotation.

Real bugs:
- ManagementTableModel: the column-removal loop was "for (i = _displayedColumnCount;
  --i >= count;)" inside "if (count > _displayedColumnCount)", so its body never ran
  and surplus table columns were never removed. The two bounds were swapped.
- json-schema Main: verbose validation checked "valid.length == 0" (a boolean[1], so
  always false) and never set the flag true, so "OK - Object is valid!" was never
  printed. Initialise the flag to true and test valid[0].

Dead / redundant comparisons removed (behaviour-identical):
- TransactionPlayer: the replay loop re-checked "type != TX.TYPE" but never re-read
  type, so the check was dead. Re-read type on each continuation record so the
  re-validation is live and consistent with the recordSize check.
- LongRecordHelper: an "if (count > MAX_LONG_RECORD_CHAIN)" was nested inside an
  identical one - flatten the duplicate.
- KeyParser.matchQuotedStringTail: the "else" branch re-tested "if (c == '\\')",
  which is unreachable there (backslash is handled above). Drop the dead branch.
- Key.nextElementIndex: "return i < _size ? i + 1 : -1" - i < _size always holds
  inside the loop, so return i + 1.
- Key.appendDisplayableStKey (BigInteger encode): removed "if (index < 0) break;"
  inside a "for (...; index >= 0; ...)" loop.
- JsonValue.get(int) (util and legacy fluent): dropped "&& index >= 0" after the
  preceding "if (index < 0) throw".
- AdminUIBufferPanel: dropped "_bufferPoolIndex >= 0 &&" after the code normalises a
  negative index to 0 two lines above.
- IOMeter.dump: dropped "op >= 0 &&" after "if (op == -1) break".
- FastIndex: removed the vestigial faultAt variable and its "if (faultAt < 0)"
  guards (assigned then immediately returned, never read).
- Buffer (diagnostic dump): removed a dead "if (keep < TAILBLOCK_HDR_SIZE_DATA &&
  keep > tbSize)" clamp (the condition can never hold and the fill loop already
  handles keep > tbSize).

Remaining alerts for this rule are dismissed as "won't fix": intentional defensive
assertions/bounds (LongRecordHelper, Key.maxStorableKeySize, Buffer page checks),
a provable invariant on the hot key-search path, and the explicit HTTP status-family
range ladder in Status.
@vharseko vharseko force-pushed the features/codeql-constant-comparison branch from a9ec6cd to c523e38 Compare July 8, 2026 10:25
@vharseko vharseko removed the java label Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug codeql CodeQL static-analysis findings refactoring Code cleanup / refactoring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants