Details

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

      Operating System: other
      Platform: All

    • Bugzilla Id:
      39140

      Description

      I found a weakness of Jakarta Commons CLI and want to explain it with a simple
      example:

      Our program provides 2 options:

      1. -a or --algo <name>: The -a option requires an argument.
      2. -k or --key <value>: The -k option requires an argument too.

      a)

      If you pass the following command line arguments everything will be ok:
      -a Caesar -k A

      After evaluation:

      • "Caesar" is the parameter of the -a option and
      • "A" is the parameter of the -k option.

      b)

      However an org.apache.commons.cli.MissingArgumentException: no argument for:k is
      thrown if you pass the following input:

      -a Caesar -k a

      The Parser assumes that the argument "a" after the -k option, is the -a option
      missing the hyphen. At the end of this description there is Java code for
      executing this problem.

      Information:

      The handling of this command line

      -a Caesar -k a

      works in Getopt without any problem:

      • "Caesar" is the parameter of the -a option and
      • "a" of the -k option.

      After parsing a valid option Getopt always takes the next (available) command
      line argument as the option's parameter if the option requires an argument -
      means if you pass to the command line

      -k -a Caesar

      After evaluation:

      • "a" is the parameter of the -k option
      • the "Caesar" argument is just ignored

      If the option's parameter (<value>) represents an optional argument the next
      argument is not required, if it represents a valid option - means if you pass to
      the command line

      -k -a Caesar

      After evaluation:

      • "Caesar" is the parameter of the -a option
      • k option is set without a parameter - in this case a default value makes sense.

      Last but not least here is the code snippet for the CLI Test:

      import org.apache.commons.cli.CommandLine;
      import org.apache.commons.cli.CommandLineParser;
      import org.apache.commons.cli.Option;
      import org.apache.commons.cli.Options;
      import org.apache.commons.cli.ParseException;
      import org.apache.commons.cli.PosixParser;

      public class TestCommonsCLI {

      /**

      • @param args
        */
        public static void main(String[] args) {

      Options options = new Options();

      Option algorithm = new Option("a" , "algo", true, "the algorithm which it to
      perform executing");
      algorithm.setArgName("algorithm name");
      options.addOption(algorithm);

      Option key = new Option("k" , "key", true, "the key the setted algorithm uses
      to process");
      algorithm.setArgName("value");
      options.addOption(key);

      CommandLineParser parser = new PosixParser();

      try {
      CommandLine line = parser.parse( options, args);

      if(line.hasOption('a'))

      { System.out.println("algo: "+ line.getOptionValue( "a" )); }

      if(line.hasOption('k'))

      { System.out.println("key: " + line.getOptionValue('k')); }

      } catch (ParseException e)

      { // TODO Auto-generated catch block e.printStackTrace(); }

      }

      }

      1. CLI71_resetvalues.patch
        6 kB
        Brian Egge
      2. Cloneable.patch
        13 kB
        Brian Egge
      3. CLI-71-fix.patch
        3 kB
        Henri Yandell
      4. CLI-71-badfix.patch
        2 kB
        Henri Yandell
      5. BugCLI71Test.java
        3 kB
        Henri Yandell
      6. BugCLI71Test.java
        2 kB
        Henri Yandell
      7. ASF.LICENSE.NOT.GRANTED--TestCommonsCLI.java
        1 kB
        Amro Al-Akkad

        Issue Links

          Activity

          Hide
          amro.alakkad Amro Al-Akkad added a comment -

          Created an attachment (id=17999)
          code which can us for executing the described problem

          Show
          amro.alakkad Amro Al-Akkad added a comment - Created an attachment (id=17999) code which can us for executing the described problem
          Hide
          amro.alakkad Amro Al-Akkad added a comment -

          Hello,

          did I get in the right way: The bug is not fixed, but the issue got a new
          id, priority etc?

          Cheers,
          Amro

          Show
          amro.alakkad Amro Al-Akkad added a comment - Hello, did I get in the right way: The bug is not fixed, but the issue got a new id, priority etc? Cheers, Amro
          Hide
          bayard Henri Yandell added a comment -

          I was cleaning up all the issues in CLI to make it easier to see which ones applied to CLI-1 and which ones applied to CLI-2 (and which to CLI-Avalon) by setting a JIRA component.

          CLI is still pretty much a dead commons component. No one's actively working on it. Personally I haven't had any need for cli stuff recently, which seems odd given that once I did use such things a lot.

          Show
          bayard Henri Yandell added a comment - I was cleaning up all the issues in CLI to make it easier to see which ones applied to CLI-1 and which ones applied to CLI-2 (and which to CLI-Avalon) by setting a JIRA component. CLI is still pretty much a dead commons component. No one's actively working on it. Personally I haven't had any need for cli stuff recently, which seems odd given that once I did use such things a lot.
          Hide
          bayard Henri Yandell added a comment -

          Last issue for CLI 1.1! Nearly there

          Amro didn't flag the 'license for asf inclusion' checkbox; so we either need to get Amro to repeat that, or write a new unit test from his description.

          grumbles about legalities

          Show
          bayard Henri Yandell added a comment - Last issue for CLI 1.1! Nearly there Amro didn't flag the 'license for asf inclusion' checkbox; so we either need to get Amro to repeat that, or write a new unit test from his description. grumbles about legalities
          Hide
          bayard Henri Yandell added a comment -

          JUnit test based on Amro's description. As expected, it fails.

          Show
          bayard Henri Yandell added a comment - JUnit test based on Amro's description. As expected, it fails.
          Hide
          bayard Henri Yandell added a comment -

          Comment from Bugzilla:

          ------- Additional Comment #2 From Amro Al-Akkad 2007-05-22 15:23 [reply] -------

          >>Amro didn't flag the 'license for asf inclusion' checkbox<<
          Sorry.

          >>JUnit test based on Amro's description. As expected, it fails.<<
          . When will this bug be fixed (approximately)?
          Will this weakness exist, in the newer version, too?

          Show
          bayard Henri Yandell added a comment - Comment from Bugzilla: ------- Additional Comment #2 From Amro Al-Akkad 2007-05-22 15:23 [reply] ------- >>Amro didn't flag the 'license for asf inclusion' checkbox<< Sorry. >>JUnit test based on Amro's description. As expected, it fails.<< . When will this bug be fixed (approximately)? Will this weakness exist, in the newer version, too?
          Hide
          bayard Henri Yandell added a comment -

          It's planned for this to be fixed in the 1.1 release, it's the last issue left in the list. Someone just needs to figure out what the fix is

          Show
          bayard Henri Yandell added a comment - It's planned for this to be fixed in the 1.1 release, it's the last issue left in the list. Someone just needs to figure out what the fix is
          Hide
          bayard Henri Yandell added a comment -

          So... in terms of problems we have the following test cases:

          1] "-a Caesar -k a" does not work. I think that should be fixed and it's in the attached unit test. It's a bit more convoluted than Amro noticed, it only fails when the parser has been setup the first time with a different structure. Something it lingering in the parser.

          2] "-k -a Caesar" does not error. I think it should. Need to add that to the test.

          3] "-k -a Caesar" where k is optional has k as being not set rather than having a default. Need to add to the unit test.

          Show
          bayard Henri Yandell added a comment - So... in terms of problems we have the following test cases: 1] "-a Caesar -k a" does not work. I think that should be fixed and it's in the attached unit test. It's a bit more convoluted than Amro noticed, it only fails when the parser has been setup the first time with a different structure. Something it lingering in the parser. 2] "-k -a Caesar" does not error. I think it should. Need to add that to the test. 3] "-k -a Caesar" where k is optional has k as being not set rather than having a default. Need to add to the unit test.
          Hide
          bayard Henri Yandell added a comment -

          Adding a new test case with the new tests in them.

          Show
          bayard Henri Yandell added a comment - Adding a new test case with the new tests in them.
          Hide
          bayard Henri Yandell added a comment -

          1] fails. As I said, something is lingering.

          2] passes. An error is created, so things have changed since the version Amro was using.

          3] passes. If you use the default value API, you get the default value back.

          Show
          bayard Henri Yandell added a comment - 1] fails. As I said, something is lingering. 2] passes. An error is created, so things have changed since the version Amro was using. 3] passes. If you use the default value API, you get the default value back.
          Hide
          bayard Henri Yandell added a comment -

          Testing by changing the test to use a fresh options object and then a fresh parser object, the Options class is the one with the issue.

          It wraps Option and OptionGroup classes. Looking at the PosixParser superclass Parser, the problem would seem to be:

          opt.addValue( Util.stripLeadingAndTrailingQuotes(str) );

          ie) It's changing one of the Option classes; which is then handed over to the CommandLine object.

          A simple solution would be to clone the Option before passing it along to modify it. This could be done in Options.getValue, but I'm wary of putting it there. Simpler to put in the Parser class for just that usage of Options.getValue. My first stab at this breaks other tests.

          Show
          bayard Henri Yandell added a comment - Testing by changing the test to use a fresh options object and then a fresh parser object, the Options class is the one with the issue. It wraps Option and OptionGroup classes. Looking at the PosixParser superclass Parser, the problem would seem to be: opt.addValue( Util.stripLeadingAndTrailingQuotes(str) ); ie) It's changing one of the Option classes; which is then handed over to the CommandLine object. A simple solution would be to clone the Option before passing it along to modify it. This could be done in Options.getValue, but I'm wary of putting it there. Simpler to put in the Parser class for just that usage of Options.getValue. My first stab at this breaks other tests.
          Hide
          bayard Henri Yandell added a comment -

          Attaching my first (test breaking) fix patch. Stopping working on this for the moment, so if anyone wants to feel free to jump in.

          Show
          bayard Henri Yandell added a comment - Attaching my first (test breaking) fix patch. Stopping working on this for the moment, so if anyone wants to feel free to jump in.
          Hide
          bayard Henri Yandell added a comment -

          Another option, which is relatively similar, is to clear the values list before each parse. It breaks much the same other tests.

          The tests that break are the ones where a parser parses the same option multiple times. ie) My clearing out of the undesirably persistant data is too deep, it needs to be higher.

          Show
          bayard Henri Yandell added a comment - Another option, which is relatively similar, is to clear the values list before each parse. It breaks much the same other tests. The tests that break are the ones where a parser parses the same option multiple times. ie) My clearing out of the undesirably persistant data is too deep, it needs to be higher.
          Hide
          bayard Henri Yandell added a comment -

          Attaching a fix for this issue. It clears the Option classes in the Options class before parsing.

          Opinions are very much desired, I'm not sure what side-effects this might have; it seems to me that it is not an intended API to be able to say:

          options.getOption("a").getValues()

          rather than going via the CommandLine; so I don't think there is anything bad with this.

          Leaving open for the moment.

          Show
          bayard Henri Yandell added a comment - Attaching a fix for this issue. It clears the Option classes in the Options class before parsing. Opinions are very much desired, I'm not sure what side-effects this might have; it seems to me that it is not an intended API to be able to say: options.getOption("a").getValues() rather than going via the CommandLine; so I don't think there is anything bad with this. Leaving open for the moment.
          Hide
          brianegge Brian Egge added a comment -

          I went down the clone route for quite a while, as this seemed to be the reasonable approach. After implementing clone and equals in several classes, I got everything working except for this test case:

          public void test15046() throws Exception {
          CommandLineParser parser = new PosixParser();
          final String[] CLI_ARGS = new String[]

          {"-z", "c"}

          ;
          Option option = new Option("z", "timezone", true,
          "affected option");
          Options cliOptions = new Options();
          cliOptions.addOption(option);
          parser.parse(cliOptions, CLI_ARGS);

          //now add conflicting option
          cliOptions.addOption("c", "conflict", true, "conflict option");
          CommandLine line = parser.parse(cliOptions, CLI_ARGS);
          assertEquals( option.getValue(), "c" );
          assertTrue( !line.hasOption("c") );
          }

          The gist of it is, the class has a reference to the option is passes into parse. It then checks this exact same reference for it's value. If parse clones the option, the code doesn't see the value show up.

          I know I've written code like this in the past, so changing the behavior would break some existing code. In 2.0, we can have immutable objects and not have to worry about these messy mutable type, but for this case, I think the best option is for parse to reset all the values. Here's the proposed fix. I'll also attach this, and the test case as an ASF safe patch.

          Index: src/java/org/apache/commons/cli/Parser.java
          ===================================================================
          — src/java/org/apache/commons/cli/Parser.java (revision 541143)
          +++ src/java/org/apache/commons/cli/Parser.java (working copy)
          @@ -131,6 +131,10 @@
          throws ParseException
          {
          // initialise members
          + for (Iterator it = options.helpOptions().iterator(); it.hasNext()

          { + Option opt = (Option) it.next(); + opt.clear(); + }

          this.options = options;
          requiredOptions = options.getRequiredOptions();
          cmd = new CommandLine();

          Show
          brianegge Brian Egge added a comment - I went down the clone route for quite a while, as this seemed to be the reasonable approach. After implementing clone and equals in several classes, I got everything working except for this test case: public void test15046() throws Exception { CommandLineParser parser = new PosixParser(); final String[] CLI_ARGS = new String[] {"-z", "c"} ; Option option = new Option("z", "timezone", true, "affected option"); Options cliOptions = new Options(); cliOptions.addOption(option); parser.parse(cliOptions, CLI_ARGS); //now add conflicting option cliOptions.addOption("c", "conflict", true, "conflict option"); CommandLine line = parser.parse(cliOptions, CLI_ARGS); assertEquals( option.getValue(), "c" ); assertTrue( !line.hasOption("c") ); } The gist of it is, the class has a reference to the option is passes into parse. It then checks this exact same reference for it's value. If parse clones the option, the code doesn't see the value show up. I know I've written code like this in the past, so changing the behavior would break some existing code. In 2.0, we can have immutable objects and not have to worry about these messy mutable type, but for this case, I think the best option is for parse to reset all the values. Here's the proposed fix. I'll also attach this, and the test case as an ASF safe patch. Index: src/java/org/apache/commons/cli/Parser.java =================================================================== — src/java/org/apache/commons/cli/Parser.java (revision 541143) +++ src/java/org/apache/commons/cli/Parser.java (working copy) @@ -131,6 +131,10 @@ throws ParseException { // initialise members + for (Iterator it = options.helpOptions().iterator(); it.hasNext() { + Option opt = (Option) it.next(); + opt.clear(); + } this.options = options; requiredOptions = options.getRequiredOptions(); cmd = new CommandLine();
          Hide
          brianegge Brian Egge added a comment -

          Here's all the work done for the clone able solution. If we later decide to go down this route, this is the patch to apply.

          Show
          brianegge Brian Egge added a comment - Here's all the work done for the clone able solution. If we later decide to go down this route, this is the patch to apply.
          Hide
          brianegge Brian Egge added a comment -

          This is the minimal patch and tests. This uses the method of reset any values when parse is called. I included Henri's test case, as to make this single patch complete.

          Show
          brianegge Brian Egge added a comment - This is the minimal patch and tests. This uses the method of reset any values when parse is called. I included Henri's test case, as to make this single patch complete.
          Hide
          brianegge Brian Egge added a comment -

          Sorry Henri - I didn't see the work you did on this issue a few hours ago before I attached my comments and files. Looks like we came to the same conclusion though.

          Show
          brianegge Brian Egge added a comment - Sorry Henri - I didn't see the work you did on this issue a few hours ago before I attached my comments and files. Looks like we came to the same conclusion though.
          Hide
          bayard Henri Yandell added a comment -

          Cool - independent confirmation I'll get the patch applied today and then it's time to start thinking about a release.

          Show
          bayard Henri Yandell added a comment - Cool - independent confirmation I'll get the patch applied today and then it's time to start thinking about a release.
          Hide
          bayard Henri Yandell added a comment -

          svn ci -m "Applying Brian Egge and my work from CLI-71 to fix a lingering data problem in the parser and to confirm that other bugs have already been fixed" src/

          Sending src/java/org/apache/commons/cli/Option.java
          Sending src/java/org/apache/commons/cli/Parser.java
          Adding src/test/org/apache/commons/cli/OptionTest.java
          Adding src/test/org/apache/commons/cli/bug/BugCLI71Test.java
          Transmitting file data ....
          Committed revision 541408.

          Show
          bayard Henri Yandell added a comment - svn ci -m "Applying Brian Egge and my work from CLI-71 to fix a lingering data problem in the parser and to confirm that other bugs have already been fixed" src/ Sending src/java/org/apache/commons/cli/Option.java Sending src/java/org/apache/commons/cli/Parser.java Adding src/test/org/apache/commons/cli/OptionTest.java Adding src/test/org/apache/commons/cli/bug/BugCLI71Test.java Transmitting file data .... Committed revision 541408.

            People

            • Assignee:
              Unassigned
              Reporter:
              amro.alakkad Amro Al-Akkad
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development