From 6a94135f6468f0aecd34d6149853da65b9aecf6c Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 29 Nov 2015 10:24:32 +0100 Subject: [PATCH] Commands - fix child not being registered in parent via Builder - Create test to ensure that commands don't define the same binding - Create stricter attribute validation in builder: throw an error if required field was not initialized --- .../authme/command/CommandDescription.java | 56 +++++++------ .../authme/command/CommandManagerTest.java | 82 ++++++++++++++----- 2 files changed, 90 insertions(+), 48 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandDescription.java b/src/main/java/fr/xephi/authme/command/CommandDescription.java index afd6ac1e3..fc5a7e4c8 100644 --- a/src/main/java/fr/xephi/authme/command/CommandDescription.java +++ b/src/main/java/fr/xephi/authme/command/CommandDescription.java @@ -6,6 +6,7 @@ import org.bukkit.command.CommandSender; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.List; @@ -144,6 +145,11 @@ public class CommandDescription { this.arguments = arguments; this.noArgumentMaximum = noArgumentMaximum; this.permissions = permissions; + + if (parent != null) { + // Passing `this` in constructor is not very nice; consider creating a "static create()" method instead + parent.addChild(this); + } } /** @@ -246,9 +252,9 @@ public class CommandDescription { } /** - * Get the absolute command label, without a slash. + * Get the absolute command label, without a starting slash. * - * @return the absolute label + * @return The absolute label */ public String getAbsoluteLabel() { return getAbsoluteLabel(false); @@ -445,20 +451,6 @@ public class CommandDescription { return this.children; } - /** - * Set the children of this command. - * - * @param children New command children. Null to remove all children. - */ - public void setChildren(List children) { - // Check whether the children list should be cleared - if (children == null) - this.children.clear(); - - else - this.children = children; - } - /** * Add a child to the command description. * @@ -757,7 +749,7 @@ public class CommandDescription { } /** - * Get the command permissions. + * Get the command permissions. Return null if the command doesn't require any permission. * * @return The command permissions. */ @@ -796,10 +788,10 @@ public class CommandDescription { */ public CommandDescription build() { return new CommandDescription( - valueOrEmptyList(labels), + getOrThrow(labels, "labels"), firstNonNull(description, ""), firstNonNull(detailedDescription, ""), - firstNonNull(executableCommand, null), // TODO ljacqu 20151128: May `executableCommand` be null? + getOrThrow(executableCommand, "executableCommand"), firstNonNull(parent, null), arguments, noArgumentMaximum, @@ -857,16 +849,28 @@ public class CommandDescription { return new ArrayList<>(Arrays.asList(items)); } - private static List valueOrEmptyList(List givenList) { - if (givenList != null) { - return givenList; - } - return new ArrayList<>(); - } - private static T firstNonNull(T first, T second) { return first != null ? first : second; } + + private static T getOrThrow(T element, String elementName) { + if (!isEmpty(element)) { + return element; + } + throw new RuntimeException("The element '" + elementName + "' may not be empty in CommandDescription"); + } + + private static boolean isEmpty(T element) { + if (element == null) { + return true; + } else if (element instanceof Collection) { + return ((Collection) element).isEmpty(); + } else if (element instanceof String) { + return StringUtils.isEmpty((String) element); + } + return false; + } + } } diff --git a/src/test/java/fr/xephi/authme/command/CommandManagerTest.java b/src/test/java/fr/xephi/authme/command/CommandManagerTest.java index 7ef8c428e..cba6ccb97 100644 --- a/src/test/java/fr/xephi/authme/command/CommandManagerTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandManagerTest.java @@ -1,9 +1,11 @@ package fr.xephi.authme.command; import fr.xephi.authme.util.StringUtils; +import org.junit.BeforeClass; import org.junit.Ignore; import org.junit.Test; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -24,12 +26,18 @@ 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; + private static int MAX_ALLOWED_DEPTH = 1; + + private static CommandManager manager; + + @BeforeClass + public static void initializeCommandManager() { + manager = new CommandManager(true); + } @Test public void shouldInitializeCommands() { // given/when - CommandManager manager = new CommandManager(true); int commandCount = manager.getCommandDescriptionCount(); List commands = manager.getCommandDescriptions(); @@ -46,8 +54,7 @@ public class CommandManagerTest { @Test public void shouldNotBeNestedExcessively() { // given - CommandManager manager = new CommandManager(true); - Consumer descriptionTester = new Consumer() { + BiConsumer descriptionTester = new BiConsumer() { @Override public void accept(CommandDescription command, int depth) { assertThat(depth <= MAX_ALLOWED_DEPTH, equalTo(true)); @@ -55,25 +62,28 @@ public class CommandManagerTest { }; // when/then - List commands = manager.getCommandDescriptions(); - walkThroughCommands(commands, descriptionTester); + walkThroughCommands(manager.getCommandDescriptions(), descriptionTester); } @Test - @Ignore public void shouldNotDefineSameLabelTwice() { // given - CommandManager manager = new CommandManager(true); - Set commandMappings = new HashSet<>(); - Consumer uniqueMappingTester = new Consumer() { + final Set commandMappings = new HashSet<>(); + BiConsumer uniqueMappingTester = new BiConsumer() { @Override public void accept(CommandDescription command, int depth) { - + int initialSize = commandMappings.size(); + List newMappings = getAbsoluteLabels(command); + commandMappings.addAll(newMappings); + // Set only contains unique entries, so we just check after adding all new mappings that the size + // of the Set corresponds to our expectation + assertThat("All bindings are unique for command with bindings '" + command.getLabels() + "'", + commandMappings.size() == initialSize + newMappings.size(), equalTo(true)); } }; - // when - // TODO finish this + // when/then + walkThroughCommands(manager.getCommandDescriptions(), uniqueMappingTester); } /** @@ -83,8 +93,7 @@ public class CommandManagerTest { @Test public void shouldHaveDescription() { // given - CommandManager manager = new CommandManager(true); - Consumer descriptionTester = new Consumer() { + BiConsumer descriptionTester = new BiConsumer() { @Override public void accept(CommandDescription command, int depth) { assertThat("has description", StringUtils.isEmpty(command.getDescription()), equalTo(false)); @@ -97,17 +106,20 @@ public class CommandManagerTest { } }; - // when - List commands = manager.getCommandDescriptions(); - walkThroughCommands(commands, descriptionTester); + // when/then + walkThroughCommands(manager.getCommandDescriptions(), descriptionTester); } + /** + * Check that the implementation of {@link ExecutableCommand} a command points to is the same for each type: + * it is inefficient to instantiate the same type multiple times. + */ @Test public void shouldNotHaveMultipleInstancesOfSameExecutableCommandSubType() { // given final Map, ExecutableCommand> implementations = new HashMap<>(); CommandManager manager = new CommandManager(true); - Consumer descriptionTester = new Consumer() { + BiConsumer descriptionTester = new BiConsumer() { @Override public void accept(CommandDescription command, int depth) { assertThat(command.getExecutableCommand(), not(nullValue())); @@ -125,6 +137,8 @@ public class CommandManagerTest { // when List commands = manager.getCommandDescriptions(); + + // then walkThroughCommands(commands, descriptionTester); } @@ -132,11 +146,11 @@ public class CommandManagerTest { // ------------ // Helper methods // ------------ - private static void walkThroughCommands(List commands, Consumer consumer) { + private static void walkThroughCommands(List commands, BiConsumer consumer) { walkThroughCommands(commands, consumer, 0); } - private static void walkThroughCommands(List commands, Consumer consumer, int depth) { + private static void walkThroughCommands(List commands, BiConsumer consumer, int depth) { for (CommandDescription command : commands) { consumer.accept(command, depth); if (command.hasChildren()) { @@ -154,8 +168,32 @@ public class CommandManagerTest { return false; } - private interface Consumer { + private interface BiConsumer { void accept(CommandDescription command, int depth); } + /** + * Get the absolute label that a command defines. Note: Assumes that only the passed command might have + * multiple labels; only considering the first label for all of the command's parents. + * + * @param command The command to verify + * + * @return The full command binding + */ + private static List getAbsoluteLabels(CommandDescription command) { + String parentPath = ""; + CommandDescription elem = command.getParent(); + while (elem != null) { + parentPath = elem.getLabels().get(0) + " " + parentPath; + elem = elem.getParent(); + } + parentPath = parentPath.trim(); + + List bindings = new ArrayList<>(command.getLabels().size()); + for (String label : command.getLabels()) { + bindings.add(StringUtils.join(" ", parentPath, label)); + } + return bindings; + } + }