Add support for custom namespaces for commands

[DerEchtePilz]

This PR adds the ability to register commands with other namespaces than just minecraft:.
However, minecraft: is still the default if no namespace is given.

A bit of works still needs to be done:

• The Kotlin DSL needs to be updated to support this feature
• Documentation
• Velocity needs to be figured out
• Tests need to be figured out
• Global changelog entry
• Example command in CommandAPIMain.java has to be removed
• Test compatibility with Registering commands during server runtime (Bukkit) #417
• Maybe a bit more, depending on what else comes to my mind or something that reviews suggest

New tasks:

• Catch all kinds of edge cases
• Make tests work with namespaces, especially the minecraft: one, everything else works

[willkroboth]

I have a suggestion on how to make this work with Velocity. It requires changing the structure a bit, but I think it can also make the system simpler on Bukkit.

Velocity doesn't do anything to generate namespaces itself and it only uses one CommandDispatcher. So, it probably makes the most sense to create the namespaces immediately when registering the nodes. That'd look something like this:

@Override
public LiteralCommandNode<CommandSource> registerCommandNode(LiteralArgumentBuilder<CommandSource> node, String namespace) {
	LiteralCommandNode<CommandSource> builtNode = getBrigadierDispatcher().register(node);
	getBrigadierDispatcher().register(LiteralArgumentBuilder.<CommandSource>literal(namespace + ":" + node.getLiteral()).redirect(builtNode));
	return builtNode;
}

On Bukkit, the same thing should work. We could create the namespaced versions inside the vanilla CommandDispatcher. We just need to deal with the fact that Bukkit will automatically add the minecraft namespace to any nodes there. I imagine this would look something like this:

List<String> namespacesToFix = new ArrayList<>();

@Override
public LiteralCommandNode<Source> registerCommandNode(LiteralArgumentBuilder<Source> node, String namespace) {
	LiteralCommandNode<Source> builtNode = getBrigadierDispatcher().register(node);
	if(!namespace.equals("minecraft")) {
		String name = node.getLiteral();
		String namespacedName = namespace + ":" + name;
		getBrigadierDispatcher().register(LiteralArgumentBuilder.<Source>literal(namespacedName).redirect(builtNode));
		
		namespacesToFix.add("minecraft:" + name);
		namespacesToFix.add("minecraft:" + namespacedName);
	}
	return builtNode;
}

private void setupNamespaces() {
	Map<String, Command> knownCommands = commandMapKnownCommands.get((SimpleCommandMap) paper.getCommandMap());
	CommandDispatcher<Source> resourcesDispatcher = getResourcesDispatcher();
	for (String command : namespacesToFix) {
		knownCommands.remove(command);
		removeBrigadierCommands(resourcesDispatcher, command, false, c -> true);
	}
}

Here, setupNamespaces (maybe fixNamespaces makes more sense) just needs to remove the minecraft: versions of the commands created by Bukkit. It's also easy to remove those commands from the resources dispatcher ourselves, similar to what CommandAPIBukkit#unregisterInternal does. This way we also don't need to call syncCommands again.

I can't test this myself right now, but I think it should work. I think other platforms should be a lot more like Velocity, with only one CommandDispatcher handling things like default vanilla. Bukkit is just a bit weird having 2 CommandDispatchers and its own CommandMap, so I think it makes sense to treat is differently from the other platforms (similar to fixPermissions).

[DerEchtePilz]

It's also easy to remove those commands from the resources dispatcher ourselves, similar to what CommandAPIBukkit#unregisterInternal does. This way we also don't need to call syncCommands again.

In fact, I did what unregisterInternal does. I double checked any mistakes I may have made. It didn't work until I called syncCommands.
Although, if there is a way to get rid of the extra call to syncCommands, I would be really happy to remove it.
As for the current structure, I think the way it is set up currently makes the most sense. At least, it makes the most sense to me. Again, happy to take suggestions if it generally improves the readability of the code and/or removes some extra stuff I am doing right now which doesn't necessarily need to be there.

[willkroboth]

In fact, I did what unregisterInternal does. It didn't work until I called syncCommands.

Ah, I think the one detail missing from setupNamespaces compared to unregisterInternal is this line of code:

CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

Lines 594 to 598 in 38a1a76

	// Remove commands from the resources dispatcher
	// If we are unregistering a Bukkit command, ONLY unregister BukkitCommandWrappers
	// If we are unregistering a Vanilla command, DO NOT unregister BukkitCommandWrappers
	removeBrigadierCommands(getResourcesDispatcher(), commandName, unregisterNamespaces,
	c -> !unregisterBukkit ^ isBukkitCommandWrapper(c));

... which uses these functions:

CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

Lines 611 to 650 in 38a1a76

	private void removeBrigadierCommands(CommandDispatcher<Source> dispatcher, String commandName,
	boolean unregisterNamespaces, Predicate<CommandNode<Source>> extraCheck) {
	RootCommandNode<?> root = dispatcher.getRoot();
	Map<String, CommandNode<Source>> children = (Map<String, CommandNode<Source>>) commandNodeChildren.get(root);
	Map<String, CommandNode<Source>> literals = (Map<String, CommandNode<Source>>) commandNodeLiterals.get(root);
	Map<String, CommandNode<Source>> arguments = (Map<String, CommandNode<Source>>) commandNodeArguments.get(root);
	removeCommandFromMapIfCheckPasses(children, commandName, extraCheck);
	removeCommandFromMapIfCheckPasses(literals, commandName, extraCheck);
	// Commands should really only be represented as literals, but it is technically possible
	// to put an ArgumentCommandNode in the root, so we'll check
	removeCommandFromMapIfCheckPasses(arguments, commandName, extraCheck);
	if (unregisterNamespaces) {
	removeCommandNamespace(children, commandName, extraCheck);
	removeCommandNamespace(literals, commandName, extraCheck);
	removeCommandNamespace(arguments, commandName, extraCheck);
	}
	}
	private static <T> void removeCommandNamespace(Map<String, T> map, String commandName, Predicate<T> extraCheck) {
	for (String key : new HashSet<>(map.keySet())) {
	if (!isThisTheCommandButNamespaced(commandName, key)) continue;
	removeCommandFromMapIfCheckPasses(map, key, extraCheck);
	}
	}
	private static <T> void removeCommandFromMapIfCheckPasses(Map<String, T> map, String key, Predicate<T> extraCheck) {
	T element = map.get(key);
	if (element == null) return;
	if (extraCheck.test(map.get(key))) map.remove(key);
	}
	private static boolean isThisTheCommandButNamespaced(String commandName, String key) {
	if(!key.contains(":")) return false;
	String[] split = key.split(":");
	if(split.length < 2) return false;
	return split[1].equalsIgnoreCase(commandName);
	}

The important thing happening there is that commands are being removed from what I call the "resources dispatcher". Bukkit copies commands from the vanilla CommandDispatcher into its own CommandMap, which is accessible in the API. Notably, the CommandMap is also used for tab-completing and running console commands. However, the code that sends commands to the player and handles player tab-complete and execution is based on Brigadier. So, Bukkit copies all the commands in the CommandMap into a second CommandDispatcher, the "resources dispatcher".

The syncCommands method is the method Bukkit calls to copy commands from the CommandMap to the "resources dispatcher". If you call it again, I'm pretty sure it does properly handle rebuilding the "resources dispatcher". However, it does process all of the commands from scratch. That's probably fine here since setupNamespaces is only called once, but I wanted to avoid it in unregisterInternal since that could be called many times.

If you do want to avoid calling syncCommands, you'll just need to add and remove same commands from the "resources dispatcher" as you're already doing for the CommandMap. My suggestion makes it so you only need to remove commands from the CommandMap and "resources dispatcher", but it should work either way. My suggestion also avoids the double nested loop over all known commands for all RegisteredCommands to find matches (and searching through the alias list), which could be a lot of work.

CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java

Lines 392 to 402 in caad9b3

	for (RegisteredCommand command : CommandAPI.getRegisteredCommands()) {
	for (String key : knownCommands.keySet()) {
	String commandName = (key.contains(":")) ? key.split(":")[1] : key;
	if (commandName.equals(command.commandName()) || Arrays.asList(command.aliases()).contains(commandName)) {
	Command registeredCommand = knownCommands.get(commandName);
	commandsToAdd.put(commandName, registeredCommand);
	commandsWithPrefix.put(commandName, command.namespace());
	commandsToRemove.add(key);
	}
	}
	}

