Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache Bukkit Command when wrapping CommandNodes #11385

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

willkroboth
Copy link
Contributor

Resolves #11378 by "restoring" the Spigot behavior where VanillaCommandNodes are only created once. This allows command frameworks that insert CommandNodes directly into the Brigadier dispatcher to change the permission String of the VanillaCommandNodes created for their commands, rather than it always being the default "minecraft.commands.<name>".

For example, the test plugin described in #11378 can do something like this now:

public final class BukkitTestPlugin extends JavaPlugin {
    @Override
    public void onEnable() {
        getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> {
            Commands commands = event.registrar();
            CommandDispatcher<CommandSourceStack> dispatcher = commands.getDispatcher();

            dispatcher.register(Commands.literal("test")...);

            dispatcher.register(Commands.literal("run")...);

            CommandMap commandMap = Bukkit.getCommandMap();
            commandMap.getCommand("test").setPermission(null);
            commandMap.getCommand("run").setPermission(null);
        });
    }
}

When the /test command is ran using Bukkit#dispatchCommand, Bukkit will now see that its permission is null and allow a Player without any permission to run it, which is the case when the same Player runs the command directly.

Without this PR, BukkitBrigForwardingMap creates a new VanillaCommandWrapper each time a CommandNode is requested via the Bukkit CommandMap. This meant that calls to Command#setPermission did not persist between retrievals from the map.

I can squash this into the Brigadier-based-command-API patch file if desired, but I figured it would be easier initially to review the changes when they are in a separate patch.

Resolves PaperMC#11378 by "restoring" the Spigot behavior where VanillaCommandNodes are only created once. This allows command frameworks that insert CommandNodes directly into the Brigadier dispatcher to change the permission String of the VanillaCommandNodes created for their commands, rather than it always being the default `"minecraft.commands.<name>"`.

Previously, BukkitBrigForwardingMap would create a new VanillaCommandWrapper each time a CommandNode was requested via the Bukkit CommandMap. This means that calls to `Command#setPermission` would not persist between retrievals from the map.
@willkroboth willkroboth requested a review from a team as a code owner September 9, 2024 23:37
@jpenilla
Copy link
Member

It's not possible to properly represent a tree of requirements as a permission string. To solve the linked issue properly we need to reimplement dispatchCommand without using the legacy command map. We could also set the permission to null for plugin commands and override hasPermission(Silent) to improve the legacy representation.

@willkroboth
Copy link
Contributor Author

To solve the linked issue properly we need to reimplement dispatchCommand without using the legacy command map.

Fair point. I imagine Paper will eventually remove the CommandMap after hard forking from Spigot, so it would be useful to get started on that by changing the command dispatch and suggestions logic available in the API to use Brigadier directly.

We could also set the permission to null for plugin commands and override hasPermission(Silent) to improve the legacy representation.

Note that this is indeed the case for commands registered using Commands#register. For those Brigadier commands which are associated with a specific PluginMeta, Paper uses the PluginCommandNode extends LiteralCommandNode class as the root node for the command. When PaperBrigadier#wrapNode is given one of these CommandNode instances, it returns a PluginVanillaCommandWrapper extends VanillaCommandWrapper, which by default has its permission set to null. That means Bukkit#dispatchCommand always skips its permissions check, which allows the CommandNodes to evaluate their requirements themselves.

#11378 only applies to commands registered through Commands.getDispatcher()#register, which places a LiteralCommandNode directly into the dispatcher. These are treated like the default vanilla commands, and so their VanillaCommandWrappers are given the "minecraft.commands.<name>" permission String, causing #11378. This aligns with Spigot's behavior if one uses NMS and reflection to place a CommandNode directly into the Brigadier dispatcher, which is something Brigadier command frameworks might do before Paper's API in #8235. However, the difference between Spigot and Paper that this PR "resolves" is that on Paper, calling Command#setPermission on a VanillaCommandWrapper retrieved from the CommandMap does not persist since Paper creates a new VanillaCommandWrapper each time.

While not a requirement of the Map interface, it is also neat when map.get(a) == map.get(a) for all a, which isn't necessarily true before this PR.

@jpenilla jpenilla self-requested a review September 10, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Awaiting review
2 participants