Fix additional parameter parsing in SCAN#2236
Fix additional parameter parsing in SCAN#2236jackyyyyyssss wants to merge 20 commits intoapache:unstablefrom
Conversation
src/commands/scan_base.h
Outdated
| if (params->count <= 0) { | ||
| return {Status::RedisParseErr, errInvalidSyntax}; | ||
| } | ||
| } else if (util::ToLower(args[i]) == "type") { |
There was a problem hiding this comment.
Please try to imagine what will happen if the input is scan xxx type.
src/commands/cmd_server.cc
Outdated
| if (args.size() % 2 != 0) { | ||
| return {Status::RedisParseErr, errWrongNumOfArguments}; | ||
| } |
There was a problem hiding this comment.
I am a C++beginner. Thank you very much for your guidance on my code. I have learned a lot
Co-authored-by: Twice <twice@apache.org>
Co-authored-by: Twice <twice@apache.org>
Co-authored-by: Twice <twice@apache.org>
|
@jackyyyyyssss CommandSubkeyScanBase should have the same issue which is used for HSCAN/SSCAN/ZSCAN: https://redis.io/docs/latest/commands/sscan/. |
Okay, can I use different PR for these modifications？ |
|
@jackyyyyyssss Perhaps you can take a look, it's in the same file and they share the same parse logic except the CommandSubkeyScanBase starts parsing the match/count from the 2nd parameter, and type is not allowed. |
|
@jackyyyyyssss Can remove ParseMatchAndCountParam if no place is using it. |
src/commands/scan_base.h
Outdated
| CommandParser parser(args, 1); | ||
| key_ = GET_OR_RET(parser.TakeStr()); | ||
| ParseCursor(GET_OR_RET(parser.TakeStr())); | ||
| while (parser.Good()) { | ||
| if (parser.EatEqICase("match")) { | ||
| prefix_ = GET_OR_RET(parser.TakeStr()); | ||
| if (!prefix_.empty() && prefix_.back() == '*') { | ||
| prefix_ = prefix_.substr(0, prefix_.size() - 1); | ||
| } else { | ||
| return {Status::RedisParseErr, "currently only key prefix matching is supported"}; | ||
| } | ||
| } else if (parser.EatEqICase("count")) { | ||
| limit_ = GET_OR_RET(parser.TakeInt()); | ||
| if (limit_ <= 0) { | ||
| return {Status::RedisParseErr, errInvalidSyntax}; | ||
| } | ||
| } else { | ||
| return {Status::RedisParseErr, errWrongNumOfArguments}; |
There was a problem hiding this comment.
I have a question about whether this parsing is written in CommandScanBase, or if both have differences written in their respective places, with the public part written in base. However, if so, what should be the template value for CommandParser as a method parameter? like this
template < typename Iter >
Status ProcessCommonParser(CommandParser &parser) {..}
ok |
src/commands/scan_base.h
Outdated
| } else if (parser.EatEqICase("type")) { | ||
| // HSCAN SSCAN ZSCAN Will not enter |
There was a problem hiding this comment.
Why do you think that "HSCAN SSCAN ZSCAN will not enter" this block?
There was a problem hiding this comment.
Maybe my description is a bit inaccurate. After other commands are added, error prompts will be return
src/commands/scan_base.h
Outdated
| Status Parse(const std::vector<std::string> &args) override { | ||
| CommandParser parser(args, 1); | ||
| if (util::ToLower(args[0]) != "scan") { | ||
| key_ = GET_OR_RET(parser.TakeStr()); |
There was a problem hiding this comment.
This should be only in CommandSubkeyScanBase.
|
I will prepare a patch to succeed this PR. |
Can you help me take a look at the optimized version thks |
|
It's more close. But I think we can save some communication effort by my patch.. I'll make you the co-author of the patch, and hope you can learn some C++ knowledege from my code. Also, in the next time, please don't forget to write test cases for your patch, as it's mandatory. |
Thank you for your patient answer |
|
4 participants
When kvrocks executes a scan command, such as SCAN 0 key match * he 1000, it will change the originally recognized match to a key without reporting any errors. Finally, the value of prefix_ will match all keys, and this command will report an error under normal Redis. Add mandatory matching verification code and use the current test case for testing