Skip to content
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.

Sign up for GitHub

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

fix issue #1517: scan 命令不支持 type 参数 #1582

Merged
merged 1 commit into from
Jun 14, 2023
Merged

fix issue #1517: scan 命令不支持 type 参数 #1582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/pika_kv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,10 +600,12 @@ class ScanCmd : public Cmd {
int64_t cursor_ = 0;
std::string pattern_ = "*";
int64_t count_ = 10;
storage::DataType type_ = storage::DataType::kAll;
void DoInitial() override;
void Clear() override {
pattern_ = "*";
count_ = 10;
type_ = storage::DataType::kAll;
}
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided code patch, a brief code review and possible suggestions are:

  • The added "storage::DataType type_ = storage::kAll;" seems fine if it's going to be used in the implementation of this class later.
  • It is not clear what the purpose of the "DoInitial()" function is or how it is being used. Therefore, reviewing the implementation for this function could be beneficial.
  • It would be better to define the initial value of "type_" at its declaration point instead of initializing it into the "Clear()" method.
  • There doesn't seem to be any bug risk in the given code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the code patch, it seems that a new member variable type_ of type storage::DataType has been added to class ScanCmd. This variable is initialized with the value kAll in the Clear() function.

Without seeing the implementation details of the class and associated functions, it is difficult to identify any bugs or potential improvements in the code. However, adding this new variable appears to be a reasonable and potentially useful enhancement to the ScanCmd class.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided code patch, it appears that a new member variable type_ of type storage::DataType has been added to the ScanCmd class with an initial value of kAll. Additionally, the Clear() method has been updated to reset type_ to kAll.

Without any additional information about the implementation of storage::DataType or the purpose of the ScanCmd class and its methods, it is difficult to identify any potential bug risks or improvement suggestions from this code patch alone.

