Unexpected match in @ArgGroup multiplicity="1" with multiple values @Option

[kacchi]

Java=11, picocli=4.0.4

I need ArgGroup at least one option required command.
But ArgGroup contains multiple values option (without arity="1..*"),
parser looks like cannot match force by multiplicity.

import java.util.List;
import java.util.concurrent.Callable;

import picocli.CommandLine;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;

@Command(name = "sample")
public class App implements Callable<Integer> {
    static class Group {
        @Option(names = { "--age" })
        Integer age;

        @Option(names = { "--id" })
        List<String> id;
    }

    @ArgGroup(exclusive = false, multiplicity = "1")
    Group group;

    public Integer call() throws Exception {
        // no-op
        return 0;
    }

    public static void main(String[] args) {
        CommandLine cmd = new CommandLine(new App());
        System.out.println("==== `` ====");
        cmd.execute(); // OK: Missing required argument.

        System.out.println("==== `--age=12` ====");
        cmd.execute("--age=12"); // OK: age=12, id=null

        System.out.println("==== `--id=123` ====");
        cmd.execute("--id=123"); // OK: age=null, id=[123]

        System.out.println("==== `--age=12 id=456` ====");
        cmd.execute("--age=12", "--id=456"); // OK: age=12, id=[456]

        System.out.println("==== `--id=123 id=456` ====");
        cmd.execute("--id=123", "--id=456"); // NG!! (expect: age=null, id=[123, 456])
    }
}

I think, input command argument looks satisfy printed usage condition.

==== `--id=123 id=456` ====
Error: expected only one match but got ([--age=<age>] [--id=<id>] [--id=<id>]...)={--id=123} and ([--age=<age>] [--id=<id>] [--id=<id>]...)={--id=456}
Usage: sample ([--age=<age>] [--id=<id>] [--id=<id>]...)
      --age=<age>
      --id=<id>

Is this a bug?

[remkop]

Currently the parser does not greedily continue to add values to the multi-value --id option. Instead, it starts a new group instance as soon as the minimum number of elements is matched.

I need to look at this in more detail.

[remkop]

As a workaround, you can define the id field with a split regex like this:

@Option(names = {"--id"}, split = ",")
List<String> id;

That way, users can specify <cmd> --id=123,abc and the id list will get values "123" and "abc".

[remkop]

One idea is to introduce a parser configuration option greedyMatchMultiValueArgsInGroup.
It might make sense to make the default true.

Test:

    static class Issue815Group {
        @Option(names = {"--age"})
        Integer age;

        @Option(names = {"--id"})
        List<String> id;
    }
    static class Issue815 {
        @ArgGroup(exclusive = false, multiplicity = "1")
        Issue815Group group;
    }
    @Test
    public void testIssue815() {
        TestUtil.setTraceLevel("DEBUG");
        Issue815 userObject = new Issue815();
        new CommandLine(userObject).parseArgs("--id=123", "--id=456");
        assertNotNull(userObject.group);
        assertNull(userObject.group.age);
        assertNotNull(userObject.group.id);
        assertEquals(Arrays.asList("123", "456"), userObject.group.id);
    }

Then, the following change would make the test pass:

Index: src/main/java/picocli/CommandLine.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/main/java/picocli/CommandLine.java	(date 1570537223431)
+++ src/main/java/picocli/CommandLine.java	(date 1570537223431)
@@ -10356,8 +10356,11 @@
                 ArgGroupSpec group = argSpec.group();
                 if (group == null || isInitializingDefaultValues) { return; }
                 GroupMatchContainer foundGroupMatchContainer = this.groupMatchContainer.findOrCreateMatchingGroup(argSpec, commandSpec.commandLine);
-                if (foundGroupMatchContainer.lastMatch().matchedMinElements() &&
-                        (argSpec.required() || foundGroupMatchContainer.lastMatch().matchCount(argSpec) > 0)) {
+                GroupMatch match = foundGroupMatchContainer.lastMatch();
+                boolean greedy = true; // commandSpec.parser().greedyMatchMultiValueArgsInGroup();
+                boolean allowMultipleMatchesInGroup = greedy && argSpec.isMultiValue();
+                if (match.matchedMinElements() &&
+                        (argSpec.required() || match.matchCount(argSpec) > 0) && !allowMultipleMatchesInGroup) {
                     // we need to create a new match; if maxMultiplicity has been reached, we need to add a new GroupMatchContainer.
                     String previousMatch = argSpec.required() ? "is required" : "has already been matched";
                     String elementDescription = ArgSpec.describe(argSpec, "=");

Perhaps a better solution would be to introduce multiplicity on @Option and @Parameters. The difference with arity is that multiplicity specifies the total number of values (min..max count range) that may be captured in the container (List, Map, array), while arity specifies the number of arguments that may be specified for each occurrence of the option on the command line.

Introducing multiplicity would also solve an outstanding issue with options that have a split regex: there currently no way to restrict the max number of values added to an option that has a split regex. Even if arity=1, user input of -x=1,2,3 is valid. With multiplicity=1, the same input would cause a MaxValuesExceededException.

A new GroupMatch would then be created if the max multiplicity of a multi-value option was reached. There would be no need for a parser configuration option greedyMatchMultiValueArgsInGroup.

Perhaps arity should be deprecated for @Parameters and applications should use multiplicity instead.

[remkop]

Fixed in master.

I decided to keep it simple. The fix should work for your use case.
I raised #832 as a future enhancement.

[kacchi]

That's great!

We could get expected result with current HEAD.
Thanks!

[remkop]

Glad to hear that! I will try to do a release for this in a week or so.