Refactorings - prepare to remove FoundCommandResult

- Reduce tight coupling by passing list of available commands as param to CommandHandler
- Split command processing method into several smaller ones
- Remove unfinished logic for unlimited command arguments (reason: was not implemented and not used, probably needs another way to handle it once the need for it arises)
- Create test for CommandHandler
This commit is contained in:
ljacqu 2015-12-04 21:33:50 +01:00
parent 0c1589f93a
commit c78e12de04
9 changed files with 223 additions and 173 deletions

View File

@ -10,6 +10,7 @@ import fr.xephi.authme.cache.backup.JsonCache;
import fr.xephi.authme.cache.limbo.LimboCache;
import fr.xephi.authme.cache.limbo.LimboPlayer;
import fr.xephi.authme.command.CommandHandler;
import fr.xephi.authme.command.CommandInitializer;
import fr.xephi.authme.converter.Converter;
import fr.xephi.authme.converter.ForceFlatToSqlite;
import fr.xephi.authme.datasource.*;
@ -433,7 +434,7 @@ public class AuthMe extends JavaPlugin {
* Set up the command handler.
*/
private void setupCommandHandler() {
this.commandHandler = new CommandHandler();
this.commandHandler = new CommandHandler(CommandInitializer.getBaseCommands());
}
/**
@ -956,14 +957,14 @@ public class AuthMe extends JavaPlugin {
@Override
public boolean onCommand(CommandSender sender, Command cmd,
String commandLabel, String[] args) {
// Get the command handler, and make sure it's valid
// Make sure the command handler has been initialized
if (commandHandler == null) {
wrapper.getLogger().warning("AuthMe command handler is not available");
wrapper.getLogger().severe("AuthMe command handler is not available");
return false;
}
// Handle the command, return the result
return commandHandler.onCommand(sender, cmd, commandLabel, args);
// Handle the command
return commandHandler.processCommand(sender, commandLabel, args);
}
/**

View File

@ -50,10 +50,6 @@ public class CommandDescription {
* The arguments the command takes.
*/
private List<CommandArgumentDescription> arguments = new ArrayList<>(); // TODO remove field initialization
/**
* Defines whether there is an argument maximum or not.
*/
private boolean noArgumentMaximum = false; // TODO remove field initialization
/**
* Defines the command permissions.
*/
@ -106,15 +102,13 @@ public class CommandDescription {
*/
private CommandDescription(List<String> labels, String description, String detailedDescription,
ExecutableCommand executableCommand, CommandDescription parent,
List<CommandArgumentDescription> arguments, boolean noArgumentMaximum,
CommandPermissions permissions) {
List<CommandArgumentDescription> arguments, CommandPermissions permissions) {
this.labels = labels;
this.description = description;
this.detailedDescription = detailedDescription;
this.executableCommand = executableCommand;
this.parent = parent;
this.arguments = arguments;
this.noArgumentMaximum = noArgumentMaximum;
this.permissions = permissions;
if (parent != null) {
@ -279,15 +273,6 @@ public class CommandDescription {
this.executableCommand = executableCommand;
}
/**
* Check whether this command is executable, based on the assigned executable command.
*
* @return True if this command is executable.
*/
public boolean isExecutable() {
return this.executableCommand != null;
}
/**
* Execute the command, if possible.
*
@ -298,10 +283,6 @@ public class CommandDescription {
* @return True on success, false on failure.
*/
public boolean execute(CommandSender sender, CommandParts commandReference, CommandParts commandArguments) {
// Make sure the command is executable
if (!isExecutable())
return false;
// Execute the command, return the result
return getExecutableCommand().executeCommand(sender, commandReference, commandArguments);
}
@ -453,10 +434,6 @@ public class CommandDescription {
return !getArguments().isEmpty();
}
public boolean hasMaximumArguments() {
return !noArgumentMaximum; // TODO ljacqu 20151130 Change variable name
}
/**
* Get the command description.
*
@ -604,7 +581,6 @@ public class CommandDescription {
private ExecutableCommand executableCommand;
private CommandDescription parent;
private List<CommandArgumentDescription> arguments = new ArrayList<>();
private boolean noArgumentMaximum;
private CommandPermissions permissions;
/**
@ -621,7 +597,6 @@ public class CommandDescription {
getOrThrow(executableCommand, "executableCommand"),
firstNonNull(parent, null),
arguments,
noArgumentMaximum,
firstNonNull(permissions, null)
);
}
@ -670,11 +645,6 @@ public class CommandDescription {
return this;
}
public CommandBuilder noArgumentMaximum(boolean noArgumentMaximum) {
this.noArgumentMaximum = noArgumentMaximum;
return this;
}
public CommandBuilder permissions(CommandPermissions.DefaultPermission defaultPermission,
PermissionNode... permissionNodes) {
this.permissions = new CommandPermissions(asMutableList(permissionNodes), defaultPermission);

View File

@ -4,7 +4,6 @@ import fr.xephi.authme.AuthMe;
import fr.xephi.authme.command.help.HelpProvider;
import fr.xephi.authme.util.StringUtils;
import org.bukkit.ChatColor;
import org.bukkit.command.Command;
import org.bukkit.command.CommandSender;
import java.util.ArrayList;
@ -28,104 +27,70 @@ public class CommandHandler {
*/
private static final double SUGGEST_COMMAND_THRESHOLD = 0.75;
private List<CommandDescription> commands;
/**
* Process a command.
* Create a command handler.
*
* @param commands The collection of available AuthMe commands
*/
public CommandHandler(List<CommandDescription> commands) {
this.commands = commands;
}
/**
* 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 bukkitCommand The command (Bukkit).
* @param bukkitCommandLabel The command label (Bukkit).
* @param bukkitArgs The command arguments (Bukkit).
*
* @return True if the command was executed, false otherwise.
*/
// TODO ljacqu 20151129: Rename onCommand() method to something not suggesting it is auto-invoked by an event
public boolean onCommand(CommandSender sender, Command bukkitCommand, String bukkitCommandLabel, String[] bukkitArgs) {
public boolean processCommand(CommandSender sender, String bukkitCommandLabel, String[] bukkitArgs) {
List<String> commandArgs = skipEmptyArguments(bukkitArgs);
// Add the Bukkit command label to the front so we get something like [authme, register, pass, passConfirm]
commandArgs.add(0, bukkitCommandLabel);
// Make sure the command isn't empty (does this happen?)
CommandParts commandReference = new CommandParts(bukkitCommandLabel, commandArgs);
if (commandReference.getCount() == 0)
return false;
// TODO: remove commandParts
CommandParts commandReference = new CommandParts(commandArgs);
// Get a suitable command for this reference, and make sure it isn't null
FoundCommandResult result = findCommand(commandReference);
if (result == null) {
// TODO ljacqu 20151204: Log more information to the console (bukkitCommandLabel)
sender.sendMessage(ChatColor.DARK_RED + "Failed to parse " + AuthMe.getPluginName() + " command!");
return false;
}
// Get the base command
String baseCommand = commandReference.get(0);
String baseCommand = commandArgs.get(0);
// Make sure the difference between the command reference and the actual command isn't too big
final double commandDifference = result.getDifference();
if (commandDifference > ASSUME_COMMAND_THRESHOLD) {
// Show the unknown command warning
sender.sendMessage(ChatColor.DARK_RED + "Unknown command!");
if (commandDifference <= ASSUME_COMMAND_THRESHOLD) {
// Show a command suggestion if available and the difference isn't too big
if (commandDifference < SUGGEST_COMMAND_THRESHOLD)
if (result.getCommandDescription() != null)
sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD + "/" + result.getCommandDescription().getCommandReference(commandReference) + ChatColor.YELLOW + "?");
// Show a message when the command handler is assuming a command
if (commandDifference > 0) {
sendCommandAssumptionMessage(sender, result, commandReference);
}
// Show the help command
sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + baseCommand + " help" + ChatColor.YELLOW + " to view help.");
return true;
if (!result.hasPermission(sender)) {
sender.sendMessage(ChatColor.DARK_RED + "You don't have permission to use this command!");
} else if (!result.hasProperArguments()) {
sendImproperArgumentsMessage(sender, result, commandReference, baseCommand);
} else {
return result.executeCommand(sender);
}
} else {
sendUnknownCommandMessage(sender, commandDifference, result, baseCommand);
}
// Show a message when the command handler is assuming a command
if (commandDifference > 0) {
// Get the suggested command
CommandParts suggestedCommandParts = new CommandParts(result.getCommandDescription().getCommandReference(commandReference));
// Show the suggested command
sender.sendMessage(ChatColor.DARK_RED + "Unknown command, assuming " + ChatColor.GOLD + "/" + suggestedCommandParts +
ChatColor.DARK_RED + "!");
}
// Make sure the command is executable
if (!result.isExecutable()) {
// Get the command reference
CommandParts helpCommandReference = new CommandParts(result.getCommandReference().getRange(1));
// Show the unknown command warning
sender.sendMessage(ChatColor.DARK_RED + "Invalid command!");
// Show the help command
sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + baseCommand + " help " + helpCommandReference + ChatColor.YELLOW + " to view help.");
return true;
}
// Make sure the command sender has permission
if (!result.hasPermission(sender)) {
// Show the no permissions warning
sender.sendMessage(ChatColor.DARK_RED + "You don't have permission to use this command!");
return true;
}
// Make sure the command sender has permission
if (!result.hasProperArguments()) {
// Get the command and the suggested command reference
CommandParts suggestedCommandReference = new CommandParts(result.getCommandDescription().getCommandReference(commandReference));
CommandParts helpCommandReference = new CommandParts(suggestedCommandReference.getRange(1));
// Show the invalid arguments warning
sender.sendMessage(ChatColor.DARK_RED + "Incorrect command arguments!");
// Show the command argument help
HelpProvider.showHelp(sender, commandReference, suggestedCommandReference, true, false, true, false, false, false);
// Show the command to use for detailed help
sender.sendMessage(ChatColor.GOLD + "Detailed help: " + ChatColor.WHITE + "/" + baseCommand + " help " + helpCommandReference);
return true;
}
// Execute the command if it's suitable
return result.executeCommand(sender);
return true;
}
/**
* Skips all entries of the given array that are simply whitespace.
* Skip all entries of the given array that are simply whitespace.
*
* @param args The array to process
* @return List of the items that are not empty
@ -162,10 +127,7 @@ public class CommandHandler {
if (queryReference.getCount() <= 0)
return null;
// TODO ljacqu 20151129: If base commands are only used in here (or in the future CommandHandler after changes),
// it might make sense to make the CommandInitializer package-private and to return its result into this class
// instead of regularly fetching the list of base commands from the other class.
for (CommandDescription commandDescription : CommandInitializer.getBaseCommands()) {
for (CommandDescription commandDescription : commands) {
// Check whether there's a command description available for the
// current command
if (!commandDescription.isSuitableLabel(queryReference))
@ -186,7 +148,7 @@ public class CommandHandler {
*
* @return The command found, or null.
*/
public CommandDescription findCommand(List<String> commandParts) {
private CommandDescription findCommand(List<String> commandParts) {
// Make sure the command reference is valid
if (commandParts.isEmpty()) {
return null;
@ -211,10 +173,63 @@ public class CommandHandler {
return null;
}
for (CommandDescription command : commands) {
if (command.getLabels().contains(label)) {
if (command.getLabels().contains(label)) { // TODO ljacqu should be case-insensitive
return command;
}
}
return null;
}
/**
* Show an "unknown command" to the user and suggests an existing command if its similarity is within
* the defined threshold.
*
* @param sender The command sender
* @param commandDifference The difference between the invoked command and the existing one
* @param result The command that was found during the mapping process
* @param baseCommand The base command (TODO: This is probably already in FoundCommandResult)
*/
private static void sendUnknownCommandMessage(CommandSender sender, double commandDifference,
FoundCommandResult result, String baseCommand) {
CommandParts commandReference = result.getCommandReference();
sender.sendMessage(ChatColor.DARK_RED + "Unknown command!");
// Show a command suggestion if available and the difference isn't too big
if (commandDifference < SUGGEST_COMMAND_THRESHOLD && result.getCommandDescription() != null) {
sender.sendMessage(ChatColor.YELLOW + "Did you mean " + ChatColor.GOLD + "/"
+ result.getCommandDescription().getCommandReference(commandReference) + ChatColor.YELLOW + "?");
}
sender.sendMessage(ChatColor.YELLOW + "Use the command " + ChatColor.GOLD + "/" + baseCommand + " help"
+ ChatColor.YELLOW + " to view help.");
}
private static void sendImproperArgumentsMessage(CommandSender sender, FoundCommandResult result,
CommandParts commandReference, String baseCommand) {
// Get the command and the suggested command reference
CommandParts suggestedCommandReference =
new CommandParts(result.getCommandDescription().getCommandReference(commandReference));
CommandParts helpCommandReference = new CommandParts(suggestedCommandReference.getRange(1));
// Show the invalid arguments warning
sender.sendMessage(ChatColor.DARK_RED + "Incorrect command arguments!");
// Show the command argument help
HelpProvider.showHelp(sender, commandReference, suggestedCommandReference,
true, false, true, false, false, false);
// Show the command to use for detailed help
sender.sendMessage(ChatColor.GOLD + "Detailed help: " + ChatColor.WHITE + "/" + baseCommand
+ " help " + helpCommandReference);
}
private static void sendCommandAssumptionMessage(CommandSender sender, FoundCommandResult result,
CommandParts commandReference) {
CommandParts assumedCommandParts =
new CommandParts(result.getCommandDescription().getCommandReference(commandReference));
sender.sendMessage(ChatColor.DARK_RED + "Unknown command, assuming " + ChatColor.GOLD + "/"
+ assumedCommandParts + ChatColor.DARK_RED + "!");
}
}

View File

@ -13,9 +13,7 @@ public final class CommandUtils {
}
public static int getMaxNumberOfArguments(CommandDescription command) {
return command.hasMaximumArguments()
? command.getArguments().size()
: -1;
return command.getArguments().size();
}
}

View File

@ -61,27 +61,13 @@ public class FoundCommandResult {
return this.commandDescription;
}
/**
* Set the command description.
*
* @param commandDescription The command description.
*/
public void setCommandDescription(CommandDescription commandDescription) {
this.commandDescription = commandDescription;
}
/**
* Check whether the command is executable.
*
* @return True if the command is executable, false otherwise.
*/
public boolean isExecutable() {
// Make sure the command description is valid
if (this.commandDescription == null)
return false;
// Check whether the command is executable, return the result
return this.commandDescription.isExecutable();
return commandDescription != null;
}
/**

View File

@ -5,7 +5,6 @@ import fr.xephi.authme.command.CommandArgumentDescription;
import fr.xephi.authme.command.CommandDescription;
import fr.xephi.authme.command.CommandParts;
import fr.xephi.authme.command.CommandPermissions;
import fr.xephi.authme.command.CommandUtils;
import fr.xephi.authme.permission.PermissionNode;
import fr.xephi.authme.util.StringUtils;
import org.bukkit.ChatColor;
@ -77,10 +76,6 @@ public class HelpPrinter {
// Print the syntax
sender.sendMessage(argString.toString());
}
// Show the unlimited arguments argument
if (!command.hasMaximumArguments())
sender.sendMessage(" " + ChatColor.YELLOW + ChatColor.ITALIC + "... : " + ChatColor.WHITE + "Any additional arguments." + ChatColor.GRAY + ChatColor.ITALIC + " (Optional)");
}
/**

View File

@ -61,11 +61,6 @@ public final class HelpSyntaxHelper {
sb.append(ChatColor.ITALIC).append(formatArgument(arg));
}
// Add some dots if the command allows unlimited arguments
if (!commandDescription.hasMaximumArguments()) {
sb.append(ChatColor.ITALIC).append(" ...");
}
// Return the build command syntax
return sb.toString();
}

View File

@ -0,0 +1,115 @@
package fr.xephi.authme.command;
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.List;
import static java.util.Arrays.asList;
import static java.util.Collections.singletonList;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.stringContainsInOrder;
import static org.mockito.BDDMockito.given;
import static org.mockito.Matchers.any;
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}.
*/
public class CommandHandlerTest {
private static List<CommandDescription> commands;
private static CommandHandler handler;
@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"),
newArgument("password", false), newArgument("confirmation", false));
CommandDescription testBase = createCommand(null, null, singletonList("test"), newArgument("test", true));
commands = asList(authMeBase, testBase);
handler = new CommandHandler(commands);
}
@Test
public void shouldForwardCommandToExecutable() {
// given
CommandSender sender = Mockito.mock(CommandSender.class);
given(sender.isOp()).willReturn(true);
String bukkitLabel = "authme";
String[] args = {"login", "password"};
// when
handler.processCommand(sender, bukkitLabel, args);
// then
final CommandDescription loginCmd = commands.get(0).getChildren().get(0);
verify(sender, never()).sendMessage(anyString());
verify(loginCmd.getExecutableCommand()).executeCommand(
eq(sender), any(CommandParts.class), any(CommandParts.class));
}
@Test
@Ignore // TODO ljacqu Fix test --> command classes too tightly coupled at the moment
public void shouldRejectCommandWithTooManyArguments() {
// given
CommandSender sender = Mockito.mock(CommandSender.class);
given(sender.isOp()).willReturn(true);
String bukkitLabel = "authme";
String[] args = {"login", "password", "__unneededArgument__"};
// when
boolean result = handler.processCommand(sender, bukkitLabel, args);
// then
assertThat(result, equalTo(true));
final CommandDescription loginCmd = commands.get(0).getChildren().get(0);
assertSenderGotMessageContaining("help", sender);
verify(loginCmd.getExecutableCommand()).executeCommand(
eq(sender), any(CommandParts.class), any(CommandParts.class));
}
private static CommandDescription createCommand(PlayerPermission permission, CommandDescription parent,
List<String> labels, CommandArgumentDescription... arguments) {
CommandDescription.CommandBuilder command = CommandDescription.builder()
.labels(labels)
.parent(parent)
.permissions(CommandPermissions.DefaultPermission.OP_ONLY, permission)
.description("Test")
.detailedDescription("Test command")
.executableCommand(mock(ExecutableCommand.class));
if (arguments != null && arguments.length > 0) {
for (CommandArgumentDescription argument : arguments) {
command.withArgument(argument.getLabel(), "Test description", argument.isOptional());
}
}
return command.build();
}
private static CommandArgumentDescription newArgument(String label, boolean isOptional) {
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));
}
}

