[
https://issues.apache.org/jira/browse/CLI-346?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ruiqi Dong updated CLI-346:
---------------------------
Affects Version/s: 1.10.0
> 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
> Affects Versions: 1.10.0
> Reporter: Ruiqi Dong
> Priority: Major
> Original Estimate: 0.5h
> Remaining Estimate: 0.5h
>
> 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)
