Validating programmatically added values in Picocli 4.6.1

[Dominik Guhr]

Hi Remko,

me again 🙈I have a question about validating programmatically added values, which boils down to: What is the best/intended way to do this?

I think about using a preprocessor or  a custom converter for this case. 

A little bit of context: We're parsing values from different config sources and show them as Options in the CLI.

High level, it goes like this:

                                                PropertyMapper                                                 OptionBuilder

<quarkusconfig>(1:1,1:n) <-----------------------------> <keycloakconfig>(1:1) <-----------------------> cli option.

So we take the quarkus config and put it inside a PropertyMapper object.  The relations may be 1:1 or 1:n. see here for an example on how we convert  database values from quarkus (db for 1:n relation): https://github.com/keycloak/keycloak/blob/main/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/DatabasePropertyMappers.java

The mappers are added to picocli  CommandLine instance here: https://github.com/keycloak/keycloak/blob/9be4c289d86328e74876920cca42dda95693f11a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java#L375-L393 

Now for the usecase: "db" has some expected values, and so do other options. I want to validate that the value we get from the CLI matches one of the pre-defined values in "supportedDatabaseVendors" array, so that e.g.

<picocli> build --db=foo 

fails with the proper validation message, and prints the expected values ("expectedValues" in the builder above).

I think about adding a .validationFunc(Predicate<string>) to the PropertyMapper builder which would then return a converter/preprocessor for each property where validation for correct value is needed, and then add this function in the optionBuilder via .converter(mapper.validationFunc()) (or preprocessor) - but I wanted to make sure first that this is the intended way or if I overlook something. 

Would be great to get some hints for the right direction. :) 

Thank you for your help.

Best regards,

Dominik 

[Remko Popma]

I believe the simplest (and easiest to maintain) would be to put all validation in one place.

Do you use the CommandLine.execute method, or the parseArgs method?

The reason I am asking is that if you use the execute method, the top-level command must have some user object set that implements Runnable or Callable.

The beginning of the run/call method is a natural place to invoke the validation logic from.

