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

Exceptions from custom Brigadier commands do not show up in console #9808

Closed
willkroboth opened this issue Oct 7, 2023 · 3 comments
Closed
Labels
status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. type: bug Something doesn't work as it was intended to. version: 1.20.2 Game version 1.20.2

Comments

@willkroboth
Copy link
Contributor

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:

@@ -325,7 +327,16 @@ public class Commands {
b1 = 0;
return b1;
} catch (CommandSyntaxException commandsyntaxexception) {
- commandlistenerwrapper.sendFailure(ComponentUtils.fromMessage(commandsyntaxexception.getRawMessage()));
+ // Paper start
+ final net.kyori.adventure.text.TextComponent.Builder builder = net.kyori.adventure.text.Component.text();
+ if ((parseresults.getContext().getNodes().isEmpty() || !this.vanillaCommandNodes.contains(parseresults.getContext().getNodes().get(0).getNode()))) {
+ if (!org.spigotmc.SpigotConfig.unknownCommandMessage.isEmpty()) {
+ builder.append(net.kyori.adventure.text.serializer.legacy.LegacyComponentSerializer.legacySection().deserialize(org.spigotmc.SpigotConfig.unknownCommandMessage));
+ }
+ } else {
+ // commandlistenerwrapper.sendFailure(ComponentUtils.fromMessage(commandsyntaxexception.getRawMessage()));
+ builder.color(net.kyori.adventure.text.format.NamedTextColor.RED).append(io.papermc.paper.brigadier.PaperBrigadier.componentFromMessage(commandsyntaxexception.getRawMessage()));
+ // Paper end
if (commandsyntaxexception.getInput() != null && commandsyntaxexception.getCursor() >= 0) {
int j = Math.min(commandsyntaxexception.getInput().length(), commandsyntaxexception.getCursor());
MutableComponent ichatmutablecomponent = Component.empty().withStyle(ChatFormatting.GRAY).withStyle((chatmodifier) -> {

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:

@@ -530,6 +530,7 @@ public final class CraftServer implements Server {
}
node = clone;
}
+ dispatcher.vanillaCommandNodes.add(node); // Paper
dispatcher.getDispatcher().getRoot().addChild(node);

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

@willkroboth willkroboth added status: needs triage type: bug Something doesn't work as it was intended to. labels Oct 7, 2023
@papermc-projects papermc-projects bot moved this to 🕑 Needs Triage in Issues: Bugs Oct 7, 2023
@Warriorrrr Warriorrrr added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. and removed status: needs triage labels Nov 22, 2023
@papermc-projects papermc-projects bot moved this from 🕑 Needs Triage to ✅ Accepted in Issues: Bugs Nov 22, 2023
@Owen1212055
Copy link
Member

Are you able to see, is this fixed by #8235?

@willkroboth
Copy link
Contributor Author

I can't use the original test plugin, since the changes made by #8235 are currently incompatible with the CommandAPI (CommandAPI/CommandAPI#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

@kashike kashike added the version: 1.20.2 Game version 1.20.2 label May 13, 2024
@willkroboth
Copy link
Contributor Author

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.

@github-project-automation github-project-automation bot moved this from ✅ Accepted to Done in Issues: Bugs Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. type: bug Something doesn't work as it was intended to. version: 1.20.2 Game version 1.20.2
Projects
Status: Done
Development

No branches or pull requests

4 participants