Registering commands during server runtime (Bukkit)

[willkroboth]

This PR makes it possible to register commands while a Bukkit server is running.

Previously, if a developer attempted to register a command while the server was running, there would be a little warning Command /(command name) is being registered after the server had loaded. Undefined behavior ahead!. Unfortunately, it would pretty much seem like nothing at all happened.

Usually, the CommandAPI can just put commands into the MinecraftServer#vanillaCommandDispatcher CommandDispatcher and be done. Behind the scenes, CraftServer would sync everything up, and the commands would work. However, that sync process usually only happens as the server is finishing up its load and enabling processes. In order to get commands registered during server runtime to work properly, those synchronization processes just need to happen again.

So, with this PR, CommandAPIBukkit#postCommandRegistration now looks like this:

public void postCommandRegistration(RegisteredCommand registeredCommand, LiteralCommandNode<Source> resultantNode, List<LiteralCommandNode<Source>> aliasNodes) {
	if(!CommandAPI.canRegister()) {
		// Usually, when registering commands during server startup, we can just put our commands into the
		// `net.minecraft.server.MinecraftServer#vanillaCommandDispatcher" and leave it. As the server finishes setup,
		// it and the CommandAPI do some extra stuff to make everything work, and we move on.
		// So, if we want to register commands while the server is running, we need to do all that extra stuff, and
		// that is what this code does.
		// We could probably call all those methods to sync everything up, but in the spirit of avoiding side effects
		// and avoiding doing things twice for existing commands, this is a distilled version of those methods.

		CommandMap map = paper.getCommandMap();
		String permNode = unpackInternalPermissionNodeString(registeredCommand.permission());
		RootCommandNode<Source> root = getResourcesDispatcher().getRoot();

		// Wrapping Brigadier nodes into VanillaCommandWrappers and putting them in the CommandMap usually happens
		// in `CraftServer#setVanillaCommands`
		Command command = wrapToVanillaCommandWrapper(resultantNode);
		map.register("minecraft", command);

		// Adding permissions to these Commands usually happens in `CommandAPIBukkit#onEnable`
		command.setPermission(permNode);

		// Adding commands to the other (Why bukkit/spigot?!) dispatcher usually happens in `CraftServer#syncCommands`
		root.addChild(resultantNode);

		// Do the same for the aliases
		for(LiteralCommandNode<Source> node: aliasNodes) {
			command = wrapToVanillaCommandWrapper(node);
			map.register("minecraft", command);

			command.setPermission(permNode);

			root.addChild(node);
		}

		// Adding the command to the help map usually happens in `CommandAPIBukkit#onEnable`
		updateHelpForCommands(List.of(registeredCommand));

		// Sending command dispatcher packets usually happens when Players join the server
		for(Player p: Bukkit.getOnlinePlayers()) {
			resendPackets(p);
		}
	}
}

If the server has finished loading (indicated by CommandAPI.canRegister() being false), this extra code is run to:

• Add commands to the server's CommandMap
• Add commands to the server's 'resources' CommandDispatcher
• 'Fix' permissions by adding them to the VanillaCommandWrapper version of the command
• Add the command to the help map
• Resend command packets to Players so they know about the new command

To help with this, two NMS methods were added:

public interface NMS<CommandListenerWrapper> {
	/**
	 * Returns the Brigadier CommandDispatcher used when commands are sent to Players
	 *
	 * @return A Brigadier CommandDispatcher
	 */
	CommandDispatcher<CommandListenerWrapper> getResourcesDispatcher();

	/**
	 * Wraps a Brigadier command node as Bukkit's VanillaCommandWrapper
	 *
	 * @param node The LiteralCommandNode to wrap
	 * @return A VanillaCommandWrapper representing the given node
	 */
	Command wrapToVanillaCommandWrapper(LiteralCommandNode<CommandListenerWrapper> node);
}

To test this feature, these commands were added.

