-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
Argument exceptions2 #476
base: dev/dev
Are you sure you want to change the base?
Argument exceptions2 #476
Conversation
Before I leave the review you requested, here's something I didn't quite understand: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few things I noticed that would (in my opinion) be great if they were changed.
I've also probably missed a lot (or not :D) because I rushed this a bit so I'll probably leave another review soon
commandapi-core/src/main/java/dev/jorel/commandapi/arguments/ExceptionHandlingArgumentType.java
Outdated
Show resolved
Hide resolved
commandapi-core/src/main/java/dev/jorel/commandapi/wrappers/WrapperStringReader.java
Outdated
Show resolved
Hide resolved
Yeah, that's a good question. If the API methods are not called, then the exception will not be intercepted. If an exception handler is not defined, then the original exception will pass through. For the initial parse, an argument node with the CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/CommandAPIHandler.java Lines 816 to 829 in 73b2d12
If the For the argument parse, this code is always run: Lines 60 to 80 in 73b2d12
If the |
...ndapi-bukkit-nms/commandapi-bukkit-1.15/src/main/java/dev/jorel/commandapi/nms/NMS_1_15.java
Outdated
Show resolved
Hide resolved
73b2d12
to
5f6ca11
Compare
Add API to use custom error handling when Argument parsing fails See #370 for the basis of these changes New changes here: - Arguments can only have an ArgumentParseExceptionHandler attached if they implement ArgumentParseExceptionArgument - The substitute value from ArgumentParseExceptionHandler doesn't have to be returned directly - ExceptionInformation can be provided by arguments - New NMS method to extract translation keys from CommandSyntaxExceptions
I'm not really sure why this build failed (https://github.com/JorelAli/CommandAPI/actions/runs/5707823776/job/15468200688), but it is definitely TriFunctions fault >:)
Also, some tweaks to the annotations on implementations for NMS#registerCustomArgumentType
Remove obsolete unused type parameter T from InitialParseExceptionParser Change test classes to default package visibility
…med `Internal`ParseExceptionHandlingArgumentType
…ion logic among Arguments that return numbers
…ts` to `dev.jorel.commandapi.arguments.parseexceptions`
…rgumentType.string()`
…t` rather than `number`
Split each Argument in ArgumentPrimitiveTests into their own classes (Boolean, Double, Float, Integer, Long)
…nContextVerifier` so that ArgumentTests classes can test both types of exceptions
…ifier and InitialParseExceptionContextVerifier to handle shared checking logic on InitialParseExceptionContext and ArgumentParseExceptionContext objects
Also some test javadocs tweaks
e25be0f
to
3c72dde
Compare
I couldn't get the maps to clear while tests were running, but I tried this on a real server and it seemed to work. Commands do need to keep reference their arguments for the parsing step, but if a command is unregistered, its arguments will be removed from the `exceptionHandlers` maps automatically.
This PR remakes #370. The original
argument-exceptions
branch had fallen quite far behind, so instead of trying to rebase that mess, I just re-coded it all here, with a few modifications. I had mostly stopped working on that because I wasn't sure how to solve the problem where developers had to interpret the exception's String to figure out why it was thrown. I think I have finally figured that out, but, I'm getting ahead of myself. There is a lot to explain in this PR before getting to that.(I feel like a lot of the PRs I've been writing lately have too much reading, but it has to happen. I'm partly glad I learned about the
<details>
tag because it makes all this easier to organize, but it also enables me to write too much :P.)What is this?
When using the CommandAPI, there are two parsing steps. First, Brigadier argument nodes use various implementations of
ArgumentType
to turn the command String into Minecraft objects, which are stored into aCommandContext
object. I refer to this as the "initial parse". Once a valid executable command is found, Brigadier runs the CommandAPI's executor. The CommandAPI parses theCommandContext
and creates Bukkit objects, which are stored in aCommandArguments
object. I call this the "argument parse". Once all theArgument
s are processed, the CommandAPI calls the developer's executor.Both of these parsing steps may throw
CommandSyntaxException
s. This PR adds API methods and the necessary backend to catch exceptions in both steps and pass them to the developer if they want to implement special handling. Developers can inspect the information given to them, and use this opportunity to throw a custom exception or supply a replacement value.For example, here are the two test cases I added as proof of concept:
The first example shows an
IntegerArgument
with a minimum value of 0 and a maximum value of 10. Usually, an exception is thrown if the given int is outside the bounds, and this bounds check happens during the initial parse. Instead, an exception handler is used to round the value to the closest valid int. For example, if 20 was given, the parse result would become 10. If -5 was given, 0 would be substituted. If the exception was thrown because there simply wasn't a valid int given, the original exception is re-thrown. While we would prefer the command sender to give us an int between 0 and 10, we can still recover with an exception handler and use a reasonable value instead.The second example is a
ListArgument
that can be given any player on the server. Usually, an exception that saysItem is not allowed in list
is thrown when a String is given that can't be turned into a Player. Instead, an exception handler replaces the exception withCould not find player: (String that wasn't a Player name)
. This exception message makes a lot more sense in this situation, and developers can generally expand upon the exceptions defined by the CommandAPI.How does it work?
The API classes for this are mostly duplicates of each other -- one for the initial parse and again for the argument parse -- so I'll talk about those together. Intercepting exceptions works differently, so that is separated. Here's all the parts that make this work:
ParseExceptionContext records
The records
InitialParseExceptionContext
andArgumentParseExceptionContext
store any available information about the exception, similar toExecutionInfo
for command executors. Those records look like this:Both records store the
CommandSyntaxException
that caused them as aWrapperCommandSyntaxException
so the developer doesn't have to import Brigadier. They also both have anExceptionInformation
object.ExceptionInformation
is a generic type for both classes, and its implementation details will be explained later. It just allows these contexts to be flexible and provide argument-specific information about the exception.InitialParseExceptionContext
also stores aWrapperStringReader
, which wraps Brigadier'sStringReader
in a similar way toWrapperCommandSyntaxException
. ThisStringReader
is the one theArgumentType
was using when the exception failed to parse, which allows developers to inspect the command String around the failed parse.ArgumentParseExceptionContext
has 3 additional fields.sender
is the source of the command,input
is the result of Brigadier's parse, andpreviousArguments
stores the arguments that have already been parsed. Note that since these records exist incommandapi-core
,CommandSender
is a generic type and does not necessarily refer to Bukkit'sCommandSender
.input
also uses the generic typeRaw
, since it depends on the Argument being used.I believe these records sum up all the information that could be given to the developer. However, since they are records, they could easily be expanded in the future to include any other information.
ParseExceptionHandler FunctionalInterfaces
InitialParseExceptionHandler
andArgumentParseExceptionHandler
areFunctionalInterfaces
that developers implement to define their exception handling behavior, similar toCommandExecutor
for command executors. They look like this:The methods are quite simple. They input the appropriate parse exception context, return a substitute value with type
T
, and may throw aWrapperCommandSyntaxException
. There's nothing else to say, other than to highlight how these have one more type parameter than their respective contexts:T
, the type of the substitute value.ParseExceptionArgument interfaces
The final copy-classes are
InitialParseExceptionArgument
andArgumentParseExceptionArgument
. If an Argument wants to support parse exception handlers, it must implement these interfaces. The classes look like this:These classes provide the API methods
with(Initial/Argument)ParseExceptionHandler
that the developer uses to specify their exception handlers. There are also theget(Initial/Argument)ParseExceptionHandler
methods for retrieving the current exception handler, or an emptyOptional
if none was set. These classes add another type parameter over their respective exception handlers: Impl - the return type for chained method calls.These classes have one special method each,
parseInitialParseException
andhandleArgumentParseException
respectively. They are used on the implementation side when catching and handling exceptions, so their implementation details will be discussed later. These methods are not expected to be used by developers.One annoying implementation detail here are the
exceptionHandlers
maps in both classes. These map an instance of the interface to its exception handler. Ideally, there would be an instance variableexceptionHandler
that would do this instead. However, Java doesn't allow instance variables in interfaces. Here are the possible solutions to this problem I've considered:Argument
class. Since Java also doesn't have multi-class inheritance, this doesn't work.(with/get)(Initial/Argument)ParseExceptionHandler
methods could be abstract instead of default. The implementing subclasses can have instance variables, so they could implement these methods using those. This works, but you would have to implement the methods in every subclass. There's like 70+ Arguments in the CommandAPI, so that repetition doesn't really make sense.Argument
. Instead of implementing the methods in every subclass, you could just implement the methods in their command class,Argument
. However, it doesn't really make sense for every argument to have an initial/argument parse handler, since some of them don't throw any errors. Besides, theRaw
andExceptionInformation
type parameters would need to be added toArgument
, which would unnecessarily break code that usedArgument<?>
.So, yeah. This is a little weird. I think that if interfaces can have default methods, they should also have instance variables, but oh well. The map solution is the most code smell, but it is also the least code repetition, which I prefer.
Intercepting initial parse exceptions
So, now that we have an argument with an
InitialParseExceptionHandler
, we need to give it some exceptions to handle. I think this is the hardest part of this PR because you need to inject custom behavior into Brigadier's system. But, it works, and it starts withExceptionHandlingArgumentType
:This class implements Brigadier's
ArgumentType
, so it can be used as the parser on an argument node. One of its parameters isArgumentType<T> baseType
, and this is just theArgumentType
being used by the original argument. ThegetExamples
andlistSuggestions
methods are just passed directly to thisbaseType
since we don't need to modify either of those.When
ExceptionHandlingArgumentType#parse
is called, it can catch anyCommandSyntaxException
thrown when thebaseType
is parsed. That's what allows the CommandAPI to actually intercept the initial parse exceptions. It creates anInitialParseExceptionContext
and handles it with theInitialParseExceptionHandler<T, ExceptionInformation> exceptionHandler
that is active for this argument.In order to create the
ExceptionInformation
object for theInitialParseExceptionContext
, theInitialParseExceptionParser<ExceptionInformation> exceptionParser
parameter is used.InitialParseExceptionParser
is just a simpleFunctionalInterface
that looks like this:It takes in the exception that was thrown and the
StringReader
being used, and gives whatever information it can find. The implementation used for this method depends on the Argument as will be seen later.These
ExceptionHandlingArgumentType
s are added to the command tree whenCommandAPIHandler
builds commands. Specifically, this is managed byCommandAPIHandler#wrapArgumentType
:When
CommandAPIHandler
makes a Brigadier required argument node,wrapArgumentType
will check if the argument has anInitialParseExceptionHandler
. If it does, instead of using the originalArgumentType
for the argument, it will insert anExceptionHandlingArgumentType
with the definedInitialParseExceptionHandler
andIntialParseExceptionParser
.This code also shows a new interface,
WrapperArgument
. This interface represents arguments that wrap other arguments. Right now, the only argument that does that isCustomArgument
.CustomArgument
uses the raw type of another argument, so it should use theInitialParseExceptionHandler
of another argument.CustomArgument
is platform-specific though, soWrapperArgument
just allowscommandapi-core
to get that information, similar to theLiteral
andMultiLiteral
interfaces.Here, you can see that the exception parser comes from the abstract method
parseInitialParseException
. Each implementation ofInitialParseExceptionArgument
will define the generic type parameterExceptionInformation
, and this method defines how to create that information object. This will be explored more in the next section about implementing initial parse exception arguments.The final detail for this section is making this work with Minecraft clients. When a client joins a server, the Commands packet will be sent to inform them about the structure of the server's Brigadier command tree. Each node in the tree is encoded with a specific format, and there are special classes for each
ArgumentType
to handle that conversion. So, in order for the player to receive a tree that uses theExeceptionHandlingArgumentType
, we need to set up its own serializer class. On Bukkit, this is a version-specific thing without an API (shocking :|). TheNMS#registerCustomArgumentType
method was added to deal with this.The implementation details here aren't super important. You can look at the NMS classes for details. Basically, each NMS version has a new
ExceptionHandlingArgumentSerializer_(VERSION)
(1.15 to 1.18) orExceptionHandlingArgumentInfo_(VERSION)
(1.19+) class that handles formatting our custom argument type into packet. Unfortunately, since the client doesn't know about theExceptionHandlingArgumentType
, it can't handle our nodes and would end up disconnecting with an error. To get around this, the argument serializers remove their own data and insert the data for the baseArgumentType
instead. So, when anExceptionHandlingArgumentType
is wrapped around a node on the server, the client doesn't actually see any changes.Implementing new InitialParseExceptionArguments
In order for an
Argument
to have anInitialParseExceptionHandler
, it needs to implementInitialParseExceptionArgument
. For the proof of concept, I madeIntegerArgument
anInitialParseExceptionArgument
, and it now looks like this:Remember that the generic types for
InitialParseExceptionArgument
are<T, ExceptionInformation, Impl>
. TheIntegerArgument
gives<Integer, IntegerArgument.InitialParseExceptionInformation, Argument<Integer>>
. So, each of these mean:Integer
.IntegerArgument.InitialParseExceptionInformation
.Argument<Integer>
.InitialParseExceptionArgument
also needs an implementation for its abstract methodExceptionInformation parseInitialParseException(CommandSyntaxException exception, StringReader reader)
, which interprets why the parse exception was thrown by Brigadier. As defined by the type parameter,IntegerArgument
's exception information is stored in the inner recordInitialParseExceptionInformation
. This record store 5 pieces of information:Exceptions type
- the type of exception that was thrown.Exceptions
is an enum with the valuesEXPECTED_INTEGER
,INVALID_INTEGER
,INTEGER_TOO_LOW
, andINTEGER_TOO_HIGH
, which are the 4 reasons why Brigadier might fail to parse andIntegerArgumentType
.String rawInput
- The string given in the command that represents this argument. This will be empty when the exception type isEXPECTED_INTEGER
, because that exception happens when no number was given.int input
- The int given in the command for this argument. This will default to 0 when the exception type isEXPECTED_INTEGER
orINVALID_INTEGER
, since those exceptions happen when the given String could not be parsed as an int.int minimum
- The minimum value set for thisIntegerArgument
.int maximum
- The maximum value set for thisIntegerArgument
.IntegerArgument#parseInitialParseException(CommandSyntaxException, StringReader)
is responsible for extracting these 5 pieces of information.String rawInput
can be found using theStringReader
,int input
comes from evaluatingInteger.parseInt(rawInput)
, andminimum
andmaximum
are stored by the reference toIntegerArgumentType
stored inArgument
when theIntegerArgument
was constructed.Figuring out the proper value to put for
Exceptions type
is a bit trickier. All there is to go off is theCommandSyntaxException
parameter. Luckily, all the builtin exception messages are translatable, so they have a consistent translation key. However, the classes involved are NMS, and the component structure changed in 1.19, so the methodNMS#extractTranslationKey
was added to deal with this. OnceIntegerArgument
gets the translation key, a simpleswitch
statement finds the properExeceptions
value for the information record.Intercepting argument parse exceptions and Implementing new ArgumentParseExceptionArguments
Since the argument parse is handled by CommandAPI code, it is much easier to intercept the exceptions and extract their data. An argument that implements
ArgumentParseExceptionArgument
simply needs to modify its parsing code to use the exception handler. So, this section covers the exception interception and argument implementation. For the proof of concept, I madeListArgumentCommon
anArgumentParseExceptionArgument
, and it now looks like this:Remember that the generic types for
InitialParseExceptionArgument
are<T, Raw, ExceptionInformation, Impl, CommandSender>
. TheListArgumentCommon
gives<T, String, ListArgumentCommon.ArgumentParseExceptionInformation<T>, Argument<List>, CommandSender>
. So, each of these mean:ArgumentParseExceptionHandler
isT
. List arguments returnList<T>
when parsed, so this exception handler actually works on each item in the list, which we'll see later.String
. The list argument either uses a greedy string or text string. MostArgumentType
s return an NMS object, but in this case we can pass the raw String to the developer.ListArgumentCommon.ArgumentParseExceptionInformation<T>
.Argument<List>
.CommandSender
.As defined by the type parameter, a list argument's exception information is stored in the inner record
ArgumentParseExceptionInformation
. This record store 3 pieces of information:Exceptions type
- the type of exception that was thrown.Exceptions
is an enum with the valuesDUPLICATES_NOT_ALLOWED
andINVALID_ITEM
, which are the 2 reasons a list argument may fail to parse.List<T> listSoFar
- A list of the items that have already been parsed.String currentItem
- The current string that was being parsed.Since we control the code that throws argument parse exceptions, this information is easy to get. In the two places that
ListArgumentCommon#parseArgument
used to throw an exception, a newArgumentParseExceptionInformation
is created with the relevant information and handled.Something interesting to note about the list argument is that exceptions are only thrown for individual items. This means that instead of substituting the entire list when an exception happens, each individual item can be substituted. So, in the code, the result of the exception handling is passed into
List#add
. If an exception is thrown it will still bubble up, but a single item with typeT
can also be substituted.To handle the exception, the default method
handleArgumentParseException
provided byArgumentParseExceptionArgument
is used. I skipped over the details here before, but this is what that method looks like:If this argument does not have an exception handler, the original exception is immediately thrown, keeping the old behavior. If there is a handler, then the
ArgumentParseExceptionContext
is constructed and passed to the handler. The handler may return a substitute value, or it can throw aWrapperCommandSyntaxException
that is unwrapped and thrown.This method mostly helps construct the
ArgumentParseExceptionContext
. The extraCommandContext<Source> cmdCtx
,String key
, andCommandArguments previousArgs
parameters of the method are used for this purpose.So, that took a long time to explain. Definitely ask questions and leave code review. As you can see with the TODO list below, there many things I still want to work on. However, I think I'm done with most of the main systems, so feedback on the API and backend would still be great.
TODO
(Initial/Argument)ParseExceptionArguments
ListTextArgument
)ArgumentParseExceptionHandler
(Initial/Argument)ParseExceptionArguments
CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-nms/commandapi-bukkit-1.19-common/src/main/java/dev/jorel/commandapi/nms/ExceptionHandlingArgumentInfo_1_19_Common.java
Line 13 in 73b2d12
CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/arguments/ArgumentParseExceptionArgument.java
Lines 32 to 34 in 73b2d12
CommandAPI/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-nms/commandapi-bukkit-1.15/src/main/java/dev/jorel/commandapi/nms/ExceptionHandlingArgumentSerializer_1_15.java
Lines 16 to 18 in 73b2d12
Test NMS code on real servers
Maybe todo