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

Option descriptions not read from parent command resource bundle #1706

Closed
rsenden opened this issue Jun 10, 2022 · 8 comments · Fixed by #1710
Closed

Option descriptions not read from parent command resource bundle #1706

rsenden opened this issue Jun 10, 2022 · 8 comments · Fixed by #1710
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨
Milestone

Comments

@rsenden
Copy link
Contributor

rsenden commented Jun 10, 2022

Suppose we have the following command hierarchy:

  RootCommand (resourceBundle=RootMessages)
  - ModuleCommand (resourceBundle=ModuleMessages)
     - Cmd1
     - Cmd2

Behaviour:

  • As expected: If we define usage.footer in the RootMessages bundle, this footer will be displayed on all commands.
  • As expected: If we define option1 in the ModuleMessages bundle, any option1 in any of the leaf commands displays the proper description.
  • Not as expected: If we define option2 in the RootMessages bundle, any option2 in any of the leaf commands displays an empty description

Since our modules are maintained by different teams, we want to have a dedicated resource bundle for each module. However, as we also have some common options that are used throughout the application (mostly through generic mixins), we would like to define the descriptions for those generic options in the RootMessages bundle.

Unfortunately that doesn't seem to work, so for now we have to replicate those command option descriptions across every ModuleMessages bundle in each module. Obviously this is something that we would like to avoid.

Is there any way to make this work with the current picocli version, or will this require a fix in picocli?

@remkop
Copy link
Owner

remkop commented Jun 10, 2022

  • As expected: If we define usage.footer in the RootMessages bundle, this footer will be displayed on all commands.
  • As expected: If we define option1 in the ModuleMessages bundle, any option1 in any of the leaf commands displays the proper description.
  • Not as expected: If we define option2 in the RootMessages bundle, any option2 in any of the leaf commands displays an empty description

To clarify: you are saying that the (non-empty) description for option2 in the RootMessages bundle gets lost because of the intermediate ModuleMessages bundle?

And, the behaviour for options is different from the behaviour for things like the footer?
So the scenario is that ModuleMessages bundle does not contain a usage.footer key, only the top-level RootMessages bundle has this key, but still, while Cmd1 and Cmd2 can access the usage.footer keys from the top-level RootMessages bundle, they cannot access the option2 in the RootMessages bundle?

That seems a bit strange, to be honest... Can you verify that this is really what is happening?

Generally speaking, the easiest way with current picocli would be to have a single ResourceBundle that is set at the top-level command, that has all the keys for all commands. This can be done by prefixing the option names with the command name.

For example:

# single resource bundle set on RootCommand, other commands do not have a resource bundle
RootCommand.ModuleCommand.Cmd1.option1 = description for this option1
RootCommand.ModuleCommand.Cmd2.option1 = description for cmd 2 option1
option2 = generic option2 description for all commands
usage.footer = generic footer for all commands
RootCommand.ModuleCommand.Cmd2.usage.footer = only this command has a special footer

One idea of how to manage this is could be to have separate teams manage separate resource bundles for their own modules, and merge them into a single bundle during the build.

@remkop remkop added the theme: usagehelp An issue or change related to the usage help message label Jun 10, 2022
@rsenden
Copy link
Contributor Author

rsenden commented Jun 10, 2022

Your understanding is correct, this is what seems to be happening. I'm currently running our application through a debugger, will add a comment if I find out more details.

Merging the bundles during the build is also an option, but that could potentially cause some other complications. For example, you can't have module-specific default descriptions. For example, multiple modules may have a --url option, for module 1 you may want to set a generic description 'URL for system 1' and for module 2 'URL for system 2'.

@rsenden
Copy link
Contributor Author

rsenden commented Jun 10, 2022

I think the reason that this is working for footers and most/all other usage.* properties is because of the UsageMessageSpec#footer() method:

public String[] footer() { return arr(resourceArr("usage.footer"), footer, DEFAULT_MULTI_LINE); }

If no footer is defined in the ModuleMessages bundle, resourceArr("usage.footer") returns null, in which case the footer instance variable is used instead. Apparently this variable gets initialized with the usage.footer value defined in the RootMessages bundle.

I guess in order for this to work for options as well, the Messages class would need to look in resource bundles of parent commands if it can't find the property in its own resource bundle. Is this something that you would consider for inclusion in a next version of picocli? I can look into creating a PR for this.

@remkop
Copy link
Owner

remkop commented Jun 10, 2022

Thank you for the analysis of why footers work differently.

I guess in order for this to work for options as well, the Messages class would need to look in resource bundles of parent commands if it can't find the property in its own resource bundle. Is this something that you would consider for inclusion in a next version of picocli? I can look into creating a PR for this.

