Required ArgGroup combined with DefaultValueProvider issue

[rsenden]

We have an ArgGroup defined like the following:

@ArgGroup(exclusive = false, multiplicity = "1", order = 1)
@Getter private UrlConfigOptions urlConfigOptions;

class UrlConfigOptions {
    @Option(names = {"--url"}, required = true, order=1)
    @Getter private String url;
    
    @Option(names = {"--proxy-host"}, required = false, order=2)
    @Getter private String proxyHost;
    
    @Option(names = {"--proxy-port"}, required = false, order=3)
    @Getter private Integer proxyPort;
    
    @Option(names = {"--proxy-user"}, required = false, order=4)
    @Getter private String proxyUser;
    
    @Option(names = {"--proxy-password"}, required = false, interactive = true, echo = false, order=5)
    @Getter private char[] proxyPassword;
    
    @Option(names = {"--insecure", "-k"}, required = false, description = "Disable SSL checks", defaultValue = "false", order=6)
    @Getter private Boolean insecureModeEnabled;
}

This usually works as expected, with picocli requiring at least the --url option and the other options being optional, with a synopsis like the following:
(--url=<url> [--proxy-host=<proxyHost>] [--proxy-port=<proxyPort>] [--proxy-user=<proxyUser>] [--proxy-password]... [-k])

However, if we configure a global default value provider (in our case retrieving default values from environment variables), the synopsis changes to the following, indicating that --url is now optional:
([--url=<url>] [--proxy-host=<proxyHost>] [--proxy-port=<proxyPort>] [--proxy-user=<proxyUser>] [--proxy-password]... [-k])

Due to multiplicitly = "1" on the ArgGroup, picocli still requires at least one of the options in the ArgGroup to be specified on the command line, even though the required --url option is already being satisfied through the default value provider.

Any suggestions on how to get this fixed?

[rsenden]

The following changes fix this issue in our application:

Add method GroupMatchContainer::containsRequiredOptions:

            private boolean containsRequiredOptions(ArgGroupSpec argGroupSpec) {
                for ( OptionSpec option : argGroupSpec.options() ) {
                    if ( option.required() ) { return true; }
                }
                return false;
            }

Call this new method in the GroupMatchContainer::validate method:

if (missing.validate() && missing.multiplicity().min > 0 && containsRequiredOptions(missing) ) {

I'm not sure though whether there's any valid use case for having multiplicity = "1" on an ArgGroup that has only optional options. If that's the case, maybe we should add a new attribute to the ArgSpec annotation to indicate that it's not required if it contains only non-required options. Any suggestions for the name for this attribute, if necessary?

[rsenden]

Actually, the above didn't work well for a nested tree of ArgGroups, where some of the ArgGroups have exclusive = true.

The following updates to GroupMatchContainer seem to work much better:

            // In GroupMatchContainer::validate:
            // if (missing.validate() && missing.multiplicity().min > 0 && containsRequiredOptionsOrSubgroups(missing) ) {

            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;
            }

The containsRequiredSubgroups method could be improved, but first want to get your opinion on this approach.

[rsenden]

@remkop Any comments on this? Because of this issue, our project still needs to use a patched version of the CommandLine class; preferably we would like to start using an official picocli release in the near future.

[remkop]

The last attempt that handles both options and subgroups looks good. Can you provide a PR with tests?

[MikeTheSnowman]

Hey @remkop. Sorry for the delay here, but I'll be submitting a PR soon with the fix from @rsenden and the associated unit test.
I have three questions for you though:

1. For the unit test, I've put it in DefaultProviderTest.java. I have doubts that this is the best location to put the unit test in, so if you want the unit test placed in some other location, please let me know.

2. I've noticed that for command options which are a part of an ArgGroup, they are unable to receive a default value via DefaultValueProvider. Is this expected behavior? I tried looking through the tests in both DefaultProviderTest.java and ArgGroupTest.java that test for ArgGroup options receiving a default value via DefaultValueProvider, but I didn't see any. If feel this is unexpected behavior, then I'll create a new issue. Or if you suspect that I might be doing something wrong, then I welcome any guidance you can provide.

3. I've noticed that for a required option which could possibly have a default value supplied via DefaultValueProvider, the help/synopsis usage information could change for that one specific option, from being required to optional. Using the example that @rsenden used earlier:

   • If the default value provider class returns null for a required option, then the help/usage output will look like the following:
     (--url=<url> ...)
   • But if a non-null value is returned for that required option then the help/usage output will look like:
     ([--url=<url>] ...)

   Maybe this last thing (my 3rd question) is a non-issue. But I figured that maybe options explicitly set to required = true should always be shown as a required option in the help/usage output, regardless if DefaultValueProvider is able to provide a value or not. What do you think?

