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

Options specified at method level is not reset in shell mode (via jline3). #1531

Closed
kaushalkumar opened this issue Dec 26, 2021 · 6 comments
Closed
Milestone

Comments

@kaushalkumar
Copy link
Contributor

kaushalkumar commented Dec 26, 2021

Picocli reuse command object when used in interactive mode. While reusing it clears all the instance variable annotated with @Option/@Paramters, but does not reset the options specified at method level. It looks like Picocli does not invokes method annotated with @option unless the matching option is specified in command line. Due to this, when the command is executed with the option, then instance variable is initialized and next the command is executed without the option, then same value gets visible.

We thought of using default value attribute to get it reset, but unfortunately, in our application, we cannot predefine the default value, thus could not take this approach. Kindly check it.

Example code

package example;

import org.jline.console.SystemRegistry;
import org.jline.console.impl.Builtins;
import org.jline.console.impl.SystemRegistryImpl;
import org.jline.reader.LineReader;
import org.jline.reader.LineReaderBuilder;
import org.jline.reader.MaskingCallback;
import org.jline.reader.Parser;
import org.jline.reader.impl.DefaultParser;
import org.jline.terminal.Terminal;
import org.jline.terminal.TerminalBuilder;
import picocli.CommandLine;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;
import picocli.shell.jline3.PicocliCommands;
import picocli.shell.jline3.PicocliCommands.PicocliCommandsFactory;

import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
import java.util.function.Supplier;


/**
 * Example.
 */
public class Example {

    /**
     * Top-level command that just prints help.
     */
    @Command(name = "", description = "Example root command.", subcommands = {MyCommand.class})
    static class CliCommands implements Runnable {
        public void run() {
            System.out.println(new CommandLine(this).getUsageMessage());
        }
    }

    /**
     * A command with some options to demonstrate issue.
     */
    @Command(name = "cmd", mixinStandardHelpOptions = true, description = "Example test command.")
    static class MyCommand implements Runnable {
        private List<String> modules;

        @Option(names = "-c", description = "comps", split = ",")
        private List<String> comps;

        public List<String> getModules() {
            return modules;
        }
        @Option(names = "-m", description = "modules", split = ",")
        public void setModules(final List<String> modules) {
            this.modules = modules;
        }
        public void run() {
            System.out.println("Modules = " + modules);
            System.out.println("Comps = " + comps);
        }
    }

    /**
     * Main method.
     * @param args arguments.
     * @throws Exception exception.
     */
    public static void main(final String[] args) throws Exception {
        Supplier<Path> workDir = () -> Paths.get(System.getProperty("user.dir"));
        // set up JLine built-in commands
        Builtins builtins = new Builtins(workDir, null, null);
        // set up picocli commands
        CliCommands commands = new CliCommands();

        PicocliCommandsFactory factory = new PicocliCommandsFactory();

        CommandLine cmd = new CommandLine(commands, factory);
        PicocliCommands picocliCommands = new PicocliCommands(cmd);

        Parser parser = new DefaultParser();
        Terminal terminal = TerminalBuilder.builder().build();
        SystemRegistry systemRegistry = new SystemRegistryImpl(parser, terminal, workDir, null);
        systemRegistry.setCommandRegistries(builtins, picocliCommands);
        LineReader reader = LineReaderBuilder.builder().terminal(terminal).completer(systemRegistry.completer())
                .parser(parser).variable(LineReader.LIST_MAX, 50).build();
        builtins.setLineReader(reader);
        factory.setTerminal(terminal);

        String prompt = "prompt> ";
        String rightPrompt = null;

        // start the shell and process input until the user quits with Ctrl-D
        String line;
        while (true) {
            systemRegistry.cleanUp();
            line = reader.readLine(prompt, rightPrompt, (MaskingCallback) null, null);
            systemRegistry.execute(line);
        }
    }
}

Execution

prompt> cmd
Modules = null
Comps = null
prompt> cmd -c c1 -m m1
Modules = [m1]
Comps = [c1]
prompt> cmd
Modules = [m1]
Comps = null
prompt> 
remkop added a commit that referenced this issue Dec 27, 2021
@remkop
Copy link
Owner

remkop commented Dec 27, 2021

Thank you for raising this.
Thanks for the example code. I used that to add a failing unit test to help debug the issue.

That said, I am currently extremely busy with Log4j, so it may take me a while to get to this.

@remkop
Copy link
Owner

