addressing comments and adding tests

Signed-off-by: Sarthak Aggarwal <[email protected]>

Author: sarthakaggarwal97

src/commands.def

@@ -10666,6 +10666,7 @@ struct COMMAND_ARG SET_expiration_Subargs[] = {
 struct COMMAND_ARG SET_Args[] = {
 {MAKE_ARG("key",ARG_TYPE_KEY,0,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
 {MAKE_ARG("value",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)},
+{MAKE_ARG("ifeq",ARG_TYPE_STRING,-1,NULL,"Sets the key's value only if the current value matches the specified comparison value.",NULL,CMD_ARG_OPTIONAL,0,NULL)},
 {MAKE_ARG("condition",ARG_TYPE_ONEOF,-1,NULL,NULL,"2.6.12",CMD_ARG_OPTIONAL,2,NULL),.subargs=SET_condition_Subargs},
 {MAKE_ARG("get",ARG_TYPE_PURE_TOKEN,-1,"GET",NULL,"6.2.0",CMD_ARG_OPTIONAL,0,NULL)},
 {MAKE_ARG("expiration",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,5,NULL),.subargs=SET_expiration_Subargs},
@@ -11139,7 +11140,7 @@ struct COMMAND_STRUCT serverCommandTable[] = {
 {MAKE_CMD("mset","Atomically creates or modifies the string values of one or more keys.","O(N) where N is the number of keys to set.","1.0.1",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,MSET_History,0,MSET_Tips,2,msetCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,MSET_Keyspecs,1,NULL,1),.args=MSET_Args},
 {MAKE_CMD("msetnx","Atomically modifies the string values of one or more keys only when all keys don't exist.","O(N) where N is the number of keys to set.","1.0.1",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,MSETNX_History,0,MSETNX_Tips,0,msetnxCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,MSETNX_Keyspecs,1,NULL,1),.args=MSETNX_Args},
 {MAKE_CMD("psetex","Sets both string value and expiration time in milliseconds of a key. The key is created if it doesn't exist.","O(1)","2.6.0",CMD_DOC_DEPRECATED,"`SET` with the `PX` argument","2.6.12","string",COMMAND_GROUP_STRING,PSETEX_History,0,PSETEX_Tips,0,psetexCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,PSETEX_Keyspecs,1,NULL,3),.args=PSETEX_Args},
-{MAKE_CMD("set","Sets the string value of a key, ignoring its type. The key is created if it doesn't exist.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,SET_History,4,SET_Tips,0,setCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SET_Keyspecs,1,setGetKeys,5),.args=SET_Args},
+{MAKE_CMD("set","Sets the string value of a key, ignoring its type. The key is created if it doesn't exist.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,SET_History,4,SET_Tips,0,setCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SET_Keyspecs,1,setGetKeys,6),.args=SET_Args},
 {MAKE_CMD("setex","Sets the string value and expiration time of a key. Creates the key if it doesn't exist.","O(1)","2.0.0",CMD_DOC_DEPRECATED,"`SET` with the `EX` argument","2.6.12","string",COMMAND_GROUP_STRING,SETEX_History,0,SETEX_Tips,0,setexCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SETEX_Keyspecs,1,NULL,3),.args=SETEX_Args},
 {MAKE_CMD("setnx","Set the string value of a key only when the key doesn't exist.","O(1)","1.0.0",CMD_DOC_DEPRECATED,"`SET` with the `NX` argument","2.6.12","string",COMMAND_GROUP_STRING,SETNX_History,0,SETNX_Tips,0,setnxCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_STRING,SETNX_Keyspecs,1,NULL,2),.args=SETNX_Args},
 {MAKE_CMD("setrange","Overwrites a part of a string value with another by an offset. Creates the key if it doesn't exist.","O(1), not counting the time taken to copy the new string in place. Usually, this string is very small so the amortized complexity is O(1). Otherwise, complexity is O(M) with M being the length of the value argument.","2.2.0",CMD_DOC_NONE,NULL,NULL,"string",COMMAND_GROUP_STRING,SETRANGE_History,0,SETRANGE_Tips,0,setrangeCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,SETRANGE_Keyspecs,1,NULL,3),.args=SETRANGE_Args},

src/commands/set.json

@@ -85,6 +85,19 @@
                 "name": "value",
                 "type": "string"
             },
+            {
+                "name": "ifeq",
+                "type": "string",
+                "optional": true,
+                "summary": "Sets the key's value only if the current value matches the specified comparison value.",
+                "arguments": [
+                    {
+                        "name": "comparison-value",
+                        "type": "string",
+                        "summary": "The value to compare with the current key's value before setting."
+                    }
+                ]
+            },
             {
                 "name": "condition",
                 "type": "oneof",

src/t_string.c

@@ -105,12 +105,21 @@ void setGenericCommand(client *c,
     found = (lookupKeyWrite(c->db, key) != NULL);
 
     /* Handle the IFEQ conditional check */
-    if ((flags & OBJ_SET_IFEQ) && found) {
+    if (flags & OBJ_SET_IFEQ && found) {
         robj *current_value = lookupKeyRead(c->db, key);
-        if (current_value == NULL || compareStringObjects(current_value, comparison) != 0) {
+
+        if (current_value == NULL || current_value->type != OBJ_STRING || checkType(c, comparison, OBJ_STRING) != 0) {
+            addReplyError(c, "value(s) must be present or string");
+            return;
+        }
+
+        if (compareStringObjects(current_value, comparison) != 0) {
             addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
             return;
         }
+    } else if (flags & OBJ_SET_IFEQ && !found) {
+        addReply(c, abort_reply ? abort_reply : shared.null[c->resp]);
+        return;
     }
 
     if ((flags & OBJ_SET_NX && found) || (flags & OBJ_SET_XX && !found)) {

tests/unit/type/string.tcl

@@ -583,18 +583,58 @@ if {[string match {*jemalloc*} [s mem_allocator]]} {
     } {*WRONGTYPE*}
 
     test "SET with IFEQ conditional" {
-        # Setting an initial value for the key
+        r del foo
+
         r set foo "initial_value"
 
-        # Trying to set the key only if the value is exactly "initial_value"
-        assert_equal OK [r set foo "new_value" ifeq "initial_value"]
+        assert_equal {OK} [r set foo "new_value" ifeq "initial_value"]
         assert_equal "new_value" [r get foo]
 
-        # Trying to set the key only if the value is NOT "initial_value"
         assert_equal {} [r set foo "should_not_set" ifeq "wrong_value"]
         assert_equal "new_value" [r get foo]
     }
 
+    test "SET with IFEQ conditional - non-string current value" {
+        r del foo
+
+        r sadd foo "some_set_value"
+        assert_error {ERR value(s) must be present or string} {r set foo "new_value" ifeq "some_set_value"}
+    }
+
+    test "SET with IFEQ conditional - with get" {
+        r del foo
+
+        assert_equal {} [r set foo "new_value" ifeq "initial_value" get]
+
+        r set foo "initial_value"
+
+        assert_equal "initial_value" [r set foo "new_value" ifeq "initial_value" get]
+        assert_equal "new_value" [r get foo]
+    }
+
+
+    test "SET with IFEQ conditional - with xx" {
+        r del foo
+
+        assert_error {} {r set foo "new_value" ifeq "initial_value" xx}
+
+        r set foo "initial_value"
+
+        assert_equal {OK} [r set foo "new_value" ifeq "initial_value" xx]
+        assert_equal "new_value" [r get foo]
+    }
+
+    test "SET with IFEQ conditional - with nx" {
+        r del foo
+
+        assert_error {ERR value(s) must be present or string} {r set foo "new_value" ifeq "initial_value" nx}
+
+        r set foo "initial_value"
+
+        assert_equal {} [r set foo "new_value" ifeq "initial_value" nx]
+        assert_equal "initial_value" [r get foo]
+    }
+
     test {Extended SET EX option} {
         r del foo
         r set foo bar ex 10