From a124e8f2830b8516731d99ff8f55caa64df1e1ef Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 29 Nov 2015 01:21:48 +0100 Subject: [PATCH] Refine CommandDescription builder; add/fix JavaDoc; add tests - Add tests to check the integrity of the commands that are defined in the CommandManager - Convert some more commands registrations to the new Builder format --- .../command/CommandArgumentDescription.java | 4 +- .../authme/command/CommandDescription.java | 47 +++--- .../xephi/authme/command/CommandManager.java | 134 +++++++++--------- .../authme/command/CommandManagerTest.java | 120 ++++++++++++++++ 4 files changed, 210 insertions(+), 95 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandArgumentDescription.java b/src/main/java/fr/xephi/authme/command/CommandArgumentDescription.java index af14a16e4..0440ddc0f 100644 --- a/src/main/java/fr/xephi/authme/command/CommandArgumentDescription.java +++ b/src/main/java/fr/xephi/authme/command/CommandArgumentDescription.java @@ -15,7 +15,7 @@ public class CommandArgumentDescription { */ private String description; /** - * Defines whether the argument is isOptional. + * Defines whether the argument is optional. */ private boolean isOptional = false; @@ -24,7 +24,7 @@ public class CommandArgumentDescription { * * @param label The argument label. * @param description The argument description. - * @param isOptional True if the argument is isOptional, false otherwise. + * @param isOptional True if the argument is optional, false otherwise. */ public CommandArgumentDescription(String label, String description, boolean isOptional) { this.label = label; diff --git a/src/main/java/fr/xephi/authme/command/CommandDescription.java b/src/main/java/fr/xephi/authme/command/CommandDescription.java index acb2c13d7..afd6ac1e3 100644 --- a/src/main/java/fr/xephi/authme/command/CommandDescription.java +++ b/src/main/java/fr/xephi/authme/command/CommandDescription.java @@ -4,14 +4,25 @@ import fr.xephi.authme.permission.PermissionNode; import fr.xephi.authme.util.StringUtils; import org.bukkit.command.CommandSender; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; /** + * Command description - defines which labels ("names") will lead to a command and points to the + * {@link ExecutableCommand} implementation that executes the logic of the command. + * + * CommandDescription is built hierarchically and have one parent or {@code null} for base commands (main commands + * such as /authme) and may have multiple children extending the mapping of the parent: e.g. if /authme has a child + * whose label is "register", then "/authme register" is the command that the child defines. */ public class CommandDescription { /** - * Defines the acceptable labels. + * Defines the labels to execute the command. For example, if labels are "register" and "r" and the parent is + * the command for "/authme", then both "/authme register" and "/authme r" will be handled by this command. */ private List labels; /** @@ -19,7 +30,7 @@ public class CommandDescription { */ private String description; /** - * Detailed description. + * Detailed description of the command. */ private String detailedDescription; /** @@ -31,12 +42,11 @@ public class CommandDescription { */ private CommandDescription parent; /** - * The child labels. + * The child commands that extend this command. */ - // TODO: Remove list instantiation once Builder is the only way to construct objects private List children = new ArrayList<>(); /** - * The command arguments. + * The arguments the command takes. */ private List arguments; /** @@ -124,14 +134,13 @@ public class CommandDescription { */ private CommandDescription(List labels, String description, String detailedDescription, ExecutableCommand executableCommand, CommandDescription parent, - List children, List arguments, - boolean noArgumentMaximum, CommandPermissions permissions) { + List arguments, boolean noArgumentMaximum, + CommandPermissions permissions) { this.labels = labels; this.description = description; this.detailedDescription = detailedDescription; this.executableCommand = executableCommand; this.parent = parent; - this.children = children; this.arguments = arguments; this.noArgumentMaximum = noArgumentMaximum; this.permissions = permissions; @@ -629,7 +638,7 @@ public class CommandDescription { * @return Command detailed description. */ public String getDetailedDescription() { - return StringUtils.isEmpty(detailedDescription) ? this.detailedDescription : this.description; + return !StringUtils.isEmpty(detailedDescription) ? this.detailedDescription : this.description; } /** @@ -776,8 +785,7 @@ public class CommandDescription { private String detailedDescription; private ExecutableCommand executableCommand; private CommandDescription parent; - private List children; - private List arguments; + private List arguments = new ArrayList<>(); private boolean noArgumentMaximum; private CommandPermissions permissions; @@ -793,8 +801,7 @@ public class CommandDescription { firstNonNull(detailedDescription, ""), firstNonNull(executableCommand, null), // TODO ljacqu 20151128: May `executableCommand` be null? firstNonNull(parent, null), - valueOrEmptyList(children), - valueOrEmptyList(arguments), + arguments, noArgumentMaximum, firstNonNull(permissions, null) ); @@ -829,19 +836,7 @@ public class CommandDescription { return this; } - public Builder children(List children) { - this.children = children; - return this; - } - - public Builder children(CommandDescription... children) { - return children(asMutableList(children)); - } - public Builder withArgument(String label, String description, boolean isOptional) { - if (arguments == null) { - arguments = new ArrayList<>(); - } arguments.add(new CommandArgumentDescription(label, description, isOptional)); return this; } diff --git a/src/main/java/fr/xephi/authme/command/CommandManager.java b/src/main/java/fr/xephi/authme/command/CommandManager.java index aa471332e..08310a7d3 100644 --- a/src/main/java/fr/xephi/authme/command/CommandManager.java +++ b/src/main/java/fr/xephi/authme/command/CommandManager.java @@ -16,6 +16,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import static fr.xephi.authme.command.CommandPermissions.DefaultPermission.ALLOWED; import static fr.xephi.authme.command.CommandPermissions.DefaultPermission.OP_ONLY; /** @@ -44,6 +45,7 @@ public class CommandManager { public void registerCommands() { // Create a list of help command labels final List helpCommandLabels = Arrays.asList("help", "hlp", "h", "sos", "?"); + ExecutableCommand helpCommandExecutable = new HelpCommand(); // Register the base AuthMe Reloaded command CommandDescription authMeBaseCommand = CommandDescription.builder() @@ -56,7 +58,7 @@ public class CommandManager { // Register the help command CommandDescription authMeHelpCommand = CommandDescription.builder() - .executableCommand(new HelpCommand()) + .executableCommand(helpCommandExecutable) .labels(helpCommandLabels) .description("View help") .detailedDescription("View detailed help pages about AuthMeReloaded commands.") @@ -88,51 +90,49 @@ public class CommandManager { .build(); // Register the forcelogin command - CommandDescription forceLoginCommand = new CommandDescription(new ForceLoginCommand(), new ArrayList() { - - { - add("forcelogin"); - add("login"); - } - }, "Enforce login player", "Enforce the specified player to login.", authMeBaseCommand); - forceLoginCommand.setCommandPermissions(UserPermission.CAN_LOGIN_BE_FORCED, OP_ONLY); - forceLoginCommand.addArgument(new CommandArgumentDescription("player", "Online player name", true)); + CommandDescription forceLoginCommand = CommandDescription.builder() + .executableCommand(new ForceLoginCommand()) + .labels("forcelogin", "login") + .description("Enforce login player") + .detailedDescription("Enforce the specified player to login.") + .parent(authMeBaseCommand) + .permissions(OP_ONLY, UserPermission.CAN_LOGIN_BE_FORCED) + .withArgument("player", "Online player name", true) + .build(); // Register the changepassword command - CommandDescription changePasswordCommand = new CommandDescription(new ChangePasswordCommand(), new ArrayList() { + CommandDescription changePasswordCommand = CommandDescription.builder() + .executableCommand(new ChangePasswordCommand()) + .labels("password", "changepassword", "changepass", "cp") + .description("Change a player's password") + .detailedDescription("Change the password of a player.") + .parent(authMeBaseCommand) + .permissions(OP_ONLY, UserPermission.CHANGE_PASSWORD) + .withArgument("player", "Player name", false) + .withArgument("pwd", "New password", false) + .build(); - { - add("password"); - add("changepassword"); - add("changepass"); - add("cp"); - } - }, "Change player's password", "Change the password of a player.", authMeBaseCommand); - changePasswordCommand.setCommandPermissions(AdminPermission.CHANGE_PASSWORD, OP_ONLY); - changePasswordCommand.addArgument(new CommandArgumentDescription("player", "Player name", false)); - changePasswordCommand.addArgument(new CommandArgumentDescription("pwd", "New password", false)); - - // Register the purge command - CommandDescription lastLoginCommand = new CommandDescription(new LastLoginCommand(), new ArrayList() { - - { - add("lastlogin"); - add("ll"); - } - }, "Player's last login", "View the date of the specified players last login", authMeBaseCommand); - lastLoginCommand.setCommandPermissions(AdminPermission.LAST_LOGIN, OP_ONLY); - lastLoginCommand.addArgument(new CommandArgumentDescription("player", "Player name", true)); + // Register the last login command + CommandDescription lastLoginCommand = CommandDescription.builder() + .executableCommand(new LastLoginCommand()) + .labels("lastlogin", "ll") + .description("Player's last login") + .detailedDescription("View the date of the specified players last login") + .parent(authMeBaseCommand) + .permissions(OP_ONLY, AdminPermission.LAST_LOGIN) + .withArgument("player", "Player name", true) + .build(); // Register the accounts command - CommandDescription accountsCommand = new CommandDescription(new AccountsCommand(), new ArrayList() { - - { - add("accounts"); - add("account"); - } - }, "Display player accounts", "Display all accounts of a player by it's player name or IP.", authMeBaseCommand); - accountsCommand.setCommandPermissions(AdminPermission.ACCOUNTS, OP_ONLY); - accountsCommand.addArgument(new CommandArgumentDescription("player", "Player name or IP", true)); + CommandDescription accountsCommand = CommandDescription.builder() + .executableCommand(new AccountsCommand()) + .labels("accounts", "account") + .description("Display player accounts") + .detailedDescription("Display all accounts of a player by his player name or IP.") + .parent(authMeBaseCommand) + .permissions(OP_ONLY, AdminPermission.ACCOUNTS) + .withArgument("player", "Player name or IP", true) + .build(); // Register the getemail command CommandDescription getEmailCommand = new CommandDescription(new GetEmailCommand(), new ArrayList() { @@ -286,28 +286,28 @@ public class CommandManager { reloadCommand.setCommandPermissions(AdminPermission.RELOAD, OP_ONLY); // Register the version command - /* TODO ljacqu 20151128: labels should not be helpCommandLabels! Previously it was: - add("version"); - add("ver"); - add("v"); - add("about"); - add("info");*/ - CommandDescription versionCommand = new CommandDescription(new VersionCommand(), helpCommandLabels, - "Version info", "Show detailed information about the installed AuthMeReloaded version, and shows the developers, contributors, license and other information.", authMeBaseCommand); + CommandDescription versionCommand = CommandDescription.builder() + .executableCommand(new VersionCommand()) + .labels("version", "ver", "v", "about", "info") + .description("Version info") + .detailedDescription("Show detailed information about the installed AuthMeReloaded version, and shows the " + + "developers, contributors, license and other information.") + .parent(authMeBaseCommand) + .build(); // Register the base login command - CommandDescription loginBaseCommand = new CommandDescription(new LoginCommand(), new ArrayList() { - - { - add("login"); - add("l"); - } - }, "Login command", "Command to login using AuthMeReloaded.", null); - loginBaseCommand.setCommandPermissions(UserPermission.LOGIN, CommandPermissions.DefaultPermission.ALLOWED); - loginBaseCommand.addArgument(new CommandArgumentDescription("password", "Login password", false)); + CommandDescription loginBaseCommand = CommandDescription.builder() + .executableCommand(new LoginCommand()) + .labels("login", "l") + .description("Login command") + .detailedDescription("Command to log in using AuthMeReloaded.") + .parent(null) + .permissions(ALLOWED, UserPermission.LOGIN) + .withArgument("password", "Login password", false) + .build(); // Register the help command - CommandDescription loginHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription loginHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded login commands.", loginBaseCommand); loginHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); @@ -321,7 +321,7 @@ public class CommandManager { logoutBaseCommand.setCommandPermissions(UserPermission.LOGOUT, CommandPermissions.DefaultPermission.ALLOWED); // Register the help command - CommandDescription logoutHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription logoutHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded logout commands.", logoutBaseCommand); logoutHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); @@ -338,7 +338,7 @@ public class CommandManager { registerBaseCommand.addArgument(new CommandArgumentDescription("verifyPassword", "Verify password", false)); // Register the help command - CommandDescription registerHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription registerHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded register commands.", registerBaseCommand); registerHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); @@ -354,7 +354,7 @@ public class CommandManager { unregisterBaseCommand.addArgument(new CommandArgumentDescription("password", "Password", false)); // Register the help command - CommandDescription unregisterHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded unregister commands.", unregisterBaseCommand); + CommandDescription unregisterHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded unregister commands.", unregisterBaseCommand); unregisterHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); // Register the base changepassword command @@ -370,12 +370,12 @@ public class CommandManager { changePasswordBaseCommand.addArgument(new CommandArgumentDescription("verifyPassword", "Verify password", false)); // Register the help command - CommandDescription changePasswordHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription changePasswordHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded change password commands.", changePasswordBaseCommand); changePasswordHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); // Register the base Dungeon Maze command - CommandDescription emailBaseCommand = new CommandDescription(new HelpCommand(), new ArrayList() { + CommandDescription emailBaseCommand = new CommandDescription(helpCommandExecutable, new ArrayList() { { add("email"); @@ -384,7 +384,7 @@ public class CommandManager { }, "E-mail command", "The AuthMe Reloaded E-mail command. The root for all E-mail commands.", null); // Register the help command - CommandDescription emailHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription emailHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded help commands.", emailBaseCommand); emailHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); @@ -439,7 +439,7 @@ public class CommandManager { captchaBaseCommand.addArgument(new CommandArgumentDescription("captcha", "The captcha", false)); // Register the help command - CommandDescription captchaHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription captchaHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded change captcha commands.", captchaBaseCommand); captchaHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); @@ -456,7 +456,7 @@ public class CommandManager { converterBaseCommand.addArgument(new CommandArgumentDescription("job", "Conversion job: flattosql / flattosqlite /| xauth / crazylogin / rakamak / royalauth / vauth / sqltoflat", false)); // Register the help command - CommandDescription converterHelpCommand = new CommandDescription(new HelpCommand(), helpCommandLabels, + CommandDescription converterHelpCommand = new CommandDescription(helpCommandExecutable, helpCommandLabels, "View help", "View detailed help pages about AuthMeReloaded change captcha commands.", converterBaseCommand); converterHelpCommand.addArgument(new CommandArgumentDescription("query", "The command or query to view help for.", true)); diff --git a/src/test/java/fr/xephi/authme/command/CommandManagerTest.java b/src/test/java/fr/xephi/authme/command/CommandManagerTest.java index 9309d4a00..7ef8c428e 100644 --- a/src/test/java/fr/xephi/authme/command/CommandManagerTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandManagerTest.java @@ -1,10 +1,18 @@ package fr.xephi.authme.command; +import fr.xephi.authme.util.StringUtils; +import org.junit.Ignore; import org.junit.Test; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; +import java.util.Map; +import java.util.Set; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThat; /** @@ -12,6 +20,12 @@ import static org.junit.Assert.assertThat; */ public class CommandManagerTest { + /** + * Defines the maximum allowed depths for nesting CommandDescription instances. + * Note that the depth starts at 0 (e.g. /authme), so a depth of 2 is something like /authme hello world + */ + private static int MAX_ALLOWED_DEPTH = 2; + @Test public void shouldInitializeCommands() { // given/when @@ -29,6 +43,108 @@ public class CommandManagerTest { assertThat(commandsIncludeLabel(commands, "help"), equalTo(false)); } + @Test + public void shouldNotBeNestedExcessively() { + // given + CommandManager manager = new CommandManager(true); + Consumer descriptionTester = new Consumer() { + @Override + public void accept(CommandDescription command, int depth) { + assertThat(depth <= MAX_ALLOWED_DEPTH, equalTo(true)); + } + }; + + // when/then + List commands = manager.getCommandDescriptions(); + walkThroughCommands(commands, descriptionTester); + } + + @Test + @Ignore + public void shouldNotDefineSameLabelTwice() { + // given + CommandManager manager = new CommandManager(true); + Set commandMappings = new HashSet<>(); + Consumer uniqueMappingTester = new Consumer() { + @Override + public void accept(CommandDescription command, int depth) { + + } + }; + + // when + // TODO finish this + } + + /** + * The description should provide a very short description of the command and shouldn't end in a ".", whereas the + * detailed description should be longer and end with a period. + */ + @Test + public void shouldHaveDescription() { + // given + CommandManager manager = new CommandManager(true); + Consumer descriptionTester = new Consumer() { + @Override + public void accept(CommandDescription command, int depth) { + assertThat("has description", StringUtils.isEmpty(command.getDescription()), equalTo(false)); + assertThat("short description doesn't end in '.'", command.getDescription().endsWith("."), + equalTo(false)); + assertThat("has detailed description", StringUtils.isEmpty(command.getDetailedDescription()), + equalTo(false)); + assertThat("detailed description ends in '.'", command.getDetailedDescription().endsWith("."), + equalTo(true)); + } + }; + + // when + List commands = manager.getCommandDescriptions(); + walkThroughCommands(commands, descriptionTester); + } + + @Test + public void shouldNotHaveMultipleInstancesOfSameExecutableCommandSubType() { + // given + final Map, ExecutableCommand> implementations = new HashMap<>(); + CommandManager manager = new CommandManager(true); + Consumer descriptionTester = new Consumer() { + @Override + public void accept(CommandDescription command, int depth) { + assertThat(command.getExecutableCommand(), not(nullValue())); + ExecutableCommand commandExec = command.getExecutableCommand(); + ExecutableCommand storedExec = implementations.get(command.getExecutableCommand().getClass()); + if (storedExec != null) { + assertThat("has same implementation of '" + storedExec.getClass().getName() + "' for command with " + + "parent " + (command.getParent() == null ? "null" : command.getParent().getLabels()), + storedExec == commandExec, equalTo(true)); + } else { + implementations.put(commandExec.getClass(), commandExec); + } + } + }; + + // when + List commands = manager.getCommandDescriptions(); + walkThroughCommands(commands, descriptionTester); + } + + + // ------------ + // Helper methods + // ------------ + private static void walkThroughCommands(List commands, Consumer consumer) { + walkThroughCommands(commands, consumer, 0); + } + + private static void walkThroughCommands(List commands, Consumer consumer, int depth) { + for (CommandDescription command : commands) { + consumer.accept(command, depth); + if (command.hasChildren()) { + walkThroughCommands(command.getChildren(), consumer, depth + 1); + } + } + } + private static boolean commandsIncludeLabel(Iterable commands, String label) { for (CommandDescription command : commands) { if (command.getLabels().contains(label)) { @@ -38,4 +154,8 @@ public class CommandManagerTest { return false; } + private interface Consumer { + void accept(CommandDescription command, int depth); + } + }