new CommandAPICommand("register")
	.withArguments(new StringArgument("command"))
	.withOptionalArguments(
		new GreedyStringArgument("aliases")
	)
	.executes(info -> {
		String name = info.args().getUnchecked("command");
		assert name != null;

		String aliasString = info.args().getUnchecked("aliases");
		String[] aliases;
		if(aliasString == null)
			aliases = new String[0];
		else
			aliases = aliasString.split(" ");

		new CommandAPICommand(name)
			.withAliases(aliases)
			.executes(i -> {
				i.sender().sendMessage("You ran the " + name + " command!");
			})
			.withPermission("dynamic." + name)
			.withShortDescription("New command!")
			.withFullDescription("This command was added while the server was running. Do you see it?")
			.register();
	})
	.register();

new CommandAPICommand("unregister")
	.withArguments(new StringArgument("command"))
	.executes(info -> {
		String name = info.args().getUnchecked("command");
		assert name != null;

		CommandAPI.unregister(name, true);
	})
	.register();

new CommandAPICommand("updateRequirements")
	.executesPlayer(info -> {
		CommandAPI.updateRequirements(info.sender());
	})
	.register();

These test commands should be removed before merging

TODO:

• Merge update 1.20 and add support
• Fix unregistering commands
• Test across all versions (registering and unregistering)
  • 1.15, 1.15.1, 1.15.2
  • 1.16.1
  • 1.16.2, 1.16.3
  • 1.16.4
  • 1.16.5
  • 1.17
  • 1.17.1
  • 1.18, 1.18.1
  • 1.18.2
  • 1.19
  • 1.19.1, 1.19.2
  • 1.19.3
  • 1.19.4
  • 1.20, 1.20.1
• Remove message that things might not work (because they do now)
• Address code review
• Fix tests
• Add tests for unregistering commands
• Update relevant documentation
  • https://commandapi-live-docs.jorel.dev/commandregistration.html
• Check compatibility with Brigadier Command Support PaperMC/Paper#8235? 9.0.3 is not currently compatible
• Remove test commands

[willkroboth]

This PR now also allows unregistering commands during server runtime. Taking a closer look at the command system, I was also able to clean up the logic as well. CommandAPIBukkit#unregister now looks like this:

public void unregister(String commandName, boolean force) {
	CommandAPI.logInfo("Unregistering command /" + commandName);

	// Remove nodes from the Brigadier dispatcher
	RootCommandNode<Source> brigRoot = getBrigadierDispatcher().getRoot();
	commandNodeChildren.get(brigRoot).remove(commandName);
	commandNodeLiterals.get(brigRoot).remove(commandName);
	// We really only expect commands to be represented as literals, but it is technically possible
	// to put an ArgumentCommandNode in here, so we'll check
	commandNodeArguments.get(brigRoot).remove(commandName);

	if (!CommandAPI.canRegister() || force) {
		// Bukkit is done with normal command stuff, so we have to modify their CommandMap ourselves
		// If we're forcing, we'll also go here to make sure commands are really gone
		Map<String, Command> knownCommands = getKnownCommands();
		knownCommands.remove(commandName);
		if (force) removeCommandNamespace(knownCommands, commandName);

		// Remove commands from the resources dispatcher
		RootCommandNode<Source> resourcesRoot = getResourcesDispatcher().getRoot();
		Map<String, CommandNode<?>> children = commandNodeChildren.get(resourcesRoot);
		Map<String, CommandNode<?>> literals = commandNodeLiterals.get(resourcesRoot);
		Map<String, CommandNode<?>> arguments = commandNodeArguments.get(resourcesRoot);

		children.remove(commandName);
		literals.remove(commandName);
		arguments.remove(commandName);

		// Since the commands in here are copied from Bukkit's map, there may be namespaced versions of the command, which we should remove
		if (force) {
			removeCommandNamespace(children, commandName);
			removeCommandNamespace(literals, commandName);
			removeCommandNamespace(arguments, commandName);
		}

		if (!CommandAPI.canRegister()) {
			// If the server actually is running (not just force unregistering), we should also update help and notify players
			getHelpMap().remove("/" + commandName);

			for (Player p : Bukkit.getOnlinePlayers()) {
				resendPackets(p);
			}
		}
	}
}

