Issue 1848 fix and unit test

Applying patch for issue 1848 (Required ArgGroup combined with DefaultValueProvider issue) with unit test.
remkop · May 28, 2023 · 4b41a21 · 4b41a21
1 parent 4f7898e
commit 4b41a21
Show file tree

Hide file tree

Showing 2 changed files with 75 additions and 1 deletion.
diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java
@@ -12994,7 +12994,7 @@ void validate(CommandLine commandLine) {
 
                 validationResult = matches.isEmpty() ? GroupValidationResult.SUCCESS_ABSENT : GroupValidationResult.SUCCESS_PRESENT;
                 for (ArgGroupSpec missing : unmatchedSubgroups) {
-                    if (missing.validate() && missing.multiplicity().min > 0) {
+                    if (missing.validate() && missing.multiplicity().min > 0 && containsRequiredOptionsOrSubgroups(missing)) {
                         int presentCount = 0;
                         boolean haveMissing = true;
                         boolean someButNotAllSpecified = false;
@@ -13024,6 +13024,33 @@ void validate(CommandLine commandLine) {
                 }
             }
 
+            private boolean containsRequiredOptionsOrSubgroups(ArgGroupSpec argGroupSpec) {
+                return containsRequiredOptions(argGroupSpec) || containsRequiredSubgroups(argGroupSpec);
+            }
+
+            private boolean containsRequiredOptions(ArgGroupSpec argGroupSpec) {
+                for ( OptionSpec option : argGroupSpec.options() ) {
+                    if ( option.required() ) { return true; }
+                }
+                return false;
+            }
+
+            private boolean containsRequiredSubgroups(ArgGroupSpec argGroupSpec) {
+                for ( ArgGroupSpec subgroup : argGroupSpec.subgroups() ) {
+                    if ( subgroup.exclusive() ) {
+                        // Only return true if all of the subgroups contain required options or subgroups
+                        boolean result = true;
+                        for ( ArgGroupSpec subsubgroup : subgroup.subgroups() ) {
+                            result &= containsRequiredOptionsOrSubgroups(subsubgroup);
+                        }
+                        return result && containsRequiredOptions(subgroup);
+                    } else {
+                        return containsRequiredOptionsOrSubgroups(subgroup);
+                    }
+                }
+                return false;
+            }
+
             private void failGroupMultiplicityExceeded(List<ParseResult.GroupMatch> groupMatches, CommandLine commandLine) {
                 Map<ArgGroupSpec, List<List<ParseResult.GroupMatch>>> matchesPerGroup = new LinkedHashMap<ArgGroupSpec, List<List<GroupMatch>>>();
                 String msg = "";

diff --git a/src/test/java/picocli/DefaultProviderTest.java b/src/test/java/picocli/DefaultProviderTest.java
@@ -7,6 +7,7 @@
 import org.junit.rules.TestRule;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.IDefaultValueProvider;
+import picocli.CommandLine.ArgGroup;
 import picocli.CommandLine.Model.ArgSpec;
 import picocli.CommandLine.Option;
 import picocli.CommandLine.Parameters;
@@ -436,4 +437,50 @@ class App {
         assertEquals(123, app1.a);
     }
 
+    /**
+     * Tests issue 1848 https://github.com/remkop/picocli/issues/1848
+     * Test to ensure that ArgGroups with a multiplicity of 1, with a required option, and with a default value
+     * provider, will properly show that required option as required, rather than optional.
+     * */
+    @Test
+    public void testIssue1848ArgGroupWithRequiredOptionWithDefaultValueProvider() {
+
+        @Command(name = "issue1848Command", defaultValueProvider = Issue1848CommandDefaultProvider.class)
+        class App {
+            @ArgGroup(exclusive = false, multiplicity = "1", order = 1)
+            public Issue1848CommandConfigOptions issue1848CommandConfigOptions;
+
+            @Option(names = {"--opt1"})
+            private String opt1;
+        }
+        String helpOutput = new CommandLine(new App()).getHelp().fullSynopsis();
+        assertTrue(helpOutput.contains("--opt2=<opt2>"));       // Check that "--opt2=<opt2>" exists.
+        assertFalse(helpOutput.contains("[--opt2=<opt2>]"));    // But make sure it's not surrounded by square brackets.
+    }
+
+    class Issue1848CommandConfigOptions {
+        @Option(names = {"--opt2"}, required = true, order = 1)
+        private String opt2;
+
+        @Option(names = {"--opt3"}, required = false, order = 2)
+        private String opt3;
+    }
+
+    static class Issue1848CommandDefaultProvider implements CommandLine.IDefaultValueProvider {
+        @Override
+        public String defaultValue(CommandLine.Model.ArgSpec argSpec) throws Exception {
+            // Commenting out for now as I'm unsure if it's expected behavior for default values supplied to a required
+            // option, should result in that option's help/usage information indicating that the option is not required.
+            /*
+            if (argSpec.isOption()) {
+                CommandLine.Model.OptionSpec option = (CommandLine.Model.OptionSpec) argSpec;
+                if ("--url".equals(option.longestName())) {
+                    return "https://localhost:8080";
+                }
+            }
+            */
+            return null;
+        }
+    }
+
 }