From a928a4092dfca78bfef95d4007adce253e6ee20a Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sat, 19 Dec 2015 16:27:00 +0100 Subject: [PATCH] #305 Fix label handling, add extensive tests for command handler --- .../xephi/authme/command/CommandHandler.java | 18 +- .../authme/command/FoundCommandResult.java | 17 +- .../authme/command/CommandHandlerTest.java | 213 +++++++++++++++++- 3 files changed, 231 insertions(+), 17 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandHandler.java b/src/main/java/fr/xephi/authme/command/CommandHandler.java index 78a8e78b..fcc65aa0 100644 --- a/src/main/java/fr/xephi/authme/command/CommandHandler.java +++ b/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 HELP_COMMAND_CLASS = HelpCommand.class; private final Set 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 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 parts) { // Return the base command with incorrect arg count error if we only have one part if (parts.size() <= 1) { - return new FoundCommandResult( - base, parts, new ArrayList(), 0.0, FoundResultStatus.INCORRECT_ARGUMENTS); + return new FoundCommandResult(base, parts, new ArrayList(), 0.0, 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 diff --git a/src/main/java/fr/xephi/authme/command/FoundCommandResult.java b/src/main/java/fr/xephi/authme/command/FoundCommandResult.java index 76f63633..f9fcce7b 100644 --- a/src/main/java/fr/xephi/authme/command/FoundCommandResult.java +++ b/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 labels; - /** - * The command arguments. - */ + /** The command arguments. */ private final List 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 labels, List 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 labels, List arguments) { this(commandDescription, labels, arguments, 0.0, FoundResultStatus.SUCCESS); } diff --git a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java b/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java index 2e46a30a..b333b164 100644 --- a/src/test/java/fr/xephi/authme/command/CommandHandlerTest.java +++ b/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 createCommand(PlayerPermission.LOGIN, authMeBase, singletonList("login"), newArgument("password", false)); + // Register /authme register , 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)); - commands = new HashSet<>(asList(authMeBase, testBase)); + // Register /email [player] + 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 , 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 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 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 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 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 argsCaptor = ArgumentCaptor.forClass(List.class); + verify(command.getExecutableCommand()).executeCommand(eq(sender), argsCaptor.capture()); + List 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 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 argsCaptor = ArgumentCaptor.forClass(List.class); + verify(command.getExecutableCommand()).executeCommand(eq(sender), argsCaptor.capture()); + List 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 argsCaptor = ArgumentCaptor.forClass(List.class); + verify(command.getExecutableCommand()).executeCommand(eq(sender), argsCaptor.capture()); + List arguments = argsCaptor.getValue(); + assertThat(arguments, contains("email", "arg1")); + } + + // ---------- // Helper methods // ---------- @@ -163,7 +368,7 @@ public class CommandHandlerTest { private static CommandDescription getCommandWithLabel(String label, Collection commands) { for (CommandDescription child : commands) { - if (child.getLabels().contains(label)) { + if (child.hasLabel(label)) { return child; } }