Yes, I think that would be a nice improvement. If you could create a PR with tests, that would be great!

@rsenden
Copy link
Contributor Author

rsenden commented Jun 13, 2022

@remkop, I looked into having the Messages class take resource bundles from parent commands into account, but ran into an issue. Apparently, CommandSpec doesn't allow for accessing the top-level command that was used to construct the CommandLine object.

Extract from our code:

// CommandLine construction
new CommandLine(FCLIRootCommands.class, micronautFactory)

// FCLIRootCommands
@Command(name="fcli", resourceBundle="...", subcommands={ConfigCommands.class, ...})
public class FCLIRootCommands { ... }

// ConfigCommands
@Command(name="config", resourceBundle="...", subcommands={...})
public class ConfigCommands { ... }

For the config command, I'd expect both parent and root to point to FCLIRootCommands, however parent is null and root points to ConfigCommands itself:

command 'fcli' (user object: class com.fortify.cli.app.FCLIRootCommands), parent: null, root: command 'fcli' (user object: class com.fortify.cli.app.FCLIRootCommands)
command 'config' (user object: class com.fortify.cli.config.picocli.command.ConfigCommands), parent: null, root: command 'config' (user object: class com.fortify.cli.config.picocli.command.ConfigCommands)

Is this behavior by design, or can this be considered a bug? Is there any other way for accessing the CommandSpec of the top-level FCLIRootCommands class while processing the CommandSpec of subcommands?

Edit: It turns out that this issue is caused by initialization order; apparently the Messages instance is constructed before the CommandSpec parent and root have been properly initialized. Once the CommandLine has been fully constructed, parent and root correctly point to FCLIRootCommands. I'll see whether there's an easy way to fix this.

@remkop
Copy link
Owner

remkop commented Jun 13, 2022

Hi @rsenden, that sounds strange, but I cannot reproduce the issue you describe.

I tried with this test:

public class HierarchyTest {

    // FCLIRootCommands
    @Command(name="fcli", /*resourceBundle="...",*/ subcommands={ConfigCommands.class})
    public static class FCLIRootCommands { }

    // ConfigCommands
    @Command(name="config", /*resourceBundle="...",*/ subcommands={ConfigChild.class})
    public static class ConfigCommands { }

    @Command(name="config-child", /*resourceBundle="...",*/ subcommands={})
    public static class ConfigChild { }

    public static void main(String[] args) {
        CommandLine.IFactory factory = CommandLine.defaultFactory();
        CommandLine cmd = new CommandLine(FCLIRootCommands.class, factory);
        print(cmd);
    }

    private static void print(CommandLine cl) {
        Object userObject = cl.getCommand();
        CommandLine.Model.CommandSpec spec = cl.getCommandSpec();
        System.out.printf("%s.name=%s, parent.name=%s, root.name=%s%n",
                userObject.getClass().getSimpleName(),
                spec.name(),
                spec.parent() == null ? "null-parent" : spec.parent().name(),
                spec.root().name());
        for (CommandLine c : cl.getSubcommands().values()) {
            print(c);
        }
    }
}

and it produces the following (correct-looking) output:

FCLIRootCommands.name=fcli, parent.name=null-parent, root.name=fcli
ConfigCommands.name=config, parent.name=fcli, root.name=fcli
ConfigChild.name=config-child, parent.name=config, root.name=fcli

@remkop
Copy link
Owner

remkop commented Jun 13, 2022

Oh, I see you edited your comment, and the issue is related to timing during the initialization phase. Understood.

@rsenden
Copy link
Contributor Author

rsenden commented Jun 13, 2022

Hi @remkop, the following implementation of the Messages class seems to work fine for my application. It uses lazy loading to avoid the issue described in my previous comment; any idea whether there's a risk of Messages instances being called by multiple threads, requiring synchronization?

Summary of changes, as visible here: fortify-ps@6530068

  • Add parent() method (and instance field) for lazy instantiation of a parent Messages instance, searching for parent CommandSpec instances that have a different resource bundle base name than the current Messages instance
  • Update isEmpty() method to return true only if both current and parent Messages instances are empty
  • Add getStringForExactKey() and getStringArrayForExactKey() methods that look up the given key in either current Message object or its parent if available
  • Update getString(), getStringArray() methods to use the getString*ForExactKey() methods, resulting in the following lookup order:
    • Fully qualified key in current Messages instance
    • Fully qualified key in the closest parent Messages instance that contains that key
    • Simple key in current Messages instance
    • Simple key in the closest parent Messages instance that contains that key

Any other ideas or suggestions for improvement? Next up would be to develop associated unit tests, not sure when I'll have time for that though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨
Projects
None yet
2 participants