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

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<CommandDescription> 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 <T> List<T> valueOrEmptyList(List<T> givenList) {
-            if (givenList != null) {
-                return givenList;
-            }
-            return new ArrayList<>();
-        }
-
         private static <T> T firstNonNull(T first, T second) {
             return first != null ? first : second;
         }
+
+        private static <T> 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 <T> 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;
+        }
+
     }
 
 }

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<CommandDescription> commands = manager.getCommandDescriptions();
 
@@ -46,8 +54,7 @@ public class CommandManagerTest {
     @Test
     public void shouldNotBeNestedExcessively() {
         // given
-        CommandManager manager = new CommandManager(true);
+        BiConsumer descriptionTester = new BiConsumer() {
-        Consumer descriptionTester = new Consumer() {
             @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<CommandDescription> commands = manager.getCommandDescriptions();
+        walkThroughCommands(manager.getCommandDescriptions(), descriptionTester);
-        walkThroughCommands(commands, descriptionTester);
     }
 
     @Test
-    @Ignore
     public void shouldNotDefineSameLabelTwice() {
         // given
-        CommandManager manager = new CommandManager(true);
+        final Set<String> commandMappings = new HashSet<>();
-        Set<String> commandMappings = new HashSet<>();
+        BiConsumer uniqueMappingTester = new BiConsumer() {
-        Consumer uniqueMappingTester = new Consumer() {
             @Override
             public void accept(CommandDescription command, int depth) {
-
+                int initialSize = commandMappings.size();
+                List<String> 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
+        // when/then
-        // TODO finish this
+        walkThroughCommands(manager.getCommandDescriptions(), uniqueMappingTester);
     }
 
     /**
@@ -83,8 +93,7 @@ public class CommandManagerTest {
     @Test
     public void shouldHaveDescription() {
         // given
-        CommandManager manager = new CommandManager(true);
+        BiConsumer descriptionTester = new BiConsumer() {
-        Consumer descriptionTester = new Consumer() {
             @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
+        // when/then
-        List<CommandDescription> commands = manager.getCommandDescriptions();
+        walkThroughCommands(manager.getCommandDescriptions(), descriptionTester);
-        walkThroughCommands(commands, 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<Class<? extends ExecutableCommand>, 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<CommandDescription> commands = manager.getCommandDescriptions();
+
+        // then
         walkThroughCommands(commands, descriptionTester);
     }
 
@@ -132,11 +146,11 @@ public class CommandManagerTest {
     // ------------
     // Helper methods
     // ------------
-    private static void walkThroughCommands(List<CommandDescription> commands, Consumer consumer) {
+    private static void walkThroughCommands(List<CommandDescription> commands, BiConsumer consumer) {
         walkThroughCommands(commands, consumer, 0);
     }
 
-    private static void walkThroughCommands(List<CommandDescription> commands, Consumer consumer, int depth) {
+    private static void walkThroughCommands(List<CommandDescription> 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<String> 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<String> bindings = new ArrayList<>(command.getLabels().size());
+        for (String label : command.getLabels()) {
+            bindings.add(StringUtils.join(" ", parentPath, label));
+        }
+        return bindings;
+    }
+
 }