#305 Fix label handling, add extensive tests for command handler

src/main/java/fr/xephi/authme/command/CommandHandler.java

@@ -29,6 +29,9 @@ public class CommandHandler {
      */
     private static final double SUGGEST_COMMAND_THRESHOLD = 0.75;
 
+    /**
+     * The class of the help command, to which the base label should also be passed in the arguments.
+     */
     private static final Class<? extends ExecutableCommand> HELP_COMMAND_CLASS = HelpCommand.class;
 
     private final Set<CommandDescription> baseCommands;
@@ -58,9 +61,8 @@ public class CommandHandler {
         // Add the Bukkit command label to the front so we get a list like [authme, register, bobby, mysecret]
         List<String> parts = skipEmptyArguments(bukkitArgs);
         parts.add(0, bukkitCommandLabel);
-
-        // Get the base command of the result, e.g. authme for [authme, register, bobby, mysecret]
         FoundCommandResult result = mapPartsToCommand(parts);
+
         switch (result.getResultStatus()) {
             case SUCCESS:
                 executeCommandIfAllowed(sender, result.getCommandDescription(), result.getArguments());
@@ -75,7 +77,7 @@ public class CommandHandler {
                 sendUnknownCommandMessage(sender, result);
                 break;
             default:
-                throw new RuntimeException("Unknown result '" + result.getResultStatus() + "'");
+                throw new IllegalStateException("Unknown result '" + result.getResultStatus() + "'");
         }
 
         return true;
@@ -191,8 +193,7 @@ public class CommandHandler {
     private FoundCommandResult getCommandWithSmallestDifference(CommandDescription base, List<String> parts) {
         // Return the base command with incorrect arg count error if we only have one part
         if (parts.size() <= 1) {
-            return new FoundCommandResult(
+            return new FoundCommandResult(base, parts, new ArrayList<String>(), 0.0, INCORRECT_ARGUMENTS);
-                base, parts, new ArrayList<String>(), 0.0, FoundResultStatus.INCORRECT_ARGUMENTS);
         }
 
         final String childLabel = parts.get(1);
@@ -207,9 +208,10 @@ public class CommandHandler {
             }
         }
 
-        // base command may have no children or no child label was present
+        // base command may have no children, in which case we return the base command with incorrect arguments error
         if (closestCommand == null) {
-            return new FoundCommandResult(null, parts, null, minDifference, UNKNOWN_LABEL);
+            return new FoundCommandResult(
+                base, parts.subList(0, 1), parts.subList(1, parts.size()), 0.0, INCORRECT_ARGUMENTS);
         }
 
         FoundResultStatus status = (minDifference == 0.0) ? INCORRECT_ARGUMENTS : UNKNOWN_LABEL;
@@ -260,7 +262,7 @@ public class CommandHandler {
 
     private static void transformResultForHelp(FoundCommandResult result) {
         if (result.getCommandDescription() != null
-            && HELP_COMMAND_CLASS.equals(result.getCommandDescription().getExecutableCommand().getClass())) {
+            && HELP_COMMAND_CLASS.isAssignableFrom(result.getCommandDescription().getExecutableCommand().getClass())) {
             // For "/authme help register" we have labels = [authme, help] and arguments = [register]
             // But for the help command we want labels = [authme, help] and arguments = [authme, register],
             // so we can use the arguments as the labels to the command to show help for

src/main/java/fr/xephi/authme/command/FoundCommandResult.java

@@ -29,13 +29,11 @@ public class FoundCommandResult {
      * if multiple labels have been defined, e.g. "/authme register" and "/authme reg".
      */
     private final List<String> labels;
-    /**
+    /** The command arguments. */
-     * The command arguments.
-     */
     private final List<String> arguments;
-
+    /** The difference between the matched command and the supplied labels. */
     private final double difference;
-
+    /** The status of the result (see class description). */
     private final FoundResultStatus resultStatus;
 
     /**
@@ -44,6 +42,8 @@ public class FoundCommandResult {
      * @param commandDescription The command description.
      * @param labels             The labels used to access the command.
      * @param arguments          The command arguments.
+     * @param difference         The difference between the supplied labels and the matched command.
+     * @param resultStatus       The status of the result.
      */
     public FoundCommandResult(CommandDescription commandDescription, List<String> labels, List<String> arguments,
                               double difference, FoundResultStatus resultStatus) {
@@ -54,6 +54,13 @@ public class FoundCommandResult {
         this.resultStatus = resultStatus;
     }
 
+    /**
+     * Constructor for a fully successfully matched command.
+     *
+     * @param commandDescription The matched command description.
+     * @param labels             The labels used to access the command.
+     * @param arguments          The command arguments.
+     */
     public FoundCommandResult(CommandDescription commandDescription, List<String> labels, List<String> arguments) {
         this(commandDescription, labels, arguments, 0.0, FoundResultStatus.SUCCESS);
     }

src/test/java/fr/xephi/authme/command/CommandHandlerTest.java

@@ -1,25 +1,38 @@
 package fr.xephi.authme.command;
 
+import fr.xephi.authme.command.executable.HelpCommand;
 import fr.xephi.authme.permission.DefaultPermission;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.permission.PlayerPermission;
+import org.bukkit.command.CommandSender;
+import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
+import org.mockito.ArgumentCaptor;
 
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 
+import static com.google.common.collect.Sets.newHashSet;
 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.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.stringContainsInOrder;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Matchers.any;
+import static org.mockito.Matchers.anyListOf;
+import static org.mockito.Matchers.anyString;
+import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
 
 /**
  * Test for {@link CommandHandler}.
@@ -32,17 +45,36 @@ public class CommandHandlerTest {
 
     @BeforeClass
     public static void setUpCommandHandler() {
+        // Register /authme
         CommandDescription authMeBase = createCommand(null, null, singletonList("authme"));
+        // Register /authme login <password>
         createCommand(PlayerPermission.LOGIN, authMeBase, singletonList("login"), newArgument("password", false));
+        // Register /authme register <password> <confirmation>, alias: /authme reg
         createCommand(PlayerPermission.LOGIN, authMeBase, asList("register", "reg"),
             newArgument("password", false), newArgument("confirmation", false));
 
-        CommandDescription testBase = createCommand(null, null, singletonList("test"), newArgument("test", true));
+        // Register /email [player]
-        commands = new HashSet<>(asList(authMeBase, testBase));
+        CommandDescription emailBase = createCommand(null, null, singletonList("email"));
+        // Register /email helptest -- use only to test for help command arguments special case
+        CommandDescription.builder().parent(emailBase).labels("helptest").executableCommand(mock(HelpCommand.class))
+            .description("test").detailedDescription("Test.").withArgument("Query", "", false).build();
+
+        // Register /unregister <player>, alias: /unreg
+        CommandDescription unregisterBase = createCommand(null, null, asList("unregister", "unreg"),
+            newArgument("player", false));
+
+        commands = newHashSet(authMeBase, emailBase, unregisterBase);
+    }
+
+    @Before
+    public void setUpMocks() {
         permissionsManagerMock = mock(PermissionsManager.class);
         handler = new CommandHandler(commands, permissionsManagerMock);
     }
 
+    // -----------
+    // mapPartsToCommand() tests
+    // -----------
     @Test
     public void shouldMapPartsToLoginChildCommand() {
         // given
@@ -56,6 +88,8 @@ public class CommandHandlerTest {
         assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS));
         assertThat(result.getArguments(), contains("test1"));
         assertThat(result.getDifference(), equalTo(0.0));
+        assertThat(result.getLabels(), equalTo(parts.subList(0, 2)));
+        assertThat(result.getArguments(), contains(parts.get(2)));
     }
 
     @Test
@@ -69,6 +103,7 @@ public class CommandHandlerTest {
         // then
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
         assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS));
+        assertThat(result.getLabels(), equalTo(parts.subList(0, 2)));
         assertThat(result.getArguments(), contains("arg1", "arg2"));
         assertThat(result.getDifference(), equalTo(0.0));
     }
@@ -85,6 +120,8 @@ public class CommandHandlerTest {
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
         assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS));
         assertThat(result.getDifference(), equalTo(0.0));
+        assertThat(result.getLabels(), equalTo(parts.subList(0, 2)));
+        assertThat(result.getArguments(), equalTo(parts.subList(2, 5)));
     }
 
     @Test
@@ -99,6 +136,8 @@ public class CommandHandlerTest {
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
         assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS));
         assertThat(result.getDifference(), equalTo(0.0));
+        assertThat(result.getLabels(), equalTo(parts));
+        assertThat(result.getArguments(), empty());
     }
 
     @Test
@@ -113,6 +152,8 @@ public class CommandHandlerTest {
         assertThat(result.getCommandDescription(), equalTo(getChildWithLabel("register", "authme")));
         assertThat(result.getResultStatus(), equalTo(FoundResultStatus.UNKNOWN_LABEL));
         assertThat(result.getDifference() < 0.75, equalTo(true));
+        assertThat(result.getLabels(), equalTo(parts.subList(0, 2)));
+        assertThat(result.getArguments(), contains("pass123", "pass123"));
     }
 
     /** In contrast to the previous test, we test a command request with a very apart label. */
@@ -128,8 +169,172 @@ public class CommandHandlerTest {
         assertThat(result.getCommandDescription(), not(nullValue()));
         assertThat(result.getResultStatus(), equalTo(FoundResultStatus.UNKNOWN_LABEL));
         assertThat(result.getDifference() > 0.75, equalTo(true));
+        assertThat(result.getLabels(), equalTo(parts));
+        assertThat(result.getArguments(), empty());
     }
 
+    @Test
+    public void shouldHandleBaseWithWrongArguments() {
+        // given
+        List<String> parts = singletonList("unregister");
+
+        // when
+        FoundCommandResult result = handler.mapPartsToCommand(parts);
+
+        // then
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS));
+        assertThat(result.getCommandDescription(), equalTo(getCommandWithLabel("unregister", commands)));
+        assertThat(result.getDifference(), equalTo(0.0));
+        assertThat(result.getArguments(), empty());
+        assertThat(result.getLabels(), equalTo(parts));
+    }
+
+    @Test
+    public void shouldHandleUnknownBase() {
+        // given
+        List<String> parts = asList("bogus", "label1", "arg1");
+
+        // when
+        FoundCommandResult result = handler.mapPartsToCommand(parts);
+
+        // then
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.MISSING_BASE_COMMAND));
+        assertThat(result.getCommandDescription(), nullValue());
+    }
+
+    @Test
+    public void shouldHandleNullInput() {
+        // given / when
+        FoundCommandResult result = handler.mapPartsToCommand(null);
+
+        // then
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.MISSING_BASE_COMMAND));
+        assertThat(result.getCommandDescription(), nullValue());
+    }
+
+    @Test
+    public void shouldMapToBaseWithProperArguments() {
+        // given
+        List<String> parts = asList("Unreg", "player1");
+
+        // when
+        FoundCommandResult result = handler.mapPartsToCommand(parts);
+
+        // then
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.SUCCESS));
+        assertThat(result.getCommandDescription(), equalTo(getCommandWithLabel("unregister", commands)));
+        assertThat(result.getDifference(), equalTo(0.0));
+        assertThat(result.getArguments(), contains("player1"));
+        assertThat(result.getLabels(), contains("Unreg"));
+    }
+
+    @Test
+    public void shouldReturnChildlessBaseCommandWithArgCountError() {
+        // given
+        List<String> parts = asList("unregistER", "player1", "wrongArg");
+
+        // when
+        FoundCommandResult result = handler.mapPartsToCommand(parts);
+
+        // then
+        assertThat(result.getResultStatus(), equalTo(FoundResultStatus.INCORRECT_ARGUMENTS));
+        assertThat(result.getCommandDescription(), equalTo(getCommandWithLabel("unregister", commands)));
+        assertThat(result.getDifference(), equalTo(0.0));
+        assertThat(result.getArguments(), contains("player1", "wrongArg"));
+        assertThat(result.getLabels(), contains("unregistER"));
+    }
+
+    // ----------
+    // processCommand() tests
+    // ----------
+    @Test
+    public void shouldCallMappedCommandWithArgs() {
+        // given
+        String bukkitLabel = "Authme";
+        String[] bukkitArgs = {"Login", "myPass"};
+        CommandSender sender = mock(CommandSender.class);
+
+        CommandDescription command = getChildWithLabel("login", "authme");
+        given(permissionsManagerMock.hasPermission(sender, command)).willReturn(true);
+
+        // when
+        handler.processCommand(sender, bukkitLabel, bukkitArgs);
+
+        // then
+        ArgumentCaptor<List> argsCaptor = ArgumentCaptor.forClass(List.class);
+        verify(command.getExecutableCommand()).executeCommand(eq(sender), argsCaptor.capture());
+        List<String> argument = argsCaptor.getValue();
+        assertThat(argument, contains("myPass"));
+        // Ensure that no error message was issued to the command sender
+        verify(sender, never()).sendMessage(anyString());
+    }
+
+    @Test
+    public void shouldNotCallExecutableCommandIfNoPermission() {
+        // given
+        String bukkitLabel = "unreg";
+        String[] bukkitArgs = {"testPlayer"};
+        CommandSender sender = mock(CommandSender.class);
+        given(permissionsManagerMock.hasPermission(any(CommandSender.class), any(CommandDescription.class)))
+            .willReturn(false);
+
+        // when
+        handler.processCommand(sender, bukkitLabel, bukkitArgs);
+
+        // then
+        CommandDescription command = getCommandWithLabel("unregister", commands);
+        verify(permissionsManagerMock).hasPermission(sender, command);
+        verify(command.getExecutableCommand(), never())
+            .executeCommand(any(CommandSender.class), anyListOf(String.class));
+
+        ArgumentCaptor<String> messageCaptor = ArgumentCaptor.forClass(String.class);
+        verify(sender).sendMessage(messageCaptor.capture());
+        String message = messageCaptor.getValue();
+        assertThat(message, stringContainsInOrder("You don't have permission"));
+    }
+
+    @Test
+    public void shouldStripWhitespace() {
+        // given
+        String bukkitLabel = "AuthMe";
+        String[] bukkitArgs = {" ", "", "LOGIN", "  ", "testArg", " "};
+        CommandSender sender = mock(CommandSender.class);
+
+        CommandDescription command = getChildWithLabel("login", "authme");
+        given(permissionsManagerMock.hasPermission(sender, command)).willReturn(true);
+
+        // when
+        handler.processCommand(sender, bukkitLabel, bukkitArgs);
+
+        // then
+        ArgumentCaptor<List> argsCaptor = ArgumentCaptor.forClass(List.class);
+        verify(command.getExecutableCommand()).executeCommand(eq(sender), argsCaptor.capture());
+        List<String> arguments = argsCaptor.getValue();
+        assertThat(arguments, contains("testArg"));
+        verify(sender, never()).sendMessage(anyString());
+    }
+
+    @Test
+    public void shouldPassCommandPathAsArgumentsToHelpCommand() {
+        // given
+        String bukkitLabel = "email";
+        String[] bukkitArgs = {"helptest", "arg1"};
+        CommandSender sender = mock(CommandSender.class);
+
+        CommandDescription command = getChildWithLabel("helptest", "email");
+        given(permissionsManagerMock.hasPermission(sender, command)).willReturn(true);
+
+        // when
+        handler.processCommand(sender, bukkitLabel, bukkitArgs);
+
+        // then
+        ArgumentCaptor<List> argsCaptor = ArgumentCaptor.forClass(List.class);
+        verify(command.getExecutableCommand()).executeCommand(eq(sender), argsCaptor.capture());
+        List<String> arguments = argsCaptor.getValue();
+        assertThat(arguments, contains("email", "arg1"));
+    }
+
+
     // ----------
     // Helper methods
     // ----------
@@ -163,7 +368,7 @@ public class CommandHandlerTest {
 
     private static CommandDescription getCommandWithLabel(String label, Collection<CommandDescription> commands) {
         for (CommandDescription child : commands) {
-            if (child.getLabels().contains(label)) {
+            if (child.hasLabel(label)) {
                 return child;
             }
         }