private void removeCommandNamespace(Map<String, ?> map, String commandName) {
	for(String key : new HashSet<>(map.keySet())) {
		if(key.contains(":") && key.split(":")[1].equalsIgnoreCase(commandName)) {
			map.remove(key);
		}
	}
}

private Map<String, Command> getKnownCommands() {
	return commandMapKnownCommands.get((SimpleCommandMap) paper.getCommandMap());
}

Notable changes:

• The Brigadier dispatcher is no longer checked for namespaced versions of commands. Namespacing happens when commands are put into Bukkit's CommandMap, so namespaces shouldn't show up in the Brigadier dispatcher.
• The CommandMap and resources dispatcher are checked for commands to unregister.
• If the server is running, the command is also removed from the help map and new command packets are sent to players

[JorelAli]

If my understanding is correct, the only thing left to do is Add tests for unregistering commands? From what I can tell, the majority of this PR looks feature complete?

[willkroboth]

Yep. I'm almost done with that too. I just need to finish applying some changes to all of the MockNMS classes that let me test executing VanillaCommandWrappers. A final code review would probably also be good.

I've also technically checked compatibility with PaperMC/Paper#8235. It seems the CommandAPI without these changes is currently incompatible. I get this exception when trying to register a command, shading version 9.0.3:

Error occurred while enabling CommandAPITest v1.0.0 (Is it up to date?)
java.lang.NoSuchFieldError: vanillaCommandDispatcher
        at me.willkroboth.CommandAPITest.commandapi.nms.NMS_1_20_R1.getBrigadierDispatcher(NMS_1_20_R1.java:333) ~[CommandAPITest-1.0-SNAPSHOT.jar:?]
        at me.willkroboth.CommandAPITest.commandapi.CommandAPIBukkit.registerCommandNode(CommandAPIBukkit.java:442) ~[CommandAPITest-1.0-SNAPSHOT.jar:?]
        at me.willkroboth.CommandAPITest.commandapi.CommandAPIHandler.register(CommandAPIHandler.java:640) ~[CommandAPITest-1.0-SNAPSHOT.jar:?]
        at me.willkroboth.CommandAPITest.commandapi.AbstractCommandAPICommand.register(AbstractCommandAPICommand.java:301) ~[CommandAPITest-1.0-SNAPSHOT.jar:?]
        at me.willkroboth.CommandAPITest.Main.register(Main.java:43) ~[CommandAPITest-1.0-SNAPSHOT.jar:?]
        at me.willkroboth.CommandAPITest.Main.onEnable(Main.java:26) ~[CommandAPITest-1.0-SNAPSHOT.jar:?]
        at org.bukkit.plugin.java.JavaPlugin.setEnabled(JavaPlugin.java:281) ~[paper-api-1.20.1-R0.1-SNAPSHOT.jar:?]
        at io.papermc.paper.plugin.manager.PaperPluginInstanceManager.enablePlugin(PaperPluginInstanceManager.java:189) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at io.papermc.paper.plugin.manager.PaperPluginManagerImpl.enablePlugin(PaperPluginManagerImpl.java:104) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at org.bukkit.plugin.SimplePluginManager.enablePlugin(SimplePluginManager.java:507) ~[paper-api-1.20.1-R0.1-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.v1_20_R1.CraftServer.enablePlugin(CraftServer.java:612) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at org.bukkit.craftbukkit.v1_20_R1.CraftServer.enablePlugins(CraftServer.java:556) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at net.minecraft.server.MinecraftServer.loadWorld0(MinecraftServer.java:636) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at net.minecraft.server.MinecraftServer.loadLevel(MinecraftServer.java:435) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at net.minecraft.server.dedicated.DedicatedServer.initServer(DedicatedServer.java:308) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at net.minecraft.server.MinecraftServer.runServer(MinecraftServer.java:1103) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at net.minecraft.server.MinecraftServer.lambda$spin$0(MinecraftServer.java:318) ~[paper-1.20.1.jar:git-Paper-"fa50e35"]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]

