Ruiqi Dong created CLI-346:
------------------------------
Summary: Options.addOptionGroup() silently overwrites existing
options while addOptions() throws exception for duplicates
Key: CLI-346
URL: https://issues.apache.org/jira/browse/CLI-346
Project: Commons CLI
Issue Type: Bug
Components: Options definition
Reporter: Ruiqi Dong
The {{Options.addOptionGroup()}} method silently overwrites existing options
when adding an {{OptionGroup}} that contains options with keys already present
in the {{Options}} instance. This behavior is inconsistent with
{{{}Options.addOptions(){}}}, which throws an {{IllegalArgumentException}} when
attempting to add duplicate options. This inconsistency violates the principle
of least surprise and can lead to subtle bugs where previously configured
options are unintentionally overwritten without any warning.
{*}Impact{*}:
* Can cause unexpected behavior in applications using Commons CLI
* Previously required options may become optional without warning
* Configuration may be silently lost
* Debugging such issues is difficult due to lack of error messages
*Test Case:*
{code:java}
@Test
void testAddOptionGroupShouldThrowExceptionForDuplicateOption() {
// Arrange
Options options = new Options();
// Add an existing option
Option existingOption = new Option("f", "file", true, "Input file");
existingOption.setRequired(true);
options.addOption(existingOption);
// Create an OptionGroup with the same option key
OptionGroup group = new OptionGroup();
Option duplicateOption = new Option("f", "file", true, "Config file");
group.addOption(duplicateOption);
// Act & Assert
// This test expects addOptionGroup to throw exception like addOptions does
// BUT IT FAILS - demonstrating the bug
IllegalArgumentException exception = assertThrows(
IllegalArgumentException.class,
() -> options.addOptionGroup(group),
"addOptionGroup should throw IllegalArgumentException for duplicate
option key, " +
"consistent with addOptions() behavior"
);
// This assertion would verify the exception message (if exception was
thrown)
assertTrue(exception.getMessage().contains("f") ||
exception.getMessage().contains("duplicate") ||
exception.getMessage().contains("Duplicate"),
"Exception message should mention the duplicate key");
}{code}
{*}Test Failure Output{*}{*}:{*}
{code:java}
[ERROR]
org.apache.commons.cli.OptionsTest.testAddOptionGroupShouldThrowExceptionForDuplicateOption
-- Time elapsed: 0.001 s <<< FAILURE!
org.opentest4j.AssertionFailedError: addOptionGroup should throw
IllegalArgumentException for duplicate option key, consistent with addOptions()
behavior ==> Expected java.lang.IllegalArgumentException to be thrown, but
nothing was thrown.
at
org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:73)
at org.junit.jupiter.api.AssertThrows.assertThrows(AssertThrows.java:39)
at org.junit.jupiter.api.Assertions.assertThrows(Assertions.java:3153)
at
org.apache.commons.cli.OptionsTest.testAddOptionGroupShouldThrowExceptionForDuplicateOption(OptionsTest.java:46)
at java.lang.reflect.Method.invoke(Method.java:498)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at java.util.ArrayList.forEach(ArrayList.java:1259) {code}
{*}Suggested Fix{*}:
Modify {{addOptionGroup()}} to check for existing options before adding, making
it consistent with {{{}addOptions(){}}}:
{code:java}
public Options addOptionGroup(final OptionGroup group) {
// Check for duplicates first - consistent with addOptions() behavior
for (final Option option : group.getOptions()) {
if (hasOption(option.getKey())) {
throw new IllegalArgumentException("Option '" + option.getKey() +
"' already exists");
}
}
// Existing logic
if (group.isRequired()) {
requiredOpts.add(group);
}
for (final Option option : group.getOptions()) {
option.setRequired(false);
addOption(option);
optionGroups.put(option.getKey(), group);
}
return this;
} {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
