fix: compatibility around list and string commands (#3565)

src/server/list_family.cc

@@ -256,8 +256,9 @@ OpResult<uint32_t> OpPush(const OpArgs& op_args, std::string_view key, ListDir d
 
   if (skip_notexist) {
     auto tmp_res = op_args.GetDbSlice().FindMutable(op_args.db_cntx, key, OBJ_LIST);
-    if (!tmp_res)
+    if (tmp_res == OpStatus::KEY_NOTFOUND)
       return 0;  // Redis returns 0 for nonexisting keys for the *PUSHX actions.
+    RETURN_ON_BAD_STATUS(tmp_res);
     res = std::move(*tmp_res);
   } else {
     auto op_res = op_args.GetDbSlice().AddOrFind(op_args.db_cntx, key);
@@ -315,6 +316,9 @@ OpResult<StringVec> OpPop(const OpArgs& op_args, string_view key, ListDir dir, u
   if (!it_res)
     return it_res.status();
 
+  if (count == 0)
+    return StringVec{};
+
   auto it = it_res->it;
   quicklist* ql = GetQL(it->second);
 
@@ -993,8 +997,8 @@ void ListFamily::LInsert(CmdArgList args, ConnectionContext* cntx) {
   };
 
   OpResult<int> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
-  if (result) {
-    return cntx->SendLong(result.value());
+  if (result || result == OpStatus::KEY_NOTFOUND) {
+    return cntx->SendLong(result.value_or(0));
   }
 
   cntx->SendError(result.status());
@@ -1014,8 +1018,10 @@ void ListFamily::LTrim(CmdArgList args, ConnectionContext* cntx) {
   auto cb = [&](Transaction* t, EngineShard* shard) {
     return OpTrim(t->GetOpArgs(shard), key, start, end);
   };
-  cntx->transaction->ScheduleSingleHop(std::move(cb));
-  cntx->SendOk();
+  OpStatus st = cntx->transaction->ScheduleSingleHop(std::move(cb));
+  if (st == OpStatus::KEY_NOTFOUND)
+    st = OpStatus::OK;
+  cntx->SendError(st);
 }
 
 void ListFamily::LRange(CmdArgList args, ConnectionContext* cntx) {
@@ -1058,11 +1064,10 @@ void ListFamily::LRem(CmdArgList args, ConnectionContext* cntx) {
     return OpRem(t->GetOpArgs(shard), key, elem, count);
   };
   OpResult<uint32_t> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));
-  if (result) {
-    cntx->SendLong(result.value());
-  } else {
-    cntx->SendLong(0);
+  if (result || result == OpStatus::KEY_NOTFOUND) {
+    return cntx->SendLong(result.value_or(0));
   }
+  cntx->SendError(result.status());
 }
 
 void ListFamily::LSet(CmdArgList args, ConnectionContext* cntx) {
@@ -1202,13 +1207,9 @@ void ListFamily::PopGeneric(ListDir dir, CmdArgList args, ConnectionContext* cnt
   }
 
   if (return_arr) {
-    if (result->empty()) {
-      rb->SendNullArray();
-    } else {
-      rb->StartArray(result->size());
-      for (const auto& k : *result) {
-        rb->SendBulkString(k);
-      }
+    rb->StartArray(result->size());
+    for (const auto& k : *result) {
+      rb->SendBulkString(k);
     }
   } else {
     DCHECK_EQ(1u, result->size());

src/server/list_family_test.cc

@@ -366,6 +366,10 @@ TEST_F(ListFamilyTest, LRem) {
   resp = Run({"lrange", kKey1, "0", "1"});
   ASSERT_THAT(resp, ArrLen(2));
   ASSERT_THAT(resp.GetVec(), ElementsAre("b", "c"));
+
+  Run({"set", "foo", "bar"});
+  ASSERT_THAT(Run({"lrem", "foo", "0", "elem"}), ErrArg("WRONGTYPE"));
+  ASSERT_THAT(Run({"lrem", "nexists", "0", "elem"}), IntArg(0));
 }
 
 TEST_F(ListFamilyTest, LTrim) {
@@ -376,6 +380,9 @@ TEST_F(ListFamilyTest, LTrim) {
   ASSERT_THAT(resp.GetVec(), ElementsAre("c", "d"));
   ASSERT_EQ(Run({"ltrim", kKey1, "0", "0"}), "OK");
   ASSERT_EQ(Run({"lrange", kKey1, "0", "1"}), "c");
+  Run({"set", "foo", "bar"});
+  ASSERT_THAT(Run({"ltrim", "foo", "0", "1"}), ErrArg("WRONGTYPE"));
+  ASSERT_EQ(Run({"ltrim", "nexists", "0", "1"}), "OK");
 }
 
 TEST_F(ListFamilyTest, LRange) {
@@ -398,6 +405,14 @@ TEST_F(ListFamilyTest, Lset) {
   ASSERT_THAT(Run({"lset", kKey2, "1", "foo"}), ErrArg("index out of range"));
 }
 
+TEST_F(ListFamilyTest, LPop) {
+  Run({"rpush", "foo", "bar"});
+  auto resp = Run({"lpop", "foo", "0"});
+  EXPECT_THAT(resp, RespArray(ElementsAre()));
+  resp = Run({"lpop", "bar", "0"});
+  EXPECT_THAT(resp, ArgType(RespExpr::NIL));
+}
+
 TEST_F(ListFamilyTest, LPos) {
   auto resp = Run({"rpush", kKey1, "1", "a", "b", "1", "1", "a", "1"});
   ASSERT_THAT(resp, IntArg(7));
@@ -845,7 +860,7 @@ TEST_F(ListFamilyTest, RPushX) {
 
 TEST_F(ListFamilyTest, LInsert) {
   // List not found.
-  EXPECT_THAT(Run({"linsert", "notfound", "before", "foo", "bar"}), ErrArg("no such key"));
+  EXPECT_THAT(Run({"linsert", "notfound", "before", "foo", "bar"}), IntArg(0));
 
   // Key is not a list.
   Run({"set", "notalist", "x"});

src/server/string_family.cc

@@ -156,6 +156,9 @@ OpResult<StringValue> OpGetRange(const OpArgs& op_args, string_view key, int32_t
 
   auto& db_slice = op_args.GetDbSlice();
   auto it_res = db_slice.FindReadOnly(op_args.db_cntx, key, OBJ_STRING);
+  if (it_res == OpStatus::KEY_NOTFOUND) {
+    return StringValue(string{});
+  }
   RETURN_ON_BAD_STATUS(it_res);
 
   if (const CompactObj& co = it_res.value()->second; co.IsExternal()) {

src/server/string_family_test.cc

@@ -892,8 +892,9 @@ TEST_F(StringFamilyTest, MultiSetWithHashtagsLockHashtags) {
   fb.Join();
 }
 
-TEST_F(StringFamilyTest, StrLen) {
+TEST_F(StringFamilyTest, EmptyKeys) {
   EXPECT_EQ(0, CheckedInt({"strlen", "foo"}));
+  EXPECT_EQ(Run({"SUBSTR", "foo", "0", "-1"}), "");
 }
 
 }  // namespace dfly

tests/fakeredis/test/test_mixins/test_string_commands.py

@@ -533,6 +533,7 @@ def test_getex(r: redis.Redis):
 
 
 @pytest.mark.min_server("7")
+@pytest.mark.unsupported_server_types("dragonfly")
 def test_lcs(r: redis.Redis):
     r.mset({"key1": "ohmytext", "key2": "mynewtext"})
     assert r.lcs("key1", "key2") == b"mytext"