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"