From 18290911a0e8a48ebb9dd532ed65db7111e7104c Mon Sep 17 00:00:00 2001 From: mmuller Date: Mon, 2 Apr 2018 12:46:13 -0700 Subject: [PATCH] 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 | 66 +++++++++---------- .../registry/tools/ShellCommandTest.java | 63 +++++++++++++++++- 2 files changed, 92 insertions(+), 37 deletions(-) diff --git a/java/google/registry/tools/RegistryCli.java b/java/google/registry/tools/RegistryCli.java index 89ad73d2e..c598af4da 100644 --- a/java/google/registry/tools/RegistryCli.java +++ b/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 commandInstances; Map> commands; - JCommander jcommander; + String programName; RegistryCli( String programName, ImmutableMap> 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> 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 > 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> 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()); - if (command == null) { - jcommander.usage(); - return; - } + // JCommander stores sub-commands as nested JCommander objects containing a list of user objects + // to be populated. Extract the subcommand by getting the JCommander wrapper and then + // retrieving the first (and, by virtue of our usage, only) object from it. + JCommander jcCommand = jcommander.getCommands().get(jcommander.getParsedCommand()); + Command command = (Command) Iterables.getOnlyElement(jcCommand.getObjects()); loggingParams.configureLogging(); // Must be called after parameters are parsed. try { diff --git a/javatests/google/registry/tools/ShellCommandTest.java b/javatests/google/registry/tools/ShellCommandTest.java index 76581f5f7..59c9323bd 100644 --- a/javatests/google/registry/tools/ShellCommandTest.java +++ b/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> calls = new ArrayList<>(); @Override - public void run(String[] args) - throws Exception { + public void run(String[] args) 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> commandInvocations = new ArrayList<>(); + + @Parameter(description = "normal argument") + List args; + + public TestCommand() {} + + @Override + public void run() { + ImmutableList.Builder callRecord = new ImmutableList.Builder<>(); + callRecord.add(xparam); + if (args != null) { + callRecord.addAll(args); + } + commandInvocations.add(callRecord.build()); + } + } }