Skip to content

Remove the never-populated globalArgumentMap from SubCommandArgumentParser - #786

Merged
vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-empty-global-argument-map
Jul 29, 2026
Merged

Remove the never-populated globalArgumentMap from SubCommandArgumentParser#786
vharseko merged 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:fix-empty-global-argument-map

Conversation

@vharseko

Copy link
Copy Markdown
Member

Clears java/empty-container alert #686 — "The contents of this container are never initialized".

Problem

SubCommandArgumentParser declares

/** The set of global arguments defined for this parser, referenced by argument name. */privatefinalMap<String, Argument> globalArgumentMap = newHashMap<>();

which is read in two places but never written to. git log -S "globalArgumentMap.put" returns nothing: the map was born empty in the OPENDJ-1303 commit that split opendj-cli out into its own module, and no commit has ever added a put. Both reads are therefore dead code.

Both reads duplicate a check that already works

The important finding is that neither read guards anything that is not already guarded by globalLongIDMap, which is populated.

SubCommand.addArgument() checked the same condition twice, 26 lines apart:

229: if (parser.hasGlobalArgument(argumentLongID)) { // dead - the map is empty230: thrownewArgumentException(ERR_ARG_SUBCOMMAND_ARGUMENT_GLOBAL_CONFLICT...);
...
255: Argumentarg = parser.getGlobalArgumentForLongID(longID); // live - globalLongIDMap256: if (arg != null) {
257: thrownewArgumentException(ERR_ARG_SUBCOMMAND_ARGUMENT_LONG_ID_GLOBAL_CONFLICT...);

The live check at line 255 is the stricter of the two: by that point longID has been normalised, so it also catches conflicts that differ only by case. The dead check is removed.

addGlobalArgument() rejected duplicate global names through the empty map, while the live equivalent against globalLongIDMap sat inside the if (!longArgumentsCaseSensitive()) branch. The dead check is removed and the live one hoisted out of that branch.

hasGlobalArgument() is public, so it is kept rather than deleted, and now resolves against globalLongIDMap through the existing ArgumentParser.formatLongIdentifier() helper — the same normalisation used when writing to that map.

Risk

The hoisted duplicate check is the only behaviour change: a case-sensitive parser now rejects two global arguments sharing a long identifier, which it previously accepted silently.

That cannot affect any shipped tool. All eight users of this parser construct it with longArgumentsCaseSensitive = false, so the check already ran for them:

ToolConstructed with
dsconfigfalse
dsreplication, status, uninstall (via SecureConnectionCliParser)false
backendstat, manage-account, upgradefalse
base64false

This was confirmed by running each tool against the patched build — no ArgumentException anywhere:

  • dsconfig --help (all subcommand groups), dsconfig list-backends --help, dsconfig set-global-configuration-prop --help
  • dsreplication --help, status --help, uninstall --help
  • backendstat --help, manage-account --help, upgrade --help
  • base64 --help, base64 encode --help

Testing

mvn -pl opendj-cli test — 46 tests run, 0 failures (43 before).

Three tests were added to TestSubCommandArgumentParserTestCase, which already exercises a case-sensitive parser:

  • testHasGlobalArgument — a registered global argument is now discoverable by long identifier, with the parser's case-sensitivity respected.
  • testDuplicateGlobalArgumentIsRejected — duplicate global long identifiers are rejected for both case-sensitive and case-insensitive parsers.
  • testSubCommandArgumentConflictingWithGlobalIsRejected — a sub-command argument may not shadow a global one.

The first two were run against the unpatched production code to confirm they actually pin the fix, and both fail there:

[ERROR] testHasGlobalArgument:176 expected:<true> but was:<false>
[ERROR] testDuplicateGlobalArgumentIsRejected:194 A duplicate global argument
should have been rejected (longArgumentsCaseSensitive=true)

The third passes before and after — it is a characterisation test pinning the live check at line 255 that the removed duplicate relied on.

Note

ERR_ARG_SUBCOMMAND_ARGUMENT_GLOBAL_CONFLICT and ERR_SUBCMDPARSER_DUPLICATE_GLOBAL_ARG_NAME are now unused in cli.properties. They were left in place so that message ordinals are not disturbed.

…arser
SubCommandArgumentParser declared a globalArgumentMap that was read in
two places but never written to, so both reads were dead. Checking the
history, the map was born that way in the OPENDJ-1303 commit that split
opendj-cli out into its own module; no commit ever added a put.
Both reads duplicate checks that already work against globalLongIDMap:
* SubCommand.addArgument() called hasGlobalArgument() and then, 26 lines
further down, getGlobalArgumentForLongID() for the same condition. The
second check is the stricter one, since by then the identifier has been
normalised, so it also catches conflicts that differ only by case. The
dead first check is removed.
* addGlobalArgument() rejected duplicate global names via the empty map,
while the live check against globalLongIDMap sat inside the
"if (!longArgumentsCaseSensitive())" branch. The dead check is removed
and the live one hoisted out of that branch, so duplicate global long
identifiers are now also rejected for a case sensitive parser.
hasGlobalArgument() is kept, since it is public, and now resolves against
globalLongIDMap through the existing formatLongIdentifier() helper.
The hoisted check is the only behaviour change. It cannot affect the
eight tools that use this parser - dsconfig, dsreplication, status,
uninstall, backendstat, manage-account, upgrade and base64 - because they
all construct the parser with longArgumentsCaseSensitive = false, so the
check already ran for them. Each was run with --help, and dsconfig also
per subcommand, without any ArgumentException.
Adds regression tests for both reads; the two that cover the fixed
behaviour fail against the previous code.
@vharsekovharseko added bug java Pull requests that update java code security Security fixes / CodeQL code-scanning alerts tests Test suites: fixing, enabling, un-disabling labels Jul 29, 2026
@vharseko
vharseko merged commit 5909163 into OpenIdentityPlatform:masterJul 29, 2026
17 checks passed
@vharseko
vharseko deleted the fix-empty-global-argument-map branch July 29, 2026 14:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugjavaPull requests that update java codesecuritySecurity fixes / CodeQL code-scanning alertstestsTest suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@vharseko@maximthomas