Unmatched argument returned when no equal sign used with option using a converter

[NicolasMassart]

Issue noticed in app using PicoCLI Java 3.9.2 but also experienced with latest info.picocli:picocli:3.9.5.

Command line option code looks like this :

 @Option(
      names = {"--bootnodes"},
      split = ",",
      arity = "0..*",
      converter = EnodeToURIPropertyConverter.class)
  private final Collection<URI> bootNodes = null;

I'm skipping some useless details as not necessary for our story, but feel free to ask, the code is open source and can be seen at https://github.com/PegaSysEng/pantheon/blob/1966f485739ed75e4a8da8b95cd6a0afd2b365af/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java#L181

This option takes a list of enode strings separated by commas as input, or nothing if you want to override the default value. Default value is provided by an IDefaultValueProvider not visible here.

Our converter transforms a string to a URI using. This converter first check the string using a regex and com.google.common.base.Preconditions#checkArgument(boolean, java.lang.Object) if there's any issue checkArgument throws an IllegalArgumentException(reason of the failure)

If you run pantheon --bootnodes enode://d2567893371ea5a6fa6371d483891ed0d129e79a8fc74d6df95a00a6545444cd4a6960bbffe0b4e2edcf35135271de57ee559c0909236bbc2074346ef2b5b47c@127.0.0.1:30304 this works as the input format is ok. But if you run for instance pantheon --bootnodes enode://invalidid@127.0.0.1:30304, the format being wrong it return :

Unmatched argument: enode://invalidid@127.0.0.1:30304
Did you mean: public-key?

It proposes public-key subcommand as a possible argument because it's probably the closest thing it found but yet we don't have any idea of the issue in the argument.

However, the strange thing, and that's where it becomes interesting, is that when we call the program with same command pantheon --bootnodes=enode://invalidid@127.0.0.1:30304 but with an equal sign between the option and the value, it returns the full issue message Invalid value for option '--bootnodes' at index 0 (<enode://id@host:port>): cannot convert 'enode://invalidid@127.0.0.1:30304' to URI (java.lang.IllegalArgumentException: Enode URL contains an invalid node ID. Node ID must have 128 characters and shouldn't include the '0x' hex prefix.) which is what we want to enable users to understand why it fails.

So having the equal sign here changes how it handles the error. I guess it's related to arity as we enable the option to have no value, but it seems to be related to the converter too as it work totally fine it you do this without a converter and an option of Collection type.

Any idea where I can search ?

[remkop]

Analysis

I haven't tried to reproduce the issue, but looking at the code, I believe you are right and this is related to arity. The bootNodes field is a Collection, so when picocli encounters the --bootnodes option, the Interpreter::consumeArguments method is invoked to process the option parameters.

When processing option parameters for a collection, first, any mandatory parameters are processed.

After the mandatory parameters, the option may have optional parameters (like when arity is 1..2, 1..*, etc). In your example, all parameters are optional.

When processing optional parameters, picocli needs to be careful: any command line argument may be an additional parameter, but it may also be a new option, or a subcommand, or a positional parameters. So, for each argument, picocli does the following:

1. check varargCanConsumeNextValue; if the argument is a known subcommand or option name, we stop processing optional parameters.
2. check canConsumeOneArgument; this tries to convert the argument to the option auxiliary type (URI), and if this fails, picocli concludes that the argument is not a parameter for the option and stops processing optional parameters.

This last logic is what is happening, I believe.

The reason why picocli can display a better error message when the user specifies --bootnodes=enode://invalidid@127.0.0.1:30304 with an equal sign between the option name and the option parameter is that picocli "bumps" the option arity from 0..* to 1..* when it sees the equals sign.

Ideas for Solutions

Question: does the bootNodes option really need accept a variable number of parameters? To be exact, do you need to allow input of the form:

-x 1 2 3 4 5

or would it also be acceptable to require users to specify the option multiple times, like this:

-x 1 -x 2 -x 3 -x 4

If the latter is acceptable, then the solution is to change the arity of the --bootnodes option to the default arity, which is arity = "1". This means that users can specify one value for each --bootnodes option on the command line:

--bootnodes enode://invalidid@127.0.0.1:30304 --bootnodes enode://invalid#2@127.0.0.1:222222 

This means that picocli will expect exactly one parameter for each option, and will be able to display the correct error message when the user give invalid input.

[NicolasMassart]

Very interesting. I never foreseen this possibility. I discuss that with my team. Thanks.

[NicolasMassart]

The idea of using one option for each value doesn't seem to please the team... We really would like the CLI to consider the faulty parameter as a parameter even if there's no equal sign. The only way I see right now is to have a string instead of a typed parameter using a converter. Then convert it later once all the command line is parsed and then raise an error if needed. It doesn't seem very clean to me but at least it would not break and the bright side is that it doesn't require any change in PicoCLI... What's your opinion ? Dirty hack or not ?

[remkop]

I don't think it's a dirty hack. You simply have complex requirements. It's a real-life app. :-)

It's like you said: in order to support variable arity, we need to postpone the type conversion to after picocli added the value to the bootNodes collection. That allows picocli to add invalid values to the collection.

How about using an annotated @Option method instead of an annotated field? Something like this:

private Collection<URI> bootNodes = null;

@Spec
CommandSpec spec;

@Option(names = {"--bootnodes"}, split = ",", arity = "0..*")
void replaceBootnodes(List<String> values) {
    bootNodes = new ArrayList<>();
    EnodeToURIPropertyConverter converter = new EnodeToURIPropertyConverter();
    String value;
    try {
        for (value : values) {
            bootNodes.add(converter.convert(value));
        }
    } catch (Exception ex) {
        throw new ParameterException(spec.commandLine(), 
                "Invalid bootNode value " + value + ": " + ex.getMessage());
    }
}

You need to experiment a bit to make sure that this also works when users do specify the --bootnodes option multiple times, but I believe the above should work.

[remkop]

You could push the looping into the converter, to get cleaner code in the setter method:

private Collection<URI> bootNodes = null;

@Spec
CommandSpec spec;

@Option(names = {"--bootnodes"}, split = ",", arity = "0..*")
void replaceBootnodes(List<String> values) {
    try {
        bootNodes = EnodeToURIPropertyConverter.convertAll(values);
    } catch (Exception ex) {
        throw new ParameterException(spec.commandLine(),
                "Invalid bootnode value: " + ex.getMessage());
    }
}

[NicolasMassart]

Awsome, I will try that and tell you. Thanks a lot !

[remkop]

One could argue that even the error handling could (and perhaps should) be pushed into the converter. That would leave you with very simple code in the annotated option setter method:

private Collection<URI> bootNodes = null;

@Spec
CommandSpec spec;

@Option(names = {"--bootnodes"}, split = ",", arity = "0..*")
void replaceBootnodes(List<String> values) {
    bootNodes = EnodeToURIPropertyConverter.convertAll(values, "bootnode", spec);
}

[NicolasMassart]

I implemented your last proposal with just a change that I don't use a converter but just a lambda and handle the error in the option method. I prefer to keep this that way for the moment as we are planning a refactoring soon. And it works like a charm. Thanks for your (as usual, always) very efficient help.