Pass along -E arguments to subcommands

Having MultiCommand silently accept and ignore -E arguments could be
confusing, so instead pass those arguments along to subcommands.

Author: pugnascotia

libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

@@ -21,11 +21,14 @@
 
 import joptsimple.NonOptionArgumentSpec;
 import joptsimple.OptionSet;
+import joptsimple.OptionSpec;
+import joptsimple.util.KeyValuePair;
 import org.elasticsearch.core.internal.io.IOUtils;
 
 import java.io.IOException;
-import java.util.Arrays;
+import java.util.ArrayList;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -36,6 +39,7 @@ public class MultiCommand extends Command {
     protected final Map<String, Command> subcommands = new LinkedHashMap<>();
 
     private final NonOptionArgumentSpec<String> arguments = parser.nonOptions("command");
+    private final OptionSpec<KeyValuePair> settingOption;
 
     /**
      * Construct the multi-command with the specified command description and runnable to execute before main is invoked.
@@ -45,9 +49,7 @@ public class MultiCommand extends Command {
      */
     public MultiCommand(final String description, final Runnable beforeMain) {
         super(description, beforeMain);
-        // Accepting -E here does not prevent subcommands receiving -E arguments, since we also set `posixlyCorrect` below.
-        // This stops option parsing when the first non-option is encountered.
-        parser.accepts("E", "Unused. Pass to a subcommand instead").withRequiredArg();
+        this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
         parser.posixlyCorrect(true);
     }
 
@@ -69,15 +71,24 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception {
         if (subcommands.isEmpty()) {
             throw new IllegalStateException("No subcommands configured");
         }
-        String[] args = arguments.values(options).toArray(new String[0]);
-        if (args.length == 0) {
+
+        // .values(...) returns an unmodifiable list
+        final List<String> args = new ArrayList<>(arguments.values(options));
+        if (args.isEmpty()) {
             throw new UserException(ExitCodes.USAGE, "Missing command");
         }
-        Command subcommand = subcommands.get(args[0]);
+
+        String subcommandName = args.remove(0);
+        Command subcommand = subcommands.get(subcommandName);
         if (subcommand == null) {
-            throw new UserException(ExitCodes.USAGE, "Unknown command [" + args[0] + "]");
+            throw new UserException(ExitCodes.USAGE, "Unknown command [" + subcommandName + "]");
         }
-        subcommand.mainWithoutErrorHandling(Arrays.copyOfRange(args, 1, args.length), terminal);
+
+        for (final KeyValuePair pair : this.settingOption.values(options)) {
+            args.add("-E" + pair);
+        }
+
+        subcommand.mainWithoutErrorHandling(args.toArray(new String[0]), terminal);
     }
 
     @Override

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

@@ -19,21 +19,25 @@
 
 package org.elasticsearch.cli;
 
+import joptsimple.ArgumentAcceptingOptionSpec;
 import joptsimple.OptionSet;
+import joptsimple.util.KeyValuePair;
 import org.junit.Before;
 
 import java.io.IOException;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static org.hamcrest.Matchers.containsString;
+
 public class MultiCommandTests extends CommandTestCase {
 
     static class DummyMultiCommand extends MultiCommand {
 
         final AtomicBoolean closed = new AtomicBoolean();
 
         DummyMultiCommand() {
-            super("A dummy multi command", () -> {
-            });
+            super("A dummy multi command", () -> {});
         }
 
         @Override
@@ -75,7 +79,23 @@ public void close() throws IOException {
         }
     }
 
-    DummyMultiCommand multiCommand;
+    static class DummySettingsSubCommand extends DummySubCommand {
+        private final ArgumentAcceptingOptionSpec<KeyValuePair> settingOption;
+
+        DummySettingsSubCommand() {
+            super();
+            this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
+        }
+
+        @Override
+        protected void execute(Terminal terminal, OptionSet options) throws Exception {
+            final List<KeyValuePair> values = this.settingOption.values(options);
+            terminal.println("Settings: " + values);
+            super.execute(terminal, options);
+        }
+    }
+
+    private DummyMultiCommand multiCommand;
 
     @Before
     public void setupCommand() {
@@ -87,27 +107,21 @@ protected Command newCommand() {
         return multiCommand;
     }
 
-    public void testNoCommandsConfigured() throws Exception {
-        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
-            execute();
-        });
+    public void testNoCommandsConfigured() {
+        IllegalStateException e = expectThrows(IllegalStateException.class, this::execute);
         assertEquals("No subcommands configured", e.getMessage());
     }
 
-    public void testUnknownCommand() throws Exception {
+    public void testUnknownCommand() {
         multiCommand.subcommands.put("something", new DummySubCommand());
-        UserException e = expectThrows(UserException.class, () -> {
-            execute("somethingelse");
-        });
+        UserException e = expectThrows(UserException.class, () -> execute("somethingelse"));
         assertEquals(ExitCodes.USAGE, e.exitCode);
         assertEquals("Unknown command [somethingelse]", e.getMessage());
     }
 
-    public void testMissingCommand() throws Exception {
+    public void testMissingCommand() {
         multiCommand.subcommands.put("command1", new DummySubCommand());
-        UserException e = expectThrows(UserException.class, () -> {
-            execute();
-        });
+        UserException e = expectThrows(UserException.class, this::execute);
         assertEquals(ExitCodes.USAGE, e.exitCode);
         assertEquals("Missing command", e.getMessage());
     }
@@ -121,6 +135,19 @@ public void testHelp() throws Exception {
         assertTrue(output, output.contains("command2"));
     }
 
+    /**
+     * Check that if -E arguments are passed to the main command, then they are accepted
+     * and passed on to the subcommand.
+     */
+    public void testSettingsOnMainCommand() throws Exception {
+        multiCommand.subcommands.put("command1", new DummySettingsSubCommand());
+        execute("-Esetting1=value1", "-Esetting2=value2", "command1", "otherArg");
+
+        String output = terminal.getOutput();
+        assertThat(output, containsString("Settings: [setting1=value1, setting2=value2]"));
+        assertThat(output, containsString("Arguments: [otherArg]"));
+    }
+
     public void testSubcommandHelp() throws Exception {
         multiCommand.subcommands.put("command1", new DummySubCommand());
         multiCommand.subcommands.put("command2", new DummySubCommand());