Also, when validation fails, the application simply throw a ParameterException from the run/call method, and picocli will show an error message and usage help to the end user.

  (If showing the full usage help message is too verbose, see the section on setting a custom ParameterExceptionHandler https://picocli.info/#_invalid_user_input ).

If you use the parseArgs method, you can do something similar, and do the validation when the parseArgs method returns.

If validation fails, the application can show an error message and some help (again, see ShortErrorMessageHandler in https://picocli.info/#_invalid_user_input ).

[Dominik Guhr]

Hey Remko, 

thanks for answering. So we have different run methods,  which also have a different subset of options (one shows buildtime + runtime options, other just runtime for example). You can find the available commands in this package here: https://github.com/keycloak/keycloak/tree/main/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/command -  so what you suggest is to create the validation in the respective run methods, e.g. having a RuntimePropertyValidator etc.? 

I thought more of having all related things for one (programmatic) picocli option in one place at the Propertymapper class, and add the converter to the builder and let the converter throw a parameterexception if validation fails (its all stringtypeconverters then, and it might be that I am "misusing" them). That'd lead to having to look only in the propertymapper to create the right mapping from cli to quarkus. But if converters do not suite this usecase, your suggestion is perfectly valid. Thanks again for the input, I'll see what I can do :)

Best regards,

Dominik 

[Dominik Guhr]

Ok, thinking out loud here a bit: 

-  seems I am on the wrong track with the converter approach, bc it needs a reference to the actual CommandLine. So not possible to add a preconfigured validate() method as a lambda to the builder and then call the lambda in the optionbuilder via .converter(lambda) (more concise it would be .inputvalidators(lambda) or sth).  

For your question:

We're using the execute method in https://github.com/keycloak/keycloak/blob/430fd35e2f055e1f4ee3c4c625301795fa2e880a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java#L104 to run the commands specified. So it might be the easiest solution to create a few Validator-classes (e.g. for the same propertygroups we have mappers for) and add them to the specific run-commands of start.java/build.java, ... - is that what you are suggesting? Just to get it right :) 

[Remko Popma]

On Tuesday, November 9, 2021 at 7:52:54 PM UTC+9 dg...@redhat.com wrote:

Ok, thinking out loud here a bit: 

-  seems I am on the wrong track with the converter approach, bc it needs a reference to the actual CommandLine. So not possible to add a preconfigured validate() method as a lambda to the builder and then call the lambda in the optionbuilder via .converter(lambda) (more concise it would be .inputvalidators(lambda) or sth).  

For your question:

We're using the execute method in https://github.com/keycloak/keycloak/blob/430fd35e2f055e1f4ee3c4c625301795fa2e880a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java#L104 to run the commands specified. So it might be the easiest solution to create a few Validator-classes (e.g. for the same propertygroups we have mappers for) and add them to the specific run-commands of start.java/build.java, ... - is that what you are suggesting? Just to get it right :) 

Yes that is what I had in mind.

Then to fail validation, all that the validation logic would need to do is throw a ParameterException.

(see https://picocli.info/#_custom_validation)

That also makes it relatively easy to test the validation logic.

[Dominik Guhr]

ok sorry I have to correct myself. so we're using parseArgs, actually, in picocli.java line 83. so line 84 would actually be the right place to put custom validation logic into, if I understood you correctly :)   

[Remko Popma]

Looks like your application is taking 2 passes parsing the input.

The 1st time is for basic sanity checking and enriching the input.

The 2nd time is to invoke the business logic.

Perhaps business-level validation "belongs" closer to the business logic?

This may be a matter of taste.

There is also something to be said for doing all validation together after the first pass (since the first pass also has some validation function).

That said, if it is nicer to have validation separate for each subcommand, then perhaps calling your Validator classes from the run/call method of each command's business logic is best.

Not sure which is better.

[Pedro Igor]

We ended up creating our own `IParameterConsumer` and it seems to help us a lot in terms of control on how we validate options.

In our case, options are dynamic (sometimes just mapping some other property from Quarkus) and they have a very strict spec (e.g.: descriptions with a single line, etc).

It should also be able to enable value conversion quite easily by looking at Picocli documentation.

Does this usage make sense for you @Remko?

Regards.

Pedro Igor

[Dominik Guhr]

I also have a working PoC here [1] with some type checking for the http-enabled applied. Just thinking: When I set this type now, can I check it in the Consumer or would the standard error message when input does not match expected values from type appear? Have to try tomorrow :)  Nevertheless, thanks for helping Remko, you're doing great! :)

[1] 
1a: impl: https://github.com/DGuhr/keycloak/blob/cli_type_validation/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java
1b: setting the type in the mapper: https://github.com/DGuhr/keycloak/blob/8d3430fe03cce44d899707fb3ceb299697a5c35e/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/HttpPropertyMappers.java#L27
1c: setting the type from mapper to optionbuilders type: https://github.com/DGuhr/keycloak/blob/8d3430fe03cce44d899707fb3ceb299697a5c35e/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java#L395

[Remko Popma]

On Wednesday, November 10, 2021 at 2:16:10 AM UTC+9 pigor.c...@gmail.com wrote:

We ended up creating our own `IParameterConsumer` and it seems to help us a lot in terms of control on how we validate options.

In our case, options are dynamic (sometimes just mapping some other property from Quarkus) and they have a very strict spec (e.g.: descriptions with a single line, etc).

It should also be able to enable value conversion quite easily by looking at Picocli documentation.

Does this usage make sense for you @Remko?

 

Hi Pedro, I am not sure I understand the question.

Are you saying you are doing validation from inside the IParameterConsumer?

That sounds like a good idea, because you can easily just throw a ParameterException when validation fails.

(And also have separate validation logic per option which may help with testing.)

Is there anything remaining that is unclear or you want to ask about? 

[Remko Popma]

On Wednesday, November 10, 2021 at 2:42:30 AM UTC+9 dg...@redhat.com wrote:

I also have a working PoC here [1] with some type checking for the http-enabled applied. Just thinking: When I set this type now, can I check it in the Consumer or would the standard error message when input does not match expected values from type appear? Have to try tomorrow :)  Nevertheless, thanks for helping Remko, you're doing great! :)

Type Converters can throw a TypeConversionException (see https://picocli.info/#_handling_invalid_input ).

From ParameterConsumers I am not sure, maybe just throw a ParameterException, because that has a reference to the command where the exception occurred, and would display the usage help message for that particular subcommand (or sub-subcommand for deeply nested command hierarchies). 

[Pedro Igor]

On Tuesday, November 9, 2021 at 10:07:44 PM UTC-3 remko...@gmail.com wrote:

On Wednesday, November 10, 2021 at 2:16:10 AM UTC+9 pigor.c...@gmail.com wrote:

We ended up creating our own `IParameterConsumer` and it seems to help us a lot in terms of control on how we validate options.

In our case, options are dynamic (sometimes just mapping some other property from Quarkus) and they have a very strict spec (e.g.: descriptions with a single line, etc).

It should also be able to enable value conversion quite easily by looking at Picocli documentation.

Does this usage make sense for you @Remko?

 

Hi Pedro, I am not sure I understand the question.

Are you saying you are doing validation from inside the IParameterConsumer?

That sounds like a good idea, because you can easily just throw a ParameterException when validation fails.

(And also have separate validation logic per option which may help with testing.)

Is there anything remaining that is unclear or you want to ask about? 

No, that is it. Thanks. I was just checking if we could rely on parameter consumer for validation (and perhaps also conversion in the same place).

Thanks.