Made NullCommandSender not throw an exception on Paper #580

SB2DD · 2024-07-21T21:15:47Z

I basically realized that when trying to use a non-vanilla command on Paper in minecraft functions with CommandAPI (I know, pretty specific), I would get a RybtuneException thrown and the function file wouldn't load

[16:50:49 ERROR]: Failed to load function code:load
java.util.concurrent.CompletionException: java.lang.RuntimeException: Failed to wrap CommandSender io.papermc.paper.brigadier.NullCommandSender@dcfd0b2 to a CommandAPI-compatible BukkitCommandSender
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[?:?]
	at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320) ~[?:?]
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1770) ~[?:?]
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) ~[?:?]
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) ~[?:?]
	at java.base/java.lang.Thread.run(Thread.java:1583) ~[?:?]
Caused by: java.lang.RuntimeException: Failed to wrap CommandSender io.papermc.paper.brigadier.NullCommandSender@dcfd0b2 to a CommandAPI-compatible BukkitCommandSender
	at CommandAPI-9.5.1.jar/dev.jorel.commandapi.CommandAPIBukkit.wrapCommandSender(CommandAPIBukkit.java:411) ~[CommandAPI-9.5.1.jar:?]
	at CommandAPI-9.5.1.jar/dev.jorel.commandapi.nms.NMS_Common.getCommandSenderFromCommandSource(NMS_Common.java:370) ~[CommandAPI-9.5.1.jar:?]
	at CommandAPI-9.5.1.jar/dev.jorel.commandapi.nms.NMS_Common.getCommandSenderFromCommandSource(NMS_Common.java:91) ~[CommandAPI-9.5.1.jar:?]
	at CommandAPI-9.5.1.jar/dev.jorel.commandapi.CommandAPIHandler.lambda$generatePermissions$1(CommandAPIHandler.java:367) ~[CommandAPI-9.5.1.jar:?]
	at com.mojang.brigadier.tree.CommandNode.canUse(CommandNode.java:81) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
	at com.mojang.brigadier.CommandDispatcher.parseNodes(CommandDispatcher.java:302) ~[paper-1.20.6.jar:?]
	at com.mojang.brigadier.CommandDispatcher.parse(CommandDispatcher.java:292) ~[paper-1.20.6.jar:?]
	at net.minecraft.commands.functions.CommandFunction.parseCommand(CommandFunction.java:103) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
	at net.minecraft.commands.functions.CommandFunction.fromLines(CommandFunction.java:84) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
	at net.minecraft.server.ServerFunctionLibrary.lambda$reload$2(ServerFunctionLibrary.java:91) ~[paper-1.20.6.jar:1.20.6-148-20f5165]
	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768) ~[?:?]
	... 3 more

After doing some digging, I found where that Exception was thrown, and realized what was happening. Looking at nms code, Minecraft loads functions with this NullCommandSender (but when they are called they do use the correct sender). This PR includes a simple test that avoids the exception by throwing null (which is only used by the CommandAPIHandler.permissionCheck(..) method, which already supports a null sender.) This fixed my issue.

(I couldn't replicate my issue on Spigot, so this fix being specific to paper is probably fine)

Tested with PaperMC 1.20.6 (latest at the time) and CommandAPI 9.5.1.

willkroboth

Looking at the Paper source, yep, this is a new Paper thing. Their Brigadier command API PR (PaperMC/Paper#8235) created the NullCommandSender class and changed the Null CommandSource Minecraft uses for parsing functions to use that sender. So, this change should only be necessary on paper-1.20.6-65 and later. I confirmed that CommandAPI 9.5.1 throws the RuntimeException on Paper 1.20.6 and 1.21, but not on paper-1.20.6-64. This PR fixes the issue, so the function loads and runs CommandAPI commands successfully.

Spigot 1.20.6 and 1.21 can successfully load a datapack that contains CommandAPI commands. I was a bit confused about how this was possible, given that on Spigot, instead of returning a NullCommandSender, the same method throws a UnsupportedOperationException, which I expected to bubble up. What I didn't realize is that the CommandAPI actually catches this exception and returns null:

https://github.com/JorelAli/CommandAPI/blob/d130bdee6647cb046fd326058b96cedfa56de59d/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-nms/commandapi-bukkit-nms-common/src/main/java/dev/jorel/commandapi/nms/NMS_Common.java#L367-L374

Looks like we already had some logic I didn't know about to deal with the special command source used when parsing functions, but Paper did thier change and now we need extra, special logic. I never thought about what that bit of code was supposed to do before :P. I was a bit skeptical when I saw your code return null from wrapCommandSender, but that seems to be the correct choice given that is what happens on Spigot.

So, uh, long story short, good changes 👍. Just the one comment.

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

Co-authored-by: willkroboth <46540330+willkroboth@users.noreply.github.com>

SB2DD · 2024-07-22T01:42:38Z

Great! Thanks so much for the feedback!

Note: This commit makes this branch backwards incompatible, especially for non-DSL Kotlin code (see `commandapi-documentation-code/.../Examples.kt`). "Standard" Java and DSL API code usage looks the same, but I'm pretty sure plugins will need to recompile due to changes to the `FunctionalInterface`s. There are also some smaller public API changes, mentioned below. Notable changes: - Removed `AbstractCommandSender` and all its implemenations (i.e. the `dev.jorel.commandapi.commandsenders` package is completely gone) - Logic in `CommandAPIHandler#generateBrigadierRequirements` for checking if a sender satisfies a `CommandPermission` was moved to `CommandAPIPlatform#getPermissionCheck` - Previously, methods in `AbstractCommandSender` provided access to `hasPermission` and `isOp`. `CommandAPIBukkit` and `CommandAPIVelocity` now handle these definitions. - `CommandPermission.TRUE()` and `CommandPermission.FALSE()` added for computing short circuits when combining permissions and requirements - `PreviewInfo` now has the generic parameter `Player` rather than an `AbstractPlayer` - Backwards-incompatible (`(Player) player.getSource()` becomes `player`) - Generic parameter propogates to `PreviewableFunction` and `Previewable` - `CommandAPIPlatform` methods for convert Brigadier Source to CommandSender simplified - `getSenderForCommand` removed - just pass `CommandContext#getSource` into `getCommandSenderFromCommandSource` - `forceNative` parameter was only used on Bukkit, which is now handled by `NMS#getNativeProxyCommandSender` (more on that below) - `wrapCommandSender` removed - Logic from #580 removed, since wrapping the NullCommandSender no longer needs to be handled as a special case - Wrapping the sender no longer necessary for `getBrigadierSourceFromCommandSender` - `CommandAPIVelocity` now does nothing in these methods :P - `CommandAPIExecutor` reworked - `ExecutorType` moved to platform modules (so Velocity can no longer try to define sender types that don't exist) - Detecting whether a certain executor can be run by the given class delegated to `BukkitTypedExecutor` and `VelocityTypedExecutor` - Priority of executors now depends on order of calling the `executes` methods (i.e the priority listed here https://commandapi.jorel.dev/9.4.1/normalexecutors.html#multiple-command-executor-implementations no longer applies) - Normal executors are no longer ignored if resulting executors exist (not sure if this was a bug or intended) - Tweaked `ExecutionInfo` - Added `CommandContext<Source> cmdCtx` to `ExecutionInfo` - This allows passing the information needed for a `NATIVE` executor on Bukkit to create a `NativeProxyCommandSender` - Uses `NMS#getNativeProxyCommandSender`, which was adapted from the removed `getSenderForCommand` method - Note: conflicts with #478, though should be easily resolved - Note: Velocity can define the `CommandSource` object, though Bukkit has it as `?`. This makes it hard for non DSL Kotlin to infer the type parameters for some reason, which is annoying because you now need to specify the type parameter. I don't know if there's a better way to handle that. - TODO: Make sure depdendents can still use `ExecutionInfo` without including Brigadier as a dependency - `ExecutionInfo` is now a record (`BukkitExecutionInfo` and `VelocityExecutionInfo` removed) - Simplified `dev.jorel.commandapi.executors` package - Platform and sender specific executor classes (e.g. `PlayerCommandExecutor` and `EntityResultingExecutionInfo`) removed - Just the functional interfaces `NormalExecutorInfo`, `NormalExecutor`, `ResultingExecutorInfo`, and `ResultingExecutor` now - `BukkitExecutable` and `VelocityExecutable` link different command sender classes as type parameters to the `ExecutorType` enum values TODO: - Add executor tests to `dev/dev` to ensure same behavior - Especially for `PROXY` and `NATIVE` senders - Especially for non-DSL Kotlin to see how its lamdba type inference works - Update documentation

Note: This commit makes this branch backwards incompatible, especially for non-DSL Kotlin code (see `commandapi-documentation-code/.../Examples.kt`). "Standard" Java and DSL API code usage looks the same, but I'm pretty sure plugins will need to recompile due to changes to the `FunctionalInterface`s. There are also some smaller public API changes, mentioned below. Notable changes: - Removed `AbstractCommandSender` and all its implemenations (i.e. the `dev.jorel.commandapi.commandsenders` package is completely gone) - Logic in `CommandAPIHandler#generateBrigadierRequirements` for checking if a sender satisfies a `CommandPermission` was moved to `CommandAPIPlatform#getPermissionCheck` - Previously, methods in `AbstractCommandSender` provided access to `hasPermission` and `isOp`. `CommandAPIBukkit` and `CommandAPIVelocity` now handle these definitions. - `CommandPermission.TRUE()` and `CommandPermission.FALSE()` added for computing short circuits when combining permissions and requirements - `PreviewInfo` now has the generic parameter `Player` rather than an `AbstractPlayer` - Backwards-incompatible (`(Player) player.getSource()` becomes `player`) - Generic parameter propogates to `PreviewableFunction` and `Previewable` - `CommandAPIPlatform` methods for convert Brigadier Source to CommandSender simplified - `getSenderForCommand` removed - just pass `CommandContext#getSource` into `getCommandSenderFromCommandSource` - `forceNative` parameter was only used on Bukkit, which is now handled by `NMS#getNativeProxyCommandSender` (more on that below) - `wrapCommandSender` removed - Logic from #580 removed, since wrapping the NullCommandSender no longer needs to be handled as a special case - Wrapping the sender no longer necessary for `getBrigadierSourceFromCommandSender` - `CommandAPIVelocity` now does nothing in these methods :P - `CommandAPIExecutor` reworked - `ExecutorType` moved to platform modules (so Velocity can no longer try to define sender types that don't exist) - Detecting whether a certain executor can be run by the given class delegated to `BukkitTypedExecutor` and `VelocityTypedExecutor` - Priority of executors now depends on order of calling the `executes` methods (i.e the priority listed here https://commandapi.jorel.dev/9.4.1/normalexecutors.html#multiple-command-executor-implementations no longer applies) - Normal executors are no longer ignored if resulting executors exist (not sure if this was a bug or intended) - Tweaked `ExecutionInfo` - Added `CommandContext<Source> cmdCtx` to `ExecutionInfo` - This allows passing the information needed for a `NATIVE` executor on Bukkit to create a `NativeProxyCommandSender` - Uses `NMS#getNativeProxyCommandSender`, which was adapted from the removed `getSenderForCommand` method - Note: conflicts with #478, though should be easily resolved - Note: Velocity can define the `CommandSource` object, though Bukkit has it as `?`. This makes it hard for non DSL Kotlin to infer the type parameters for some reason, which is annoying because you now need to specify the type parameter. I don't know if there's a better way to handle that. - TODO: Make sure depdendents can still use `ExecutionInfo` without including Brigadier as a dependency - `ExecutionInfo` is now a record (`BukkitExecutionInfo` and `VelocityExecutionInfo` removed) - Simplified `dev.jorel.commandapi.executors` package - Platform and sender specific executor classes (e.g. `PlayerCommandExecutor` and `EntityResultingExecutionInfo`) removed - Just the functional interfaces `NormalExecutorInfo`, `NormalExecutor`, `ResultingExecutorInfo`, and `ResultingExecutor` now - `BukkitExecutable` and `VelocityExecutable` link different 8000 command sender classes as type parameters to the `ExecutorType` enum values TODO: - Add executor tests to `dev/dev` to ensure same behavior - Especially for `PROXY` and `NATIVE` senders - Especially for non-DSL Kotlin to see how its lamdba type inference works - Update documentation

Note: This commit makes this branch backwards incompatible, especially for non-DSL Kotlin code (see `commandapi-documentation-code/.../Examples.kt`). "Standard" Java and DSL API code usage looks the same, but I'm pretty sure plugins will need to recompile due to changes to the `FunctionalInterface`s. There are also some smaller public API changes, mentioned below. Notable changes: - Removed `AbstractCommandSender` and all its implemenations (i.e. the `dev.jorel.commandapi.commandsenders` package is completely gone) - Logic in `CommandAPIHandler#generateBrigadierRequirements` for checking if a sender satisfies a `CommandPermission` was moved to `CommandAPIPlatform#getPermissionCheck` - Previously, methods in `AbstractCommandSender` provided access to `hasPermission` and `isOp`. `CommandAPIBukkit` and `CommandAPIVelocity` now handle these definitions. - `CommandPermission.TRUE()` and `CommandPermission.FALSE()` added for computing short circuits when combining permissions and requirements - `PreviewInfo` now has the generic parameter `Player` rather than an `AbstractPlayer` - Backwards-incompatible (`(Player) player.getSource()` becomes `player`) - Generic parameter propogates to `PreviewableFunction` and `Previewable` - `CommandAPIPlatform` methods for convert Brigadier Source to CommandSender simplified - `getSenderForCommand` removed - just pass `CommandContext#getSource` into `getCommandSenderFromCommandSource` - `forceNative` parameter was only used on Bukkit, which is now handled by `NMS#getNativeProxyCommandSender` (more on that below) - `wrapCommandSender` removed - Logic from #580 removed, since wrapping the NullCommandSender no longer needs to be handled as a special case - Wrapping the sender no longer necessary for `getBrigadierSourceFromCommandSender` - `CommandAPIVelocity` now does nothing in these methods :P - `CommandAPIExecutor` reworked - `ExecutorType` moved to platform modules (so Velocity can no longer try to define sender types that don't exist) - Detecting whether a certain executor can be run by the given class delegated to `BukkitTypedExecutor` and `VelocityTypedExecutor` - Priority of executors now depends on order of calling the `executes` methods (i.e the priority listed here https://commandapi.jorel.dev/9.4.1/normalexecutors.html#multiple-command-executor-implementations no longer applies) - Normal executors are no longer ignored if resulting executors exist (not sure if this was a bug or intended) - Tweaked `ExecutionInfo` - Added `CommandContext<Source> cmdCtx` to `ExecutionInfo` - This allows passing the information needed for a `NATIVE` executor on Bukkit to create a `NativeProxyCommandSender` - Uses `NMS#getNativeProxyCommandSender`, which was adapted from the removed `getSenderForCommand` method - Note: conflicts with #478, though should be easily resolved - Note: Velocity can define the `CommandSource` object, though Bukkit has it as `?`. This makes it hard for non DSL Kotlin to infer the type parameters for some reason, which is annoying because you now need to specify the type parameter. I don't know if there's a better way to handle that. - TODO: Make sure depdendents can still use `ExecutionInfo` without including Brigadier as a dependency - `ExecutionInfo` is now a record (`BukkitExecutionInfo` and `VelocityExecutionInfo` removed) - Simplified `dev.jorel.commandapi.executors` package - Platform and sender specific executor classes (e.g. `PlayerCommandExecutor` and `EntityResultingExecutionInfo`) removed - Just the functional interfaces `NormalExecutorInfo`, `NormalExecutor`, `ResultingExecutorInfo`, and `ResultingExecutor` now - `BukkitExecutable` and `VelocityExecutable` link different command sender classes as type parameters to the `ExecutorType` enum values TODO: - Add executor tests to `dev/dev` to ensure same behavior - Especially for `PROXY` and `NATIVE` senders - Especially for non-DSL Kotlin to see how its lamdba type inference works - Update documentation

SB2DD added 2 commits July 20, 2024 13:50

Made NullCommandSender not throw an exception on Paper

47aa428

Merge branch 'dev/dev' into fix/nullcommandsender-exception

eca31b7

willkroboth self-requested a review July 21, 2024 21:51

willkroboth reviewed Jul 22, 2024

View reviewed changes

...ndapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/CommandAPIBukkit.java Outdated Show resolved Hide resolved

Fixed possible NPE for NullCommandSender

b521185

Co-authored-by: willkroboth <46540330+willkroboth@users.noreply.github.com>

willkroboth approved these changes Jul 22, 2024

View reviewed changes

willkroboth mentioned this pull request Jul 24, 2024

Error when registering datapacks #582

Closed

willkroboth merged commit b440802 into CommandAPI:dev/dev Jul 25, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Made NullCommandSender not throw an exception on Paper #580

Made NullCommandSender not throw an exception on Paper #580

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Made NullCommandSender not throw an exception on Paper #580

Made NullCommandSender not throw an exception on Paper #580

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!