Fix DefaultParser keeping property values across parses

src/changes/changes.xml

@@ -36,6 +36,7 @@
       <action type="fix" dev="ggregory" due-to="Naveed Khan, Gary Gregory">Fix integer overflow in TextHelpAppendable.indexOfWrap (#437).</action>
       <action type="fix" dev="ggregory" due-to="Naveed Khan, Gary Gregory">Reject UTF-16 surrogate values in Character converter (#438).</action>
       <action type="fix" dev="ggregory" due-to="dev_Hakaze, Gary Gregory" issue="CLI-354">Fix HelpFormatter wrapped description indent (#439).</action>
+      <action type="fix" dev="acosentino" due-to="Andrea Cosentino">Fix DefaultParser keeping values from its Properties argument across parses that reuse the same Options.</action>
       <action type="fix" dev="ggregory" due-to="Gary Gregory, farkhalit rida">Load classes without initializing them in Converter.CLASS (#432).</action>
       <!-- ADD -->
       <action type="fix" dev="ggregory" due-to="Elric, Gary Gregory">Add AbstractHelpFormatter.printHelp(String, Options).</action>

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.
+     * <p>
+     * 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.
+     * </p>
      *
      * @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);

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\"")