Mercurial > hg > thermostat-ng > agent
changeset 2472:54ed75a2852d
Fix bug where subcommand-specific required options are required unconditionally
PR3192
Reviewed-by: jerboaa
Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2016-October/021198.html
author | Andrew Azores <aazores@redhat.com> |
---|---|
date | Thu, 13 Oct 2016 14:03:12 -0400 |
parents | 6c527f28ead0 |
children | 7659652d08ea |
files | launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java |
diffstat | 2 files changed, 153 insertions(+), 9 deletions(-) [+] |
line wrap: on
line diff
--- a/launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java Thu Oct 13 09:37:20 2016 -0400 +++ b/launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java Thu Oct 13 14:03:12 2016 -0400 @@ -361,22 +361,15 @@ notifier.addActionListener(listener); } } - Options options = cmdInfo.getOptions(); - for (PluginConfiguration.Subcommand subcommand : cmdInfo.getSubcommands()) { - for (Option option : (Collection<Option>) subcommand.getOptions().getOptions()) { - options.addOption(option); - } - } - Arguments args = null; try { - args = parseCommandArguments(cmdArgs, options); + Options options = mergeSubcommandOptionsWithParent(cmdInfo, cmdArgs); + Arguments args = parseCommandArguments(cmdArgs, options); setupLogLevel(args); CommandContext ctx = setupCommandContext(cmd, args); cmd.run(ctx); } catch (CommandLineArgumentParseException e) { out.println(e.getMessage()); runHelpCommandFor(cmdName); - return; } } @@ -390,6 +383,42 @@ out.println(message.getContents()); } + // Note: this has the side-effect of adding subcommands' options to the parent command's Options. + // An Options copy-constructor or Options.remove(Option) method could help us here. The problem with this side-effect + // is that the subcommand options cannot be removed, only overridden again later, which prevents us from "resetting" + // the state to cause the Options parser to reject options for subcommands which have not been invoked in the current + // command line. This means that subcommand-specific non-required options are always accepted when passed to the parent + // command or any "sibling" subcommand and it is left up to the command implementation to reject or ignore the errant + // option. + // See http://icedtea.classpath.org/pipermail/thermostat/2016-October/021198.html + private Options mergeSubcommandOptionsWithParent(CommandInfo cmdInfo, String[] cmdArgs) { + Options options = cmdInfo.getOptions(); + PluginConfiguration.Subcommand selectedSubcommand = getSelectedSubcommand(cmdInfo, cmdArgs); + String selectedName = selectedSubcommand == null ? "" : selectedSubcommand.getName(); + + for (PluginConfiguration.Subcommand subcommand : cmdInfo.getSubcommands()) { + for (Option option : (Collection<Option>) subcommand.getOptions().getOptions()) { + Option copy = new Option(option.getOpt(), option.getLongOpt(), option.hasArg(), option.getDescription()); + boolean required = selectedName.equals(subcommand.getName()) && option.isRequired(); + copy.setRequired(required); + options.addOption(copy); + } + } + + return options; + } + + private PluginConfiguration.Subcommand getSelectedSubcommand(CommandInfo cmdInfo, String[] cmdArgs) { + for (PluginConfiguration.Subcommand subcommand : cmdInfo.getSubcommands()) { + for (String arg : cmdArgs) { + if (subcommand.getName().equals(arg)) { + return subcommand; + } + } + } + return null; + } + private void setupLogLevel(Arguments args) { if (args.hasArgument(CommonOptions.LOG_LEVEL_ARG)) { String levelOption = args.getArgument(CommonOptions.LOG_LEVEL_ARG);
--- a/launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java Thu Oct 13 09:37:20 2016 -0400 +++ b/launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java Thu Oct 13 14:03:12 2016 -0400 @@ -424,6 +424,55 @@ Options subOptions = mock(Options.class); Option optOption = new Option("o", "opt", false, "mock opt option"); + optOption.setRequired(false); + when(subOptions.getOptions()).thenReturn(Collections.singleton(optOption)); + when(subInfo.getOptions()).thenReturn(subOptions); + + when(info1.getSubcommands()).thenReturn(Collections.singletonList(subInfo)); + String expected = "foo, bar"; + runAndVerifyCommand(new String[] {"test1", "sub", "--opt", "--arg1", "foo", "--arg2", "bar"}, expected, false); + } + + @Test + public void testSubcommandOptionRequiredAndNotProvided() { + PluginConfiguration.Subcommand subInfo = mock(PluginConfiguration.Subcommand.class); + when(subInfo.getName()).thenReturn("sub"); + when(subInfo.getDescription()).thenReturn("subcommand description"); + + Options subOptions = mock(Options.class); + Option optOption = new Option("o", "opt", false, "mock opt option"); + optOption.setRequired(true); + when(subOptions.getOptions()).thenReturn(Collections.singleton(optOption)); + when(subInfo.getOptions()).thenReturn(subOptions); + + when(info1.getSubcommands()).thenReturn(Collections.singletonList(subInfo)); + String expected = "Missing required option: -o\n" + + "usage: thermostat test1 <--arg1 <arg>> [--arg2 <arg>]\n" + + " description 1\n" + + "\n" + + "thermostat test1\n" + + " --arg1 <arg>\n" + + " --arg2 <arg>\n" + + " --help show usage of command\n" + + " -l,--logLevel <arg>\n" + + " -o,--opt mock opt option\n" + + "\n" + + "Subcommands:\n" + + "\n" + + "sub:\n" + + "subcommand description\n\n\n"; + runAndVerifyCommand(new String[] {"test1", "sub", "--arg1", "foo", "--arg2", "bar"}, expected, false); + } + + @Test + public void testSubcommandOptionRequired() { + PluginConfiguration.Subcommand subInfo = mock(PluginConfiguration.Subcommand.class); + when(subInfo.getName()).thenReturn("sub"); + when(subInfo.getDescription()).thenReturn("subcommand description"); + + Options subOptions = mock(Options.class); + Option optOption = new Option("o", "opt", false, "mock opt option"); + optOption.setRequired(true); when(subOptions.getOptions()).thenReturn(Collections.singleton(optOption)); when(subInfo.getOptions()).thenReturn(subOptions); @@ -433,6 +482,72 @@ } @Test + public void testSubcommandOptionNotRequiredIfSubcommandNotInvoked() { + PluginConfiguration.Subcommand subInfo = mock(PluginConfiguration.Subcommand.class); + when(subInfo.getName()).thenReturn("sub"); + when(subInfo.getDescription()).thenReturn("subcommand description"); + + Options subOptions = mock(Options.class); + Option optOption = new Option("o", "opt", false, "mock opt option"); + optOption.setRequired(true); + when(subOptions.getOptions()).thenReturn(Collections.singleton(optOption)); + when(subInfo.getOptions()).thenReturn(subOptions); + + when(info1.getSubcommands()).thenReturn(Collections.singletonList(subInfo)); + String expected = "foo, bar"; + runAndVerifyCommand(new String[] {"test1", "--arg1", "foo", "--arg2", "bar"}, expected, false); + } + + // This tests the case where we have a parent command "test1" with subcommand "sub", + // which has a subcommand-specific option -o/--opt which is not required. "sub" is not + // invoked, but --opt is passed anyway. + // Due to limitations in our options processing, this case cannot be easily rejected as + // invalid. It is accepted instead and the option passed on to the parent command, which + // must on its own decide to reject or ignore the errant --opt. + // See http://icedtea.classpath.org/pipermail/thermostat/2016-October/021198.html + @Test + public void testSubcommandOptionStillRecognizedWhenSubcommandNotInvoked() { + PluginConfiguration.Subcommand subInfo = mock(PluginConfiguration.Subcommand.class); + when(subInfo.getName()).thenReturn("sub"); + when(subInfo.getDescription()).thenReturn("subcommand description"); + + Options subOptions = mock(Options.class); + Option optOption = new Option("o", "opt", false, "mock opt option"); + optOption.setRequired(true); + when(subOptions.getOptions()).thenReturn(Collections.singleton(optOption)); + when(subInfo.getOptions()).thenReturn(subOptions); + + when(info1.getSubcommands()).thenReturn(Collections.singletonList(subInfo)); + String expected = "foo, bar"; + runAndVerifyCommand(new String[] {"test1", "--opt", "--arg1", "foo", "--arg2", "bar"}, expected, false); + } + + // This tests the case where we have a parent command "test1" with subcommand "sub", + // which has a subcommand-specific option -o/--opt which is not required. "sub" is not + // invoked, but --opt is passed anyway. + // Due to limitations in our options processing, this case cannot be easily rejected as + // invalid. It is accepted instead and the option passed on to the parent command, which + // must on its own decide to reject or ignore the errant --opt. + // See http://icedtea.classpath.org/pipermail/thermostat/2016-October/021198.html + @Test + public void testSubcommandOptionOverridesParentOption() { + PluginConfiguration.Subcommand subInfo = mock(PluginConfiguration.Subcommand.class); + when(subInfo.getName()).thenReturn("sub"); + when(subInfo.getDescription()).thenReturn("subcommand description"); + + Options subOptions = mock(Options.class); + // parent command also has a --arg1 option which *does* take an argument + Option optOption = new Option(null, "arg1", false, null); + optOption.setRequired(true); + when(subOptions.getOptions()).thenReturn(Collections.singleton(optOption)); + when(subInfo.getOptions()).thenReturn(subOptions); + + when(info1.getSubcommands()).thenReturn(Collections.singletonList(subInfo)); + String expected = "null, bar"; + runAndVerifyCommand(new String[] {"test1", "sub", "--arg1", "foo", "--arg2", "bar"}, expected, false); + } + + @Test public void testBadOption() { String expected = "Could not parse options: Unrecognized option: --argNotAccepted\n" + "usage: thermostat test1 <--arg1 <arg>> [--arg2 <arg>]\n"