jihuayu commented on code in PR #3502:
URL: https://github.com/apache/kvrocks/pull/3502#discussion_r3449377231
##########
src/commands/cmd_hash.cc:
##########
@@ -144,53 +144,25 @@ Status ParseHashExpireFields(const
std::vector<std::string> &args, size_t start,
HashFieldExpireCondition *condition_out,
std::vector<std::string> *fields) {
*condition_out = HashFieldExpireCondition::kNone;
fields->clear();
- bool fields_seen = false;
- for (size_t i = start; i < args.size();) {
- if (util::EqualICase(args[i], "FIELDS")) {
- if (fields_seen) {
- return {Status::RedisParseErr, errInvalidSyntax};
- }
- fields_seen = true;
- if (i + 1 >= args.size()) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
- }
-
- auto num_fields = ParseInt<int64_t>(args[i + 1], 10);
- if (!num_fields || *num_fields < 1) {
- return {Status::RedisParseErr, errValueNotInteger};
- }
-
- size_t first_field = i + 2;
- auto field_count = static_cast<size_t>(*num_fields);
- if (field_count > args.size() - first_field) {
- return {Status::RedisParseErr, errWrongNumOfArguments};
- }
+ size_t i = start;
+ if (i < args.size()) {
+ auto condition = ParseHashExpireCondition(args[i]);
+ if (condition) {
+ *condition_out = *condition;
+ i++;
- fields->clear();
- fields->reserve(field_count);
- for (size_t j = 0; j < field_count; j++) {
- fields->emplace_back(args[first_field + j]);
+ if (i < args.size() && ParseHashExpireCondition(args[i])) {
+ return {Status::RedisParseErr, errInvalidSyntax};
}
- i = first_field + field_count;
- continue;
}
-
- auto condition = ParseHashExpireCondition(args[i]);
- if (!condition) {
- return {Status::RedisParseErr, errInvalidSyntax};
- }
- if (*condition_out != HashFieldExpireCondition::kNone && *condition_out !=
*condition) {
- return {Status::RedisParseErr, errInvalidSyntax};
- }
- *condition_out = *condition;
- i++;
}
- if (!fields_seen) {
+ CommandParser parser(args, i);
+ if (!parser.EatEqICase("FIELDS")) {
return {Status::RedisParseErr, errInvalidSyntax};
}
- return Status::OK();
+ return ParseHashFieldListTail(parser, fields);
Review Comment:
Why change this?
##########
src/types/redis_stream_base.cc:
##########
@@ -59,16 +92,16 @@ Status ParseStreamEntryID(const std::string &input,
StreamEntryID *id) {
if (pos != std::string::npos) {
auto ms_str = input.substr(0, pos);
auto seq_str = input.substr(pos + 1);
- auto parse_ms = ParseInt<uint64_t>(ms_str, 10);
- auto parse_seq = ParseInt<uint64_t>(seq_str, 10);
+ auto parse_ms = ParseStreamEntryIDComponent(ms_str, false);
+ auto parse_seq = ParseStreamEntryIDComponent(seq_str, true);
if (!parse_ms || !parse_seq) {
return {Status::RedisParseErr, kErrInvalidEntryIdSpecified};
}
id->ms = *parse_ms;
id->seq = *parse_seq;
} else {
- auto parse_input = ParseInt<uint64_t>(input, 10);
+ auto parse_input = ParseStreamEntryIDComponent(input, false);
Review Comment:
Why change it?
This changes the shared stream ID parser to accept signed stream ID
components such as +1-0, 1-+0, and 1--0. The change is broader than XDELEX
because other stream commands also use ParseStreamEntryID. Redis rejects these
forms, so this is a protocol compatibility regression. Please keep the shared
parser strict and update the XDELEX tests to expect errors for signed
components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
