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 <roman@dragonflydb.io>

.gitignore

@@ -17,5 +17,5 @@ _deps
 releases
 .DS_Store
 .idea/*
-
+.hypothesis
 .secrets

src/server/common.cc

@@ -291,9 +291,9 @@ OpResult<ScanOpts> 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> 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 {

src/server/common.h

@@ -295,7 +295,7 @@ class Context : protected Cancellation {
 };
 
 struct ScanOpts {
-  std::string_view pattern;
+  std::optional<std::string_view> pattern;
   size_t limit = 10;
   std::optional<CompactObjType> type_filter;
   unsigned bucket_id = UINT_MAX;

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<SortEntryList> 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<SortEntryList> 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<RedisReplyBuilder*>(cntx->reply_builder());

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) {

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")