remkop commented Jan 18, 2022

I did some analysis, here are my observations:

With tracing on, I see the following output for the last run (when cmd is invoked again, but this time without any parameters):

[picocli DEBUG] Initializing command 'null' (user object: picocli.Issue1531ResetOptionMethods$MyCommand@78186a70): 2 options, 0 positional parameters, 0 required, 0 groups, 0 subcommands.
[picocli DEBUG] Set initial value for field java.util.List<String> picocli.Issue1531ResetOptionMethods$MyCommand.comps of type interface java.util.List to null.
[picocli DEBUG] Initial value not available for method void picocli.Issue1531ResetOptionMethods$MyCommand.setModules(java.util.List<String>)
[picocli DEBUG] Applying default values for command '<main class>'
[picocli DEBUG] defaultValue not defined for field java.util.List<String> picocli.Issue1531ResetOptionMethods$MyCommand.comps
[picocli DEBUG] defaultValue not defined for method void picocli.Issue1531ResetOptionMethods$MyCommand.setModules(java.util.List<String>)

Note that the comps field (the -c option) has an initial value.
For the -m option, the output says "Initial value not available for method (...)setModules".
Additionally, neither option has an explicit defaultValue defined in the annotation.

This is the cause of the issue: in the absence of an explicit defaultValue, picocli uses the initial value as the default value. But what should that be for the -m (setModules) option?

For annotated fields, it is clear what their "initial value" is. See this example:

class C {
   @Option(names = "-a") int a;            // initial value is primitive int 0 (zero)
   @Option(names = "-b") int b = 123;      // initial value is primitive 123
   @Option(names = "-c") String c;         // initial value is null
   @Option(names = "-d") String d = "abc"; // initial value is "abc"
}

For annotated methods, it is unknowable what the initial value should be (picocli does not know where the value is stored), so at the time that the annotated setter method options feature was introduced, I decided to do nothing.
This results in annotated methods not getting reset between invocations of parseArgs or execute.

I need to think a bit on what to do. Maybe setting to null, not sure yet.

@remkop
Copy link
Owner

remkop commented Jan 18, 2022

The solution I am thinking of now requires annotated setter method options to explicitly set defaultValue = Option.NULL_VALUE.

class M {
   String x;
   @Option(names = "-x", defaultValue = Option.NULL) // reset to null between  parseArgs invocations
   void setX(String x) { this.x = x; }
}

Unfortunately this does not work correctly with picocli 4.6.2, this will be fixed in the next release.

@kaushalkumar Does that meet your requirements?

remkop added a commit that referenced this issue Jan 18, 2022
@remkop
Copy link
Owner

remkop commented Jan 18, 2022

@kaushalkumar I pushed a fix for this to the master branch, can you verify?

@remkop
Copy link
Owner

remkop commented Feb 7, 2022

This has been fixed in the main branch and the fix will be included in the upcoming picocli 4.6.3 release.

note to self:
TODO: update the user manual with the below

[WARNING]
====
Applications that reuse a single `CommandLine` instance for multiple `parseArgs` invocations should be aware of the following:

Annotated setter method options that explicitly set `defaultValue = Option.NULL_VALUE` will get a call with a `null` parameter when the command line did not include that option. 

If the setter method annotation does _not_ explicitly set `defaultValue = Option.NULL_VALUE`, then this method will not be called when the command line did not include that option. This means then the value will retain the value of the previous `parseArgs` invocation, which may not be desirable. (This only happens for applications that reuse a single `CommandLine` instance for multiple `parseArgs` invocations.)

.Java
[source,java,role="primary"]
----
class App {
   String x;
   @Option(names = "-x", defaultValue = Option.NULL) // reset to null when option not specified
   void setX(String x) { this.x = x; }

   String y;
   @Option(names = "-y") // retains value of previous parseArgs invocation when option not specified
   void setY(String y) { this.y = y; }
}
----

.Kotlin
[source,kotlin,role="secondary"]
----
class App {
   var x
   @Option(names = ["-x"], defaultValue = Option.NULL) // reset to null when option not specified
   fun setX(x: String) { this.x = x }

   var y
   @Option(names = ["-y"]) // retains value of previous parseArgs invocation when option not specified
   fun setY(y: String) { this.y = y }
}
----

====

remkop added a commit that referenced this issue Feb 7, 2022
@remkop
Copy link
Owner

remkop commented Feb 8, 2022

Closing this ticket, the documentation has been updated.

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