Change of behaviour 1.0 -> 1.1

• Key: CLI-137
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Blocker
• Assignee: Unassigned
• Reporter: Russel Winder
• Component/s: CLI-1.x
• Affects Version/s: 1.1
• Fix Version/s: 1.2

Description

The code:

import org.apache.commons.cli.*;

public class Trial {

  private void execute (String[] commandLine) throws ParseException {
    Options options = new Options();
    options.addOption ( OptionBuilder.withLongOpt("flob").hasArg().create('F') );

    CommandLine line = new GnuParser().parse(options, commandLine);

    String[] results = line.getOptionValues('F');

    if ( results != null ) { 
      for ( String s : results ) { 
        System.out.println( "-F " + s );
      } 
    }

    results = line.getOptionValues("flob") ;

    if ( results != null ) { 
      for ( String s : results ) { 
        System.out.println( "--blah " + s ); 
      }
    }

    String[] theRest = line.getArgs() ;
    for ( String s : theRest ) { 
      System.out.print( s + " " ); 
    }
    System.out.println();
  }

  public static void main (String[] args) throws ParseException {
    Trial trial = new Trial() ;
    trial.execute ( new String[] { "-F1" , "-F3" , "-Fbla" , "-F 76" , "--flob" , "54" } ) ;
  }
}

when compiled and executed under 1.0 produces:

trial:
[java] -F 1
[java] -F 3
[java] -F bla
[java] -F 76
[java] -F 54
[java]

However, when compiled and executed under 1.1 produces:

trial:
[java] -F 1
[java] --blah 1
[java] 3 bla 76 54

Comments

[Brian Egge]

In 1.1, the hasArg() set the number of expected values to '1'. Apparently, in 1.0 it defaulted to unlimited. You can get the previous behavior by changing the code like this:

Option option = OptionBuilder.withLongOpt("flob").hasArgs().create('F');

The output will then look like:
-F 1
-F 3
-F bla
-F 76
-F 54
--blah 1
--blah 3
--blah bla
--blah 76
--blah 54

In 1.1, you can call getOptionValues by either the short name or the long name. Since they are the same option, they return the same values.

hasArg expects a single value, while hasArgs allows unlimited values. I think this behavior, although different, is what the original API has intended.

[Henri Yandell]

Is this a viable workaround Russel?

[Russel Winder]

Yes and no. There appear to be problems but it is not clear whether these are problems in the Groovy use of Commons CLI, or whether there are bugs in Commons CLI. I am hesitant to report bugs in 1.0 since most are probably all removed in 1.1. However, 1.1 is not working as it should with Groovy. A bit of a chicken and egg situation.

[Steve Ogden]

This broke us as well. We expected that an option may be repeated multiple times on the command line with the same semantics. For example, assume that -F takes only one arg. This was true in CLI 1.0 for every occurrence of -F. We didn't want every arg to be lumped into a general -F bucket. This allowed us to do this in 1.0:

trial -F 1 -F 2 -F 3 file1 file2

and have the output be:

-F 1
-F 2
-F 3
file1 file2

not as it is now in 1.1:

-F 1
file1 file2

[Henri Yandell]

It seems that this is a simple backwards compat issue. The default behaviour of Option/OptionBuilder mistakenly changed from hasArgs to hasArg. Now it's there - it's probably best to document this very clearly and mark this as wontfix.

[Emmanuel Bourg]

This bug seems to be a blocker for projects willing to upgrade from 1.0 to 1.1. Considering that the original behavior is right, and that the usage of CLI 1.0 [1] outweighs greatly the usage of CLI 1.1 [2], I think we'd better fix this. The recommended upgrade path would be 1.0->1.2.

[1] http://mvnrepository.com/artifact/commons-cli/commons-cli/1.0
[2] http://mvnrepository.com/artifact/commons-cli/commons-cli/1.1

[Emmanuel Bourg]

I misunderstood the issue, I tend to agree with Henri, the hasArg vs hasArgs semantic seems sensible, even if it breaks the compatibility with CLI 1.0. It should be properly documented, currently the 1.1 upgrade notes make no mention of this issue.

Regarding the case suggested by Steve, it seems that with hasArgs() his example would return an array with [1, 2, 3] and not only 1, provided the getOptionValues() method is called instead of getOptionValue(), is this correct ?

[Russel Winder]

Someone reformated my code, it now looks really ugly

The problem is that the 1.1 implementation doesn't match the semantics specified, so you go and change all the code to the new semantics and you find you can't have multiple options and if you try it eats all the parameters whether they are options or not.

[Steve Ogden]

Source code:

import org.apache.commons.cli.*;
import java.util.*;

public class TestBed
{
   private String[] args_;

   private TestBed( String[] args ) throws Exception
   {
      args_ = args;
   }

   private void cliTest() throws Exception
   {
      Options options = new Options();
      Option option = null;

      option = new Option( "a", "alpha", true, "comment" );
      option.setRequired( true );
      option.setArgs( Option.UNLIMITED_VALUES );
      options.addOption( option );

      CommandLineParser parser = new GnuParser();
      CommandLine commandLine = parser.parse( options, args_ );
      
      for ( Iterator iter = commandLine.iterator(); iter.hasNext(); )
      {
         Option opt = (Option)iter.next();
         char id = (char)opt.getId();
         String[] values = opt.getValues();
         System.out.println( "values for " + opt );

         for ( int ii = 0; ii < values.length; ii++ )
         {
            System.out.print( values[ ii ] );

            if ( ii < ( values.length - 1 ) )
            {
               System.out.print( ", " );
            }
         }
         System.out.println();
      }
   }


   public static void main( String[] args ) throws Throwable
   {
      TestBed testBed = new TestBed( args );

      testBed.cliTest();
   }
}

Running with ant:

<target name="run" depends="jar">
      <java classname="${main.class}"
            fork="true">
         <jvmarg value="-showversion"/>
         <arg value="-a 1"/>
         <arg value="-a 2"/>
         <arg value="-a 3"/>
         <arg value="-a 4"/>
         <arg value="-b x"/>
         <arg value="output/output.bin"/>
         <classpath>
            <pathelement location="${classes.dir}"/>
            <fileset dir="${cots.dir}">
               <include name="*.jar"/>
            </fileset>
         </classpath>
      </java>
   </target>

Produces this output:

===============================================================================
1.0:
===============================================================================
run:
     [java] java version "1.5.0_15"
     [java] Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_15-b04)
     [java] Java HotSpot(TM) Server VM (build 1.5.0_15-b04, mixed mode)

     [java] values for [ option: a alpha  :: comment ]
     [java]  1
     [java] values for [ option: a alpha  :: comment ]
     [java]  4, -b x, output/output.bin
     [java] values for [ option: a alpha  :: comment ]
     [java]  3
     [java] values for [ option: a alpha  :: comment ]
     [java]  2

===============================================================================
1.1
===============================================================================
run:
     [java] java version "1.5.0_15"
     [java] Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_15-b04)
     [java] Java HotSpot(TM) Server VM (build 1.5.0_15-b04, mixed mode)

     [java] values for [ option: a alpha +ARG :: comment ]
     [java]  1,  2,  3,  4, -b x, output/output.bin

So it seems we can't have multiple instances of equivalent options when iterating. The opt values get grouped into a single option instance.

[Emmanuel Bourg]

Sorry for the reformatting Russel, it was a bit compact for my eyes

Steve, do you always use the option iterator to fetch the arguments from the CommandLine ? Or do you use getOptionValues() ? I'm comparing the code in 1.0 and 1.1, if I understand well the result should be the same in both versions with getOptionValues(). But the output of iterator() is clearly not compatible.

Why is it important to keep the repeated options isolated instead of grouping their values ? That looks equivalent to me. Unless you start having several values per option, and the number of values may vary. Something like :

grantaccess --user scott admin --user tom --user dean manager

This example can't be properly interpreted with CLI 1.1, it returns a list with the names and the roles and it's impossible to know what value is a name or a role because the number of values per option is not the same. It could be worked around by changing the format to --user name:role, but that's the only use case I can imagine that requires the original behavior of iterator().

[Steve Ogden]

Emmanuel, your example is very insightful. We are actually spawning multiple processes and using a flag to pass values to them. As for your questions, I don't know how to answer at the moment. I'll need to do some more research. This gives me ideas for work-arounds, though.

[Emmanuel Bourg]

Here is the patch I intend to apply, I put it here first for review. CommandLine not longer stores the options in a Set, and the parsers clone the options matched. The consequences are:

