Navigation Menu

Skip to content

Commit

Permalink
Fix multiple invocations of the "shell" command
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@mindhog @jianglai
mindhog authored and jianglai committed Apr 2, 2018
1 parent 54a8cd0 commit 1829091
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 32 deletions.
56 changes: 26 additions & 30 deletions java/google/registry/tools/RegistryCli.java
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -61,50 +61,46 @@ 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<>();
// The <? extends Class<? extends Command>> wildcard looks a little funny, but is needed so that
// we can accept maps with value types that are subtypes of Class<? extends Command> 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<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
// we can accept maps with value types that are subtypes of Class<? extends Command> 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) {
Expand Down Expand Up @@ -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 {
Expand Down
63 changes: 61 additions & 2 deletions javatests/google/registry/tools/ShellCommandTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -60,9 +65,63 @@ static class MockCli implements CommandRunner {
public ArrayList<ImmutableList<String>> 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<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());
}
}
}

0 comments on commit 1829091

Please sign in to comment.