fix: binary list key randomly droped by lrem (#2118)

* memcmp replace strcmp * fix * pivot covert to Slice * add lists unit test * no need convert to slice * Update src/storage/src/redis_lists.cc * fix space in redis_lists.cc --------- Co-authored-by: yaxin0710 <[email protected]> Co-authored-by: Ted Mostly <[email protected]>
OpenAtomFoundation · Nov 13, 2023 · 6cde9c1 · 6cde9c1
1 parent f0e5234
commit 6cde9c1
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 5 deletions.
diff --git a/src/storage/src/redis_lists.cc b/src/storage/src/redis_lists.cc
@@ -251,7 +251,7 @@ Status RedisLists::LInsert(const Slice& key, const BeforeOrAfter& before_or_afte
       ListsDataKey start_data_key(key, version, current_index);
       for (iter->Seek(start_data_key.Encode()); iter->Valid() && current_index < parsed_lists_meta_value.right_index();
            iter->Next(), current_index++) {
-        if (strcmp(iter->value().ToString().data(), pivot.data()) == 0) {
+        if (iter->value() == Slice(pivot)) {
           find_pivot = true;
           pivot_index = current_index;
           break;
@@ -538,7 +538,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
         for (iter->Seek(start_data_key.Encode());
              iter->Valid() && current_index <= stop_index && ((count == 0) || rest != 0);
              iter->Next(), current_index++) {
-          if (strcmp(iter->value().ToString().data(), value.data()) == 0) {
+          if (iter->value() == value) {
             target_index.push_back(current_index);
             if (count != 0) {
               rest--;
@@ -552,7 +552,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
         for (iter->Seek(stop_data_key.Encode());
              iter->Valid() && current_index >= start_index && ((count == 0) || rest != 0);
              iter->Prev(), current_index--) {
-          if (strcmp(iter->value().ToString().data(), value.data()) == 0) {
+          if (iter->value() == value) {
             target_index.push_back(current_index);
             if (count != 0) {
               rest--;
@@ -577,7 +577,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
           rocksdb::Iterator* iter = db_->NewIterator(default_read_options_, handles_[1]);
           for (iter->Seek(sublist_right_key.Encode()); iter->Valid() && current_index >= start_index;
                iter->Prev(), current_index--) {
-            if ((strcmp(iter->value().ToString().data(), value.data()) == 0) && rest > 0) {
+            if ((iter->value() == value) && rest > 0) {
               rest--;
             } else {
               ListsDataKey lists_data_key(key, version, left--);
@@ -597,7 +597,7 @@ Status RedisLists::LRem(const Slice& key, int64_t count, const Slice& value, uin
           rocksdb::Iterator* iter = db_->NewIterator(default_read_options_, handles_[1]);
           for (iter->Seek(sublist_left_key.Encode()); iter->Valid() && current_index <= stop_index;
                iter->Next(), current_index++) {
-            if ((strcmp(iter->value().ToString().data(), value.data()) == 0) && rest > 0) {
+            if ((iter->value() == value) && rest > 0) {
               rest--;
             } else {
               ListsDataKey lists_data_key(key, version, right++);

diff --git a/tests/integration/list_test.go b/tests/integration/list_test.go
@@ -554,6 +554,29 @@ var _ = Describe("List Commands", func() {
 			Expect(lRange.Val()).To(Equal([]string{"hello", "key"}))
 		})
 
+		It("should LRem binary", func() {
+			rPush := client.RPush(ctx, "list", "\x00\xa2\x00")
+			Expect(rPush.Err()).NotTo(HaveOccurred())
+			rPush = client.RPush(ctx, "list", "\x00\x9d")
+			Expect(rPush.Err()).NotTo(HaveOccurred())
+
+			lInsert := client.LInsert(ctx, "list", "BEFORE", "\x00\x9d", "\x00\x5f")
+			Expect(lInsert.Err()).NotTo(HaveOccurred())
+			Expect(lInsert.Val()).To(Equal(int64(3)))
+
+			lRange := client.LRange(ctx, "list", 0, -1)
+			Expect(lRange.Err()).NotTo(HaveOccurred())
+			Expect(lRange.Val()).To(Equal([]string{"\x00\xa2\x00", "\x00\x5f", "\x00\x9d"}))
+
+			lRem := client.LRem(ctx, "list", -1, "\x00\x5f")
+			Expect(lRem.Err()).NotTo(HaveOccurred())
+			Expect(lRem.Val()).To(Equal(int64(1)))
+
+			lRange = client.LRange(ctx, "list", 0, -1)
+			Expect(lRange.Err()).NotTo(HaveOccurred())
+			Expect(lRange.Val()).To(Equal([]string{"\x00\xa2\x00", "\x00\x9d"}))
+		})
+
 		It("should LSet", func() {
 			rPush := client.RPush(ctx, "list", "one")
 			Expect(rPush.Err()).NotTo(HaveOccurred())