Uploaded image for project: 'Commons CLI'
  1. Commons CLI
  2. CLI-265

Optional argument picking up next regular option as its argument

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.3.1
    • Fix Version/s: 1.4
    • Component/s: CLI-1.x
    • Labels:
      None

      Activity

      Hide
      lynnskii Lynn Henderson added a comment -

      I have recently migrated a project from CLI 1.2 to 1.3.1 and have encountered what may be a bug or difference in the way optional arguments are being processed.

      I have a command that opens several different kinds of databases by type, or alternately, the last opened database of that type.

      Option TYPE1 = Option.builder("t1").hasArg().numberOfArgs(1).optionalArg(true).argName("t1_path").build();
      Option TYPE2 = Option.builder("t2").hasArg().numberOfArgs(1).optionalArg(true).argName("t2_path").build();
      Option LAST = Option.builder("last").hasArg(false).build();

      Commands then look like "open -t1 path/to/my/db" or "open -t1 -last"

      If I use the now deprecated GnuParser, both commands work as expected. However, if I use the new DefaultParser, for the 2nd example, it thinks "-last" is the argument for -t1 rather than an option in its own right.

      I added the numberOfArgs(1) after reading a post on StackOverflow, but it made no difference in the behavior. Only switching back to the GnuParser seemed to work.

      Show
      lynnskii Lynn Henderson added a comment - I have recently migrated a project from CLI 1.2 to 1.3.1 and have encountered what may be a bug or difference in the way optional arguments are being processed. I have a command that opens several different kinds of databases by type, or alternately, the last opened database of that type. Option TYPE1 = Option.builder("t1").hasArg().numberOfArgs(1).optionalArg(true).argName("t1_path").build(); Option TYPE2 = Option.builder("t2").hasArg().numberOfArgs(1).optionalArg(true).argName("t2_path").build(); Option LAST = Option.builder("last").hasArg(false).build(); Commands then look like "open -t1 path/to/my/db" or "open -t1 -last" If I use the now deprecated GnuParser, both commands work as expected. However, if I use the new DefaultParser, for the 2nd example, it thinks "-last" is the argument for -t1 rather than an option in its own right. I added the numberOfArgs(1) after reading a post on StackOverflow, but it made no difference in the behavior. Only switching back to the GnuParser seemed to work.
      Hide
      msandiford Martin Sandiford added a comment -

      See also this question/answer on Stackoverflow for a slightly different symptom and potential fix.

      http://stackoverflow.com/questions/38964866/defaultparser-in-commons-cli-doesnt-behave-like-the-deprecated-parsers/38965293#38965293

      Show
      msandiford Martin Sandiford added a comment - See also this question/answer on Stackoverflow for a slightly different symptom and potential fix. http://stackoverflow.com/questions/38964866/defaultparser-in-commons-cli-doesnt-behave-like-the-deprecated-parsers/38965293#38965293
      Hide
      vguna Veit Guna added a comment -
      Show
      vguna Veit Guna added a comment - Seeing the exact same behavior as described here: http://markmail.org/thread/ozeke3q7ni572psi#query:+page:1+mid:i6gssbpyvxozt633+state:results
      Hide
      britter Benedikt Ritter added a comment -
      ~/w/a/c/cli > svn ci -m "CLI-265: Optional argument picking up next regular option as its argument. Thank you to Lynn Henderson, Martin Sandiford and Veit Guna for providing reproductions."
      Sending        src/changes/changes.xml
      Sending        src/main/java/org/apache/commons/cli/DefaultParser.java
      Adding         src/test/java/org/apache/commons/cli/bug/BugCLI265Test.java
      Transmitting file data ...done
      Committing transaction...
      Committed revision 1759695.
      

      Thank you!

      Show
      britter Benedikt Ritter added a comment - ~/w/a/c/cli > svn ci -m "CLI-265: Optional argument picking up next regular option as its argument. Thank you to Lynn Henderson, Martin Sandiford and Veit Guna for providing reproductions." Sending src/changes/changes.xml Sending src/main/java/org/apache/commons/cli/DefaultParser.java Adding src/test/java/org/apache/commons/cli/bug/BugCLI265Test.java Transmitting file data ...done Committing transaction... Committed revision 1759695. Thank you!
      Hide
      msandiford Martin Sandiford added a comment -

      Hey Benedikt, thanks for looking at this quickly.

      I'm not sure if this is a complete fix. It seems to miss the case where short options are concatenated after an option that takes an optional argument.

      A failing test case for this would be to modify setUp() in BugCLI265Test.java to include short options "a" and "b":

          @Before
          public void setUp() throws Exception {
              parser = new DefaultParser();
      
              Option TYPE1 = Option.builder("t1").hasArg().numberOfArgs(1).optionalArg(true).argName("t1_path").build();
              Option OPTION_A = Option.builder("a").hasArg(false).build();
              Option OPTION_B = Option.builder("b").hasArg(false).build();
              Option LAST = Option.builder("last").hasArg(false).build();
      
              options = new Options().addOption(TYPE1).addOption(OPTION_A).addOption(OPTION_B).addOption(LAST);
          }
      

      Add add a test for the concatenated options following an option with optional argument case:

          @Test
          public void shouldParseConcatenatedShortOptions() throws Exception {
            String[] concatenatedShortOptions = new String[] { "-t1", "-ab" };
      
            final CommandLine commandLine = parser.parse(options, concatenatedShortOptions);
      
            assertTrue(commandLine.hasOption("t1"));
            assertEquals(null, commandLine.getOptionValue("t1"));
            assertTrue(commandLine.hasOption("a"));
            assertTrue(commandLine.hasOption("b"));
            assertFalse(commandLine.hasOption("last"));
          }
      

      One possible fix is to check that at least the first character of the option is a short option if all the other cases fail in isShortOption(...) like so:

          private boolean isShortOption(String token)
          {
              // short options (-S, -SV, -S=V, -SV1=V2, -S1S2)
              if (!token.startsWith("-") || token.length() == 1)
              {
                  return false;
              }
      
              // remove leading "-" and "=value"
              int pos = token.indexOf("=");
              String optName = pos == -1 ? token.substring(1) : token.substring(1, pos);
              if (options.hasShortOption(optName))
              {
                  return true;
              }
              return optName.length() > 0 && options.hasShortOption(String.valueOf(optName.charAt(0)));
          }
      
      Show
      msandiford Martin Sandiford added a comment - Hey Benedikt, thanks for looking at this quickly. I'm not sure if this is a complete fix. It seems to miss the case where short options are concatenated after an option that takes an optional argument. A failing test case for this would be to modify setUp() in BugCLI265Test.java to include short options "a" and "b": @Before public void setUp() throws Exception { parser = new DefaultParser(); Option TYPE1 = Option.builder( "t1" ).hasArg().numberOfArgs(1).optionalArg( true ).argName( "t1_path" ).build(); Option OPTION_A = Option.builder( "a" ).hasArg( false ).build(); Option OPTION_B = Option.builder( "b" ).hasArg( false ).build(); Option LAST = Option.builder( "last" ).hasArg( false ).build(); options = new Options().addOption(TYPE1).addOption(OPTION_A).addOption(OPTION_B).addOption(LAST); } Add add a test for the concatenated options following an option with optional argument case: @Test public void shouldParseConcatenatedShortOptions() throws Exception { String [] concatenatedShortOptions = new String [] { "-t1" , "-ab" }; final CommandLine commandLine = parser.parse(options, concatenatedShortOptions); assertTrue(commandLine.hasOption( "t1" )); assertEquals( null , commandLine.getOptionValue( "t1" )); assertTrue(commandLine.hasOption( "a" )); assertTrue(commandLine.hasOption( "b" )); assertFalse(commandLine.hasOption( "last" )); } One possible fix is to check that at least the first character of the option is a short option if all the other cases fail in isShortOption(...) like so: private boolean isShortOption( String token) { // short options (-S, -SV, -S=V, -SV1=V2, -S1S2) if (!token.startsWith( "-" ) || token.length() == 1) { return false ; } // remove leading "-" and "=value" int pos = token.indexOf( "=" ); String optName = pos == -1 ? token.substring(1) : token.substring(1, pos); if (options.hasShortOption(optName)) { return true ; } return optName.length() > 0 && options.hasShortOption( String .valueOf(optName.charAt(0))); }
      Hide
      britter Benedikt Ritter added a comment - - edited

      Reopen issue to address the problem identified by Martin Sandiford.

      Show
      britter Benedikt Ritter added a comment - - edited Reopen issue to address the problem identified by Martin Sandiford.
      Hide
      britter Benedikt Ritter added a comment -
      Show
      britter Benedikt Ritter added a comment - See http://svn.apache.org/r1759745

        People

        • Assignee:
          britter Benedikt Ritter
          Reporter:
          lynnskii Lynn Henderson
        • Votes:
          1 Vote for this issue
          Watchers:
          4 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development