Exceptions from custom Brigadier commands do not show up in console

willkroboth commented 1 year ago

Expected behavior

On Spigot, if a custom Brigadier command throws a CommandSyntaxException, that exception can appear in the console:

>version
[11:26:28] [Server thread/INFO]: This server is running CraftBukkit version 3908-Spigot-e0e223f-b3efca5 (MC: 1.20.2) (Implementing API version 1.20.2-R0.1-SNAPSHOT)
[11:26:28] [Server thread/INFO]: Checking version, please wait...
[11:26:29] [Thread-12/INFO]: You are running the latest version
>test @p
[11:26:31] [Server thread/INFO]: This is a custom exception message: No players were found

Observed/Actual behavior

On Paper, instead of showing the exception message, the Unknown command message is given:

> version
[11:27:59 INFO]: Checking version, please wait...
[11:27:59 INFO]: This server is running Paper version git-Paper-217 (MC: 1.20.2) (Implementing API version 1.20.2-R0.1-SNAPSHOT) (Git: cfe311d)
You are running the latest version
> test @p
[11:28:09 INFO]: Unknown command. Type "/help" for help.

Steps/models to reproduce

I am using the CommandAPI plugin (currently version 9.2.0) to register custom Brigadier commands. You can download the latest jar from here: https://commandapi.jorel.dev/.

Using the CommandAPI, I registered this command:

@Override
public void onEnable() {
    new CommandAPICommand("test")
            .withArguments(new EntitySelectorArgument.ManyPlayers("players"))
            .executes((sender, args) -> {
                List<Player> players = args.getUnchecked(0);

                if(players.isEmpty()) {
                    throw CommandAPI.failWithString("This is a custom exception message: No players were found");
                }

                sender.sendMessage(players.size() + " player(s) were found");
            })
            .register();
}

Here is my TestPlugin that uses this code TestPlugin-1.0-SNAPSHOT.zip. Alternatively, you can build my TestPlugin yourself with this maven project: TestPluginProject.zip.

To reproduce this issue, run the test command in the console, giving it a player selector that will not find any players. For example, test @p when there are no players connected.

Note that in the Spigot console, or for a player, when the command fails the This is a custom exception message: No players were found message will appear. However, the Paper console will say Unknown command. Type "/help" for help. when the command fails.

Plugin and Datapack List

> plugins
[11:39:29 INFO]: Server Plugins (2):
[11:39:29 INFO]: Bukkit Plugins:
[11:39:29 INFO]:  - CommandAPI, TestPlugin
> datapack list
[11:39:38 INFO]: There are 2 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)]
[11:39:38 INFO]: There are no more data packs available

Paper version

> version
[11:40:05 INFO]: Checking version, please wait...
[11:40:06 INFO]: This server is running Paper version git-Paper-217 (MC: 1.20.2) (Implementing API version 1.20.2-R0.1-SNAPSHOT) (Git: cfe311d)
You are running the latest version

Other

I believe the inconsistency here between Spigot and Paper comes from this patch:

https://github.com/PaperMC/Paper/blob/9df2066642d32a1f7a372ce528930d48f9fe51f7/patches/server/0140-Add-UnknownCommandEvent.patch#L47-L64

Specifically, line 54, where if the statement is true, it will send the Unknown command message

if ((parseresults.getContext().getNodes().isEmpty() || !this.vanillaCommandNodes.contains(parseresults.getContext().getNodes().get(0).getNode()))) {

I think the semantics here are wrong. Instead, it should just be:

if (parseresults.getContext().getNodes().isEmpty()) {

If the parse results are empty, then the Unknown command message should be sent. There's no reason to check if the node it found is in the vanillaCommandNodes list. If it found a node, then the command is known.

I'm also not really sure why the vanillaCommandNodes list exists. The value should be equivalent to this.dispatcher.getRoot().getChildren(). Relying on the dispatcher would also be safer, since code doesn't need to worry about updating two lists. For example, this code in CraftServer:

https://github.com/PaperMC/Paper/blob/9df2066642d32a1f7a372ce528930d48f9fe51f7/patches/server/0140-Add-UnknownCommandEvent.patch#L89-L95

Instead of having to call dispatcher.vanillaCommandNodes.add(node), dispatcher.getDispatcher().getRoot().addChild(node) already updates this.dispatcher.getRoot().getChildren().

Owen1212055 commented 6 months ago

Are you able to see, is this fixed by https://github.com/PaperMC/Paper/pull/8235?

willkroboth commented 6 months ago

I can't use the original test plugin, since the changes made by #8235 are currently incompatible with the CommandAPI (https://github.com/JorelAli/CommandAPI/issues/554).

However, using #8235's API like so:

@Override
public void onEnable() {
    getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> {
        event.registrar().register(
                Commands.literal("test")
                        .executes(context -> {
                            throw new SimpleCommandExceptionType(() -> "Custom exception message").create();
                        })
                        .build()
        );
    });
}

I am able to see the exception in the console

Exception

and it looks to work the same as with the client

Client exception

willkroboth commented 2 months ago

I tested the original TestPlugin with an updated CommandAPI plugin that works with #8235, and verified that the orginal comment of this issue has been resolved.

PaperMC / Paper