[JorelAli]

A few minor points to address.

One-liner methods that are only ever called once by one area of code aren't necessary nor needed - just inline the function.

To ensure we don't shoot ourselves in the foot, we should probably state how unregistering a command without using the force parameter will still cause the command to be present but unusable. In the case of "unusable", we should state what happens if they try to run the command (e.g. does it crash? does it throw an exception? does it tell the user the command doesn't exist?). In this case, we should also probably provide a recommendation (e.g. always unregister commands by force).

[willkroboth]

While I was trying to figure out how to document the unregistering process, I noticed some limitations with the current API for Bukkit unregistering. Specifically, it was impossible to:

• Unregister the Bukkit/Plugin version of a command without unregistering the Vanilla version. For example, you couldn't unregister bukkit:reload without also unregistering minecraft:reload. If force was false, the Bukkit command wouldn't be unregistered, and if force was true, the Vanilla version would also be unregistered.
• Unregister only the un-namespaced version of a Bukkit/Plugin command. For example, you couldn't unregister luckperms without also unregistering luckperms:luckperms. If force was false, the Bukkit command wouldn't be unregistered, and if force was true, the namespaced version would also be unregistered.

This happened because CommandAPIBukkit#unregister was doing three things:

• Unregistering Vanilla/CommandAPI commands (those in the Vanilla dispatcher)
• Unregistering Bukkit/Plugin commands (those in the Bukkit CommandMap)
• Unregistering namespaced versions of commands

You can't choose between all the possible combinations there with only one boolean. This is related to the discussion over at #210 (comment).

To fix this, I added a parameter unregisterBukkit for CommandAPIBukkit#unregister. I also renamed force to unregisterNamespaces.

For records sake, this is what CommandAPIBukkit#unregister and the methods around it now look like:

@Override
public void unregister(String commandName, boolean unregisterNamespaces) {
	unregister(commandName, unregisterNamespaces, false);
}

public void unregister(String commandName, boolean unregisterNamespaces, boolean unregisterBukkit) {
	CommandAPI.logInfo("Unregistering command /" + commandName);

	if(!unregisterBukkit) {
		// Remove nodes from the Vanilla dispatcher
		// This dispatcher doesn't usually have namespaced version of commands (those are created when commands
		//  are transferred to Bukkit's CommandMap), but if they ask, we'll do it
		removeBrigadierCommands(getBrigadierDispatcher(), commandName, unregisterNamespaces);

		// Update the dispatcher file
		CommandAPIHandler.getInstance().writeDispatcherToFile();
	}

	if(unregisterBukkit || !CommandAPI.canRegister()) {
		// We need to remove commands from Bukkit's CommandMap if we're unregistering a Bukkit command, or
		//  if we're unregistering after the server is enabled, because `CraftServer#setVanillaCommands` will have
		//  moved the Vanilla command into the CommandMap
		// If we are unregistering a Bukkit command, DO NOT unregister VanillaCommandWrappers
		// If we are unregistering a Vanilla command, ONLY unregister VanillaCommandWrappers

		Map<String, Command> knownCommands = commandMapKnownCommands.get((SimpleCommandMap) paper.getCommandMap());

		boolean isMainVanilla = isVanillaCommandWrapper(knownCommands.get(commandName));
		if(unregisterBukkit ^ isMainVanilla) knownCommands.remove(commandName);

		if(unregisterNamespaces) {
			for (String key : new HashSet<>(knownCommands.keySet())) {
				if(!isThisTheCommandButNamespaced(commandName, key)) continue;

				boolean isVanilla = isVanillaCommandWrapper(knownCommands.get(key));
				if(unregisterBukkit ^ isVanilla) knownCommands.remove(key);
			}
		}
	}

	if(!CommandAPI.canRegister()) {
		// If the server is enabled, we have extra cleanup to do

		// Remove commands from the resources dispatcher
		removeBrigadierCommands(getResourcesDispatcher(), commandName, unregisterNamespaces);

		// Help topics (from Bukkit and CommandAPI) are only setup after plugins enable, so we only need to worry
		//  about removing them once the server is loaded.
		getHelpMap().remove("/" + commandName);

		// Notify players
		for (Player p : Bukkit.getOnlinePlayers()) {
			p.updateCommands();
		}
	}
}

