Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Variable Interpolation appears to ignore required = true #676

Closed
ifedorenko opened this issue Apr 30, 2019 · 8 comments
Closed

Variable Interpolation appears to ignore required = true #676

ifedorenko opened this issue Apr 30, 2019 · 8 comments

Comments

@ifedorenko
Copy link

As a user, I expect the code below to trigger MissingParameterException if neither --mypath parameter nor $MYPATH environment variable are set at runtime. Currently the parameter value is set to null.

	@Option(names="--mypath", defaultValue = "${env:MYPATH}", required = true)
	private String path;

Note that I don't specifically care to use defaultValue annotation attribute, I just need a way to populate parameter value from --mypath or $MYPATH.

@remkop
Copy link
Owner

remkop commented May 1, 2019

Thanks for raising this!
I’m guessing (I haven’t tried yet) that the default value in your example becomes the string "null" instead of the null reference when the environment variable is not defined. Like you point out, that won’t be very useful.

I’ll try to take a more detailed look later today.

@remkop remkop added this to the 4.0-alpha-3 milestone May 2, 2019
remkop added a commit that referenced this issue May 2, 2019
@remkop
Copy link
Owner

remkop commented May 2, 2019

I pushed a fix for this to master.
Can you verify?

@ifedorenko
Copy link
Author

I can confirm that interpolated value is null now (as opposed to "null" string), however required = true is still not respected. In other words, I expect MissingParameterException if the option value was not provided, I don't expect null.

And to reiterate, I don't specifically want to use defaultValue attribute. I am looking for a way to define an option that will check explicit command line arg (--mypath=...) first, then check environment variable ($MYPATH), then fail with clear error message if neither is available (and obviously use --mypath=... or $MYPATH value if available). Do you consider this a reasonable usecase or this isn't something picocli is meant to do?

@remkop
Copy link
Owner

remkop commented May 2, 2019

@ifedorenko Thanks for the confirmation. I've been able to reproduce the validation issue.

This is a bug introduced by the variable interpolation: picocli should not consider options required if a default value exists. The current code simply checks that the defaultValue attribute is non-null, it does not try to resolve the value of the defaultValue attribute. This should not be difficult to fix.

Your use case certainly sounds reasonable and I imagine it is fairly common for applications to have environment variables that can be overridden with command line options. It would be nice if we can use the defaultValue attribute for this instead of introducing some other mechanism.

@remkop remkop closed this as completed in 04d8ae6 May 2, 2019
@remkop
Copy link
Owner

remkop commented May 2, 2019

@ifedorenko I've pushed a fix and an additional test to master. I believe the validation should work correctly now, but please verify. Don't hesitate to reopen this or open additional tickets if there's any problem.

The fix will be included in 4.0-alpha-3. Thanks again for raising this!

@ifedorenko
Copy link
Author

Yes, I can confirm current master correctly fails with Missing required option error message. Thank you! I'd be nice if the message suggested $MYPATH was a way to provide the value, but I can certainly live with the current behaviour. Again, thank you very much for the quick fix for what is likely an edge usecase. Very much appreciate it.

@remkop
Copy link
Owner

remkop commented May 2, 2019

From 4.0-alpha-3, you will be able to customize the error message with a custom IParameterExceptionHandler. This could look something like this:

final CommandLine commandLine = new CommandLine(new MyApp());

IParameterExceptionHandler handler = new IParameterExceptionHandler() {
    IParameterExceptionHandler defaultHandler = commandLine.getParameterExceptionHandler();
    public int handleParseException(ParameterException ex, String[] args) throws Exception {
        if ("Missing required option '--mypath=<path>'".equals(ex.getMessage())) {
            CommandLine cmd = ex.getCommandLine();
            cmd.getErr().println(ex.getMessage() + ", alternatively specify environment variable MYPATH");
            cmd.usage(cmd.getErr());
            return cmd.getCommandSpec().exitCodeOnInvalidInput();
        }
        return defaultHandler.handleParseException(ex, args);
    }
};
int exitCode = commandLine
        .setParameterExceptionHandler(handler)
        .execute(args);

@remkop
Copy link
Owner

remkop commented May 4, 2019

FYI: this is a good use case for demonstrating custom parameter exception handlers. I added a slightly more robust variation of the above to the picocli-examples module: https:/remkop/picocli/blob/master/picocli-examples/src/main/java/picocli/examples/exceptionhandler/ParameterExceptionHandlerDemo.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants