diff --git a/src/commands/cmd_string.cc b/src/commands/cmd_string.cc index d9ff39770de..900d8fe2d8a 100644 --- a/src/commands/cmd_string.cc +++ b/src/commands/cmd_string.cc @@ -108,51 +108,6 @@ class CommandGetEx : public Commander { std::optional expire_; }; -class CommandDelEX : public Commander { - public: - Status Parse(const std::vector &args) override { - if (args.size() > 4) { - return {Status::RedisParseErr, errWrongNumOfArguments}; - } - - CommandParser parser(args, 2); - while (parser.Good()) { - if (parser.EatEqICase("ifdeq")) { - option_ = {DelExOption::IFDEQ, GET_OR_RET(parser.TakeStr())}; - } else if (parser.EatEqICase("ifdne")) { - option_ = {DelExOption::IFDNE, GET_OR_RET(parser.TakeStr())}; - } else if (parser.EatEqICase("ifeq")) { - option_ = {DelExOption::IFEQ, GET_OR_RET(parser.TakeStr())}; - } else if (parser.EatEqICase("ifne")) { - option_ = {DelExOption::IFNE, GET_OR_RET(parser.TakeStr())}; - } else { - return {Status::RedisParseErr, errInvalidSyntax}; - } - } - return Status::OK(); - } - - Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override { - redis::String string_db(srv->storage, conn->GetNamespace()); - bool deleted = false; - auto s = string_db.DelEX(ctx, args_[1], option_, deleted); - - if (!s.ok() && !s.IsNotFound()) { - return {Status::RedisExecErr, s.ToString()}; - } - - if (s.IsNotFound() || !deleted) { - *output = redis::Integer(0); - } else { - *output = redis::Integer(1); - } - return Status::OK(); - } - - private: - DelExOption option_; -}; - class CommandStrlen : public Commander { public: Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override { @@ -344,6 +299,18 @@ class CommandSet : public Commander { set_flag_ = StringSetType::NX; } else if (parser.EatEqICaseFlag("XX", set_flag)) { set_flag_ = StringSetType::XX; + } else if (parser.EatEqICaseFlag("IFEQ", set_flag)) { + set_flag_ = StringSetType::IFEQ; + cmp_value_ = GET_OR_RET(parser.TakeStr()); + } else if (parser.EatEqICaseFlag("IFNE", set_flag)) { + set_flag_ = StringSetType::IFNE; + cmp_value_ = GET_OR_RET(parser.TakeStr()); + } else if (parser.EatEqICaseFlag("IFDEQ", set_flag)) { + set_flag_ = StringSetType::IFDEQ; + cmp_value_ = GET_OR_RET(parser.TakeStr()); + } else if (parser.EatEqICaseFlag("IFDNE", set_flag)) { + set_flag_ = StringSetType::IFDNE; + cmp_value_ = GET_OR_RET(parser.TakeStr()); } else if (parser.EatEqICase("GET")) { get_ = true; } else { @@ -358,7 +325,7 @@ class CommandSet : public Commander { std::optional ret; redis::String string_db(srv->storage, conn->GetNamespace()); - rocksdb::Status s = string_db.Set(ctx, args_[1], args_[2], {expire_, set_flag_, get_, keep_ttl_}, ret); + rocksdb::Status s = string_db.Set(ctx, args_[1], args_[2], {expire_, set_flag_, get_, keep_ttl_, cmp_value_}, ret); if (!s.ok()) { return {Status::RedisExecErr, s.ToString()}; @@ -385,6 +352,7 @@ class CommandSet : public Commander { bool get_ = false; bool keep_ttl_ = false; StringSetType set_flag_ = StringSetType::NONE; + std::string cmp_value_; }; class CommandSetEX : public Commander { @@ -857,7 +825,6 @@ REDIS_REGISTER_COMMANDS( MakeCmdAttr("getrange", 4, "read-only", 1, 1, 1), MakeCmdAttr("substr", 4, "read-only", 1, 1, 1), MakeCmdAttr("getdel", 2, "write no-dbsize-check", 1, 1, 1), - MakeCmdAttr("delex", -2, "write", 1, 1, 1), MakeCmdAttr("setrange", 4, "write", 1, 1, 1), MakeCmdAttr("mget", -2, "read-only", 1, -1, 1), MakeCmdAttr("append", 3, "write", 1, 1, 1), MakeCmdAttr("set", -3, "write", 1, 1, 1), diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index e056d341355..d08c33a69e7 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -183,41 +183,6 @@ rocksdb::Status String::GetEx(engine::Context &ctx, const std::string &user_key, return rocksdb::Status::OK(); } -rocksdb::Status String::DelEX(engine::Context &ctx, const std::string &user_key, const DelExOption &option, - bool &deleted) { - deleted = false; - std::string val; - std::string ns_key = AppendNamespacePrefix(user_key); - rocksdb::Status s = getValue(ctx, ns_key, &val); - if (!s.ok()) return s; - - bool matched = false; - switch (option.type) { - case DelExOption::NONE: - matched = true; - break; - case DelExOption::IFDEQ: - matched = option.value == util::StringDigest(val); - break; - case DelExOption::IFDNE: - matched = option.value != util::StringDigest(val); - break; - case DelExOption::IFEQ: - matched = option.value == val; - break; - case DelExOption::IFNE: - matched = option.value != val; - break; - default: - return rocksdb::Status::InvalidArgument(); - } - if (matched) { - s = storage_->Delete(ctx, storage_->DefaultWriteOptions(), metadata_cf_handle_, ns_key); - deleted = s.ok(); - } - return s; -} - rocksdb::Status String::GetSet(engine::Context &ctx, const std::string &user_key, const std::string &new_value, std::optional &old_value) { auto s = @@ -239,7 +204,7 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c } rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, const std::string &value, - StringSetArgs args, std::optional &ret) { + const StringSetArgs &args, std::optional &ret) { uint64_t expire = 0; std::string ns_key = AppendNamespacePrefix(user_key); @@ -249,6 +214,21 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c uint64_t old_expire = 0; auto s = getValueAndExpire(ctx, ns_key, &old_value, &old_expire); if (!s.ok() && !s.IsNotFound() && !s.IsInvalidArgument()) return s; + // If the existing key is not a string type, enforce expected behaviors: + if (s.IsInvalidArgument()) { + // For conditional comparisons (IFEQ/IFNE/IFDEQ/IFDNE), reading the old value is required, + // so return the underlying WRONGTYPE (InvalidArgument) error. + if (args.type == StringSetType::IFEQ || args.type == StringSetType::IFNE || args.type == StringSetType::IFDEQ || + args.type == StringSetType::IFDNE) { + return s; + } + // For NX option, treat a wrong type as "key exists" so the condition is not met. + if (args.type == StringSetType::NX) { + if (!args.get) ret = std::nullopt; + return rocksdb::Status::OK(); + } + // For other options, continue (e.g., XX may still proceed since key exists). + } // GET option if (args.get) { if (s.IsInvalidArgument()) { @@ -271,6 +251,38 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c // if XX option given, the key didn't exist before: return nil if (!args.get) ret = std::nullopt; return rocksdb::Status::OK(); + } else if (args.type == StringSetType::IFEQ) { + // condition met only when key exists AND value matches + bool matched = s.ok() && (old_value == args.cmp_value); + if (!matched) { + if (!args.get) ret = std::nullopt; + return rocksdb::Status::OK(); + } + if (!args.get) ret = ""; + } else if (args.type == StringSetType::IFNE) { + // condition not met when key exists AND value matches; key-not-found counts as met + bool not_matched = s.ok() && (old_value == args.cmp_value); + if (not_matched) { + if (!args.get) ret = std::nullopt; + return rocksdb::Status::OK(); + } + if (!args.get) ret = ""; + } else if (args.type == StringSetType::IFDEQ) { + // condition met only when key exists AND digest matches + bool matched = s.ok() && (util::StringDigest(old_value) == args.cmp_value); + if (!matched) { + if (!args.get) ret = std::nullopt; + return rocksdb::Status::OK(); + } + if (!args.get) ret = ""; + } else if (args.type == StringSetType::IFDNE) { + // condition not met when key exists AND digest matches; key-not-found counts as met + bool not_matched = s.ok() && (util::StringDigest(old_value) == args.cmp_value); + if (not_matched) { + if (!args.get) ret = std::nullopt; + return rocksdb::Status::OK(); + } + if (!args.get) ret = ""; } else { // if GET option not given, make ret not nil if (!args.get) ret = ""; diff --git a/src/types/redis_string.h b/src/types/redis_string.h index b160d3d6da3..8a1539f462e 100644 --- a/src/types/redis_string.h +++ b/src/types/redis_string.h @@ -34,16 +34,7 @@ struct StringPair { Slice value; }; -struct DelExOption { - enum Type { NONE, IFDEQ, IFDNE, IFEQ, IFNE }; - Type type; - std::string value; - - DelExOption() : type(NONE) {} - DelExOption(Type type, std::string value) : type(type), value(std::move(value)) {} -}; - -enum class StringSetType { NONE, NX, XX }; +enum class StringSetType { NONE, NX, XX, IFEQ, IFNE, IFDEQ, IFDNE }; struct StringSetArgs { // Expire time in mill seconds. @@ -51,6 +42,7 @@ struct StringSetArgs { StringSetType type; bool get; bool keep_ttl; + std::string cmp_value; // valid only when type is IFEQ/IFNE/IFDEQ/IFDNE }; struct StringMSetArgs { @@ -98,13 +90,12 @@ class String : public Database { rocksdb::Status Get(engine::Context &ctx, const std::string &user_key, std::string *value); rocksdb::Status GetEx(engine::Context &ctx, const std::string &user_key, std::string *value, std::optional expire); - rocksdb::Status DelEX(engine::Context &ctx, const std::string &user_key, const DelExOption &option, bool &deleted); rocksdb::Status GetSet(engine::Context &ctx, const std::string &user_key, const std::string &new_value, std::optional &old_value); rocksdb::Status GetDel(engine::Context &ctx, const std::string &user_key, std::string *value); rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value); - rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value, StringSetArgs args, - std::optional &ret); + rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value, + const StringSetArgs &args, std::optional &ret); rocksdb::Status SetEX(engine::Context &ctx, const std::string &user_key, const std::string &value, uint64_t expire_ms); rocksdb::Status SetNX(engine::Context &ctx, const std::string &user_key, const std::string &value, uint64_t expire_ms, diff --git a/tests/cppunit/types/string_test.cc b/tests/cppunit/types/string_test.cc index d7f79eab0f6..9bf44fb0d30 100644 --- a/tests/cppunit/types/string_test.cc +++ b/tests/cppunit/types/string_test.cc @@ -191,151 +191,6 @@ TEST_F(RedisStringTest, GetSet) { auto s = string_->Del(*ctx_, key_); } -TEST_F(RedisStringTest, DelEX) { - DelExOption option = {DelExOption::NONE, ""}; - bool deleted = false; - - std::string key = "test-string-key69"; - std::string value = "test-strings-value69"; - auto status = string_->Set(*ctx_, key, value); - ASSERT_TRUE(status.ok()); - status = string_->Get(*ctx_, key, &value); - ASSERT_TRUE(status.ok() && !status.IsNotFound()); - EXPECT_EQ("test-strings-value69", value); - - // Check no args delete works - auto s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_TRUE(deleted); - EXPECT_EQ(option.type, DelExOption::NONE); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(!status.ok() && status.IsNotFound()); - EXPECT_NE("test-strings-value69", value); - - // Check no args delete on same key - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.IsNotFound()); - EXPECT_FALSE(deleted); - - // Check no args delete on invalid/notfound key - key = "random"; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.IsNotFound()); - EXPECT_FALSE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(!status.ok() && status.IsNotFound()); - - // Checking true false cases for all args - key = "test-string-key69"; - value = "test-strings-value69"; - status = string_->Set(*ctx_, key, value); - EXPECT_TRUE(status.ok()); - option.type = DelExOption::IFDEQ; - option.value = "xxxxxxxxxxxxxxxx"; - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_FALSE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(status.ok() && !status.IsNotFound()); - EXPECT_EQ("test-strings-value69", value); - - option.type = DelExOption::IFDEQ; - option.value = util::StringDigest(value); - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_TRUE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(!status.ok()); - EXPECT_TRUE(status.IsNotFound()); - EXPECT_NE("test-strings-value69", value); - - key = "test-string-key69"; - value = "test-strings-value69"; - status = string_->Set(*ctx_, key, value); - EXPECT_TRUE(status.ok()); - option.type = DelExOption::IFDNE; - option.value = util::StringDigest(value); - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_FALSE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(status.ok() && !status.IsNotFound()); - EXPECT_EQ("test-strings-value69", value); - - option.type = DelExOption::IFDNE; - option.value = "xxxxxxxxxxxxxxxx"; - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_TRUE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(!status.ok()); - EXPECT_TRUE(status.IsNotFound()); - EXPECT_NE("test-strings-value69", value); - - key = "test-string-key69"; - value = "test-strings-value69"; - status = string_->Set(*ctx_, key, value); - EXPECT_TRUE(status.ok()); - option.type = DelExOption::IFEQ; - option.value = "random"; - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_FALSE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(status.ok() && !status.IsNotFound()); - EXPECT_EQ("test-strings-value69", value); - - option.type = DelExOption::IFEQ; - option.value = "test-strings-value69"; - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_TRUE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(!status.ok()); - EXPECT_TRUE(status.IsNotFound()); - EXPECT_NE("test-strings-value69", value); - - key = "test-string-key69"; - value = "test-strings-value69"; - status = string_->Set(*ctx_, key, value); - EXPECT_TRUE(status.ok()); - option.type = DelExOption::IFNE; - option.value = "test-strings-value69"; - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_FALSE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(status.ok() && !status.IsNotFound()); - EXPECT_EQ("test-strings-value69", value); - - option.type = DelExOption::IFNE; - option.value = "random"; - deleted = false; - s = string_->DelEX(*ctx_, key, option, deleted); - EXPECT_TRUE(s.ok()); - EXPECT_FALSE(s.IsNotFound()); - EXPECT_TRUE(deleted); - status = string_->Get(*ctx_, key, &value); - EXPECT_TRUE(!status.ok()); - EXPECT_TRUE(status.IsNotFound()); - EXPECT_NE("test-strings-value69", value); -} - TEST_F(RedisStringTest, GetDel) { for (auto &pair : pairs_) { string_->Set(*ctx_, pair.key.ToString(), pair.value.ToString()); diff --git a/tests/gocase/unit/type/strings/strings_test.go b/tests/gocase/unit/type/strings/strings_test.go index 8f6648a51ef..921ecde3cc3 100644 --- a/tests/gocase/unit/type/strings/strings_test.go +++ b/tests/gocase/unit/type/strings/strings_test.go @@ -1205,3 +1205,19 @@ func testString(t *testing.T, configs util.KvrocksServerConfigs) { require.Equal(t, []redis.LCSMatchedPosition{}, rdb.LCS(ctx, &redis.LCSQuery{Key1: "virus1", Key2: "virus2", Idx: true, WithMatchLen: true}).Val().Matches) }) } + +func TestSetConditional(t *testing.T) { + srv := util.StartServer(t, map[string]string{}) + defer srv.Close() + ctx := context.Background() + rdb := srv.NewClient() + defer func() { require.NoError(t, rdb.Close()) }() + + // Failure scenario: Extended SET GET and NX option on wrong type + t.Run("Extended SET GET and NX option on wrong type", func(t *testing.T) { + require.NoError(t, rdb.Del(ctx, "listkey").Err()) + require.NoError(t, rdb.LPush(ctx, "listkey", "v1").Err()) + + require.ErrorContains(t, rdb.Do(ctx, "SET", "listkey", "v", "NX", "GET").Err(), "WRONGTYPE") + }) +}