CLI-254: "test" gets parsed as test, quotes die :-( (apache#58)

stoty · web-flow · commit 44173949e816 · 2021-10-17T10:37:00.000-04:00
* CLI-254 "test" gets parsed as test, quotes die :-(

* address review comments, improve backwards compatibility
diff --git a/src/main/java/org/apache/commons/cli/DefaultParser.java b/src/main/java/org/apache/commons/cli/DefaultParser.java
@@ -55,6 +55,10 @@ public class DefaultParser implements CommandLineParser {
     /** Flag indicating if partial matching of long options is supported. */
     private final boolean allowPartialMatching;
 
+    /** Flag indicating if balanced leading and trailing double quotes should be stripped from option arguments.
+     * null represents the historic arbitrary behaviour */
+    private final Boolean stripLeadingAndTrailingQuotes;
+
     /**
      * Creates a new DefaultParser instance with partial matching enabled.
      *
@@ -76,6 +80,7 @@ public class DefaultParser implements CommandLineParser {
      */
     public DefaultParser() {
         this.allowPartialMatching = true;
+        this.stripLeadingAndTrailingQuotes = null;
     }
 
     /**
@@ -101,6 +106,20 @@ public DefaultParser() {
      */
     public DefaultParser(final boolean allowPartialMatching) {
         this.allowPartialMatching = allowPartialMatching;
+        this.stripLeadingAndTrailingQuotes = null;
+    }
+
+    /**
+     * Creates a new DefaultParser instance with the specified partial matching and quote
+     * stripping policy.
+     *
+     * @param allowPartialMatching if partial matching of long options shall be enabled
+     * @param stripLeadingAndTrailingQuotes if balanced outer double quoutes should be stripped
+     */
+    private DefaultParser(final boolean allowPartialMatching,
+            final Boolean stripLeadingAndTrailingQuotes) {
+        this.allowPartialMatching = allowPartialMatching;
+        this.stripLeadingAndTrailingQuotes = stripLeadingAndTrailingQuotes;
     }
 
     /**
@@ -196,7 +215,7 @@ protected void handleConcatenatedOptions(final String token) throws ParseExcepti
 
             if (currentOption != null && token.length() != i + 1) {
                 // add the trail as an argument of the option
-                currentOption.addValueForProcessing(token.substring(i + 1));
+                currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(token.substring(i + 1)));
                 break;
             }
         }
@@ -242,7 +261,7 @@ private void handleLongOptionWithEqual(final String token) throws ParseException
 
             if (option.acceptsArg()) {
                 handleOption(option);
-                currentOption.addValueForProcessing(value);
+                currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(value));
                 currentOption = null;
             } else {
                 handleUnknownToken(currentToken);
@@ -314,7 +333,7 @@ private void handleProperties(final Properties properties) throws ParseException
 
                 if (opt.hasArg()) {
                     if (opt.getValues() == null || opt.getValues().length == 0) {
-                        opt.addValueForProcessing(value);
+                        opt.addValueForProcessing(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
@@ -361,12 +380,12 @@ private void handleShortAndLongOption(final String token) throws ParseException
 
                 if (opt != null && options.getOption(opt).acceptsArg()) {
                     handleOption(options.getOption(opt));
-                    currentOption.addValueForProcessing(t.substring(opt.length()));
+                    currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(t.substring(opt.length())));
                     currentOption = null;
                 } else if (isJavaProperty(t)) {
                     // -SV1 (-Dflag)
                     handleOption(options.getOption(t.substring(0, 1)));
-                    currentOption.addValueForProcessing(t.substring(1));
+                    currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOff(t.substring(1)));
                     currentOption = null;
                 } else {
                     // -S1S2S3 or -S1S2V
@@ -415,7 +434,7 @@ private void handleToken(final String token) throws ParseException {
         } else if ("--".equals(token)) {
             skipParsing = true;
         } else if (currentOption != null && currentOption.acceptsArg() && isArgument(token)) {
-            currentOption.addValueForProcessing(Util.stripLeadingAndTrailingQuotes(token));
+            currentOption.addValueForProcessing(stripLeadingAndTrailingQuotesDefaultOn(token));
         } else if (token.startsWith("--")) {
             handleLongOption(token);
         } else if (token.startsWith("-") && !"-".equals(token)) {
@@ -625,4 +644,134 @@ private void updateRequiredOptions(final Option option) throws AlreadySelectedEx
             group.setSelected(option);
         }
     }
+
+    /**
+     * Strip balanced leading and trailing quotes if the stripLeadingAndTrailingQuotes is set
+     * If stripLeadingAndTrailingQuotes is null, then do not strip
+     *
+     * @param token a string
+     * @return token with the quotes stripped (if set)
+     */
+    private String stripLeadingAndTrailingQuotesDefaultOff(final String token) {
+        if (stripLeadingAndTrailingQuotes != null && stripLeadingAndTrailingQuotes) {
+            return Util.stripLeadingAndTrailingQuotes(token);
+        } else {
+            return token;
+        }
+    }
+
+    /**
+     * Strip balanced leading and trailing quotes if the stripLeadingAndTrailingQuotes is set
+     * If stripLeadingAndTrailingQuotes is null, then do not strip
+     *
+     * @param token a string
+     * @return token with the quotes stripped (if set)
+     */
+    private String stripLeadingAndTrailingQuotesDefaultOn(final String token) {
+        if (stripLeadingAndTrailingQuotes == null || stripLeadingAndTrailingQuotes) {
+            return Util.stripLeadingAndTrailingQuotes(token);
+        } else {
+            return token;
+        }
+    }
+
+    /**
+     * Returns a {@link Builder} to create an {@link DefaultParser} using descriptive
+     * methods.
+     *
+     * @return a new {@link Builder} instance
+     * @since 1.5
+     */
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    /**
+     * A nested builder class to create <code>DefaultParser</code> instances
+     * using descriptive methods.
+     *
+     * Example usage:
+     * <pre>
+     * DefaultParser parser = Option.builder()
+     *     .setAllowPartialMatching(false)
+     *     .setStripLeadingAndTrailingQuotes(false)
+     *     .build();
+     * </pre>
+     *
+     * @since 1.5
+     */
+    public static final class Builder {
+
+        /** Flag indicating if partial matching of long options is supported. */
+        private boolean allowPartialMatching = true;
+
+        /** Flag indicating if balanced leading and trailing double quotes should be stripped from option arguments. */
+        private Boolean stripLeadingAndTrailingQuotes;
+
+        /**
+         * Constructs a new <code>Builder</code> for a <code>DefaultParser</code> instance.
+         *
+         * Both allowPartialMatching and stripLeadingAndTrailingQuotes are true by default,
+         * mimicking the argument-less constructor.
+         */
+        private Builder() {
+        }
+
+        /**
+         * Sets if partial matching of long options is supported.
+         *
+         * By "partial matching" we mean that given the following code:
+         *
+         * <pre>
+         * {
+         *     &#64;code
+         *     final Options options = new Options();
+         *     options.addOption(new Option("d", "debug", false, "Turn on debug."));
+         *     options.addOption(new Option("e", "extract", false, "Turn on extract."));
+         *     options.addOption(new Option("o", "option", true, "Turn on option with argument."));
+         * }
+         * </pre>
+         *
+         * If "partial matching" is turned on, {@code -de} only matches the {@code "debug"} option. However, with
+         * "partial matching" disabled, {@code -de} would enable both {@code debug} as well as {@code extract}
+         *
+         * @param allowPartialMatching whether to allow partial matching of long options
+         * @return this builder, to allow method chaining
+         * @since 1.5
+         */
+        public Builder setAllowPartialMatching(final boolean allowPartialMatching) {
+            this.allowPartialMatching = allowPartialMatching;
+            return this;
+        }
+
+        /**
+         * Sets if balanced leading and trailing double quotes should be stripped from option arguments.
+         *
+         * If "stripping of balanced leading and trailing double quotes from option arguments" is true,
+         * the outermost balanced double quotes of option arguments values will be removed.
+         * For example, <code>-o '"x"'</code> getValue() will return <code>x</code>, instead of <code>"x"</code>
+         *
+         * If "stripping of balanced leading and trailing double quotes from option arguments" is null,
+         * then quotes will be stripped from option values separated by space from the option, but
+         * kept in other cases, which is the historic behaviour.
+         *
+         * @param stripLeadingAndTrailingQuotes whether balanced leading and trailing double quotes should be stripped from option arguments.
+         * @return this builder, to allow method chaining
+         * @since 1.5
+         */
+        public Builder setStripLeadingAndTrailingQuotes(final Boolean stripLeadingAndTrailingQuotes) {
+            this.stripLeadingAndTrailingQuotes = stripLeadingAndTrailingQuotes;
+            return this;
+        }
+
+        /**
+         * Builds an DefaultParser with the values declared by this {@link Builder}.
+         *
+         * @return the new {@link DefaultParser}
+         * @since 1.5
+         */
+        public DefaultParser build() {
+            return new DefaultParser(allowPartialMatching, stripLeadingAndTrailingQuotes);
+        }
+    }
 }
diff --git a/src/test/java/org/apache/commons/cli/BasicParserTest.java b/src/test/java/org/apache/commons/cli/BasicParserTest.java
@@ -173,4 +173,16 @@ public void testUnambiguousPartialLongOption4() throws Exception {
     @Ignore("not supported by the BasicParser")
     public void testUnrecognizedOptionWithBursting() throws Exception {
     }
+
+    @Override
+    @Test
+    @Ignore("not supported by the BasicParser")
+    public void testShortOptionConcatenatedQuoteHandling() throws Exception {
+    }
+
+    @Override
+    @Test
+    @Ignore("not supported by the BasicParser")
+    public void testLongOptionWithEqualsQuoteHandling() throws Exception {
+    }
 }
diff --git a/src/test/java/org/apache/commons/cli/DefaultParserTest.java b/src/test/java/org/apache/commons/cli/DefaultParserTest.java
@@ -17,7 +17,10 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 
 package org.apache.commons.cli;
 
+import static org.junit.Assert.assertEquals;
+
 import org.junit.Before;
+import org.junit.Test;
 
 public class DefaultParserTest extends ParserTestCase {
 
@@ -27,4 +30,94 @@ public void setUp() {
         super.setUp();
         parser = new DefaultParser();
     }
+
+    @Override
+    @Test
+    public void testShortOptionConcatenatedQuoteHandling() throws Exception {
+        final String[] args = new String[] {"-b\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        //This is behaviour is not consistent with the other parsers, but is required for backwards compatibility
+        assertEquals("Confirm -b\"arg\" keeps quotes",  "\"quoted string\"", cl.getOptionValue("b"));
+    }
+
+    @Override
+    @Test
+    public void testLongOptionWithEqualsQuoteHandling() throws Exception {
+        final String[] args = new String[] {"--bfile=\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile=\"arg\" strips quotes",  "\"quoted string\"", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testShortOptionQuoteHandlingWithStrip() throws Exception {
+        parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(true).build();
+        final String[] args = new String[] {"-b", "\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm -b \"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testShortOptionQuoteHandlingWithoutStrip() throws Exception {
+        parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(false).build();
+        final String[] args = new String[] {"-b", "\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm -b \"arg\" keeps quotes",  "\"quoted string\"", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testLongOptionQuoteHandlingWithStrip() throws Exception {
+        parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(true).build();
+        final String[] args = new String[] {"--bfile", "\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile \"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testLongOptionQuoteHandlingWithoutStrip() throws Exception {
+        parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(false).build();
+        final String[] args = new String[] {"--bfile", "\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile \"arg\" keeps quotes",  "\"quoted string\"", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testLongOptionWithEqualsQuoteHandlingWithStrip() throws Exception {
+        parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(true).build();
+        final String[] args = new String[] {"--bfile=\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile=\"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testLongOptionWithEqualsQuoteHandlingWithoutStrip() throws Exception {
+        parser = DefaultParser.builder().setStripLeadingAndTrailingQuotes(false).build();
+        final String[] args = new String[] {"--bfile=\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile=\"arg\" keeps quotes",  "\"quoted string\"", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testBuilder() throws Exception {
+        parser = DefaultParser.builder()
+                .setStripLeadingAndTrailingQuotes(false)
+                .setAllowPartialMatching(false)
+                .build();
+        assertEquals(DefaultParser.class, parser.getClass());
+    }
 }
diff --git a/src/test/java/org/apache/commons/cli/ParserTestCase.java b/src/test/java/org/apache/commons/cli/ParserTestCase.java
@@ -955,6 +955,42 @@ public void testUnrecognizedOptionWithBursting() throws Exception {
         }
     }
 
+    @Test
+    public void testShortOptionQuoteHandling() throws Exception {
+        final String[] args = new String[] {"-b", "\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm -b \"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testLongOptionQuoteHandling() throws Exception {
+        final String[] args = new String[] {"--bfile", "\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile \"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testLongOptionWithEqualsQuoteHandling() throws Exception {
+        final String[] args = new String[] {"--bfile=\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm --bfile=\"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
+    @Test
+    public void testShortOptionConcatenatedQuoteHandling() throws Exception {
+        final String[] args = new String[] {"-b\"quoted string\""};
+
+        final CommandLine cl = parser.parse(options, args);
+
+        assertEquals("Confirm -b\"arg\" strips quotes",  "quoted string", cl.getOptionValue("b"));
+    }
+
     @Test
     public void testWithRequiredOption() throws Exception {
         final String[] args = {"-b", "file"};
@@ -970,4 +1006,5 @@ public void testWithRequiredOption() throws Exception {
         assertEquals("Confirm arg of -b", "file", cl.getOptionValue("b"));
         assertTrue("Confirm NO of extra args", cl.getArgList().isEmpty());
     }
+
 }
