Currently, the /server command uses StringArgumentType#word for its argument. This argument only accepts a limited set of characters, as defined by Brigadier's StringReader#isAllowedInUnquotedString method. It is possible to give a backend server a name that contains characters outside of this set, making that server unusable with the /server command.
This PR changes the /server command to use the StringArgumentType#string argument type. This argument accepts unquoted strings like word, so previously valid commands are still valid. However, any characters can now be entered, as long as the argument is surrounded by ".
The suggestion provider for the argument was updated to suggest quotes around server names that need to be quoted. If the user places a quote at the start of the argument, then all the server names are suggested surrounded by quotes. If the user types a character other than a quote, names that require quotes are not suggested.
I wasn't sure if I should write automated tests for this new code. I didn't see any tests for the functionality of builtin Velocity commands, so I decided not to add any suggestions tests in this PR.
I also noticed that the /glist command and the /send command also use StringArgumentType#word to accept a target server name. Should those commands be updated to use StringArgumentType#string as well? Also, would it be a good idea to somehow share the code for the server name suggestions provider, rather than duplicating (mostly, glist also suggests "all") the same logic across these classes?
As an alternative solution, I believe StringArgumentType#greedy would be appropriate here. This is the last argument in the command, and the string argument type allows the user to input arbitrary strings anyway. On the user side, using a greedy string would mean they wouldn't need to put quotes around the server name. On the code side, the suggestions provider wouldn't have to worry about handling server names that are not valid unquoted strings. However, https://github.com/PaperMC/Velocity/issues/1362#issuecomment-2184170368 alluded to the idea that a greedy string here may be problematic ¯\(ツ)/¯.
As another alternative solution, I suppose Velocity could prohibit server names that are not unquoted strings. https://github.com/PaperMC/Velocity/issues/1362#issuecomment-2184163824 suggests that this is a niche setup that often causes other issues anyway. However, I imagine this isn't really an acceptable solution for users whose language doesn't exclusively use the letters a-z.
Resolves #1362
Currently, the
/servercommand usesStringArgumentType#wordfor its argument. This argument only accepts a limited set of characters, as defined by Brigadier'sStringReader#isAllowedInUnquotedStringmethod. It is possible to give a backend server a name that contains characters outside of this set, making that server unusable with the/servercommand.This PR changes the
/servercommand to use theStringArgumentType#stringargument type. This argument accepts unquoted strings likeword, so previously valid commands are still valid. However, any characters can now be entered, as long as the argument is surrounded by".The suggestion provider for the argument was updated to suggest quotes around server names that need to be quoted. If the user places a quote at the start of the argument, then all the server names are suggested surrounded by quotes. If the user types a character other than a quote, names that require quotes are not suggested.
I wasn't sure if I should write automated tests for this new code. I didn't see any tests for the functionality of builtin Velocity commands, so I decided not to add any suggestions tests in this PR.
I also noticed that the
/glistcommand and the/sendcommand also useStringArgumentType#wordto accept a target server name. Should those commands be updated to useStringArgumentType#stringas well? Also, would it be a good idea to somehow share the code for the server name suggestions provider, rather than duplicating (mostly,glistalso suggests"all") the same logic across these classes?As an alternative solution, I believe
StringArgumentType#greedywould be appropriate here. This is the last argument in the command, and thestringargument type allows the user to input arbitrary strings anyway. On the user side, using a greedy string would mean they wouldn't need to put quotes around the server name. On the code side, the suggestions provider wouldn't have to worry about handling server names that are not valid unquoted strings. However, https://github.com/PaperMC/Velocity/issues/1362#issuecomment-2184170368 alluded to the idea that a greedy string here may be problematic ¯\(ツ)/¯.As another alternative solution, I suppose Velocity could prohibit server names that are not unquoted strings. https://github.com/PaperMC/Velocity/issues/1362#issuecomment-2184163824 suggests that this is a niche setup that often causes other issues anyway. However, I imagine this isn't really an acceptable solution for users whose language doesn't exclusively use the letters a-z.