[MikeTheSnowman]

Hello @remkop and @rsenden .
I think that the initially proposed fix needs to be updated slightly as I noticed that it was causing the unit test testIssue1027RepeatingPositionalParamsWithMinMultiplicity to fail as the fix doesn't check for positional parameters. PR #2030 has been raised with the updated fix and unit test. Please let me know if you see any issues.

[remkop]

@wtfacoconut @rsenden I will try to get to this soon.

[MikeTheSnowman]

@wtfacoconut @rsenden I will try to get to this soon.

Roger that, take your time. 🙂

[MikeTheSnowman]

Hello @remkop. Apologies, but on further testing, we're finding out that the PR we submitted (#2030) for this issue doesn't cover all scenarios. I'd like to request for this issue to be reopened. We've created a new fix for which we'll be submitting a new PR for. I'm just evaluating the existing unit test to see if it's sufficient before raising the next PR.

[remkop]

Absolutely, no worries.
Just reference the issue and previous PR in the new PR.

Just a heads-up that I'll be traveling from Saturday for 2 weeks and I won't be taking my pc, so please be patient. 😅

[MikeTheSnowman]

No worries! Enjoy your trip and bring me back a t-shirt. 😉

[remkop]

Hi @wtfacoconut and @rsenden, it looks like #2030 has caused a regression:

when an arg-group has multiplicity=1, then picocli should require that at least one of the options in the group is specified.
See #2059: the test case in #947 now no longer works... (I should have added that test to the ArgGroupTest class, apologies.)

This conflicts with your expectations in the description for this ticket.

One idea is to revert the changes made in #2030, and satisfy your requirements by changing your application: change the UrlConfigOptions group to have multiplicity = "0..1" (and if necessary validate that the url option has a default).

Thoughts?

[MikeTheSnowman]

Hey @remkop, @rsenden. I am currently handling some family related matters. I noticed this issue as well but won't be able to update untill the day after tomorrow.
Essentially what needs to happen is that our check needs to be updated to handle required parameters.

[remkop]

Hi @wtfacoconut, thanks for the quick reply.

Essentially what needs to happen is that our check needs to be updated to handle required parameters.

I am not sure that the problem is really related to required options/parameters:
note that the test case in #947 has two non-required options, and we still want picocli to show a "missing option" error when none of these options are specified because the group's multiplicity is 1. The idea is that the user must specify one or both of these options, but omitting all options should trigger an error.

I am not sure how that squares with the description of this ticket (#1848), where you don't want that error (because the required option has a default value). To be honest I am not sure yet what the solution is. One idea is to model the multiplicity of the UrlConfigOptions group in your application as "0..1". There may be other ideas.

But it does appear that #2030 introduced a regression, and some change is needed to make the test in #947 pass again.

[MikeTheSnowman]

Hello @remkop, I'm fully back on line now. I used the unit test that you provided in #947 to reproduce the issue that you mentioned.

Are you saying that you're "not sure yet what the solution is" because there are some potentially deeper issues/implications, or something?

Also, in the cotext of the unit test that you wrote for #947, would it be a reasonable expectation to have ArgGroups with a multiplicity of 1, recognize default values for options (provided by either an annotation or default value provider class), so that it doesn't cause an error about a missing option/value?

[remkop]

Sorry for the late response.

---

First (and this is an aside), looking at a question in an earlier comment:

2. I've noticed that for command options which are a part of an ArgGroup, they are unable to receive a default value via DefaultValueProvider. Is this expected behavior? I tried looking through the tests in both DefaultProviderTest.java and ArgGroupTest.java that test for ArgGroup options receiving a default value via DefaultValueProvider, but I didn't see any. If feel this is unexpected behavior, then I'll create a new issue. Or if you suspect that I might be doing something wrong, then I welcome any guidance you can provide.

This is not expected behaviour. I don't see why options/positional params in a group should not be able to get default values via DefaultValueProvider... If this is still the case please raise a separate ticket.

---

Back to the main topic:

I just pushed a change to improve the solution introduced in [4b41a21] by adding support that allows detecting whether an ArgSpec's value came from a default value or from the command line.
Can you review?