Logic for FoundCommandResult.Status / Add tests for CommandHandler

- Fix other tests
- Add logic for smaller pending FIXME's
This commit is contained in:
ljacqu 2015-12-17 22:23:43 +01:00
parent 3e928cddf0
commit cbf0996197
9 changed files with 154 additions and 133 deletions

View File

@ -440,13 +440,6 @@ public class AuthMe extends JavaPlugin {
new API(this);
}
/**
* Set up the command handler.
*/
private void setupCommandHandler(PermissionsManager permissionsManager) {
this.commandHandler = new CommandHandler(CommandInitializer.getBaseCommands(), permissionsManager);
}
/**
* Load the plugin's settings.
*

View File

@ -150,6 +150,7 @@ public class CommandDescription {
*
* @return The number of parents, e.g. for "/authme abc def" the parent count is 2 ("/authme abc", "/authme")
*/
// TODO ljacqu 20151217: If we always use `getParentCount() + 1` rewrite this method to a `getLabelCount()`
public int getParentCount() {
if (parent == null) {
return 0;

View File

@ -12,6 +12,10 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.INCORRECT_ARGUMENTS;
import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.MISSING_BASE_COMMAND;
import static fr.xephi.authme.command.FoundCommandResult.ResultStatus.UNKNOWN_LABEL;
/**
* The AuthMe command handler, responsible for mapping incoming commands to the correct {@link CommandDescription}
* or to display help messages for unknown invocations.
@ -41,7 +45,7 @@ public class CommandHandler {
* Map a command that was invoked to the proper {@link CommandDescription} or return a useful error
* message upon failure.
*
* @param sender The command sender (Bukkit).
* @param sender The command sender.
* @param bukkitCommandLabel The command label (Bukkit).
* @param bukkitArgs The command arguments (Bukkit).
*
@ -74,6 +78,13 @@ public class CommandHandler {
return true;
}
/**
* Check a command's permissions and execute it with the given arguments if the check succeeds.
*
* @param sender The command sender
* @param command The command to process
* @param arguments The arguments to pass to the command
*/
private void executeCommandIfAllowed(CommandSender sender, CommandDescription command, List<String> arguments) {
if (permissionsManager.hasPermission(sender, command)) {
command.getExecutableCommand().executeCommand(sender, arguments);
@ -112,7 +123,7 @@ public class CommandHandler {
if (result.getDifference() < SUGGEST_COMMAND_THRESHOLD && result.getCommandDescription() != null) {
sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD + "/"
+ result.getCommandDescription() + ChatColor.YELLOW + "?");
// TODO: Define a proper string representation of command description
// FIXME: Define a proper string representation of command description
}
sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + result.getLabels().get(0)
@ -144,14 +155,20 @@ public class CommandHandler {
sender.sendMessage(ChatColor.DARK_RED + "You don't have permission to use this command!");
}
/**
* Map incoming command parts to a command. This processes all parts and distinguishes the labels from arguments.
*
* @param parts The parts to map to commands and arguments
* @return The generated {@link FoundCommandResult}
*/
public FoundCommandResult mapPartsToCommand(final List<String> parts) {
if (CollectionUtils.isEmpty(parts)) {
return new FoundCommandResult(null, parts, null, 0.0, FoundCommandResult.ResultStatus.MISSING_BASE_COMMAND);
return new FoundCommandResult(null, parts, null, 0.0, MISSING_BASE_COMMAND);
}
CommandDescription base = getBaseCommand(parts.get(0));
if (base == null) {
return new FoundCommandResult(null, parts, null, 0.0, FoundCommandResult.ResultStatus.MISSING_BASE_COMMAND);
return new FoundCommandResult(null, parts, null, 0.0, MISSING_BASE_COMMAND);
}
// Prefer labels: /register help goes to "Help command", not "Register command" with argument 'help'
@ -167,21 +184,31 @@ public class CommandHandler {
}
private FoundCommandResult getCommandWithSmallestDifference(CommandDescription base, List<String> parts) {
final String label = parts.get(0);
final String childLabel = parts.size() >= 2 ? parts.get(1) : null;
double minDifference = Double.POSITIVE_INFINITY;
CommandDescription closestCommand = null;
for (CommandDescription child : base.getChildren()) {
double difference = getLabelDifference(child, label);
if (difference < minDifference) {
minDifference = difference;
closestCommand = child;
if (childLabel != null) {
for (CommandDescription child : base.getChildren()) {
double difference = getLabelDifference(child, childLabel);
if (difference < minDifference) {
minDifference = difference;
closestCommand = child;
}
}
}
// FIXME: Return the full list of labels and arguments
// FIXME: Return INVALID_ARGUMENTS instead of UNKNOWN_LABEL when more accurate
return new FoundCommandResult(
closestCommand, null, null, minDifference, FoundCommandResult.ResultStatus.UNKNOWN_LABEL);
// base command may have no children or no child label was present
if (closestCommand == null) {
return new FoundCommandResult(null, null, parts, minDifference, UNKNOWN_LABEL);
}
FoundCommandResult.ResultStatus status = (minDifference == 0.0) ? INCORRECT_ARGUMENTS : UNKNOWN_LABEL;
final int partsSize = parts.size();
List<String> labels = parts.subList(0, Math.min(closestCommand.getParentCount() + 1, partsSize));
List<String> arguments = parts.subList(Math.min(partsSize - 1, labels.size()), partsSize);
return new FoundCommandResult(closestCommand, labels, arguments, minDifference, status);
}
private CommandDescription getBaseCommand(String label) {

View File

@ -10,9 +10,11 @@ import java.util.List;
* <ul>
* <li>{@link ResultStatus#SUCCESS} entails that mapping the input to a command was successful. Therefore,
* the command description, labels and arguments are set. The difference is 0.0.</li>
* <li>{@link ResultStatus#INCORRECT_ARGUMENTS}</li>
* <li>{@link ResultStatus#UNKNOWN_LABEL}</li>
* <li>{@link ResultStatus#MISSING_BASE_COMMAND} should never occur. Any other fields may not be present and any
* <li>{@link ResultStatus#INCORRECT_ARGUMENTS}: The received parts could be mapped to a command but the argument
* count doesn't match. Guarantees that the command description field is not null; difference is 0.0</li>
* <li>{@link ResultStatus#UNKNOWN_LABEL}: The labels could not be mapped to a command. The command description may
* be set to the most similar command, or it may be null. Difference is above 0.0.</li>
* <li>{@link ResultStatus#MISSING_BASE_COMMAND} should never occur. All other fields may be null and any further
* processing of the object should be aborted.</li>
* </ul>
*/
@ -56,30 +58,14 @@ public class FoundCommandResult {
this(commandDescription, arguments, labels, 0.0, ResultStatus.SUCCESS);
}
/**
* Get the command description.
*
* @return Command description.
*/
public CommandDescription getCommandDescription() {
return this.commandDescription;
}
/**
* Get the command arguments.
*
* @return The command arguments.
*/
public List<String> getArguments() {
return this.arguments;
}
/**
* Get the original query reference.
*
* @return Original query reference.
*/
public List<String> getLabels() {
return this.labels;
}

View File

@ -1,6 +1,7 @@
package fr.xephi.authme.command.help;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import fr.xephi.authme.command.CommandArgumentDescription;
import fr.xephi.authme.command.CommandDescription;
import fr.xephi.authme.command.CommandPermissions;
@ -62,12 +63,11 @@ public final class HelpProvider {
lines.add(ChatColor.GOLD + "==========[ " + Settings.helpHeader + " HELP ]==========");
CommandDescription command = foundCommand.getCommandDescription();
// TODO ljacqu 20151212: Remove immutability once class is stable. We don't want mutability but the overhead
// isn't worth it either. This is just a temporary safeguard during development
List<String> labels = ImmutableList.copyOf(foundCommand.getLabels());
List<String> correctLabels = ImmutableList.copyOf(filterCorrectLabels(command, labels));
if (!hasFlag(HIDE_COMMAND, options)) {
printCommand(command, labels, lines); // FIXME: Pass `correctLabels` and not `labels`
printCommand(command, correctLabels, lines);
}
if (hasFlag(SHOW_LONG_DESCRIPTION, options)) {
printDetailedDescription(command, lines);
@ -79,7 +79,7 @@ public final class HelpProvider {
printPermissions(command, sender, permissionsManager, lines);
}
if (hasFlag(SHOW_ALTERNATIVES, options)) {
printAlternatives(command, labels, lines);
printAlternatives(command, correctLabels, lines);
}
if (hasFlag(SHOW_CHILDREN, options)) {
printChildren(command, labels, lines);
@ -89,19 +89,8 @@ public final class HelpProvider {
}
private static void printCommand(CommandDescription command, List<String> correctLabels, List<String> lines) {
// Ensure that we have all labels to go to the command
int requiredLabels = command.getParentCount() + 1;
List<String> givenLabels = new ArrayList<>(correctLabels);
// Only case this is possible: givenLabels.size() == 1 && requiredLabels == 2,
// since command.getParentCount() never exceeds 1 in AuthMe
// FIXME: Might be smart to put this logic outside and to pass it as `correctLabels`? We will need this at a few
// places annotated with a FIXME
if (givenLabels.size() < requiredLabels) {
givenLabels.add(command.getLabels().get(0));
}
// FIXME: Create highlight logic to mark arguments and the 2nd label as yellow
String syntaxLine = "/" + CommandUtils.labelsToString(givenLabels);
String syntaxLine = "/" + CommandUtils.labelsToString(correctLabels);
for (CommandArgumentDescription argument : command.getArguments()) {
syntaxLine += " " + formatArgument(argument);
}
@ -132,19 +121,14 @@ public final class HelpProvider {
}
}
// FIXME: labels is currently assumed to be only the ones leading to the given command, but we have scenarios where
// we're guessing the command, so the final label isn't any existing one
private static void printAlternatives(CommandDescription command, List<String> labels, List<String> lines) {
private static void printAlternatives(CommandDescription command, List<String> correctLabels, List<String> lines) {
if (command.getLabels().size() <= 1) {
return;
}
// Print the header
lines.add(ChatColor.GOLD + "Alternatives:");
// Get the label used
// fixme this is not correct if help is triggered by incorrect number of arguments
final String usedLabel = labels.get(labels.size() - 1);
final String usedLabel = correctLabels.get(correctLabels.size() - 1);
// Create a list of alternatives
List<String> alternatives = new ArrayList<>();
@ -169,11 +153,11 @@ public final class HelpProvider {
for (String alternative : alternatives) {
// fixme add highlight functionality (see commented old line)
// sender.sendMessage(" " + _HelpSyntaxHelper.getCommandSyntax(command, commandReference, alternative, true));
lines.add(" " + CommandUtils.labelsToString(labels) + " " + alternative);
lines.add(" " + CommandUtils.labelsToString(correctLabels) + " " + alternative);
}
}
public static void printPermissions(CommandDescription command, CommandSender sender,
private static void printPermissions(CommandDescription command, CommandSender sender,
PermissionsManager permissionsManager, List<String> lines) {
CommandPermissions permissions = command.getCommandPermissions();
if (permissions == null || CollectionUtils.isEmpty(permissions.getPermissionNodes())) {
@ -220,6 +204,7 @@ public final class HelpProvider {
}
}
/** Format a command argument with the proper type of brackets. */
private static String formatArgument(CommandArgumentDescription argument) {
if (argument.isOptional()) {
return " [" + argument.getName() + "]";
@ -231,4 +216,26 @@ public final class HelpProvider {
return (flag & options) != 0;
}
private static List<String> filterCorrectLabels(CommandDescription command, List<String> labels) {
List<CommandDescription> commands = new ArrayList<>(command.getParentCount() + 1);
CommandDescription currentCommand = command;
while (currentCommand != null) {
commands.add(command);
currentCommand = currentCommand.getParent();
}
commands = Lists.reverse(commands);
List<String> correctLabels = new ArrayList<>();
boolean foundIncorrectLabel = false;
for (int i = 0; i < commands.size(); ++i) {
if (!foundIncorrectLabel && i < labels.size() && commands.get(i).hasLabel(labels.get(i))) {
correctLabels.add(labels.get(i));
} else {
foundIncorrectLabel = true;
correctLabels.add(commands.get(i).getLabels().get(0));
}
}
return correctLabels;
}
}

View File

@ -3,14 +3,10 @@ package fr.xephi.authme.command;
import fr.xephi.authme.permission.DefaultPermission;
import fr.xephi.authme.permission.PermissionsManager;
import fr.xephi.authme.permission.PlayerPermission;
import fr.xephi.authme.util.WrapperMock;
import org.bukkit.command.CommandSender;
import org.junit.BeforeClass;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
@ -19,15 +15,11 @@ import java.util.Set;
import static java.util.Arrays.asList;
import static java.util.Collections.singletonList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.stringContainsInOrder;
import static org.mockito.BDDMockito.given;
import static org.mockito.Matchers.anyListOf;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.nullValue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
/**
* Test for {@link CommandHandler}.
@ -40,8 +32,6 @@ public class CommandHandlerTest {
@BeforeClass
public static void setUpCommandHandler() {
WrapperMock.createInstance();
CommandDescription authMeBase = createCommand(null, null, singletonList("authme"));
createCommand(PlayerPermission.LOGIN, authMeBase, singletonList("login"), newArgument("password", false));
createCommand(PlayerPermission.LOGIN, authMeBase, asList("register", "reg"),
@ -54,48 +44,89 @@ public class CommandHandlerTest {
}
@Test
@Ignore
public void shouldForwardCommandToExecutable() {
public void shouldMapPartsToLoginChildCommand() {
// given
CommandSender sender = Mockito.mock(CommandSender.class);
given(sender.isOp()).willReturn(true);
String bukkitLabel = "authme";
String[] args = {"login", "password"};
List<String> parts = Arrays.asList("authme", "login", "test1");
// when
handler.processCommand(sender, bukkitLabel, args);
FoundCommandResult result = handler.mapPartsToCommand(parts);
// then
final CommandDescription loginCmd = getChildWithLabel("login", getCommandWithLabel("authme", commands));
verify(sender, never()).sendMessage(anyString());
verify(loginCmd.getExecutableCommand()).executeCommand(eq(sender), anyListOf(String.class));
assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("login", "authme")));
assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.SUCCESS));
assertThat(result.getArguments(), contains("test1"));
assertThat(result.getDifference(), equalTo(0.0));
}
@Test
@Ignore // TODO ljacqu Fix test --> command classes too tightly coupled at the moment
public void shouldRejectCommandWithTooManyArguments() {
public void shouldMapPartsToCommandWithNoCaseSensitivity() {
// given
CommandSender sender = Mockito.mock(CommandSender.class);
given(sender.isOp()).willReturn(true);
String bukkitLabel = "authme";
String[] args = {"login", "password", "__unneededArgument__"};
List<String> parts = Arrays.asList("Authme", "REG", "arg1", "arg2");
// when
boolean result = handler.processCommand(sender, bukkitLabel, args);
FoundCommandResult result = handler.mapPartsToCommand(parts);
// then
assertThat(result, equalTo(true));
assertSenderGotMessageContaining("help", sender);
assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.SUCCESS));
assertThat(result.getArguments(), contains("arg1", "arg2"));
assertThat(result.getDifference(), equalTo(0.0));
}
@Test
public void shouldRejectCommandWithTooManyArguments() {
// given
List<String> parts = Arrays.asList("authme", "register", "pass123", "pass123", "pass123");
// when
FoundCommandResult result = handler.mapPartsToCommand(parts);
// then
assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.INCORRECT_ARGUMENTS));
assertThat(result.getDifference(), equalTo(0.0));
}
@Test
public void shouldSuggestCommandWithSimilarLabel() {
// given
List<String> parts = Arrays.asList("authme", "reh", "pass123", "pass123");
// when
FoundCommandResult result = handler.mapPartsToCommand(parts);
// then
assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.UNKNOWN_LABEL));
assertThat(result.getDifference() < 0.75, equalTo(true));
}
/** In contrast to the previous test, we test a command request with a very apart label. */
@Test
public void shouldSuggestMostSimilarCommand() {
// given
List<String> parts = Arrays.asList("authme", "asdfawetawty4asdca");
// when
FoundCommandResult result = handler.mapPartsToCommand(parts);
// then
assertThat(result.getCommandDescription(), not(nullValue()));
assertThat(result.getResultStatus(), equalTo(FoundCommandResult.ResultStatus.UNKNOWN_LABEL));
assertThat(result.getDifference() > 0.75, equalTo(true));
}
// ----------
// Helper methods
// ----------
private static CommandDescription createCommand(PlayerPermission permission, CommandDescription parent,
List<String> labels, CommandArgumentDescription... arguments) {
CommandDescription.CommandBuilder command = CommandDescription.builder()
.labels(labels)
.parent(parent)
.permissions(DefaultPermission.OP_ONLY, permission)
.description("Test")
.detailedDescription("Test command")
.description(labels.get(0))
.detailedDescription("'" + labels.get(0) + "' test command")
.executableCommand(mock(ExecutableCommand.class));
if (arguments != null && arguments.length > 0) {
@ -111,27 +142,17 @@ public class CommandHandlerTest {
return new CommandArgumentDescription(label, "Test description", isOptional);
}
private void assertSenderGotMessageContaining(String text, CommandSender sender) {
ArgumentCaptor<String> captor = ArgumentCaptor.forClass(String.class);
verify(sender).sendMessage(captor.capture());
assertThat(captor.getValue(), stringContainsInOrder(text));
private static CommandDescription getChildWithLabel(String childLabel, String parentLabel) {
CommandDescription parent = getCommandWithLabel(parentLabel, commands);
return getCommandWithLabel(childLabel, parent.getChildren());
}
private static CommandDescription getCommandWithLabel(String label, Collection<CommandDescription> commands) {
for (CommandDescription command : commands) {
if (command.getLabels().contains(label)) {
return command;
}
}
return null;
}
private static CommandDescription getChildWithLabel(String label, CommandDescription command) {
for (CommandDescription child : command.getChildren()) {
for (CommandDescription child : commands) {
if (child.getLabels().contains(label)) {
return child;
}
}
return null;
throw new RuntimeException("Could not find command with label '" + label + "'");
}
}

View File

@ -72,7 +72,7 @@ public class ChangePasswordCommandTest {
ChangePasswordCommand command = new ChangePasswordCommand();
// when
command.executeCommand(sender, Collections.singletonList("pass"));
command.executeCommand(sender, Arrays.asList("pass", "pass"));
// then
verify(messagesMock).send(sender, MessageKey.NOT_LOGGED_IN);

View File

@ -11,6 +11,7 @@ import org.junit.Test;
import org.mockito.Mockito;
import java.util.ArrayList;
import java.util.Arrays;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
@ -52,7 +53,7 @@ public class AddEmailCommandTest {
AddEmailCommand command = new AddEmailCommand();
// when
command.executeCommand(sender, new ArrayList<String>());
command.executeCommand(sender, Arrays.asList("mail@example", "other_example"));
// then
verify(authMeMock).getManagement();

View File

@ -59,19 +59,4 @@ public class LoginCommandTest {
Mockito.verify(managementMock).performLogin(eq(sender), eq("password"), eq(false));
}
@Test
public void shouldHandleMissingPassword() {
// given
Player sender = mock(Player.class);
LoginCommand command = new LoginCommand();
// when
command.executeCommand(sender, new ArrayList<String>());
// then
// TODO ljacqu 20151121: May make sense to handle null password in LoginCommand instead of forwarding the call
String password = null;
Mockito.verify(managementMock).performLogin(eq(sender), eq(password), eq(false));
}
}