diff --git a/src/changes/changes.xml b/src/changes/changes.xml index d3f7b92d2..844f21b79 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -36,6 +36,7 @@ Fix integer overflow in TextHelpAppendable.indexOfWrap (#437). Reject UTF-16 surrogate values in Character converter (#438). Fix HelpFormatter wrapped description indent (#439). + Fix DefaultParser keeping values from its Properties argument across parses that reuse the same Options. Load classes without initializing them in Converter.CLASS (#432). Add AbstractHelpFormatter.printHelp(String, Options). diff --git a/src/main/java/org/apache/commons/cli/DefaultParser.java b/src/main/java/org/apache/commons/cli/DefaultParser.java index 4dd7dfec2..0060e3e80 100644 --- a/src/main/java/org/apache/commons/cli/DefaultParser.java +++ b/src/main/java/org/apache/commons/cli/DefaultParser.java @@ -506,14 +506,16 @@ private void handleProperties(final Properties properties) throws ParseException // get the value from the properties final String value = properties.getProperty(option); if (opt.hasArg()) { - if (opt.isValuesEmpty()) { - opt.processValue(stripLeadingAndTrailingQuotesDefaultOff(value)); + // add the value to the copy of the option in the CommandLine, not to the option registered in Options, + // and keep a value the option already has + handleOption(opt); + if (currentOption.isValuesEmpty()) { + currentOption.processValue(stripLeadingAndTrailingQuotesDefaultOff(value)); } - } else if (!("yes".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value) || "1".equalsIgnoreCase(value))) { - // if the value is not yes, true or 1 then don't add the option to the CommandLine - continue; + } else if ("yes".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value) || "1".equalsIgnoreCase(value)) { + // only add the option to the CommandLine if the value is yes, true or 1 + handleOption(opt); } - handleOption(opt); currentOption = null; } } @@ -727,6 +729,10 @@ private boolean isShortOption(final String token) { /** * Parses the arguments according to the specified options and properties. + *

+ * A value from {@code properties} is only used for an option that is not on the command line and has no value yet. Property values are added to the + * returned {@link CommandLine} without changing the values of the registered options. + *

* * @param options The specified Options. * @param properties command line option name-value pairs. @@ -790,6 +796,7 @@ public CommandLine parse(final Options options, final String[] arguments, final * @param properties command line option name-value pairs. * @return The list of atomic option and value tokens. * @throws ParseException if there are any problems encountered while parsing the command line tokens. + * @see #parse(Options, Properties, NonOptionAction, String[]) */ public CommandLine parse(final Options options, final String[] arguments, final Properties properties) throws ParseException { return parse(options, arguments, properties, false); diff --git a/src/test/java/org/apache/commons/cli/DefaultParserTest.java b/src/test/java/org/apache/commons/cli/DefaultParserTest.java index eb21171fd..0e7782fbe 100644 --- a/src/test/java/org/apache/commons/cli/DefaultParserTest.java +++ b/src/test/java/org/apache/commons/cli/DefaultParserTest.java @@ -19,6 +19,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -401,6 +402,36 @@ void testParseSkipNonHappyPath() throws ParseException { assertTrue(e.getMessage().contains("-d")); } + @Test + void testPropertiesDoNotChangeRegisteredOption() throws ParseException { + final Options options = new Options(); + options.addOption(Option.builder("o").hasArg().get()); + final Properties properties = new Properties(); + properties.setProperty("o", "fromProperties"); + final CommandLine cmd = new DefaultParser().parse(options, new String[0], properties); + assertEquals("fromProperties", cmd.getOptionValue("o")); + assertNull(options.getOption("o").getValues()); + } + + @Test + void testPropertiesDoNotLeakIntoNextParse() throws ParseException { + final Options options = new Options(); + options.addOption(Option.builder("o").hasArg().get()); + options.addOption(Option.builder("D").hasArgs().valueSeparator('=').get()); + final Properties properties = new Properties(); + properties.setProperty("o", "fromProperties"); + properties.setProperty("D", "key1=value1"); + final DefaultParser defaultParser = new DefaultParser(); + defaultParser.parse(options, new String[0], properties); + // parse again with the same Options and without properties + final CommandLine cmd = defaultParser.parse(options, new String[] { "-o", "fromArgs", "-Dkey2=value2" }); + assertEquals("fromArgs", cmd.getOptionValue("o")); + assertTrue(cmd.getArgList().isEmpty()); + final Properties expected = new Properties(); + expected.setProperty("key2", "value2"); + assertEquals(expected, cmd.getOptionProperties("D")); + } + @Test void testPropertiesPreserveExistingOptionValue() throws ParseException { final Option option = Option.builder("o").hasArg().get(); @@ -415,6 +446,19 @@ void testPropertiesPreserveExistingOptionValue() throws ParseException { assertEquals("preset", option.getValue()); } + @Test + void testPropertiesReuseOptions() throws ParseException { + final Options options = new Options(); + options.addOption(Option.builder("o").hasArg().get()); + final DefaultParser defaultParser = new DefaultParser(); + final Properties first = new Properties(); + first.setProperty("o", "first"); + assertEquals("first", defaultParser.parse(options, new String[0], first).getOptionValue("o")); + final Properties second = new Properties(); + second.setProperty("o", "second"); + assertEquals("second", defaultParser.parse(options, new String[0], second).getOptionValue("o")); + } + @Override @Test @Disabled("Test case handled in the parameterized tests as \"DEFAULT behavior\"")