It's probably fine since setupNamespaces is only called once, but it's just something to think about in combination with how Velocity namespaces will work.

[DerEchtePilz]

Oh, I see.
Yeah, I guess I'll leave it as it is right now.
Once I do the Velocity part I guess I could revisit this as some form of consistency(?)
As you said, it is called only once which is why I am leaving it as it is for now but yeah, it is something to think about.

Also, does Velocity even have namespaces? Commands with namespaces are a Bukkit concept as Brigadier doesn't really know about them. As Velocity is its own project, does it have namespaces? I also briefly tried to get a Velocity plugin's name but couldn't find a way...
Additionally, do CommandAPI commands in Velocity snapshots have namespaces?

[willkroboth]

Yeah, currently, Velocity commands do not have namespaces. Since it's just a proxy, it doesn't include Minecraft server commands like Bukkit does, so I guess they didn't bother with automatically assigning minecraft or plugin namespaces. We can definitely still create namespaced versions of commands ourself if we want to (just put register namespace:command as well), and I think it is helpful in case another plugin tries to use the same command name.

Velocity plugins do have IDs (see https://docs.papermc.io/velocity/dev/api-basics#create-the-plugin-class), which would make sense as the namespace to use for a Velocity Plugin. It looks like you can get that ID given a plugin object with something like this:

CommandAPIVelocity.getConfiguration().getServer().getPluginManager().fromInstance(/* Plugin object here */).get().getDescription().getId();

[DerEchtePilz]

So this is basically done. I need to test compatibility with #417 because when I read it I got worried it would only work with a minecraft: namespace. It's probably fine, I just want to make sure.
As for the tests, I tested this ingame extensively so with confidence I can say it works. Also, I have no idea how to test for a correct namespace registration so I'll leave this to someone who knows more :D

@willkroboth Would be great if you'd review this, after all you made this issue #367 and probably had an idea how it could work.

[DerEchtePilz]

So, this is done. If there's nothing else, we could even say we still put this into 9.3.0.

[willkroboth]

We should definitely add some tests before merging. All the ways of setting namespaces should work. Two commands registered with different namespaces also shouldn't conflict.

Everything should also work the same once these lines are called to simulate the server finishing enabling:

disablePaperImplementations();
assertDoesNotThrow(() -> server.getScheduler().performOneTick());

We should make sure the namespace logic that is triggered after the server is enabled works the same as before enabling. The minecraft: version of commands should also be properly removed from the CommandMap and resources dispatcher.

Those tests can probably be added to CommandRegistrationTests and OnEnableTests. I'd be happy to figure out the OnEnableTests, mostly because they're a big mess that I should probably clean up anyway.

[DerEchtePilz]

Cool, apparently I committed something stupid and now the tests fail. I am not gonna do this today though.

[DerEchtePilz]

We should definitely add some tests before merging. All the ways of setting namespaces should work. Two commands registered with different namespaces also shouldn't conflict.

Everything should also work the same once these lines are called to simulate the server finishing enabling:

disablePaperImplementations();
assertDoesNotThrow(() -> server.getScheduler().performOneTick());

We should make sure the namespace logic that is triggered after the server is enabled works the same as before enabling. The minecraft: version of commands should also be properly removed from the CommandMap and resources dispatcher.

Those tests can probably be added to CommandRegistrationTests and OnEnableTests. I'd be happy to figure out the OnEnableTests, mostly because they're a big mess that I should probably clean up anyway.

Yeah, well, I thought about tests too. It's probably enough to simply test the registration and then whether the correct values have been removed and added by simply invoking the commands.
It should be that easy, right?

[DerEchtePilz]

Alright. I now consider this feature finished. After multiple revisions started by @willkroboth pointing out mistakes I made, I am now confident in saying that this feature works now.

The current caveat is that some test don't succeed but manual test have confirmed that the failing automated tests actually succeed in a real Minecraft environment.

@willkroboth @JorelAli
If one of you or even both of you could do a code review, I believe this PR could be merged until Sunday or so :)

[willkroboth]

No major changes, just some details I noticed in the implementation and wording of various bits and pieces.

I would prefer to resolve all the disabled tests before merging. While it is probably just a problem with the testing framework and doesn't affect a real server, leaving a disabled test without an explanation indicates that we don't understand why the code we wrote is doing what it does.