From cbf0996197782d8f5ca98462b3158610b648cd74 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Thu, 17 Dec 2015 22:23:43 +0100 Subject: [PATCH] Logic for FoundCommandResult.Status / Add tests for CommandHandler - Fix other tests - Add logic for smaller pending FIXME's --- src/main/java/fr/xephi/authme/AuthMe.java | 7 - .../authme/command/CommandDescription.java | 1 + .../xephi/authme/command/CommandHandler.java | 57 ++++++--- .../authme/command/FoundCommandResult.java | 24 +--- .../authme/command/help/HelpProvider.java | 57 +++++---- .../authme/command/CommandHandlerTest.java | 121 ++++++++++-------- .../ChangePasswordCommandTest.java | 2 +- .../executable/email/AddEmailCommandTest.java | 3 +- .../executable/login/LoginCommandTest.java | 15 --- 9 files changed, 154 insertions(+), 133 deletions(-) diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index 162c1405b..639a1f3ab 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/src/main/java/fr/xephi/authme/AuthMe.java @@ -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. * diff --git a/src/main/java/fr/xephi/authme/command/CommandDescription.java b/src/main/java/fr/xephi/authme/command/CommandDescription.java index 6903916d8..6e69c7653 100644 --- a/src/main/java/fr/xephi/authme/command/CommandDescription.java +++ b/src/main/java/fr/xephi/authme/command/CommandDescription.java @@ -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; diff --git a/src/main/java/fr/xephi/authme/command/CommandHandler.java b/src/main/java/fr/xephi/authme/command/CommandHandler.java index bf97d173b..1222eeb03 100644 --- a/src/main/java/fr/xephi/authme/command/CommandHandler.java +++ b/src/main/java/fr/xephi/authme/command/CommandHandler.java @@ -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 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 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 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 labels = parts.subList(0, Math.min(closestCommand.getParentCount() + 1, partsSize)); + List arguments = parts.subList(Math.min(partsSize - 1, labels.size()), partsSize); + + return new FoundCommandResult(closestCommand, labels, arguments, minDifference, status); } private CommandDescription getBaseCommand(String label) { diff --git a/src/main/java/fr/xephi/authme/command/FoundCommandResult.java b/src/main/java/fr/xephi/authme/command/FoundCommandResult.java index e35a3f256..99361d768 100644 --- a/src/main/java/fr/xephi/authme/command/FoundCommandResult.java +++ b/src/main/java/fr/xephi/authme/command/FoundCommandResult.java @@ -10,9 +10,11 @@ import java.util.List; *
    *
  • {@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.
    • - *
  • {@link ResultStatus#INCORRECT_ARGUMENTS}
    • - *
  • {@link ResultStatus#UNKNOWN_LABEL}
    • - *
  • {@link ResultStatus#MISSING_BASE_COMMAND} should never occur. Any other fields may not be present and any + *
  • {@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
    • + *
  • {@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.
    • + *
  • {@link ResultStatus#MISSING_BASE_COMMAND} should never occur. All other fields may be null and any further * processing of the object should be aborted.
    • *
*/ @@ -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 getArguments() { return this.arguments; } - /** - * Get the original query reference. - * - * @return Original query reference. - */ public List getLabels() { return this.labels; } diff --git a/src/main/java/fr/xephi/authme/command/help/HelpProvider.java b/src/main/java/fr/xephi/authme/command/help/HelpProvider.java index 97556ab36..151f3e1bc 100644 --- a/src/main/java/fr/xephi/authme/command/help/HelpProvider.java +++ b/src/main/java/fr/xephi/authme/command/help/HelpProvider.java @@ -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 labels = ImmutableList.copyOf(foundCommand.getLabels()); + List 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 correctLabels, List lines) { - // Ensure that we have all labels to go to the command - int requiredLabels = command.getParentCount() + 1; - List 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 labels, List lines) { + private static void printAlternatives(CommandDescription command, List correctLabels, List 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 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 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 filterCorrectLabels(CommandDescription command, List labels) { + List commands = new ArrayList<>(command.getParentCount() + 1); + CommandDescription currentCommand = command; + while (currentCommand != null) { + commands.add(command); + currentCommand = currentCommand.getParent(); + } + commands = Lists.reverse(commands); + + List 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; + } + } diff --git a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java index e64ad84d3..31810210f 100644 --- a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java @@ -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 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 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 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 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 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 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 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 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 + "'"); } } diff --git a/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java index dd7c1b7a2..a55ffeb3d 100644 --- a/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/changepassword/ChangePasswordCommandTest.java @@ -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); diff --git a/src/test/java/fr/xephi/authme/command/executable/email/AddEmailCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/email/AddEmailCommandTest.java index 64b09b295..1a9c5a122 100644 --- a/src/test/java/fr/xephi/authme/command/executable/email/AddEmailCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/email/AddEmailCommandTest.java @@ -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()); + command.executeCommand(sender, Arrays.asList("mail@example", "other_example")); // then verify(authMeMock).getManagement(); diff --git a/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java index a4773c577..27de64a8a 100644 --- a/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/login/LoginCommandTest.java @@ -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()); - - // 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)); - } - }