private void removeBrigadierCommands(CommandDispatcher<Source> dispatcher, String commandName, boolean unregisterNamespaces) {
	RootCommandNode<Source> root = dispatcher.getRoot();
	Map<String, CommandNode<?>> children = commandNodeChildren.get(root);
	Map<String, CommandNode<?>> literals = commandNodeLiterals.get(root);
	Map<String, CommandNode<?>> arguments = commandNodeArguments.get(root);

	children.remove(commandName);
	literals.remove(commandName);
	// Commands should really only be represented as literals, but it is technically possible
	// to put an ArgumentCommandNode in the root, so we'll check
	arguments.remove(commandName);

	if (unregisterNamespaces) {
		removeBrigadierCommandNamespace(children, commandName);
		removeBrigadierCommandNamespace(literals, commandName);
		removeBrigadierCommandNamespace(arguments, commandName);
	}
}

private void removeBrigadierCommandNamespace(Map<String, CommandNode<?>> map, String commandName) {
	for(String key : new HashSet<>(map.keySet())) {
		if(isThisTheCommandButNamespaced(commandName, key)) {
			map.remove(key);
		}
	}
}

private static boolean isThisTheCommandButNamespaced(String commandName, String key) {
	return key.contains(":") && key.split(":")[1].equalsIgnoreCase(commandName);
}

No major logic changes. I just needed to separate out the logic that was merged together under force before.

CommandAPI#unregister(String, boolean) is still available. I just renamed force to unregisterNamespaces there too. When that method is called on Bukkit, unregisterBukkit defaults to false. That should mean CommandAPI#unregister have the same behavior as before when being run before the server is enabled.

With these changes though, the original situations I mentioned before are possible like so:

// Unregister `reload` and `bukkit:reload`, but not `minecraft:reload`
CommandAPIBukkit.get().unregister("reload", true, true);

// Unregister `luckperms` but not `luckperms:luckperms`
CommandAPIBukkit.get().unregister("luckperms", false, true);

TODO:

• Add tests for using unregisterBukkit
• Document CommandAPI#unregister behavior

[DerEchtePilz]

Mainly some anchor and typo stuff.
Regarding the anchors, I've written I explained the reason in the initial comment starting this review which I am doing now.

I think I've never really explained what my goal was when refactoring the documentation examples during 9.0.0 development besides being better to find.
By "better to find" I mean something like "I look at a page's link (as this mirrors the file name), find the example I want to edit, find its place (for example I want to edit the first example on a page called x.html) and then look for the example with the anchor [pageName]1 (so for the page x.html the anchor name would be x1) which is placed in an alphabetical order that follows the anchor names."

That results in an order where the examples of the advancement page are pretty much at the top while the examples for tooltips are pretty much at the bottom.

To keep consistency with basically the rest of the documentation I would suggest following the anchor name changes I am suggesting in the relevant comments.

[willkroboth]

Regarding the anchors, I've written I explained the reason in the initial comment starting this review which I am doing now.

Ah, I didn't know about that naming convention, so I came up with my own based on what the example did. I can change it to the numbering 👍

Also thanks for pointing out those typos. Not sure how spellcheck didn't catch unreigster, but I didn't notice either :P

[JorelAli]

That's one hell of a documentation for unregistering commands.

It's long, but the examples are really easy to digest. While normally I'd consider this to be a bit of information-overload, I think the explanation that there is a specific order to how commands are set up in the server loading sequence is informative and helpful in understanding how things work and guiding developers to know what they need to do to achieve the output they want.

The example for the specific edge cases (e.g. Bukkit help and Minecraft namespaced commands only) is helpful and useful.

I'm happy with the result. I think that's everything for this PR, feel free to merge this to dev/dev.

[DerEchtePilz]

Well, a changelog entry is missing but yes, this is definitely ready to be merged.