Throw a clear error when an Execute command contains whitespace#3122
Open
arpitjain099 wants to merge 1 commit into
Open
Throw a clear error when an Execute command contains whitespace#3122arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Execute("ACL SETUSER x") passes a whole command line as the single command
token, which gets sent as one unknown command and comes back as an opaque
server error. Since a redis command token never contains internal whitespace,
this is always a caller mistake, so the ExecuteMessage constructor now fails
fast with an ArgumentException-style RedisCommandException that names the
offending command and shows the correct token-per-argument form. Resolves StackExchange#2689.
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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.
Fixes #2689, which you reopened as a usability bug. Calling something like
db.Execute("ACL SETUSER x")passes the whole line as a single command token, so it goes out as one unknown command and the only feedback is an opaqueERR unknown commandfrom the server. Since a redis command token never has internal whitespace, that call is always a mistake, so theExecuteMessageconstructor now checks for it up front (right alongside the existing too-many-args and command-disabled guards) and throws aRedisCommandExceptionthat names the command and points at the correct token-per-argument form.I couldn't run the integration suite locally without a server, but the guard fires purely client-side in the constructor, so I added unit tests to
AdhocMessageRoundTripthat build the message directly (matching the existing disabled-command tests): a few whitespace inputs throw, andExecute("ACL", "SETUSER", "x")still constructs fine.Happy to switch to auto-splitting instead if you'd prefer that direction, but your reopen comment read as leaning toward the throw, so I went with that.