Incorrect results when using ArgGroup + defaultValue + split + List/Set

Hello,

When using the features ArgsGroup along with defaultValue and split, the value reported from the second group is not correct as per the arguments provided.

Sample code with error:

public class TestParser {

    public static void main(String[] ignore) {
        String[] rawArgs = new String[]{
                "--type=ONE", "--actions=CREATE,DELETE",
                "--type=TWO", "--actions=CREATE",
                "--type=THREE",
                "--type=FOUR", "--actions=CREATE,UPDATE"
        };
        Args args = new Args();
        new CommandLine(args).parseArgs(rawArgs);

        args.groups.forEach(g -> System.out.printf("%5s = %s\n", g.type, g.actions));
    }

    public static class Args {
        @ArgGroup(exclusive = false, multiplicity = "1..4")
        private List<Group> groups;
    }

    public static class Group {
        @Option(names = "--type", required = true)
        private String type;

        @Option(names = "--actions", defaultValue = "CREATE,UPDATE,DELETE", split = ",")
        private Set<String> actions;
    }

}

Actual Result:

  ONE = [CREATE, DELETE]
  TWO = [CREATE, UPDATE, DELETE]
THREE = [CREATE, UPDATE, DELETE]
 FOUR = [CREATE, UPDATE, DELETE]

Expected Result:

  ONE = [CREATE, DELETE]
  TWO = [CREATE]
THREE = [CREATE, UPDATE, DELETE]
 FOUR = [CREATE, UPDATE]

Here, we can see that the results are using the default value starting from the second item in the ArgGroup.

Details:

JDK: temurin-21.jdk
Picocli version: 4.7.6

Thank you for raising this!

I'll have to look with -Dpicocli.trace=INFO (or =DEBUG) to see what is happening.

Note to self:

package picocli;

import org.junit.Test;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Option;

import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import static org.junit.Assert.assertEquals;

public class Issue2349 {
    static class TestParser {
        static class Args {
            @ArgGroup(exclusive = false, multiplicity = "1..4")
            private List<Group> groups;
        }

        static class Group {
            @Option(names = "--type", required = true)
            private String type;

            @Option(names = "--actions", defaultValue = "CREATE,UPDATE,DELETE", split = ",")
            private Set<String> actions;

            public Group() {}

            Group(String type, Set<String> actions) {
                this.type = type;
                this.actions = actions;
            }

            @Override
            public boolean equals(Object o) {
                if (this == o) return true;
                if (o == null || getClass() != o.getClass()) return false;
                Group group = (Group) o;
                return equals(type, group.type) && equals(actions, group.actions);
            }
            public static boolean equals(Object a, Object b) {
                return (a == b) || (a != null && a.equals(b));
            }

            @Override
            public int hashCode() {
                return Arrays.hashCode(new Object[] {type, actions});
            }

            @Override
            public String toString() {
                return String.format("%s %s", type, actions);
            }
        }
    }

    @Test
    public void issue2349() {
        String[] rawArgs = new String[]{
            "--type=ONE", "--actions=CREATE,DELETE",
            "--type=TWO", "--actions=CREATE",
            "--type=THREE",
            "--type=FOUR", "--actions=CREATE,UPDATE"
        };
        TestParser.Args args = new TestParser.Args();
        new CommandLine(args).parseArgs(rawArgs);

        TestParser.Group[] expected = new TestParser.Group[] {
            new TestParser.Group("ONE", new HashSet<>(Arrays.asList("CREATE", "DELETE"))),
            new TestParser.Group("TWO", new HashSet<>(Arrays.asList("CREATE"))),
            new TestParser.Group("THREE", new HashSet<>(Arrays.asList("CREATE", "UPDATE", "DELETE"))),
            new TestParser.Group("FOUR", new HashSet<>(Arrays.asList("CREATE", "UPDATE"))),
        };

        for (int i = 0; i < expected.length; i++) {
            TestParser.Group actual = args.groups.get(i);
            assertEquals("group[" + i + "]", expected[i], actual);
        }
    }
}

