From 62be3a5da5384c600d25cb305e0bd43289b2d39e Mon Sep 17 00:00:00 2001 From: Hugo Manrique Date: Thu, 10 Jun 2021 20:25:52 +0200 Subject: [PATCH] Fix Brigadier command alias redirects --- .../proxy/command/CommandGraphInjector.java | 8 +++--- .../proxy/command/SuggestionsProvider.java | 2 +- .../proxy/command/VelocityCommandManager.java | 25 +++++++++++++------ .../command/CommandGraphInjectorTests.java | 7 ++++-- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java b/proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java index 015501a12..0d692891a 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/command/CommandGraphInjector.java @@ -88,9 +88,7 @@ public final class CommandGraphInjector { final VelocityArgumentCommandNode 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 { } final ArgumentBuilder 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 target = this.filterNode(node.getRedirect(), source); builder.forward(target, builder.getRedirectModifier(), builder.isFork()); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/SuggestionsProvider.java b/proxy/src/main/java/com/velocitypowered/proxy/command/SuggestionsProvider.java index e6af2d62b..a4faf9fd0 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/SuggestionsProvider.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/command/SuggestionsProvider.java @@ -145,7 +145,7 @@ final class SuggestionsProvider { final StringReader reader, final CommandContextBuilder 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> aliases = contextSoFar.getRootNode().getChildren(); diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java b/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java index 10efb0f3d..d206b5e8f 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java +++ b/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 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 boolean tryRegister(final CommandRegistrar registrar, final Command command, final CommandMeta meta) { final Class superInterface = registrar.registrableSuperInterface(); if (!superInterface.isInstance(command)) { return false; } - try { - registrar.register(meta, superInterface.cast(command)); - return true; - } catch (final IllegalArgumentException ignored) { - return false; - } + registrar.register(meta, superInterface.cast(command)); + return true; } @Override diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java index 92a5b8b19..6f854aa96 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java @@ -119,11 +119,11 @@ public class CommandGraphInjectorTests extends CommandTestSuite { } @Test - void testBrigadierCommandAliasRedirectsNotAllowed() { + void testInjectPreservesBrigadierCommandAliasRedirect() { final var registered = LiteralArgumentBuilder .literal("origin") .redirect(LiteralArgumentBuilder - .literal("target") + .literal("target") .build()) .build(); manager.register(new BrigadierCommand(registered)); @@ -131,6 +131,9 @@ public class CommandGraphInjectorTests extends CommandTestSuite { final var expected = LiteralArgumentBuilder .literal("origin") + .redirect(LiteralArgumentBuilder + .literal("target") + .build()) .build(); assertEquals(expected, dest.getChild("origin")); }