diff --git a/java/google/registry/tools/RegistryCli.java b/java/google/registry/tools/RegistryCli.java index 89ad73d2ef..c598af4da4 100644 --- a/java/google/registry/tools/RegistryCli.java +++ b/java/google/registry/tools/RegistryCli.java @@ -25,11 +25,11 @@ 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,50 +61,46 @@ 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<>(); + // The > wildcard looks a little funny, but is needed so that + // we can accept maps with value types that are subtypes of Class rather than + // literally that type. For more explanation, see: + // http://www.angelikalanger.com/GenericsFAQ/FAQSections/TypeArguments.html#FAQ104 + @Override + public void run(String[] args) throws Exception { - HelpCommand helpCommand = new HelpCommand(jcommander); - jcommander.addCommand("help", helpCommand); - commandInstances.put("help", helpCommand); + // Create the JCommander instance. + JCommander jcommander = new JCommander(this); + jcommander.addConverterFactory(new ParameterFactory()); + jcommander.setProgramName(programName); - // Add the shell command. - ShellCommand shellCommand = new ShellCommand(System.in, this); - jcommander.addCommand("shell", shellCommand); - commandInstances.put("shell", shellCommand); + // 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); - commandInstances.put(entry.getKey(), command); } } catch (ReflectiveOperationException e) { throw new RuntimeException(e); } - } - // The > wildcard looks a little funny, but is needed so that - // we can accept maps with value types that are subtypes of Class rather than - // literally that type. For more explanation, see: - // http://www.angelikalanger.com/GenericsFAQ/FAQSections/TypeArguments.html#FAQ104 - @Override - public void run(String[] args) throws Exception { try { jcommander.parse(args); } catch (ParameterException e) { @@ -132,11 +128,11 @@ public void run(String[] args) throws Exception { 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 76581f5f7a..59c9323bd8 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 @@ static class MockCli implements CommandRunner { 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()); + } + } }