ArgGroup with exclusive=false and multiplicity=1 not behaving as expected

[utmittal]

Here's a minimal example to illustrate the problem:

import java.util.concurrent.Callable;
import picocli.CommandLine;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;

@Command(name = "multiplicity")
class Multiplicity implements Callable<Integer> {

  @ArgGroup(exclusive = false, multiplicity = "1")
  private RequiredGroup requiredGroup;

  static class RequiredGroup {
    @Option(names = "--option_one")
    private String optionOne;

    @Option(names = "--option_two")
    private String optionTwo;
  }

  @Override
  public Integer call() throws Exception {
    return 0;
  }

  public static void main(String... args) {
    int exitCode = new CommandLine(new Multiplicity()).execute(args);
    System.exit(exitCode);
  }
}

With this example, I would expect that the command without any options (i.e. multiplicity), would throw a MissingParameterException. However, in the current version (4.7.4) the command executes and the call() method is executed. Note, that the requiredGroup object is null.

On a much older version of picocli (4.1.4), running this command works as expected. It throws a MissingParameterException with the text Error: Missing required argument(s):.

I am not sure exactly where the regression happened or if this is even a regression? From my reading of the docs, it does seem like one. In the ArgGroup documentation:

define a set of arguments that must co-occur. Set exclusive = false to define a group of options and positional parameters that must always be specified together. Picocli will throw a MissingParameterException if not all the options and positional parameters in a co-occurring group are specified together.

Groups may be optional (multiplicity = "0..1"), required (multiplicity = "1")

When the parent group is required, at least one subgroup must be present.

[ppazos]

Maybe this could be added to the test cases to prevent the regression, also to test it on previous versions.

[remkop]

None of the options in the RequiredGroup group is required, so even though the group itself is required (because it has multiplicity = "1"), the validation logic will not throw a MissingParameterException: it will only raise an alarm if any of the required options/parameters in that group is missing.

Should this be changed? I am not sure... The current logic does make logical sense to me, it allows groups that have one optional and one required option to work as expected.

For your use case, I would recommend making all options in the group required.

I am not sure when the validation changed; I don't remember intentionally changing it but I could have forgotten... 😅

[utmittal]

The use case I am trying to solve for is when at least one of the options is required. I.e. a user should be able to say multiplicity --option_one or multiplicity --option_two or multiplicity --option_one --option_two, but should not be able to say multiplicity.

If I set all the options to required, then the user would necessarily have to specify both options right? Or am I misunderstanding how required works?

A colleague also helped track down this past issue: #947 which addresses our exact use case. The approach suggested there is exactly the one I have outlined in the minimal code above.

I can work around this problem fairly easily by adding custom validation to throw a ParameterException if requiredGroup==null. However, I wanted to confirm whether this is a regression first.

[remkop]

Ouch! Thanks for that link. Now it does look like a regression! 😅

I'll be traveling the next 2 weeks but I'll look at it when I get back.
Thank you for raising this! 🙏

[utmittal]

No problem! Thanks in advance for looking into it (and creating picocli)! We love picocli in our team :)

[vorburger]

@remkop really sorry to re-ping you here, but I was wondering if you already had / already still planning to have a look at this one?

[remkop]

Thanks for the reminder! I'll try to take a look this weekend.

[remkop]

Looks like the regression was introduced by this commit: 4b41a21 (PR #2030).

I will look into a fix that addresses both this ticket and #1848.

[vorburger]

@remkop thank you! Were you hoping to find the time for fixing this yourself?

Or do you need or would welcome contributions, if anyone (not me...) was motivated to?

[utmittal]

I have been poking at this problem this week. I have a potential solution which probably requires a fair bit of polish to actually work. But basically, I would remove the method added in [4b41a21] and instead add logic in the ArgGroupSpec constructor to recalculate the multiplicity based on whether any default values were provided.

Example:

int defaultedCount = 0;
for (ArgSpec arg : args) {
    if (arg.calcDefaultValue(true) != null) {
        defaultedCount++;
    }
}
multiplicity = multiplicity.min(multiplicity.max() - defaultedCount);

I.e. if you have an ArgGroup with multiplicity=1 and an Option within it that has a default value, the multiplicity will become 0..1. If you have multiplicity=3 and 2 options are defaulted, the multiplicity becomes 1..3. This calculation probably needs some more thought to cover all the edge cases.

This should handle both this issue and #1848.

@remkop What do you think? Am I breaking some implicit assumption within the codebase by having the "actual" multiplicity be different to the "defined" multiplicity?

[remkop]

@utmittal Thank you for helping think this through.

I agree that [4b41a21] needs to be rolled back, or refined. Still working on a solution...

[remkop]

Should be fixed in main now. Thank you again for raising this!