View File

@ -27,8 +27,7 @@ public class HelpSyntaxHelperTest {
.build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), "", false);
String result = HelpSyntaxHelper.getCommandSyntax(description, new CommandParts(), "", false);
// then
assertThat(result, equalTo(WHITE + "/authme register" + ITALIC + " [name]"));
@ -42,8 +41,7 @@ public class HelpSyntaxHelperTest {
.build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), null, false);
String result = HelpSyntaxHelper.getCommandSyntax(description, new CommandParts(), null, false);
// then
assertThat(result, equalTo(WHITE + "/authme register" + ITALIC + " <test>"));
@ -58,8 +56,7 @@ public class HelpSyntaxHelperTest {
.build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), "", false);
String result = HelpSyntaxHelper.getCommandSyntax(description, new CommandParts(), "", false);
// then
assertThat(result, equalTo(WHITE + "/authme register" + ITALIC + " [name]" + ITALIC + " <test>"));
@ -74,8 +71,7 @@ public class HelpSyntaxHelperTest {
.build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), "", true);
String result = HelpSyntaxHelper.getCommandSyntax(description, new CommandParts(), "", true);
// then
assertThat(result, equalTo(WHITE + "/authme "
@ -89,8 +85,7 @@ public class HelpSyntaxHelperTest {
CommandDescription description = getDescriptionBuilder().build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), null, true);
String result = HelpSyntaxHelper.getCommandSyntax(description, new CommandParts(), null, true);
// then
assertThat(result, equalTo(WHITE + "/authme " + YELLOW + BOLD + "register" + YELLOW));
@ -104,32 +99,12 @@ public class HelpSyntaxHelperTest {
.build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), "alt", false);
String result = HelpSyntaxHelper.getCommandSyntax(description, new CommandParts(), "alt", false);
// then
assertThat(result, equalTo(WHITE + "/authme alt" + ITALIC + " [name]"));
}
@Test
public void shouldHighlightCommandWithAltLabelAndUnlimitedArguments() {
// given
CommandDescription description = getDescriptionBuilder()
.withArgument("name", "", true)
.withArgument("test", "", false)
.noArgumentMaximum(true)
.build();
// when
String result = HelpSyntaxHelper.getCommandSyntax(
description, new CommandParts(), "test", true);
// then
assertThat(result, equalTo(WHITE + "/authme "
+ YELLOW + BOLD + "test"
+ YELLOW + ITALIC + " [name]" + ITALIC + " <test>" + ITALIC + " ..."));
}
private static CommandDescription.CommandBuilder getDescriptionBuilder() {
CommandDescription base = CommandDescription.builder()