Skip to content

Commit fbd986c

Browse files
kirito632kirito632
committed
feat(string): support IFEQ/IFNE/IFDEQ/IFDNE conditions for SET command
- Add conditional SET options: IFEQ, IFNE, IFDEQ, IFDNE - Propagate WRONGTYPE error when key type mismatch for conditional operations - Handle NX option with wrong type correctly (treat as key exists) - Add test case for SET NX GET on wrong type This commit only includes SET command changes, DelEX changes will be in a separate PR as requested.
1 parent 505108c commit fbd986c

4 files changed

Lines changed: 82 additions & 96 deletions

File tree

‎src/commands/cmd_string.cc‎

Lines changed: 14 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -108,51 +108,6 @@ class CommandGetEx : public Commander {
108108
std::optional<uint64_t> expire_;
109109
};
110110

111-
class CommandDelEX : public Commander {
112-
public:
113-
Status Parse(const std::vector<std::string> &args) override {
114-
if (args.size() > 4) {
115-
return {Status::RedisParseErr, errWrongNumOfArguments};
116-
}
117-
118-
CommandParser parser(args, 2);
119-
while (parser.Good()) {
120-
if (parser.EatEqICase("ifdeq")) {
121-
option_ = {DelExOption::IFDEQ, GET_OR_RET(parser.TakeStr())};
122-
} else if (parser.EatEqICase("ifdne")) {
123-
option_ = {DelExOption::IFDNE, GET_OR_RET(parser.TakeStr())};
124-
} else if (parser.EatEqICase("ifeq")) {
125-
option_ = {DelExOption::IFEQ, GET_OR_RET(parser.TakeStr())};
126-
} else if (parser.EatEqICase("ifne")) {
127-
option_ = {DelExOption::IFNE, GET_OR_RET(parser.TakeStr())};
128-
} else {
129-
return {Status::RedisParseErr, errInvalidSyntax};
130-
}
131-
}
132-
return Status::OK();
133-
}
134-
135-
Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override {
136-
redis::String string_db(srv->storage, conn->GetNamespace());
137-
bool deleted = false;
138-
auto s = string_db.DelEX(ctx, args_[1], option_, deleted);
139-
140-
if (!s.ok() && !s.IsNotFound()) {
141-
return {Status::RedisExecErr, s.ToString()};
142-
}
143-
144-
if (s.IsNotFound() || !deleted) {
145-
*output = redis::Integer(0);
146-
} else {
147-
*output = redis::Integer(1);
148-
}
149-
return Status::OK();
150-
}
151-
152-
private:
153-
DelExOption option_;
154-
};
155-
156111
class CommandStrlen : public Commander {
157112
public:
158113
Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override {
@@ -344,6 +299,18 @@ class CommandSet : public Commander {
344299
set_flag_ = StringSetType::NX;
345300
} else if (parser.EatEqICaseFlag("XX", set_flag)) {
346301
set_flag_ = StringSetType::XX;
302+
} else if (parser.EatEqICaseFlag("IFEQ", set_flag)) {
303+
set_flag_ = StringSetType::IFEQ;
304+
cmp_value_ = GET_OR_RET(parser.TakeStr());
305+
} else if (parser.EatEqICaseFlag("IFNE", set_flag)) {
306+
set_flag_ = StringSetType::IFNE;
307+
cmp_value_ = GET_OR_RET(parser.TakeStr());
308+
} else if (parser.EatEqICaseFlag("IFDEQ", set_flag)) {
309+
set_flag_ = StringSetType::IFDEQ;
310+
cmp_value_ = GET_OR_RET(parser.TakeStr());
311+
} else if (parser.EatEqICaseFlag("IFDNE", set_flag)) {
312+
set_flag_ = StringSetType::IFDNE;
313+
cmp_value_ = GET_OR_RET(parser.TakeStr());
347314
} else if (parser.EatEqICase("GET")) {
348315
get_ = true;
349316
} else {
@@ -358,7 +325,7 @@ class CommandSet : public Commander {
358325
std::optional<std::string> ret;
359326
redis::String string_db(srv->storage, conn->GetNamespace());
360327

361-
rocksdb::Status s = string_db.Set(ctx, args_[1], args_[2], {expire_, set_flag_, get_, keep_ttl_}, ret);
328+
rocksdb::Status s = string_db.Set(ctx, args_[1], args_[2], {expire_, set_flag_, get_, keep_ttl_, cmp_value_}, ret);
362329

363330
if (!s.ok()) {
364331
return {Status::RedisExecErr, s.ToString()};
@@ -385,6 +352,7 @@ class CommandSet : public Commander {
385352
bool get_ = false;
386353
bool keep_ttl_ = false;
387354
StringSetType set_flag_ = StringSetType::NONE;
355+
std::string cmp_value_;
388356
};
389357

390358
class CommandSetEX : public Commander {
@@ -857,7 +825,6 @@ REDIS_REGISTER_COMMANDS(
857825
MakeCmdAttr<CommandGetRange>("getrange", 4, "read-only", 1, 1, 1),
858826
MakeCmdAttr<CommandSubStr>("substr", 4, "read-only", 1, 1, 1),
859827
MakeCmdAttr<CommandGetDel>("getdel", 2, "write no-dbsize-check", 1, 1, 1),
860-
MakeCmdAttr<CommandDelEX>("delex", -2, "write", 1, 1, 1),
861828
MakeCmdAttr<CommandSetRange>("setrange", 4, "write", 1, 1, 1),
862829
MakeCmdAttr<CommandMGet>("mget", -2, "read-only", 1, -1, 1),
863830
MakeCmdAttr<CommandAppend>("append", 3, "write", 1, 1, 1), MakeCmdAttr<CommandSet>("set", -3, "write", 1, 1, 1),

‎src/types/redis_string.cc‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -183,41 +183,6 @@ rocksdb::Status String::GetEx(engine::Context &ctx, const std::string &user_key,
183183
return rocksdb::Status::OK();
184184
}
185185

186-
rocksdb::Status String::DelEX(engine::Context &ctx, const std::string &user_key, const DelExOption &option,
187-
bool &deleted) {
188-
deleted = false;
189-
std::string val;
190-
std::string ns_key = AppendNamespacePrefix(user_key);
191-
rocksdb::Status s = getValue(ctx, ns_key, &val);
192-
if (!s.ok()) return s;
193-
194-
bool matched = false;
195-
switch (option.type) {
196-
case DelExOption::NONE:
197-
matched = true;
198-
break;
199-
case DelExOption::IFDEQ:
200-
matched = option.value == util::StringDigest(val);
201-
break;
202-
case DelExOption::IFDNE:
203-
matched = option.value != util::StringDigest(val);
204-
break;
205-
case DelExOption::IFEQ:
206-
matched = option.value == val;
207-
break;
208-
case DelExOption::IFNE:
209-
matched = option.value != val;
210-
break;
211-
default:
212-
return rocksdb::Status::InvalidArgument();
213-
}
214-
if (matched) {
215-
s = storage_->Delete(ctx, storage_->DefaultWriteOptions(), metadata_cf_handle_, ns_key);
216-
deleted = s.ok();
217-
}
218-
return s;
219-
}
220-
221186
rocksdb::Status String::GetSet(engine::Context &ctx, const std::string &user_key, const std::string &new_value,
222187
std::optional<std::string> &old_value) {
223188
auto s =
@@ -239,7 +204,7 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c
239204
}
240205

241206
rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, const std::string &value,
242-
StringSetArgs args, std::optional<std::string> &ret) {
207+
const StringSetArgs &args, std::optional<std::string> &ret) {
243208
uint64_t expire = 0;
244209
std::string ns_key = AppendNamespacePrefix(user_key);
245210

@@ -249,6 +214,21 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c
249214
uint64_t old_expire = 0;
250215
auto s = getValueAndExpire(ctx, ns_key, &old_value, &old_expire);
251216
if (!s.ok() && !s.IsNotFound() && !s.IsInvalidArgument()) return s;
217+
// If the existing key is not a string type, enforce expected behaviors:
218+
if (s.IsInvalidArgument()) {
219+
// For conditional comparisons (IFEQ/IFNE/IFDEQ/IFDNE), reading the old value is required,
220+
// so return the underlying WRONGTYPE (InvalidArgument) error.
221+
if (args.type == StringSetType::IFEQ || args.type == StringSetType::IFNE || args.type == StringSetType::IFDEQ ||
222+
args.type == StringSetType::IFDNE) {
223+
return s;
224+
}
225+
// For NX option, treat a wrong type as "key exists" so the condition is not met.
226+
if (args.type == StringSetType::NX) {
227+
if (!args.get) ret = std::nullopt;
228+
return rocksdb::Status::OK();
229+
}
230+
// For other options, continue (e.g., XX may still proceed since key exists).
231+
}
252232
// GET option
253233
if (args.get) {
254234
if (s.IsInvalidArgument()) {
@@ -271,6 +251,38 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c
271251
// if XX option given, the key didn't exist before: return nil
272252
if (!args.get) ret = std::nullopt;
273253
return rocksdb::Status::OK();
254+
} else if (args.type == StringSetType::IFEQ) {
255+
// condition met only when key exists AND value matches
256+
bool matched = s.ok() && (old_value == args.cmp_value);
257+
if (!matched) {
258+
if (!args.get) ret = std::nullopt;
259+
return rocksdb::Status::OK();
260+
}
261+
if (!args.get) ret = "";
262+
} else if (args.type == StringSetType::IFNE) {
263+
// condition not met when key exists AND value matches; key-not-found counts as met
264+
bool not_matched = s.ok() && (old_value == args.cmp_value);
265+
if (not_matched) {
266+
if (!args.get) ret = std::nullopt;
267+
return rocksdb::Status::OK();
268+
}
269+
if (!args.get) ret = "";
270+
} else if (args.type == StringSetType::IFDEQ) {
271+
// condition met only when key exists AND digest matches
272+
bool matched = s.ok() && (util::StringDigest(old_value) == args.cmp_value);
273+
if (!matched) {
274+
if (!args.get) ret = std::nullopt;
275+
return rocksdb::Status::OK();
276+
}
277+
if (!args.get) ret = "";
278+
} else if (args.type == StringSetType::IFDNE) {
279+
// condition not met when key exists AND digest matches; key-not-found counts as met
280+
bool not_matched = s.ok() && (util::StringDigest(old_value) == args.cmp_value);
281+
if (not_matched) {
282+
if (!args.get) ret = std::nullopt;
283+
return rocksdb::Status::OK();
284+
}
285+
if (!args.get) ret = "";
274286
} else {
275287
// if GET option not given, make ret not nil
276288
if (!args.get) ret = "";

‎src/types/redis_string.h‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,23 +34,15 @@ struct StringPair {
3434
Slice value;
3535
};
3636

37-
struct DelExOption {
38-
enum Type { NONE, IFDEQ, IFDNE, IFEQ, IFNE };
39-
Type type;
40-
std::string value;
41-
42-
DelExOption() : type(NONE) {}
43-
DelExOption(Type type, std::string value) : type(type), value(std::move(value)) {}
44-
};
45-
46-
enum class StringSetType { NONE, NX, XX };
37+
enum class StringSetType { NONE, NX, XX, IFEQ, IFNE, IFDEQ, IFDNE };
4738

4839
struct StringSetArgs {
4940
// Expire time in mill seconds.
5041
uint64_t expire;
5142
StringSetType type;
5243
bool get;
5344
bool keep_ttl;
45+
std::string cmp_value; // valid only when type is IFEQ/IFNE/IFDEQ/IFDNE
5446
};
5547

5648
struct StringMSetArgs {
@@ -98,13 +90,12 @@ class String : public Database {
9890
rocksdb::Status Get(engine::Context &ctx, const std::string &user_key, std::string *value);
9991
rocksdb::Status GetEx(engine::Context &ctx, const std::string &user_key, std::string *value,
10092
std::optional<uint64_t> expire);
101-
rocksdb::Status DelEX(engine::Context &ctx, const std::string &user_key, const DelExOption &option, bool &deleted);
10293
rocksdb::Status GetSet(engine::Context &ctx, const std::string &user_key, const std::string &new_value,
10394
std::optional<std::string> &old_value);
10495
rocksdb::Status GetDel(engine::Context &ctx, const std::string &user_key, std::string *value);
10596
rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value);
106-
rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value, StringSetArgs args,
107-
std::optional<std::string> &ret);
97+
rocksdb::Status Set(engine::Context &ctx, const std::string &user_key, const std::string &value,
98+
const StringSetArgs &args, std::optional<std::string> &ret);
10899
rocksdb::Status SetEX(engine::Context &ctx, const std::string &user_key, const std::string &value,
109100
uint64_t expire_ms);
110101
rocksdb::Status SetNX(engine::Context &ctx, const std::string &user_key, const std::string &value, uint64_t expire_ms,

‎tests/gocase/unit/type/strings/strings_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,3 +1205,19 @@ func testString(t *testing.T, configs util.KvrocksServerConfigs) {
12051205
require.Equal(t, []redis.LCSMatchedPosition{}, rdb.LCS(ctx, &redis.LCSQuery{Key1: "virus1", Key2: "virus2", Idx: true, WithMatchLen: true}).Val().Matches)
12061206
})
12071207
}
1208+
1209+
func TestSetConditional(t *testing.T) {
1210+
srv := util.StartServer(t, map[string]string{})
1211+
defer srv.Close()
1212+
ctx := context.Background()
1213+
rdb := srv.NewClient()
1214+
defer func() { require.NoError(t, rdb.Close()) }()
1215+
1216+
// Failure scenario: Extended SET GET and NX option on wrong type
1217+
t.Run("Extended SET GET and NX option on wrong type", func(t *testing.T) {
1218+
require.NoError(t, rdb.Del(ctx, "listkey").Err())
1219+
require.NoError(t, rdb.LPush(ctx, "listkey", "v1").Err())
1220+
1221+
require.ErrorContains(t, rdb.Do(ctx, "SET", "listkey", "v", "NX", "GET").Err(), "WRONGTYPE")
1222+
})
1223+
}

0 commit comments

Comments
 (0)