From 38677a240e10f9e5f425b15f97f7f0e5daacaf9b Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Thu, 5 Dec 2024 17:25:15 +0200 Subject: [PATCH] chore: fixes the parse error for xread/xreadgroup with unbalanced ids Partially addresses #4193 Signed-off-by: Roman Gershman --- src/server/stream_family.cc | 10 +++++++--- src/server/stream_family_test.cc | 11 ++++++++--- src/server/transaction.cc | 3 --- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/server/stream_family.cc b/src/server/stream_family.cc index 1a19a2278af6..a27c4e324ee8 100644 --- a/src/server/stream_family.cc +++ b/src/server/stream_family.cc @@ -361,7 +361,7 @@ int StreamAppendItem(stream* s, CmdArgList fields, uint64_t now_ms, streamID* ad } /* Avoid overflow when trying to add an element to the stream (listpack - * can only host up to 32bit length sttrings, and also a total listpack size + * can only host up to 32bit length strings, and also a total listpack size * can't be bigger than 32bit length. */ size_t totelelen = 0; for (size_t i = 0; i < fields.size(); i++) { @@ -2114,8 +2114,12 @@ std::optional ParseReadArgsOrReply(CmdArgList args, bool read_group, size_t pair_count = args.size() - opts.streams_arg; if ((pair_count % 2) != 0) { - const auto m = "Unbalanced list of streams: for each stream key an ID must be specified"; - builder->SendError(m, kSyntaxErr); + const char* cmd_name = read_group ? "xreadgroup" : "xread"; + const char* symbol = read_group ? ">" : "$"; + const auto msg = absl::StrCat("Unbalanced '", cmd_name, + "' list of streams: for each stream key an ID or '", symbol, + "' must be specified"); + builder->SendError(msg, kSyntaxErr); return std::nullopt; } streams_count = pair_count / 2; diff --git a/src/server/stream_family_test.cc b/src/server/stream_family_test.cc index d91e947b7c0e..cec5b8fa0152 100644 --- a/src/server/stream_family_test.cc +++ b/src/server/stream_family_test.cc @@ -404,8 +404,8 @@ TEST_F(StreamFamilyTest, XReadInvalidArgs) { EXPECT_THAT(resp, ErrArg("syntax error")); // Unbalanced list of streams. - resp = Run({"xread", "count", "invalid", "streams", "s1", "s2", "s3", "0", "0"}); - EXPECT_THAT(resp, ErrArg("syntax error")); + resp = Run({"xread", "count", "invalid", "streams", "s1", "s2", "0", "0"}); + EXPECT_THAT(resp, ErrArg("value is not an integer")); // Wrong type. Run({"set", "foo", "v"}); @@ -442,7 +442,12 @@ TEST_F(StreamFamilyTest, XReadGroupInvalidArgs) { // Unbalanced list of streams. resp = Run({"xreadgroup", "group", "group", "alice", "streams", "s1", "s2", "s3", "0", "0"}); - EXPECT_THAT(resp, ErrArg("syntax error")); + EXPECT_THAT(resp, ErrArg("Unbalanced 'xreadgroup' list of streams: for each stream key an ID or " + "'>' must be specified")); + + resp = Run({"XREAD", "COUNT", "1", "STREAMS", "mystream"}); + ASSERT_THAT(resp, ErrArg("Unbalanced 'xread' list of streams: for each stream key an ID or '$' " + "must be specified")); } TEST_F(StreamFamilyTest, XReadGroupEmpty) { diff --git a/src/server/transaction.cc b/src/server/transaction.cc index 67f9104381a8..ca7939cbabd6 100644 --- a/src/server/transaction.cc +++ b/src/server/transaction.cc @@ -1563,9 +1563,6 @@ OpResult DetermineKeys(const CommandId* cid, CmdArgList args) { string_view arg = ArgS(args, i); if (absl::EqualsIgnoreCase(arg, "STREAMS")) { size_t left = args.size() - i - 1; - if (left < 2 || left % 2 != 0) - return OpStatus::SYNTAX_ERR; - return KeyIndex(i + 1, i + 1 + (left / 2)); } }