From 20b8817148a8e7ccbdc02ce9cc08b62f963e8aed Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Sun, 25 Aug 2024 16:30:55 +0300 Subject: [PATCH] fix: compatibility around list,string and sort commands (#3568) 1. Fix corner cases around non existing keys 2. Fix matching logic for * glob, as well as '' glob. 3. Improve SORT option parsing. Signed-off-by: Roman Gershman --- .gitignore | 2 +- src/server/common.cc | 10 ++++----- src/server/common.h | 2 +- src/server/generic_family.cc | 22 +++++++++++++++---- src/server/generic_family_test.cc | 13 +++++++++++ .../test/test_mixins/test_generic_commands.py | 5 +++++ 6 files changed, 43 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index d936b4919..a0268393d 100644 --- a/.gitignore +++ b/.gitignore @@ -17,5 +17,5 @@ _deps releases .DS_Store .idea/* - +.hypothesis .secrets diff --git a/src/server/common.cc b/src/server/common.cc index 732907ead..137872f7e 100644 --- a/src/server/common.cc +++ b/src/server/common.cc @@ -291,9 +291,9 @@ OpResult ScanOpts::TryFrom(CmdArgList args) { else if (scan_opts.limit > 4096) scan_opts.limit = 4096; } else if (opt == "MATCH") { - scan_opts.pattern = ArgS(args, i + 1); - if (scan_opts.pattern == "*") - scan_opts.pattern = string_view{}; + string_view pattern = ArgS(args, i + 1); + if (pattern != "*") + scan_opts.pattern = pattern; } else if (opt == "TYPE") { auto obj_type = ObjTypeFromString(ArgS(args, i + 1)); if (!obj_type) { @@ -312,9 +312,9 @@ OpResult ScanOpts::TryFrom(CmdArgList args) { } bool ScanOpts::Matches(std::string_view val_name) const { - if (pattern.empty()) + if (!pattern) return true; - return stringmatchlen(pattern.data(), pattern.size(), val_name.data(), val_name.size(), 0) == 1; + return stringmatchlen(pattern->data(), pattern->size(), val_name.data(), val_name.size(), 0) == 1; } GenericError::operator std::error_code() const { diff --git a/src/server/common.h b/src/server/common.h index e24148ef6..77b79c610 100644 --- a/src/server/common.h +++ b/src/server/common.h @@ -295,7 +295,7 @@ class Context : protected Cancellation { }; struct ScanOpts { - std::string_view pattern; + std::optional pattern; size_t limit = 10; std::optional type_filter; unsigned bucket_id = UINT_MAX; diff --git a/src/server/generic_family.cc b/src/server/generic_family.cc index 6e948a456..6c8acf1ac 100644 --- a/src/server/generic_family.cc +++ b/src/server/generic_family.cc @@ -907,7 +907,10 @@ void GenericFamily::Keys(CmdArgList args, ConnectionContext* cntx) { StringVec keys; ScanOpts scan_opts; - scan_opts.pattern = pattern; + if (pattern != "*") { + scan_opts.pattern = pattern; + } + scan_opts.limit = 512; auto output_limit = absl::GetFlag(FLAGS_keys_output_limit); @@ -1091,9 +1094,12 @@ OpResultTyped OpFetchSortEntries(const OpArgs& op_args, std::stri using namespace container_utils; auto it = op_args.GetDbSlice().FindReadOnly(op_args.db_cntx, key).it; - if (!IsValid(it) || !IsContainer(it->second)) { + if (!IsValid(it)) { return OpStatus::KEY_NOTFOUND; } + if (!IsContainer(it->second)) { + return OpStatus::WRONG_TYPE; + } auto result = MakeSortEntryList(alpha); bool success = std::visit( @@ -1106,7 +1112,7 @@ OpResultTyped OpFetchSortEntries(const OpArgs& op_args, std::stri result); auto res = OpResultTyped{std::move(result)}; res.setType(it->second.ObjType()); - return success ? res : OpStatus::WRONG_TYPE; + return success ? res : OpStatus::INVALID_NUMERIC_RESULT; } void GenericFamily::Sort(CmdArgList args, ConnectionContext* cntx) { @@ -1123,6 +1129,8 @@ void GenericFamily::Sort(CmdArgList args, ConnectionContext* cntx) { alpha = true; } else if (arg == "DESC") { reversed = true; + } else if (arg == "ASC") { + reversed = false; } else if (arg == "LIMIT") { int offset, limit; if (i + 2 >= args.size()) { @@ -1134,6 +1142,9 @@ void GenericFamily::Sort(CmdArgList args, ConnectionContext* cntx) { } bounds = {offset, limit}; i += 2; + } else { + LOG_EVERY_T(ERROR, 1) << "Unsupported option " << arg; + return cntx->SendError(kSyntaxErr, kSyntaxErrType); } } @@ -1142,7 +1153,10 @@ void GenericFamily::Sort(CmdArgList args, ConnectionContext* cntx) { return OpFetchSortEntries(t->GetOpArgs(shard), key, alpha); }); - if (fetch_result.status() == OpStatus::WRONG_TYPE) + if (fetch_result == OpStatus::WRONG_TYPE) + return cntx->SendError(fetch_result.status()); + + if (fetch_result.status() == OpStatus::INVALID_NUMERIC_RESULT) return cntx->SendError("One or more scores can't be converted into double"); auto* rb = static_cast(cntx->reply_builder()); diff --git a/src/server/generic_family_test.cc b/src/server/generic_family_test.cc index 0916e4908..c749c1d38 100644 --- a/src/server/generic_family_test.cc +++ b/src/server/generic_family_test.cc @@ -400,6 +400,15 @@ TEST_F(GenericFamilyTest, Scan) { vec = StrArray(resp.GetVec()[1]); EXPECT_EQ(10, vec.size()); EXPECT_THAT(vec, Each(StartsWith("zset"))); + + Run({"flushdb"}); + + Run({"set", "", "foo"}); + Run({"set", "bar", "1"}); + resp = Run({"keys", "*"}); + EXPECT_THAT(resp, RespArray(ElementsAre("bar", ""))); + resp = Run({"keys", ""}); + EXPECT_EQ(resp, ""); } TEST_F(GenericFamilyTest, Sort) { @@ -465,6 +474,10 @@ TEST_F(GenericFamilyTest, Sort) { // Test not convertible to double Run({"lpush", "list-2", "NOTADOUBLE"}); ASSERT_THAT(Run({"sort", "list-2"}), ErrArg("One or more scores can't be converted into double")); + + Run({"set", "foo", "bar"}); + ASSERT_THAT(Run({"sort", "foo"}), ErrArg("WRONGTYPE ")); + ; } TEST_F(GenericFamilyTest, TimeNoKeys) { diff --git a/tests/fakeredis/test/test_mixins/test_generic_commands.py b/tests/fakeredis/test/test_mixins/test_generic_commands.py index e0ac4dc80..f75647727 100644 --- a/tests/fakeredis/test/test_mixins/test_generic_commands.py +++ b/tests/fakeredis/test/test_mixins/test_generic_commands.py @@ -127,6 +127,7 @@ def test_sort_wrong_type(r: redis.Redis): r.sort("string") +@pytest.mark.unsupported_server_types("dragonfly") def test_sort_with_store_option(r: redis.Redis): r.rpush("foo", "2") r.rpush("foo", "1") @@ -137,6 +138,7 @@ def test_sort_with_store_option(r: redis.Redis): assert r.lrange("bar", 0, -1) == [b"1", b"2", b"3", b"4"] +@pytest.mark.unsupported_server_types("dragonfly") def test_sort_with_by_and_get_option(r: redis.Redis): r.rpush("foo", "2") r.rpush("foo", "1") @@ -186,6 +188,7 @@ def test_sort_with_by_and_get_option(r: redis.Redis): ] +@pytest.mark.unsupported_server_types("dragonfly") def test_sort_with_hash(r: redis.Redis): r.rpush("foo", "middle") r.rpush("foo", "eldest") @@ -665,7 +668,9 @@ def test_key_patterns(r: redis.Redis): assert sorted(r.keys()) == [b"four", b"one", b"three", b"two"] +# seems like a rather peculiar behavior of Redis, maybe a bug? Disabling for Dragonfly for now. @pytest.mark.min_server("7") +@pytest.mark.unsupported_server_types("dragonfly") def test_watch_when_setbit_does_not_change_value(r: redis.Redis): r.set("foo", b"0")