Expand Down
20 changes: 18 additions & 2 deletions src/pika_kv.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,14 +1090,30 @@ void ScanCmd::DoInitial() {

while (index < argc) {
std::string opt = argv_[index];
if ((strcasecmp(opt.data(), "match") == 0) || (strcasecmp(opt.data(), "count") == 0)) {
if ((strcasecmp(opt.data(), "match") == 0) || (strcasecmp(opt.data(), "count") == 0) ||
(strcasecmp(opt.data(), "type") == 0)) {
index++;
if (index >= argc) {
res_.SetRes(CmdRes::kSyntaxErr);
return;
}
if (strcasecmp(opt.data(), "match") == 0) {
pattern_ = argv_[index];
} else if (strcasecmp(opt.data(), "type") == 0) {
std::string str_type = argv_[index];
if (strcasecmp(str_type.data(), "string") == 0) {
type_ = storage::DataType::kStrings;
} else if (strcasecmp(str_type.data(), "zset") == 0) {
type_ = storage::DataType::kZSets;
} else if (strcasecmp(str_type.data(), "set") == 0) {
type_ = storage::DataType::kSets;
} else if (strcasecmp(str_type.data(), "list") == 0) {
type_ = storage::DataType::kLists;
} else if (strcasecmp(str_type.data(), "hash") == 0) {
type_ = storage::DataType::kHashes;
} else {
res_.SetRes(CmdRes::kSyntaxErr);
}
} else if ((pstd::string2int(argv_[index].data(), argv_[index].size(), &count_) == 0) || count_ <= 0) {
res_.SetRes(CmdRes::kInvalidInt);
return;
Expand All @@ -1123,7 +1139,7 @@ void ScanCmd::Do(std::shared_ptr<Slot> slot) {
keys.clear();
batch_count = left < PIKA_SCAN_STEP_LENGTH ? left : PIKA_SCAN_STEP_LENGTH;
left = left > PIKA_SCAN_STEP_LENGTH ? left - PIKA_SCAN_STEP_LENGTH : 0;
cursor_ret = slot->db()->Scan(storage::DataType::kAll, cursor_ret, pattern_, batch_count, &keys);
cursor_ret = slot->db()->Scan(type_, cursor_ret, pattern_, batch_count, &keys);
for (const auto& key : keys) {
RedisAppendLen(raw, key.size(), "$");
RedisAppendContent(raw, key);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the code patch is adding functionality to the existing code. It introduces a new option "-type" to filter results by key type, and assigns it to the variable "type_". The types will be mapped to enumerated values from storage::DataType.

An improvement would be to add proper error messages to make it easier to understand why the command has failed. Another suggestion would be to handle exceptions in case of errors within std::string2int.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch appears to add functionality related to scanning Redis data. Here are some potential issues and improvements that could be made:

  • The current code may have an issue with error handling when a type other than string, zset, set, list and hash is specified. If an incorrect type is entered, the SyntaxErr is returned without a clear message about what went wrong. It would be better to provide a more specific error message to help the user identify the problem.
  • The use of strcasecmp can be a security risk due to its vulnerability to timing attacks, which can be mitigated using constant-time comparison or modern secure comparison functions like secure_compare() provided by libsodium.
  • It is unclear what PIKA_SCAN_STEP_LENGTH represents, which makes it difficult to understand the purpose of batch_count.
  • Adding comments would be helpful to make it easy for developers who unfamiliar with the project to quickly understand what each function does, why it was added, and its important details.
  • Another possible improvement suggestion would be to add unit tests for the new functionality to ensure proper functionality over time.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch looks good overall, as it introduces a new option type to allow scanning specific data types and handles it appropriately in the Do function by passing the chosen type to the Scan method.

One minor suggestion for improvement would be to add some error handling in the DoInitial function to check if an invalid data type is provided after the type option and set the response accordingly.

Also, there might be room for improvement in terms of code readability by extracting the code for parsing the data type options into a separate function to reduce the complexity of the DoInitial function.

Expand Down
45 changes: 45 additions & 0 deletions tests/unit/scan.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,51 @@ start_server {tags {"scan"}} {
assert_equal 100 [llength $keys]
}

test "SCAN TYPE" {
r flushdb
# populate only creates strings
r debug populate 1000

# Check non-strings are excluded
set cur 0
set keys {}
while 1 {
set res [r scan $cur type "list"]
set cur [lindex $res 0]
set k [lindex $res 1]
lappend keys {*}$k
if {$cur == 0} break
}

assert_equal 0 [llength $keys]

# Check strings are included
set cur 0
set keys {}
while 1 {
set res [r scan $cur type "string"]
set cur [lindex $res 0]
set k [lindex $res 1]
lappend keys {*}$k
if {$cur == 0} break
}

assert_equal 1000 [llength $keys]

# Check all three args work together
set cur 0
set keys {}
while 1 {
set res [r scan $cur type "string" match "key:*" count 10]
set cur [lindex $res 0]
set k [lindex $res 1]
lappend keys {*}$k
if {$cur == 0} break
}

assert_equal 1000 [llength $keys]
}

foreach enc {intset hashtable} {
test "SSCAN with encoding $enc" {
# Create the Set
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems fine, and I don't see any bug risks. However, here are some suggestions to improve the code:

  1. The test case "SCAN TYPE" could be split into multiple tests for better readability and maintainability.

  2. Consider adding more test cases to cover different scenarios and edge cases.

  3. Use meaningful variable names instead of short ones such as "cur," "res," and "k."

  4. Consider documenting the purpose of each test case.

  5. Consider using the "incr" command instead of "*}$k" to append elements to a list.

  6. Identify specific Redis versions that the code is tested on.

  7. Consider using a consistent style throughout the code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be a new test added to an existing implementation of Redis Command Line Interface (CLI) module, specifically scanning keys with a specific type. The test seems to check if the implemented command SCAN filters by key type as expected.

The code looks fine in terms of structure and syntax. However, here are my suggestions:

  1. One improvement suggestion is to introduce fixtures or helper methods that generate predefined keys of different types, instead of using populate of Redis debug commands to add test data.

  2. There could have been some comments added under each of the test cases defining what they are trying to test which would help future maintainers of this code.

  3. Another good practice that can always be considered is adding additional edge test cases like empty values being evaluated, non-existing types, etc.

  4. Lastly, it's unclear from the provided snippet whether other important details such as dependencies, imports, and system level requirements, etc. are present or not.

Expand Down