From 00666ea73915af08d17db40010a5dc951d54598e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=AA=E5=8D=8E=E8=A3=95?= <8042833@qq.com> Date: Tue, 12 Dec 2023 08:53:27 +0000 Subject: [PATCH 1/8] add new string#set impl --- src/commands/cmd_string.cc | 38 +++++++++------ src/types/redis_string.cc | 98 +++++++++++++++++++++++++------------- src/types/redis_string.h | 5 ++ 3 files changed, 94 insertions(+), 47 deletions(-) diff --git a/src/commands/cmd_string.cc b/src/commands/cmd_string.cc index 99783172c42..297beb251df 100644 --- a/src/commands/cmd_string.cc +++ b/src/commands/cmd_string.cc @@ -20,10 +20,12 @@ #include #include +#include #include "commander.h" #include "commands/command_parser.h" #include "error_constants.h" +#include "server/redis_reply.h" #include "server/server.h" #include "storage/redis_db.h" #include "time_util.h" @@ -281,10 +283,14 @@ class CommandSet : public Commander { while (parser.Good()) { if (auto v = GET_OR_RET(ParseTTL(parser, ttl_flag))) { ttl_ = *v; + } else if (parser.EatEqICaseFlag("KEEPTTL", ttl_flag)) { + keep_ttl_ = true; } else if (parser.EatEqICaseFlag("NX", set_flag)) { - set_flag_ = NX; + set_flag_ = StringSetType::NX; } else if (parser.EatEqICaseFlag("XX", set_flag)) { - set_flag_ = XX; + set_flag_ = StringSetType::XX; + } else if (parser.EatEqICase("GET")) { + get_ = true; } else { return parser.InvalidSyntax(); } @@ -294,7 +300,7 @@ class CommandSet : public Commander { } Status Execute(Server *srv, Connection *conn, std::string *output) override { - bool ret = false; + std::optional ret; redis::String string_db(srv->storage, conn->GetNamespace()); if (ttl_ < 0) { @@ -307,29 +313,33 @@ class CommandSet : public Commander { } rocksdb::Status s; - if (set_flag_ == NX) { - s = string_db.SetNX(args_[1], args_[2], ttl_, &ret); - } else if (set_flag_ == XX) { - s = string_db.SetXX(args_[1], args_[2], ttl_, &ret); - } else { - s = string_db.SetEX(args_[1], args_[2], ttl_); - } + s = string_db.Set(args_[1], args_[2], ttl_, set_flag_, get_, keep_ttl_, ret); if (!s.ok()) { return {Status::RedisExecErr, s.ToString()}; } - if (set_flag_ != NONE && !ret) { - *output = redis::NilString(); + if (get_) { + if (ret.has_value()) { + *output = redis::NilString(); + } else { + *output = redis::BulkString(*ret); + } } else { - *output = redis::SimpleString("OK"); + if (ret.has_value()) { + *output = redis::SimpleString("OK"); + } else { + *output = redis::NilString(); + } } return Status::OK(); } private: uint64_t ttl_ = 0; - enum { NONE, NX, XX } set_flag_ = NONE; + bool get_ = false; + bool keep_ttl_ = false; + StringSetType set_flag_ = StringSetType::NONE; }; class CommandSetEX : public Commander { diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 6153fe4eeec..7ab8d3bed79 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -188,19 +187,10 @@ rocksdb::Status String::GetEx(const std::string &user_key, std::string *value, u } rocksdb::Status String::GetSet(const std::string &user_key, const std::string &new_value, std::string *old_value) { - std::string ns_key = AppendNamespacePrefix(user_key); - - LockGuard guard(storage_->GetLockManager(), ns_key); - rocksdb::Status s = getValue(ns_key, old_value); - if (!s.ok() && !s.IsNotFound()) return s; - - std::string raw_value; - Metadata metadata(kRedisString, false); - metadata.Encode(&raw_value); - raw_value.append(new_value); - auto write_status = updateRawValue(ns_key, raw_value); - // prev status was used to tell whether old value was empty or not - return !write_status.ok() ? write_status : s; + std::optional ret; + auto s = Set(user_key, new_value, 0, StringSetType::NX, /*get*/ true, /*keep_ttl*/ false, ret); + *old_value = *ret; + return s; } rocksdb::Status String::GetDel(const std::string &user_key, std::string *value) { std::string ns_key = AppendNamespacePrefix(user_key); @@ -217,33 +207,56 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu return MSet(pairs, /*ttl=*/0, /*lock=*/true); } -rocksdb::Status String::SetEX(const std::string &user_key, const std::string &value, uint64_t ttl) { - std::vector pairs{StringPair{user_key, value}}; - return MSet(pairs, /*ttl=*/ttl, /*lock=*/true); -} +rocksdb::Status String::Set(const std::string &user_key, const std::string &value, uint64_t ttl, StringSetType type, + bool get, bool keep_ttl, std::optional &ret) { + std::string ns_key = AppendNamespacePrefix(user_key); + std::string old_value = nullptr; -rocksdb::Status String::SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { - std::vector pairs{StringPair{user_key, value}}; - return MSetNX(pairs, ttl, flag); -} + LockGuard guard(storage_->GetLockManager(), ns_key); + std::string raw_value; + auto s = getRawValue(ns_key, &raw_value); + + if (!s.ok() && !s.IsNotFound()) return s; + auto old_key_found = !s.IsNotFound(); + if (get) { + if (old_key_found) { + // if GET option given: return The previous value of the key. + auto offset = Metadata::GetOffsetAfterExpire(raw_value[0]); + ret = std::make_optional(raw_value.substr(offset)); + } else { + // if GET option given, the key didn't exist before the SET: return nil + ret = std::nullopt; + } + } else { + // if GET option not given, operation aborted: return nil + if (old_key_found && type == StringSetType::NX) { + ret = std::nullopt; + return rocksdb::Status::OK(); + } + // if GET option not given, operation aborted: return nil + else if (!old_key_found && type == StringSetType::XX) { + ret = std::nullopt; + return rocksdb::Status::OK(); + } else { + // make ret not nil + ret = std::make_optional(""); + } + } -rocksdb::Status String::SetXX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { - *flag = false; - int exists = 0; uint64_t expire = 0; if (ttl > 0) { uint64_t now = util::GetTimeStampMS(); expire = now + ttl; + } else if (keep_ttl && old_key_found) { + Metadata metadata(kRedisString, false); + auto s = metadata.Decode(raw_value); + if (!s.ok()) { + return s; + } + expire = metadata.expire; } - std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - auto s = Exists({user_key}, &exists); - if (!s.ok()) return s; - if (exists != 1) return rocksdb::Status::OK(); - - *flag = true; - std::string raw_value; + // create new key Metadata metadata(kRedisString, false); metadata.expire = expire; metadata.Encode(&raw_value); @@ -251,6 +264,25 @@ rocksdb::Status String::SetXX(const std::string &user_key, const std::string &va return updateRawValue(ns_key, raw_value); } +rocksdb::Status String::SetEX(const std::string &user_key, const std::string &value, uint64_t ttl) { + std::optional ret; + return Set(user_key, value, ttl, StringSetType::NONE, /*get*/ false, /*keep_ttl*/ false, ret); +} + +rocksdb::Status String::SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { + std::optional ret; + auto s = Set(user_key, value, ttl, StringSetType::NX, /*get*/ false, /*keep_ttl*/ false, ret); + *flag = ret.has_value(); + return s; +} + +rocksdb::Status String::SetXX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { + std::optional ret; + auto s = Set(user_key, value, ttl, StringSetType::XX, /*get*/ false, /*keep_ttl*/ false, ret); + *flag = ret.has_value(); + return s; +} + rocksdb::Status String::SetRange(const std::string &user_key, size_t offset, const std::string &value, uint64_t *new_size) { std::string ns_key = AppendNamespacePrefix(user_key); diff --git a/src/types/redis_string.h b/src/types/redis_string.h index 41be5bddd0e..3de102187c6 100644 --- a/src/types/redis_string.h +++ b/src/types/redis_string.h @@ -21,6 +21,7 @@ #pragma once #include +#include #include #include @@ -32,6 +33,8 @@ struct StringPair { Slice value; }; +enum class StringSetType { NONE, NX, XX }; + namespace redis { class String : public Database { @@ -43,6 +46,8 @@ class String : public Database { rocksdb::Status GetSet(const std::string &user_key, const std::string &new_value, std::string *old_value); rocksdb::Status GetDel(const std::string &user_key, std::string *value); rocksdb::Status Set(const std::string &user_key, const std::string &value); + rocksdb::Status Set(const std::string &user_key, const std::string &value, uint64_t ttl, StringSetType type, bool get, + bool keep_ttl, std::optional &ret); rocksdb::Status SetEX(const std::string &user_key, const std::string &value, uint64_t ttl); rocksdb::Status SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag); rocksdb::Status SetXX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag); From 5e60dc255dde6da3ef4aa9913acfb9741b38fbb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=AA=E5=8D=8E=E8=A3=95?= <8042833@qq.com> Date: Thu, 14 Dec 2023 02:31:23 +0000 Subject: [PATCH 2/8] fix tidy --- src/types/redis_string.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 7ab8d3bed79..92be20ebdce 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -210,7 +210,6 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu rocksdb::Status String::Set(const std::string &user_key, const std::string &value, uint64_t ttl, StringSetType type, bool get, bool keep_ttl, std::optional &ret) { std::string ns_key = AppendNamespacePrefix(user_key); - std::string old_value = nullptr; LockGuard guard(storage_->GetLockManager(), ns_key); std::string raw_value; From e5590e0662a5aa608443fd161d9142481dd603a3 Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sat, 16 Dec 2023 10:46:27 +0800 Subject: [PATCH 3/8] Refactor GetSet method in redis_string.cc --- src/commands/cmd_string.cc | 12 ++++++------ src/types/redis_string.cc | 24 ++++++++++++------------ src/types/redis_string.h | 3 ++- tests/cppunit/types/string_test.cc | 6 +++--- 4 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/commands/cmd_string.cc b/src/commands/cmd_string.cc index 297beb251df..034688debe3 100644 --- a/src/commands/cmd_string.cc +++ b/src/commands/cmd_string.cc @@ -133,16 +133,16 @@ class CommandGetSet : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { redis::String string_db(srv->storage, conn->GetNamespace()); - std::string old_value; - auto s = string_db.GetSet(args_[1], args_[2], &old_value); - if (!s.ok() && !s.IsNotFound()) { + std::optional old_value; + auto s = string_db.GetSet(args_[1], args_[2], old_value); + if (!s.ok()) { return {Status::RedisExecErr, s.ToString()}; } - if (s.IsNotFound()) { - *output = redis::NilString(); + if (old_value.has_value()) { + *output = redis::BulkString(old_value.value()); } else { - *output = redis::BulkString(old_value); + *output = redis::NilString(); } return Status::OK(); } diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 92be20ebdce..9cb351191aa 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -186,10 +186,9 @@ rocksdb::Status String::GetEx(const std::string &user_key, std::string *value, u return rocksdb::Status::OK(); } -rocksdb::Status String::GetSet(const std::string &user_key, const std::string &new_value, std::string *old_value) { - std::optional ret; - auto s = Set(user_key, new_value, 0, StringSetType::NX, /*get*/ true, /*keep_ttl*/ false, ret); - *old_value = *ret; +rocksdb::Status String::GetSet(const std::string &user_key, const std::string &new_value, + std::optional &old_value) { + auto s = Set(user_key, new_value, 0, StringSetType::NX, /*get*/ true, /*keep_ttl*/ false, old_value); return s; } rocksdb::Status String::GetDel(const std::string &user_key, std::string *value) { @@ -212,16 +211,16 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu std::string ns_key = AppendNamespacePrefix(user_key); LockGuard guard(storage_->GetLockManager(), ns_key); - std::string raw_value; - auto s = getRawValue(ns_key, &raw_value); + std::string old_raw_value; + auto s = getRawValue(ns_key, &old_raw_value); if (!s.ok() && !s.IsNotFound()) return s; auto old_key_found = !s.IsNotFound(); if (get) { if (old_key_found) { // if GET option given: return The previous value of the key. - auto offset = Metadata::GetOffsetAfterExpire(raw_value[0]); - ret = std::make_optional(raw_value.substr(offset)); + auto offset = Metadata::GetOffsetAfterExpire(old_raw_value[0]); + ret = std::make_optional(old_raw_value.substr(offset)); } else { // if GET option given, the key didn't exist before the SET: return nil ret = std::nullopt; @@ -248,7 +247,7 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu expire = now + ttl; } else if (keep_ttl && old_key_found) { Metadata metadata(kRedisString, false); - auto s = metadata.Decode(raw_value); + auto s = metadata.Decode(old_raw_value); if (!s.ok()) { return s; } @@ -256,11 +255,12 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu } // create new key + std::string new_raw_value; Metadata metadata(kRedisString, false); metadata.expire = expire; - metadata.Encode(&raw_value); - raw_value.append(value); - return updateRawValue(ns_key, raw_value); + metadata.Encode(&new_raw_value); + new_raw_value.append(value); + return updateRawValue(ns_key, new_raw_value); } rocksdb::Status String::SetEX(const std::string &user_key, const std::string &value, uint64_t ttl) { diff --git a/src/types/redis_string.h b/src/types/redis_string.h index 3de102187c6..94f1f91becd 100644 --- a/src/types/redis_string.h +++ b/src/types/redis_string.h @@ -43,7 +43,8 @@ class String : public Database { rocksdb::Status Append(const std::string &user_key, const std::string &value, uint64_t *new_size); rocksdb::Status Get(const std::string &user_key, std::string *value); rocksdb::Status GetEx(const std::string &user_key, std::string *value, uint64_t ttl, bool persist); - rocksdb::Status GetSet(const std::string &user_key, const std::string &new_value, std::string *old_value); + rocksdb::Status GetSet(const std::string &user_key, const std::string &new_value, + std::optional &old_value); rocksdb::Status GetDel(const std::string &user_key, std::string *value); rocksdb::Status Set(const std::string &user_key, const std::string &value); rocksdb::Status Set(const std::string &user_key, const std::string &value, uint64_t ttl, StringSetType type, bool get, diff --git a/tests/cppunit/types/string_test.cc b/tests/cppunit/types/string_test.cc index fe916adc5ca..396714f2fe8 100644 --- a/tests/cppunit/types/string_test.cc +++ b/tests/cppunit/types/string_test.cc @@ -142,15 +142,15 @@ TEST_F(RedisStringTest, GetSet) { rocksdb::Env::Default()->GetCurrentTime(&now); std::vector values = {"a", "b", "c", "d"}; for (size_t i = 0; i < values.size(); i++) { - std::string old_value; + std::optional old_value; auto s = string_->Expire(key_, now * 1000 + 100000); - string_->GetSet(key_, values[i], &old_value); + string_->GetSet(key_, values[i], old_value); if (i != 0) { EXPECT_EQ(values[i - 1], old_value); auto s = string_->TTL(key_, &ttl); EXPECT_TRUE(ttl == -1); } else { - EXPECT_TRUE(old_value.empty()); + EXPECT_TRUE(old_value->empty()); } } auto s = string_->Del(key_); From 852420e9b6e982382f8414f2867baaa38cf7a3df Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sat, 16 Dec 2023 11:35:22 +0800 Subject: [PATCH 4/8] Add go test --- src/commands/cmd_string.cc | 4 +- src/types/redis_string.cc | 37 ++++---- .../gocase/unit/type/strings/strings_test.go | 88 +++++++++++++++++++ 3 files changed, 111 insertions(+), 18 deletions(-) diff --git a/src/commands/cmd_string.cc b/src/commands/cmd_string.cc index 034688debe3..13ded4d713a 100644 --- a/src/commands/cmd_string.cc +++ b/src/commands/cmd_string.cc @@ -321,9 +321,9 @@ class CommandSet : public Commander { if (get_) { if (ret.has_value()) { - *output = redis::NilString(); + *output = redis::BulkString(ret.value()); } else { - *output = redis::BulkString(*ret); + *output = redis::NilString(); } } else { if (ret.has_value()) { diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 9cb351191aa..659576f390f 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -188,7 +188,7 @@ rocksdb::Status String::GetEx(const std::string &user_key, std::string *value, u rocksdb::Status String::GetSet(const std::string &user_key, const std::string &new_value, std::optional &old_value) { - auto s = Set(user_key, new_value, 0, StringSetType::NX, /*get*/ true, /*keep_ttl*/ false, old_value); + auto s = Set(user_key, new_value, 0, StringSetType::NONE, /*get*/ true, /*keep_ttl*/ false, old_value); return s; } rocksdb::Status String::GetDel(const std::string &user_key, std::string *value) { @@ -211,36 +211,41 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu std::string ns_key = AppendNamespacePrefix(user_key); LockGuard guard(storage_->GetLockManager(), ns_key); + + // Get old value for NX/XX/GET/KEEPTTL option std::string old_raw_value; auto s = getRawValue(ns_key, &old_raw_value); - if (!s.ok() && !s.IsNotFound()) return s; auto old_key_found = !s.IsNotFound(); + + // The reply following Redis doc: https://redis.io/commands/set/ + // Handle GET option if (get) { if (old_key_found) { // if GET option given: return The previous value of the key. auto offset = Metadata::GetOffsetAfterExpire(old_raw_value[0]); ret = std::make_optional(old_raw_value.substr(offset)); } else { - // if GET option given, the key didn't exist before the SET: return nil + // if GET option given, the key didn't exist before: return nil ret = std::nullopt; } - } else { + } + + // Handle NX/XX option + if (old_key_found && type == StringSetType::NX) { // if GET option not given, operation aborted: return nil - if (old_key_found && type == StringSetType::NX) { - ret = std::nullopt; - return rocksdb::Status::OK(); - } + if (!get) ret = std::nullopt; + return rocksdb::Status::OK(); + } else if (!old_key_found && type == StringSetType::XX) { // if GET option not given, operation aborted: return nil - else if (!old_key_found && type == StringSetType::XX) { - ret = std::nullopt; - return rocksdb::Status::OK(); - } else { - // make ret not nil - ret = std::make_optional(""); - } + if (!get) ret = std::nullopt; + return rocksdb::Status::OK(); + } else { + // if GET option not given, make ret not nil + if (!get) ret = std::make_optional(""); } + // Handle expire time uint64_t expire = 0; if (ttl > 0) { uint64_t now = util::GetTimeStampMS(); @@ -254,7 +259,7 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu expire = metadata.expire; } - // create new key + // Create new value std::string new_raw_value; Metadata metadata(kRedisString, false); metadata.expire = expire; diff --git a/tests/gocase/unit/type/strings/strings_test.go b/tests/gocase/unit/type/strings/strings_test.go index f255228d18f..fc799fc5cd8 100644 --- a/tests/gocase/unit/type/strings/strings_test.go +++ b/tests/gocase/unit/type/strings/strings_test.go @@ -678,6 +678,94 @@ func TestString(t *testing.T) { util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) }) + t.Run("Extended SET KEEPTTL and EX/PX/EXAT/PXAT option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Error(t, rdb.Do(ctx, "SET", "foo", "xx", "keepttl", "ex", "100").Err()) + require.Error(t, rdb.Do(ctx, "SET", "foo", "xx", "keepttl", "px", "100").Err()) + require.Error(t, rdb.Do(ctx, "SET", "foo", "xx", "keepttl", "exat", "100").Err()) + require.Error(t, rdb.Do(ctx, "SET", "foo", "xx", "keepttl", "pxat", "100").Err()) + }) + + t.Run("Extended SET KEEPTTL WITH option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, "OK", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{KeepTTL: true}).Val()) + ttl := rdb.TTL(ctx, "foo").Val() + require.Equal(t, time.Duration(-1), ttl) + require.Equal(t, "OK", rdb.Set(ctx, "foo", "bar", 10*time.Second).Val()) + require.Equal(t, "OK", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{KeepTTL: true}).Val()) + ttl = rdb.TTL(ctx, "foo").Val() + util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) + }) + + t.Run("Extended SET GET option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, "", rdb.SetArgs(ctx, "foo", "bar", redis.SetArgs{Get: true}).Val()) + require.Equal(t, "bar", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true}).Val()) + require.Equal(t, "xx", rdb.Get(ctx, "foo").Val()) + }) + + t.Run("Extended SET GET and NX option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, "", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true, Mode: "NX"}).Val()) + require.Equal(t, "xx", rdb.Get(ctx, "foo").Val()) + require.Equal(t, "OK", rdb.Set(ctx, "foo", "bar", 0).Val()) + require.Equal(t, "bar", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true, Mode: "NX"}).Val()) + require.Equal(t, "bar", rdb.Get(ctx, "foo").Val()) + }) + + t.Run("Extended SET GET and XX option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, "", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true, Mode: "XX"}).Val()) + require.Equal(t, "", rdb.Get(ctx, "foo").Val()) + require.Equal(t, "OK", rdb.Set(ctx, "foo", "bar", 0).Val()) + require.Equal(t, "bar", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true, Mode: "XX"}).Val()) + require.Equal(t, "xx", rdb.Get(ctx, "foo").Val()) + }) + + t.Run("Extended SET GET and KEEPTTL option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, "", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true, KeepTTL: true}).Val()) + ttl := rdb.TTL(ctx, "foo").Val() + require.Equal(t, time.Duration(-1), ttl) + require.Equal(t, "OK", rdb.Set(ctx, "foo", "bar", 10*time.Second).Val()) + require.Equal(t, "bar", rdb.SetArgs(ctx, "foo", "xx", redis.SetArgs{Get: true, KeepTTL: true}).Val()) + ttl = rdb.TTL(ctx, "foo").Val() + util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) + }) + + t.Run("Extended SET GET and EX option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, nil, rdb.Do(ctx, "SET", "foo", "bar", "ex", "10", "get").Val()) + ttl := rdb.TTL(ctx, "foo").Val() + util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) + }) + + t.Run("Extended SET GET and PX option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + require.Equal(t, nil, rdb.Do(ctx, "SET", "foo", "bar", "px", "10000", "get").Val()) + ttl := rdb.TTL(ctx, "foo").Val() + util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) + }) + + t.Run("Extended SET GET and EXAT option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + + expireAt := strconv.FormatInt(time.Now().Add(10*time.Second).Unix(), 10) + require.Equal(t, nil, rdb.Do(ctx, "SET", "foo", "bar", "exat", expireAt, "get").Val()) + ttl := rdb.TTL(ctx, "foo").Val() + util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) + }) + + t.Run("Extended SET GET and PXAT option", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "foo").Err()) + + expireAt := strconv.FormatInt(time.Now().Add(10*time.Second).UnixMilli(), 10) + require.Equal(t, nil, rdb.Do(ctx, "SET", "foo", "bar", "pxat", expireAt, "get").Val()) + + ttl := rdb.TTL(ctx, "foo").Val() + util.BetweenValues(t, ttl, 5*time.Second, 10*time.Second) + }) + t.Run("GETRANGE with huge ranges, Github issue redis/redis#1844", func(t *testing.T) { require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err()) require.Equal(t, "bar", rdb.GetRange(ctx, "foo", 0, 2094967291).Val()) From 2c6797e07fe7f2cf383684c8ddd74883161eaaab Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sat, 16 Dec 2023 13:18:05 +0800 Subject: [PATCH 5/8] Fix error handling in String::Set method --- src/types/redis_string.cc | 6 ++++-- tests/cppunit/types/string_test.cc | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 659576f390f..47d53dc7a69 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -215,12 +215,14 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu // Get old value for NX/XX/GET/KEEPTTL option std::string old_raw_value; auto s = getRawValue(ns_key, &old_raw_value); - if (!s.ok() && !s.IsNotFound()) return s; + if (!s.ok() && !s.IsNotFound() && !s.IsInvalidArgument()) return s; auto old_key_found = !s.IsNotFound(); - // The reply following Redis doc: https://redis.io/commands/set/ // Handle GET option if (get) { + if (s.IsInvalidArgument()) { + return s; + } if (old_key_found) { // if GET option given: return The previous value of the key. auto offset = Metadata::GetOffsetAfterExpire(old_raw_value[0]); diff --git a/tests/cppunit/types/string_test.cc b/tests/cppunit/types/string_test.cc index 396714f2fe8..631b4ec8114 100644 --- a/tests/cppunit/types/string_test.cc +++ b/tests/cppunit/types/string_test.cc @@ -150,7 +150,7 @@ TEST_F(RedisStringTest, GetSet) { auto s = string_->TTL(key_, &ttl); EXPECT_TRUE(ttl == -1); } else { - EXPECT_TRUE(old_value->empty()); + EXPECT_TRUE(!old_value.has_value()); } } auto s = string_->Del(key_); From 95d097b1e44aa9d9def43155a2542d378602428d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=AA=E5=8D=8E=E8=A3=95?= <8042833@qq.com> Date: Sun, 17 Dec 2023 06:17:55 +0800 Subject: [PATCH 6/8] Update src/types/redis_string.cc Co-authored-by: Twice --- src/types/redis_string.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 47d53dc7a69..2d8e9027d81 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -244,7 +244,7 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu return rocksdb::Status::OK(); } else { // if GET option not given, make ret not nil - if (!get) ret = std::make_optional(""); + if (!get) ret = ""; } // Handle expire time From 59b8956be16778b054bd1a5f2c69fe25d1f27401 Mon Sep 17 00:00:00 2001 From: jihuayu <8042833@qq.com> Date: Sun, 17 Dec 2023 06:29:44 +0800 Subject: [PATCH 7/8] Refactor Set to use a struct for arguments --- src/commands/cmd_string.cc | 2 +- src/types/redis_string.cc | 30 +++++++++++++++--------------- src/types/redis_string.h | 11 +++++++++-- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/commands/cmd_string.cc b/src/commands/cmd_string.cc index 13ded4d713a..a0e1a690b5c 100644 --- a/src/commands/cmd_string.cc +++ b/src/commands/cmd_string.cc @@ -313,7 +313,7 @@ class CommandSet : public Commander { } rocksdb::Status s; - s = string_db.Set(args_[1], args_[2], ttl_, set_flag_, get_, keep_ttl_, ret); + s = string_db.Set(args_[1], args_[2], {ttl_, set_flag_, get_, keep_ttl_}, ret); if (!s.ok()) { return {Status::RedisExecErr, s.ToString()}; diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 2d8e9027d81..2fef9f4bd52 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -188,7 +188,7 @@ rocksdb::Status String::GetEx(const std::string &user_key, std::string *value, u rocksdb::Status String::GetSet(const std::string &user_key, const std::string &new_value, std::optional &old_value) { - auto s = Set(user_key, new_value, 0, StringSetType::NONE, /*get*/ true, /*keep_ttl*/ false, old_value); + auto s = Set(user_key, new_value, {/*ttl=*/0, StringSetType::NONE, /*get*/ true, /*keep_ttl*/ false}, old_value); return s; } rocksdb::Status String::GetDel(const std::string &user_key, std::string *value) { @@ -206,8 +206,8 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu return MSet(pairs, /*ttl=*/0, /*lock=*/true); } -rocksdb::Status String::Set(const std::string &user_key, const std::string &value, uint64_t ttl, StringSetType type, - bool get, bool keep_ttl, std::optional &ret) { +rocksdb::Status String::Set(const std::string &user_key, const std::string &value, StringSetArgs args, + std::optional &ret) { std::string ns_key = AppendNamespacePrefix(user_key); LockGuard guard(storage_->GetLockManager(), ns_key); @@ -219,7 +219,7 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu auto old_key_found = !s.IsNotFound(); // The reply following Redis doc: https://redis.io/commands/set/ // Handle GET option - if (get) { + if (args.get) { if (s.IsInvalidArgument()) { return s; } @@ -234,25 +234,25 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu } // Handle NX/XX option - if (old_key_found && type == StringSetType::NX) { + if (old_key_found && args.type == StringSetType::NX) { // if GET option not given, operation aborted: return nil - if (!get) ret = std::nullopt; + if (!args.get) ret = std::nullopt; return rocksdb::Status::OK(); - } else if (!old_key_found && type == StringSetType::XX) { + } else if (!old_key_found && args.type == StringSetType::XX) { // if GET option not given, operation aborted: return nil - if (!get) ret = std::nullopt; + if (!args.get) ret = std::nullopt; return rocksdb::Status::OK(); } else { // if GET option not given, make ret not nil - if (!get) ret = ""; + if (!args.get) ret = ""; } // Handle expire time uint64_t expire = 0; - if (ttl > 0) { + if (args.ttl > 0) { uint64_t now = util::GetTimeStampMS(); - expire = now + ttl; - } else if (keep_ttl && old_key_found) { + expire = now + args.ttl; + } else if (args.keep_ttl && old_key_found) { Metadata metadata(kRedisString, false); auto s = metadata.Decode(old_raw_value); if (!s.ok()) { @@ -272,19 +272,19 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu rocksdb::Status String::SetEX(const std::string &user_key, const std::string &value, uint64_t ttl) { std::optional ret; - return Set(user_key, value, ttl, StringSetType::NONE, /*get*/ false, /*keep_ttl*/ false, ret); + return Set(user_key, value, {ttl, StringSetType::NONE, /*get*/ false, /*keep_ttl*/ false}, ret); } rocksdb::Status String::SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { std::optional ret; - auto s = Set(user_key, value, ttl, StringSetType::NX, /*get*/ false, /*keep_ttl*/ false, ret); + auto s = Set(user_key, value, {ttl, StringSetType::NX, /*get*/ false, /*keep_ttl*/ false}, ret); *flag = ret.has_value(); return s; } rocksdb::Status String::SetXX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { std::optional ret; - auto s = Set(user_key, value, ttl, StringSetType::XX, /*get*/ false, /*keep_ttl*/ false, ret); + auto s = Set(user_key, value, {ttl, StringSetType::XX, /*get*/ false, /*keep_ttl*/ false}, ret); *flag = ret.has_value(); return s; } diff --git a/src/types/redis_string.h b/src/types/redis_string.h index 94f1f91becd..bfb4ef99005 100644 --- a/src/types/redis_string.h +++ b/src/types/redis_string.h @@ -35,6 +35,13 @@ struct StringPair { enum class StringSetType { NONE, NX, XX }; +struct StringSetArgs { + uint64_t ttl; + StringSetType type; + bool get; + bool keep_ttl; +}; + namespace redis { class String : public Database { @@ -47,8 +54,8 @@ class String : public Database { std::optional &old_value); rocksdb::Status GetDel(const std::string &user_key, std::string *value); rocksdb::Status Set(const std::string &user_key, const std::string &value); - rocksdb::Status Set(const std::string &user_key, const std::string &value, uint64_t ttl, StringSetType type, bool get, - bool keep_ttl, std::optional &ret); + rocksdb::Status Set(const std::string &user_key, const std::string &value, StringSetArgs args, + std::optional &ret); rocksdb::Status SetEX(const std::string &user_key, const std::string &value, uint64_t ttl); rocksdb::Status SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag); rocksdb::Status SetXX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag); From 888e0ca45cdf2143a3c663e29ffaa3b16d605ea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=AA=E5=8D=8E=E8=A3=95?= <8042833@qq.com> Date: Mon, 18 Dec 2023 03:32:11 +0000 Subject: [PATCH 8/8] format code --- src/types/redis_string.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 2fef9f4bd52..420d3994f9e 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -188,7 +188,7 @@ rocksdb::Status String::GetEx(const std::string &user_key, std::string *value, u rocksdb::Status String::GetSet(const std::string &user_key, const std::string &new_value, std::optional &old_value) { - auto s = Set(user_key, new_value, {/*ttl=*/0, StringSetType::NONE, /*get*/ true, /*keep_ttl*/ false}, old_value); + auto s = Set(user_key, new_value, {/*ttl=*/0, StringSetType::NONE, /*get=*/true, /*keep_ttl=*/false}, old_value); return s; } rocksdb::Status String::GetDel(const std::string &user_key, std::string *value) { @@ -272,19 +272,19 @@ rocksdb::Status String::Set(const std::string &user_key, const std::string &valu rocksdb::Status String::SetEX(const std::string &user_key, const std::string &value, uint64_t ttl) { std::optional ret; - return Set(user_key, value, {ttl, StringSetType::NONE, /*get*/ false, /*keep_ttl*/ false}, ret); + return Set(user_key, value, {ttl, StringSetType::NONE, /*get=*/false, /*keep_ttl=*/false}, ret); } rocksdb::Status String::SetNX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { std::optional ret; - auto s = Set(user_key, value, {ttl, StringSetType::NX, /*get*/ false, /*keep_ttl*/ false}, ret); + auto s = Set(user_key, value, {ttl, StringSetType::NX, /*get=*/false, /*keep_ttl=*/false}, ret); *flag = ret.has_value(); return s; } rocksdb::Status String::SetXX(const std::string &user_key, const std::string &value, uint64_t ttl, bool *flag) { std::optional ret; - auto s = Set(user_key, value, {ttl, StringSetType::XX, /*get*/ false, /*keep_ttl*/ false}, ret); + auto s = Set(user_key, value, {ttl, StringSetType::XX, /*get=*/false, /*keep_ttl=*/false}, ret); *flag = ret.has_value(); return s; }