Fix Brigadier command alias redirects

proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java

@@ -88,9 +88,7 @@ public final class CommandGraphInjector<S> {
         final VelocityArgumentCommandNode<S, ?> argsNode =
                 VelocityCommands.getArgumentsNode(asLiteral);
         if (argsNode == null) {
-          // This literal is associated to a BrigadierCommand, filter normally
-          // TODO Document alias redirects are not supported, see
-          // CommandGraphInjectorTests#testBrigadierCommandAliasRedirectsNotAllowed.
+          // This literal is associated to a BrigadierCommand, filter normally.
           this.copyChildren(node, copy, source);
         } else {
           // Copy all children nodes (arguments node and hints)
@@ -116,7 +114,9 @@ public final class CommandGraphInjector<S> {
     }
     final ArgumentBuilder<S, ?> builder = node.createBuilder();
     if (node.getRedirect() != null) {
-      // TODO Document redirects to non-Brigadier commands are not supported
+      // Redirects to non-Brigadier commands are not supported. Luckily,
+      // we don't expose the root node to API users, so they can't access
+      // nodes associated to other commands.
       final CommandNode<S> target = this.filterNode(node.getRedirect(), source);
       builder.forward(target, builder.getRedirectModifier(), builder.isFork());
     }

proxy/src/main/java/com/velocitypowered/proxy/command/SuggestionsProvider.java

@@ -145,7 +145,7 @@ final class SuggestionsProvider<S> {
           final StringReader reader, final CommandContextBuilder<S> contextSoFar) {
     final S source = contextSoFar.getSource();
     // Lowercase the alias here so all comparisons can be case-sensitive (cheaper)
-    // TODO Is this actually faster?
+    // TODO Is this actually faster? It may incur an allocation
     final String input = reader.getRead().toLowerCase(Locale.ENGLISH);
 
     final Collection<CommandNode<S>> aliases = contextSoFar.getRootNode().getChildren();

proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java

@@ -101,29 +101,38 @@ public class VelocityCommandManager implements CommandManager {
   public void register(final CommandMeta meta, final Command command) {
     Preconditions.checkNotNull(meta, "meta");
     Preconditions.checkNotNull(command, "command");
-    // TODO This is quite ugly; find registrar and then attempt registering.
 
+    // TODO Warn if command implements multiple registrable interfaces?
     for (final CommandRegistrar<?> registrar : this.registrars) {
       if (this.tryRegister(registrar, command, meta)) {
-        return;
+        return; // success
       }
     }
     throw new IllegalArgumentException(
             command + " does not implement a registrable Command subinterface");
   }
 
+  /**
+   * Attempts to register the given command if it implements the
+   * {@linkplain CommandRegistrar#registrableSuperInterface() registrable superinterface}
+   * of the given registrar.
+   *
+   * @param registrar the registrar to register the command
+   * @param command the command to register
+   * @param meta the command metadata
+   * @param <T> the type of the command
+   * @return true if the command implements the registrable superinterface of the registrar;
+   *         false otherwise.
+   * @throws IllegalArgumentException if the registrar cannot register the command
+   */
   private <T extends Command> boolean tryRegister(final CommandRegistrar<T> registrar,
                                                   final Command command, final CommandMeta meta) {
     final Class<T> superInterface = registrar.registrableSuperInterface();
     if (!superInterface.isInstance(command)) {
       return false;
     }
-    try {
     registrar.register(meta, superInterface.cast(command));
     return true;
-    } catch (final IllegalArgumentException ignored) {
-      return false;
-    }
   }
 
   @Override

proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java

@@ -119,7 +119,7 @@ public class CommandGraphInjectorTests extends CommandTestSuite {
   }
 
   @Test
-  void testBrigadierCommandAliasRedirectsNotAllowed() {
+  void testInjectPreservesBrigadierCommandAliasRedirect() {
     final var registered = LiteralArgumentBuilder
             .<CommandSource>literal("origin")
             .redirect(LiteralArgumentBuilder
@@ -131,6 +131,9 @@ public class CommandGraphInjectorTests extends CommandTestSuite {
 
     final var expected = LiteralArgumentBuilder
             .<CommandSource>literal("origin")
+            .redirect(LiteralArgumentBuilder
+                .<CommandSource>literal("target")
+                .build())
             .build();
     assertEquals(expected, dest.getChild("origin"));
   }