From 0b0c36dcfc77a89d215fd618b5b139c5f3ce9f9a Mon Sep 17 00:00:00 2001
From: Andrew Steinborn <git@steinborn.me>
Date: Sun, 31 Oct 2021 19:05:21 -0400
Subject: [PATCH] Correct command meta alias removal behavior and add
 appropriate unit tests.

Apparently @hugmanrique caught the issue but suggested the wrong fix. This is the correct fix, and respects the Javadoc.
---
 .../api/command/CommandManager.java           |  7 ++++
 .../proxy/command/VelocityCommandManager.java | 20 ++++++++--
 .../proxy/command/CommandManagerTests.java    | 39 +++++++++++++++++++
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/api/src/main/java/com/velocitypowered/api/command/CommandManager.java b/api/src/main/java/com/velocitypowered/api/command/CommandManager.java
index 928d55d6f..021b6ca1e 100644
--- a/api/src/main/java/com/velocitypowered/api/command/CommandManager.java
+++ b/api/src/main/java/com/velocitypowered/api/command/CommandManager.java
@@ -75,6 +75,13 @@ public interface CommandManager {
    */
   void unregister(String alias);
 
+  /**
+   * Unregisters the specified command from the manager, if registered.
+   *
+   * @param meta the command to unregister
+   */
+  void unregister(CommandMeta meta);
+
   /**
    * Retrieves the {@link CommandMeta} from the specified command alias, if registered.
    * @param alias the command alias to lookup
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 7f9280c8e..2feb88dce 100644
--- a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java
+++ b/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java
@@ -156,11 +156,23 @@ public class VelocityCommandManager implements CommandManager {
       // The literals of secondary aliases will preserve the children of
       // the removed literal in the graph.
       dispatcher.getRoot().removeChildByName(alias.toLowerCase(Locale.ENGLISH));
+      commandMetas.remove(alias);
+    } finally {
+      lock.writeLock().unlock();
+    }
+  }
 
-      CommandMeta meta = commandMetas.get(alias);
-      if (meta != null) {
-        for (String metaAlias : meta.getAliases()) {
-          commandMetas.remove(metaAlias, meta);
+  @Override
+  public void unregister(CommandMeta meta) {
+    Preconditions.checkNotNull(meta, "meta");
+    lock.writeLock().lock();
+    try {
+      // The literals of secondary aliases will preserve the children of
+      // the removed literal in the graph.
+      for (String alias : meta.getAliases()) {
+        final String lowercased = alias.toLowerCase(Locale.ENGLISH);
+        if (commandMetas.remove(lowercased, meta)) {
+          dispatcher.getRoot().removeChildByName(lowercased);
         }
       }
     } finally {
diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java
index bc5c826b4..37cccc4f6 100644
--- a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java
+++ b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java
@@ -19,6 +19,7 @@ package com.velocitypowered.proxy.command;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
@@ -44,6 +45,7 @@ public class CommandManagerTests extends CommandTestSuite {
 
     assertTrue(manager.hasCommand("hello"));
     assertRegisteredAliases("hello");
+    assertEquals(meta, manager.getCommandMeta("hello"));
   }
 
   @Test
@@ -59,6 +61,10 @@ public class CommandManagerTests extends CommandTestSuite {
     assertTrue(manager.hasCommand("baz"));
     assertTrue(manager.hasCommand("qux"));
     assertRegisteredAliases("foo", "bar", "baz", "qux");
+    assertEquals(meta, manager.getCommandMeta("foo"));
+    assertEquals(meta, manager.getCommandMeta("bar"));
+    assertEquals(meta, manager.getCommandMeta("baz"));
+    assertEquals(meta, manager.getCommandMeta("qux"));
   }
 
   @Test
@@ -90,8 +96,11 @@ public class CommandManagerTests extends CommandTestSuite {
 
     final var oldMeta = manager.metaBuilder("foo").build();
     manager.register(oldMeta, DummyCommand.INSTANCE); // fails on execution
+    assertEquals(oldMeta, manager.getCommandMeta("foo"));
+
     final var newMeta = manager.metaBuilder("foo").build();
     manager.register(newMeta, (RawCommand) invocation -> called.set(true));
+    assertEquals(newMeta, manager.getCommandMeta("foo"));
     manager.executeAsync(MockCommandSource.INSTANCE, "foo").join();
 
     assertTrue(called.get());
@@ -153,9 +162,39 @@ public class CommandManagerTests extends CommandTestSuite {
 
     assertFalse(manager.hasCommand("bar"));
     assertTrue(manager.hasCommand("foo"));
+    assertEquals(meta, manager.getCommandMeta("foo"));
     assertRegisteredAliases("foo");
   }
 
+  @Test
+  void testUnregisterAllAliases() {
+    final var meta = manager.metaBuilder("foo")
+        .aliases("bar")
+        .build();
+    manager.register(meta, DummyCommand.INSTANCE);
+    manager.unregister(meta);
+
+    assertFalse(manager.hasCommand("bar"));
+    assertFalse(manager.hasCommand("foo"));
+  }
+
+  @Test
+  void testUnregisterAliasOverlap() {
+    final var meta1 = manager.metaBuilder("foo")
+        .aliases("bar")
+        .build();
+    manager.register(meta1, DummyCommand.INSTANCE);
+    final var meta2 = manager.metaBuilder("bar")
+        .build();
+    manager.register(meta2, DummyCommand.INSTANCE);
+    assertEquals(meta1, manager.getCommandMeta("foo"));
+    assertEquals(meta2, manager.getCommandMeta("bar"));
+
+    manager.unregister(meta1);
+    assertNull(manager.getCommandMeta("foo"));
+    assertEquals(meta2, manager.getCommandMeta("bar"));
+  }
+
   // Execution
 
   @Test