[#2349] Added test to reproduce the issue.

NOTE TO SELF:
With ArgGroups, the same ArgSpec object is used as a template to initialize multiple user objects.
This means that the logic to determine whether an ArgSpec has been seen by the parser (and therefore, the value is a command line value, not a default value), cannot rely on the ArgSpec instance. But it does.

// Interpreter::applyValuesToCollectionField
if (collection == null ||  !initialized.contains(argSpec))  { // <-- 
    // for the --type=TWO group, the `initialized` set contains the --actions ArgSpec. 
    // Therefore, the binding will not be initialized with an empty collection, 
    // and the command line value is added to the collection with the default value(s)...

NOTE TO SELF:

Index: src/main/java/picocli/CommandLine.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
--- a/src/main/java/picocli/CommandLine.java	
+++ b/src/main/java/picocli/CommandLine.java	(date 1744359357420)
@@ -14339,6 +14339,9 @@
             if (tracer.isInfo()) { tracer.info(traceMessage, argSpec.toString(), String.valueOf(oldValue), displayVal, argDescription, argSpec.scopeString()); }
             int pos = getPosition(argSpec);
             argSpec.setValue(newValue);
+            if (argSpec.group() != null && !parseResultBuilder.isInitializingDefaultValues && argSpec.valueIsDefaultValue) { // #2349 bugfix for ArgGroup / Set / default value
+                argSpec.valueIsDefaultValue = false;
+            }
             parseResultBuilder.addOriginalStringValue(argSpec, actualValue);// #279 track empty string value if no command line argument was consumed
             parseResultBuilder.addStringValue(argSpec, actualValue);
             parseResultBuilder.addTypedValues(argSpec, pos, newValue);
@@ -14355,10 +14358,12 @@
             if (argSpec.auxiliaryTypes().length < 2) { throw new ParameterException(CommandLine.this, argSpec.toString() + " needs two types (one for the map key, one for the value) but only has " + argSpec.auxiliaryTypes().length + " types configured.",argSpec, null); }
             Map<Object, Object> map = argSpec.getValue();
             Tracer tracer = CommandLine.tracer();
-            if (map == null || !initialized.contains(argSpec)) {
+            boolean shouldReset = argSpec.group() != null && !parseResultBuilder.isInitializingDefaultValues && argSpec.valueIsDefaultValue; // #2349 bugfix for ArgGroup / Set / default value
+            if (map == null || !initialized.contains(argSpec) || shouldReset) {
                 tracer.debug("Initializing binding for %s on %s with empty %s", optionDescription("", argSpec, 0), argSpec.scopeString(), argSpec.type().getSimpleName());
                 map = createMap(argSpec.type()); // map class
                 argSpec.setValue(map);
+                argSpec.valueIsDefaultValue = false;
             }
             addToInitialized(argSpec, initialized);
             int originalSize = map.size();
@@ -14541,7 +14546,8 @@
             int pos = getPosition(argSpec);
             List<Object> converted = consumeArguments(argSpec, negated, lookBehind, alreadyUnquoted, alreadyUnquoted, arity, args, argDescription);
             List<Object> newValues = new ArrayList<Object>();
-            if (initialized.contains(argSpec)) { // existing values are default values if initialized does NOT contain argsSpec
+            boolean shouldReset = argSpec.group() != null && !parseResultBuilder.isInitializingDefaultValues && argSpec.valueIsDefaultValue; // #2349 bugfix for ArgGroup / Set / default value
+            if (initialized.contains(argSpec) || shouldReset) { // existing values are default values if initialized does NOT contain argsSpec
                 for (int i = 0; i < length; i++) {
                     newValues.add(Array.get(existing, i)); // keep non-default values
                 }
@@ -14559,6 +14565,9 @@
                 Array.set(array, i, newValues.get(i));
             }
             argSpec.setValue(array);
+            if (shouldReset) {
+                argSpec.valueIsDefaultValue = false;
+            }
             parseResultBuilder.add(argSpec, pos);
             return converted.size(); // return how many args were consumed
         }
@@ -14574,11 +14583,12 @@
             Collection<Object> collection = argSpec.getValue();
             int pos = getPosition(argSpec);
             List<Object> converted = consumeArguments(argSpec, negated, lookBehind, alreadyUnquoted, alreadyUnquoted, arity, args, argDescription);
-            boolean shouldReset = argSpec.group() != null && !parseResultBuilder.isInitializingDefaultValues && argSpec.valueIsDefaultValue;
-            if (collection == null ||  !initialized.contains(argSpec) )  {
+            boolean shouldReset = argSpec.group() != null && !parseResultBuilder.isInitializingDefaultValues && argSpec.valueIsDefaultValue; // #2349 bugfix for ArgGroup / Set / default value
+            if (collection == null ||  !initialized.contains(argSpec) || shouldReset)  {
                 tracer().debug("Initializing binding for %s on %s with empty %s", optionDescription("", argSpec, 0), argSpec.scopeString(), argSpec.type().getSimpleName());
                 collection = createCollection(argSpec.type(), argSpec.auxiliaryTypes()); // collection type, element type
                 argSpec.setValue(collection);
+                argSpec.valueIsDefaultValue = false;
             }
             addToInitialized(argSpec, initialized);
             for (Object element : converted) {

[remkop#2349] Fix incorrect results when using `ArgGroup` + defaultVa…

Navigation Menu

Repository navigation

Incorrect results when using ArgGroup + defaultValue + split + List/Set #2349

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Incorrect results when using ArgGroup + defaultValue + split + List/Set #2349

Description

Activity

remkop commented on Nov 4, 2024

remkop commented on Dec 20, 2024

remkop commented on Apr 11, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions