fix(kord): don't NPE when publishing commands that take arguments - #112
Open
NightmarePog wants to merge 1 commit into
Open
fix(kord): don't NPE when publishing commands that take arguments#112NightmarePog wants to merge 1 commit into
NightmarePog wants to merge 1 commit into
Conversation
CommandNode#command() is only set on nodes that terminate a command, so a root literal has one only when the command is just that literal. Publishing /foo <bar>, or anything with subcommands, blew up in StandardKordCommandFactory while reading the sender type for the default member permission lookup. jda5 got a null check for this in Incendo#60, kord's copy of the same code didn't.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Registering any command whose root literal isn't executable on its own kills the publish:
CommandNode#command()is@MonotonicNonNulland only gets populated on nodes that terminate a command, so a root literal carries one only when the whole command is that literal and nothing else./pingis fine;/warn <user>or a command with subcommands is not. Kotlin sees the getter as a platform type, so nothing complains at compile time and the firstcreateGlobalCommandscall takes the bot's command registration down with it.The same permission lookup landed in jda5 in #60 with a
rootNode.command() != nullguard around it (still there in cloud-jda6). kord's copy never got one. This ports it over.Reproduced and verified against 1.0.0-beta.4 in a bot with
/warn <user>,/clear-chat <amount>and a couple of subcommand groups. Before: publish dies on the first one. After: everything registers.about the test
There was no test source set for cloud-kord, so I added one. To reach
createCommandsI had to widen it fromprivatetointernal— everything above it wants a liveKord, andMultiApplicationCommandBuilder's concrete subclasses are constructible on their own, so this was the cheapest seam. The class itself is alreadyinternal, so no public API moves. Say the word if you'd rather I drop the test and the visibility bump and keep this to the two-line fix.testCreateCommandWithArgumentfails with the NPE above on master and passes with the fix;testCreateCommandWithoutArgumentpasses either way.what this doesn't fix
Commands that take arguments still won't get a default member permission applied, since there's no root command to read a sender type from. I think that's what #91 is about. Resolving it properly means deciding which descendant's sender type a root should answer with, which felt like a different PR.