Fix multiple invocations of the "shell" command

JCommander doesn't seem to reset objects when it populates them with data from
an argument list during command processing, so recreate the command objects
every time we do a run().

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=191332392

java/google/registry/tools/RegistryCli.java

@@ -25,11 +25,11 @@ import com.beust.jcommander.ParametersDelegate;
 import com.google.appengine.tools.remoteapi.RemoteApiInstaller;
 import com.google.appengine.tools.remoteapi.RemoteApiOptions;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import google.registry.model.ofy.ObjectifyService;
 import google.registry.tools.Command.RemoteApiCommand;
 import google.registry.tools.params.ParameterFactory;
 import java.security.Security;
-import java.util.HashMap;
 import java.util.Map;
 import org.bouncycastle.jce.provider.BouncyCastleProvider;
 
@@ -61,42 +61,15 @@ final class RegistryCli implements AutoCloseable, CommandRunner {
   private AppEngineConnection connection;
   private RemoteApiInstaller installer;
 
-
-  Map<String, Command> commandInstances;
   Map<String, ? extends Class<? extends Command>> commands;
-  JCommander jcommander;
+  String programName;
 
   RegistryCli(
       String programName, ImmutableMap<String, ? extends Class<? extends Command>> commands) {
+    this.programName = programName;
     this.commands = commands;
 
     Security.addProvider(new BouncyCastleProvider());
-    jcommander = new JCommander(this);
-    jcommander.addConverterFactory(new ParameterFactory());
-    jcommander.setProgramName(programName);
-
-    // Store the instances of each Command class here so we can retrieve the same one for the
-    // called command later on.  JCommander could have done this for us, but it doesn't.
-    commandInstances = new HashMap<>();
-
-    HelpCommand helpCommand = new HelpCommand(jcommander);
-    jcommander.addCommand("help", helpCommand);
-    commandInstances.put("help", helpCommand);
-
-    // Add the shell command.
-    ShellCommand shellCommand = new ShellCommand(System.in, this);
-    jcommander.addCommand("shell", shellCommand);
-    commandInstances.put("shell", shellCommand);
-
-    try {
-      for (Map.Entry<String, ? extends Class<? extends Command>> entry : commands.entrySet()) {
-        Command command = entry.getValue().getDeclaredConstructor().newInstance();
-        jcommander.addCommand(entry.getKey(), command);
-        commandInstances.put(entry.getKey(), command);
-      }
-    } catch (ReflectiveOperationException e) {
-      throw new RuntimeException(e);
-    }
   }
 
   // The <? extends Class<? extends Command>> wildcard looks a little funny, but is needed so that
@@ -105,6 +78,29 @@ final class RegistryCli implements AutoCloseable, CommandRunner {
   //   http://www.angelikalanger.com/GenericsFAQ/FAQSections/TypeArguments.html#FAQ104
   @Override
   public void run(String[] args) throws Exception {
+
+    // Create the JCommander instance.
+    JCommander jcommander = new JCommander(this);
+    jcommander.addConverterFactory(new ParameterFactory());
+    jcommander.setProgramName(programName);
+
+    // Create the "help" and "shell" commands (these are special in that they don't have a default
+    // constructor).
+    jcommander.addCommand("help", new HelpCommand(jcommander));
+    jcommander.addCommand("shell", new ShellCommand(System.in, this));
+
+    // Create all command instances. It would be preferrable to do this in the constructor, but
+    // JCommander mutates the command instances and doesn't reset them so we have to do it for every
+    // run.
+    try {
+      for (Map.Entry<String, ? extends Class<? extends Command>> entry : commands.entrySet()) {
+        Command command = entry.getValue().getDeclaredConstructor().newInstance();
+        jcommander.addCommand(entry.getKey(), command);
+      }
+    } catch (ReflectiveOperationException e) {
+      throw new RuntimeException(e);
+    }
+
     try {
       jcommander.parse(args);
     } catch (ParameterException e) {
@@ -132,11 +128,11 @@ final class RegistryCli implements AutoCloseable, CommandRunner {
     checkState(RegistryToolEnvironment.get() == environment,
         "RegistryToolEnvironment argument pre-processing kludge failed.");
 
-    Command command = commandInstances.get(jcommander.getParsedCommand());
+    // JCommander stores sub-commands as nested JCommander objects containing a list of user objects
-    if (command == null) {
+    // to be populated.  Extract the subcommand by getting the JCommander wrapper and then
-      jcommander.usage();
+    // retrieving the first (and, by virtue of our usage, only) object from it.
-      return;
+    JCommander jcCommand = jcommander.getCommands().get(jcommander.getParsedCommand());
-    }
+    Command command = (Command) Iterables.getOnlyElement(jcCommand.getObjects());
     loggingParams.configureLogging();  // Must be called after parameters are parsed.
 
     try {

javatests/google/registry/tools/ShellCommandTest.java

@@ -15,13 +15,18 @@
 package google.registry.tools;
 
 import static com.google.common.truth.Truth.assertThat;
+import static google.registry.testing.JUnitBackports.assertThrows;
 import static java.nio.charset.StandardCharsets.US_ASCII;
 import static org.mockito.Mockito.mock;
 
+import com.beust.jcommander.MissingCommandException;
+import com.beust.jcommander.Parameter;
+import com.beust.jcommander.Parameters;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import java.io.ByteArrayInputStream;
 import java.util.ArrayList;
+import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -60,9 +65,63 @@ public class ShellCommandTest {
     public ArrayList<ImmutableList<String>> calls = new ArrayList<>();
 
     @Override
-    public void run(String[] args)
+    public void run(String[] args) throws Exception {
-        throws Exception {
       calls.add(ImmutableList.copyOf(args));
     }
   }
+
+  @Test
+  public void testMultipleCommandInvocations() throws Exception {
+    RegistryCli cli =
+        new RegistryCli("unittest", ImmutableMap.of("test_command", TestCommand.class));
+    RegistryToolEnvironment.UNITTEST.setup();
+    cli.setEnvironment(RegistryToolEnvironment.UNITTEST);
+    cli.run(new String[] {"test_command", "-x", "xval", "arg1", "arg2"});
+    cli.run(new String[] {"test_command", "-x", "otherxval", "arg3"});
+    cli.run(new String[] {"test_command"});
+    assertThat(TestCommand.commandInvocations)
+        .containsExactly(
+            ImmutableList.of("xval", "arg1", "arg2"),
+            ImmutableList.of("otherxval", "arg3"),
+            ImmutableList.of("default value"));
+  }
+
+  @Test
+  public void testNonExistentCommand() throws Exception {
+    RegistryCli cli =
+        new RegistryCli("unittest", ImmutableMap.of("test_command", TestCommand.class));
+
+    cli.setEnvironment(RegistryToolEnvironment.UNITTEST);
+    assertThrows(MissingCommandException.class, () -> cli.run(new String[] {"bad_command"}));
+  }
+
+  @Parameters(commandDescription = "Test command")
+  static class TestCommand implements Command {
+    @Parameter(
+      names = {"-x", "--xparam"},
+      description = "test parameter"
+    )
+    String xparam = "default value";
+
+    // List for recording command invocations by run().
+    //
+    // This has to be static because it gets populated by multiple TestCommand instances, which are
+    // created in RegistryCli by using reflection to call the constructor.
+    static final List<List<String>> commandInvocations = new ArrayList<>();
+
+    @Parameter(description = "normal argument")
+    List<String> args;
+
+    public TestCommand() {}
+
+    @Override
+    public void run() {
+      ImmutableList.Builder<String> callRecord = new ImmutableList.Builder<>();
+      callRecord.add(xparam);
+      if (args != null) {
+        callRecord.addAll(args);
+      }
+      commandInvocations.add(callRecord.build());
+    }
+  }
 }