Conversation
|
Before I leave the review you requested, here's something I didn't quite understand: |
DerEchtePilz
left a comment
There was a problem hiding this comment.
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
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 If the For the argument parse, this code is always run: If the |
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-exceptionsbranch 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
ArgumentTypeto turn the command String into Minecraft objects, which are stored into aCommandContextobject. I refer to this as the "initial parse". Once a valid executable command is found, Brigadier runs the CommandAPI's executor. The CommandAPI parses theCommandContextand creates Bukkit objects, which are stored in aCommandArgumentsobject. I call this the "argument parse". Once all theArguments are processed, the CommandAPI calls the developer's executor.Both of these parsing steps may throw
CommandSyntaxExceptions. 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
IntegerArgumentwith 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
ListArgumentthat can be given any player on the server. Usually, an exception that saysItem is not allowed in listis 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
InitialParseExceptionContextandArgumentParseExceptionContextstore any available information about the exception, similar toExecutionInfofor command executors. Those records look like this:Both records store the
CommandSyntaxExceptionthat caused them as aWrapperCommandSyntaxExceptionso the developer doesn't have to import Brigadier. They also both have anExceptionInformationobject.ExceptionInformationis 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.InitialParseExceptionContextalso stores aWrapperStringReader, which wraps Brigadier'sStringReaderin a similar way toWrapperCommandSyntaxException. ThisStringReaderis the one theArgumentTypewas using when the exception failed to parse, which allows developers to inspect the command String around the failed parse.ArgumentParseExceptionContexthas 3 additional fields.senderis the source of the command,inputis the result of Brigadier's parse, andpreviousArgumentsstores the arguments that have already been parsed. Note that since these records exist incommandapi-core,CommandSenderis a generic type and does not necessarily refer to Bukkit'sCommandSender.inputalso 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
InitialParseExceptionHandlerandArgumentParseExceptionHandlerareFunctionalInterfacesthat developers implement to define their exception handling behavior, similar toCommandExecutorfor 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
InitialParseExceptionArgumentandArgumentParseExceptionArgument. 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)ParseExceptionHandlerthat the developer uses to specify their exception handlers. There are also theget(Initial/Argument)ParseExceptionHandlermethods for retrieving the current exception handler, or an emptyOptionalif 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,
parseInitialParseExceptionandhandleArgumentParseExceptionrespectively. 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
exceptionHandlersmaps in both classes. These map an instance of the interface to its exception handler. Ideally, there would be an instance variableexceptionHandlerthat would do this instead. However, Java doesn't allow instance variables in interfaces. Here are the possible solutions to this problem I've considered:Argumentclass. Since Java also doesn't have multi-class inheritance, this doesn't work.(with/get)(Initial/Argument)ParseExceptionHandlermethods 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, theRawandExceptionInformationtype 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 theArgumentTypebeing used by the original argument. ThegetExamplesandlistSuggestionsmethods are just passed directly to thisbaseTypesince we don't need to modify either of those.When
ExceptionHandlingArgumentType#parseis called, it can catch anyCommandSyntaxExceptionthrown when thebaseTypeis parsed. That's what allows the CommandAPI to actually intercept the initial parse exceptions. It creates anInitialParseExceptionContextand handles it with theInitialParseExceptionHandler<T, ExceptionInformation> exceptionHandlerthat is active for this argument.In order to create the
ExceptionInformationobject for theInitialParseExceptionContext, theInitialParseExceptionParser<ExceptionInformation> exceptionParserparameter is used.InitialParseExceptionParseris just a simpleFunctionalInterfacethat looks like this:It takes in the exception that was thrown and the
StringReaderbeing used, and gives whatever information it can find. The implementation used for this method depends on the Argument as will be seen later.These
ExceptionHandlingArgumentTypes are added to the command tree whenCommandAPIHandlerbuilds commands. Specifically, this is managed byCommandAPIHandler#wrapArgumentType:When
CommandAPIHandlermakes a Brigadier required argument node,wrapArgumentTypewill check if the argument has anInitialParseExceptionHandler. If it does, instead of using the originalArgumentTypefor the argument, it will insert anExceptionHandlingArgumentTypewith the definedInitialParseExceptionHandlerandIntialParseExceptionParser.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.CustomArgumentuses the raw type of another argument, so it should use theInitialParseExceptionHandlerof another argument.CustomArgumentis platform-specific though, soWrapperArgumentjust allowscommandapi-coreto get that information, similar to theLiteralandMultiLiteralinterfaces.Here, you can see that the exception parser comes from the abstract method
parseInitialParseException. Each implementation ofInitialParseExceptionArgumentwill 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
ArgumentTypeto 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#registerCustomArgumentTypemethod 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 baseArgumentTypeinstead. So, when anExceptionHandlingArgumentTypeis wrapped around a node on the server, the client doesn't actually see any changes.Implementing new InitialParseExceptionArguments
In order for an
Argumentto have anInitialParseExceptionHandler, it needs to implementInitialParseExceptionArgument. For the proof of concept, I madeIntegerArgumentanInitialParseExceptionArgument, and it now looks like this:Remember that the generic types for
InitialParseExceptionArgumentare<T, ExceptionInformation, Impl>. TheIntegerArgumentgives<Integer, IntegerArgument.InitialParseExceptionInformation, Argument<Integer>>. So, each of these mean:Integer.IntegerArgument.InitialParseExceptionInformation.Argument<Integer>.InitialParseExceptionArgumentalso 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.Exceptionsis 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_INTEGERorINVALID_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 rawInputcan be found using theStringReader,int inputcomes from evaluatingInteger.parseInt(rawInput), andminimumandmaximumare stored by the reference toIntegerArgumentTypestored inArgumentwhen theIntegerArgumentwas constructed.Figuring out the proper value to put for
Exceptions typeis a bit trickier. All there is to go off is theCommandSyntaxExceptionparameter. 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#extractTranslationKeywas added to deal with this. OnceIntegerArgumentgets the translation key, a simpleswitchstatement finds the properExeceptionsvalue 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
ArgumentParseExceptionArgumentsimply 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 madeListArgumentCommonanArgumentParseExceptionArgument, and it now looks like this:Remember that the generic types for
InitialParseExceptionArgumentare<T, Raw, ExceptionInformation, Impl, CommandSender>. TheListArgumentCommongives<T, String, ListArgumentCommon.ArgumentParseExceptionInformation<T>, Argument<List>, CommandSender>. So, each of these mean:ArgumentParseExceptionHandlerisT. 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. MostArgumentTypes 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.Exceptionsis an enum with the valuesDUPLICATES_NOT_ALLOWEDandINVALID_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#parseArgumentused to throw an exception, a newArgumentParseExceptionInformationis 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 typeTcan also be substituted.To handle the exception, the default method
handleArgumentParseExceptionprovided byArgumentParseExceptionArgumentis 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
ArgumentParseExceptionContextis constructed and passed to the handler. The handler may return a substitute value, or it can throw aWrapperCommandSyntaxExceptionthat is unwrapped and thrown.This method mostly helps construct the
ArgumentParseExceptionContext. The extraCommandContext<Source> cmdCtx,String key, andCommandArguments previousArgsparameters 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)ParseExceptionArgumentsListTextArgument)ArgumentParseExceptionHandler(Initial/Argument)ParseExceptionArgumentsTest NMS code on real servers
Maybe todo