• CommandLine.iterator() may return the same option more than once, with different arguments. This is like CLI 1.0
• the hasArgs( n ) semantic is 'n arguments per occurrence', and not ' n arguments for all occurrences' of the option. This is like CLI 1.0
• the option put in the Options instance before parsing the command line no longer receives any argument. The right way of querying the option is to use the CommandLine instance. (this breaks test15046() in BugsTest, but this isn't the purpose of this test)

[Sebb]

Is there any merit in allowing both behaviours?

I.e. revert the normal behaviour to 1.0 as per the patch, but have a flag (perhaps settable via a system property) that can be used to specify 1.1-compliant behaviour?

It does not look like it would complicate the code hugely, but I could be wrong...

[Russel Winder]

I have been trying to create a TestNG test program to try and determine what I think should work and what shouldn't. Currently the single biggest problem I have with 1.2-SNAPSHOT (r662829) is exemplified by the test:

@Test public void multipleOptionsWithResidue_Posix ( ) throws ParseException {
    final Options options = new Options ( ) ;
    options.addOption (
                       OptionBuilder.withArgName ( "property=value" )
                       .hasArgs ( )
                       .create ( "D" ) ) ;
    final CommandLine line = posixParser.parse ( options , new String[] { "-D" , "x=2" , "-Dz=1" , "blah" , "Flobadob" } ) ;
    final String[] strings = line.getOptionValues ( 'D' ) ;
    assertEquals ( strings.length , 2 ) ;
    assertEquals ( strings[0] , "x=2" ) ;
    assertEquals ( strings[1] , "z=1" ) ;
   final List<String> excess = line.getArgList ( ) ;
    assertEquals ( excess.size ( ) , 2 ) ;
    assertEquals ( excess.get ( 0 ) , "blah" ) ;
    assertEquals ( excess.get ( 1 ) , "Flobadob" ) ;
  }
}

This test fails:

java.lang.AssertionError: expected:<2> but was:<4>
	at org.testng.Assert.fail(Assert.java:84)
	at org.testng.Assert.failNotEquals(Assert.java:438)
	at org.testng.Assert.assertEquals(Assert.java:108)
	at org.testng.Assert.assertEquals(Assert.java:323)
	at org.testng.Assert.assertEquals(Assert.java:333)
	at CommonsCLI_1_1_Test.multipleOptionsWithResidue_Gnu(CommonsCLI_1_1_Test.java:141)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at org.testng.internal.MethodHelper.invokeMethod(MethodHelper.java:580)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:478)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:617)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:885)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:126)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:110)
	at org.testng.TestRunner.runWorkers(TestRunner.java:712)
	at org.testng.TestRunner.privateRun(TestRunner.java:582)
	at org.testng.TestRunner.run(TestRunner.java:477)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:324)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:319)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:292)
	at org.testng.SuiteRunner.run(SuiteRunner.java:198)
	at org.testng.TestNG.createAndRunSuiteRunners(TestNG.java:821)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:788)
	at org.testng.TestNG.run(TestNG.java:708)
	at org.testng.TestNG.privateMain(TestNG.java:858)
	at org.testng.TestNG.main(TestNG.java:831)

The -D option processing is collecting all parameters not just the ones with a -D. The same result obtains with the GnuParser.

[Emmanuel Bourg]

I don't think it's worth supporting both behaviors, the vast majority of CLI users are still with the 1.0 version, and those using CLI 1.1 are unlikely to rely on this weird behavior (I checked the code for JEuclid, James JSPF, easyb and OpenEJB which are depending on CLI 1.1 according to mvnrepository.com, none uses more than 1 argument per option)

[Emmanuel Bourg]

I tried your test Russel, it fails on CLI 1.0. I don't think your example is right, you declare that -D has an unlimited number of arguments, so there shouldn't be remaining arguments, the last -D option gets 3 values. Your example would work with hasArg() instead of hasArgs().

With hasArg() the test fails on CLI 1.1, and it works on CLI 1.2 with my patch applied.

[Emmanuel Bourg]

The patch has been applied. Russel and Steve, if you could give it a try that would be great.

[Emmanuel Bourg]

Just a thought on Russel's last example, I believe we hit a flaw in the parser, if the command line is:

-Dz=1 blah

The parser should know that "blah" can't be an argument of the option because the first argument is attached to the option, even if the option is defined with an unlimited number of arguments. That would not be true if the first value was detached from the option, like:

-D z=1 blah

I'll file this in another JIRA issue.

[Russel Winder]

As at 2008-09-20 08:08:00+01:00, the original code of this issue generates:

-F 1
-F 3
-F bla
-F  76
-F 54
{code>
using CLI 1.0:

-F 1
--blah 1
3 bla 76 54

using CLI 1.1, and:

-F 1
-F 3
-F bla
-F 76
-F 54
--blah 1
--blah 3
--blah bla
--blah 76
--blah 54

using 1.2-SNAPSHOT (a local build of Subversion HEAD, this is not in the Maven repository). This would seem to confirm the status of this as "fixed".

[Russel Winder]

The TestNG test above (posting of 2008-06-03T08:46 <in whatever timezone this JIRA exists in>) now passes further confirming that this issue is correctly marked as "Fixed".