Fix hgetf/hsetf reply type by returning string (#13263)

If encoding is listpack, hgetf and hsetf commands reply field value type as integer. This PR fixes it by returning string. Problematic cases: ``` 127.0.0.1:6379> hset hash one 1 (integer) 1 127.0.0.1:6379> hgetf hash fields 1 one 1) (integer) 1 127.0.0.1:6379> hsetf hash GETOLD fvs 1 one 2 1) (integer) 1 127.0.0.1:6379> hsetf hash DOF GETNEW fvs 1 one 2 1) (integer) 2 ``` Additional fixes: - hgetf/hsetf command description text Fixes #13261, #13262
redis · May 13, 2024 · 5066e6e · 5066e6e
1 parent 7010f41
commit 5066e6e
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 6 deletions.
diff --git a/src/commands.def b/src/commands.def
@@ -11102,7 +11102,7 @@ struct COMMAND_STRUCT redisCommandTable[] = {
 {MAKE_CMD("hexpiretime","Returns the expiration time of a hash field as a Unix timestamp, in seconds.","O(N) where N is the number of arguments to the command","8.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HEXPIRETIME_History,0,HEXPIRETIME_Tips,0,hexpiretimeCommand,-4,CMD_READONLY|CMD_FAST,ACL_CATEGORY_HASH,HEXPIRETIME_Keyspecs,1,NULL,3),.args=HEXPIRETIME_Args},
 {MAKE_CMD("hget","Returns the value of a field in a hash.","O(1)","2.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HGET_History,0,HGET_Tips,0,hgetCommand,3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_HASH,HGET_Keyspecs,1,NULL,2),.args=HGET_Args},
 {MAKE_CMD("hgetall","Returns all fields and values in a hash.","O(N) where N is the size of the hash.","2.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HGETALL_History,0,HGETALL_Tips,1,hgetallCommand,2,CMD_READONLY,ACL_CATEGORY_HASH,HGETALL_Keyspecs,1,NULL,1),.args=HGETALL_Args},
-{MAKE_CMD("hgetf","For each specified field, returns its value and optionally set the field's remaining expiration time in seconds / milliseconds","O(N) where N is the number of arguments to the command","8.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HGETF_History,0,HGETF_Tips,0,hgetfCommand,-5,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HGETF_Keyspecs,1,NULL,6),.args=HGETF_Args},
+{MAKE_CMD("hgetf","For each specified field: get its value and optionally set the field's remaining time to live / UNIX expiration timestamp in seconds / milliseconds","O(N) where N is the number of arguments to the command","8.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HGETF_History,0,HGETF_Tips,0,hgetfCommand,-5,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HGETF_Keyspecs,1,NULL,6),.args=HGETF_Args},
 {MAKE_CMD("hincrby","Increments the integer value of a field in a hash by a number. Uses 0 as initial value if the field doesn't exist.","O(1)","2.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HINCRBY_History,0,HINCRBY_Tips,0,hincrbyCommand,4,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HINCRBY_Keyspecs,1,NULL,3),.args=HINCRBY_Args},
 {MAKE_CMD("hincrbyfloat","Increments the floating point value of a field by a number. Uses 0 as initial value if the field doesn't exist.","O(1)","2.6.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HINCRBYFLOAT_History,0,HINCRBYFLOAT_Tips,0,hincrbyfloatCommand,4,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HINCRBYFLOAT_Keyspecs,1,NULL,3),.args=HINCRBYFLOAT_Args},
 {MAKE_CMD("hkeys","Returns all fields in a hash.","O(N) where N is the size of the hash.","2.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HKEYS_History,0,HKEYS_Tips,1,hkeysCommand,2,CMD_READONLY,ACL_CATEGORY_HASH,HKEYS_Keyspecs,1,NULL,1),.args=HKEYS_Args},
@@ -11117,7 +11117,7 @@ struct COMMAND_STRUCT redisCommandTable[] = {
 {MAKE_CMD("hrandfield","Returns one or more random fields from a hash.","O(N) where N is the number of fields returned","6.2.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HRANDFIELD_History,0,HRANDFIELD_Tips,1,hrandfieldCommand,-2,CMD_READONLY,ACL_CATEGORY_HASH,HRANDFIELD_Keyspecs,1,NULL,2),.args=HRANDFIELD_Args},
 {MAKE_CMD("hscan","Iterates over fields and values of a hash.","O(1) for every call. O(N) for a complete iteration, including enough command calls for the cursor to return back to 0. N is the number of elements inside the collection.","2.8.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HSCAN_History,0,HSCAN_Tips,1,hscanCommand,-3,CMD_READONLY,ACL_CATEGORY_HASH,HSCAN_Keyspecs,1,NULL,5),.args=HSCAN_Args},
 {MAKE_CMD("hset","Creates or modifies the value of a field in a hash.","O(1) for each field/value pair added, so O(N) to add N field/value pairs when the command is called with multiple field/value pairs.","2.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HSET_History,1,HSET_Tips,0,hsetCommand,-4,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HSET_Keyspecs,1,NULL,2),.args=HSET_Args},
-{MAKE_CMD("hsetf","For each specified field, returns its value and optionally set the field's remaining expiration time in seconds / milliseconds","O(N) where N is the number of arguments to the command","8.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HSETF_History,0,HSETF_Tips,0,hsetfCommand,-6,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HSETF_Keyspecs,1,NULL,9),.args=HSETF_Args},
+{MAKE_CMD("hsetf","For each specified field value pair: set field to value and optionally set the field's remaining time to live / UNIX expiration timestamp in seconds / milliseconds","O(N) where N is the number of arguments to the command","8.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HSETF_History,0,HSETF_Tips,0,hsetfCommand,-6,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HSETF_Keyspecs,1,NULL,9),.args=HSETF_Args},
 {MAKE_CMD("hsetnx","Sets the value of a field in a hash only when the field doesn't exist.","O(1)","2.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HSETNX_History,0,HSETNX_Tips,0,hsetnxCommand,4,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_HASH,HSETNX_Keyspecs,1,NULL,3),.args=HSETNX_Args},
 {MAKE_CMD("hstrlen","Returns the length of the value of a field.","O(1)","3.2.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HSTRLEN_History,0,HSTRLEN_Tips,0,hstrlenCommand,3,CMD_READONLY|CMD_FAST,ACL_CATEGORY_HASH,HSTRLEN_Keyspecs,1,NULL,2),.args=HSTRLEN_Args},
 {MAKE_CMD("httl","Returns the TTL in seconds of a hash field.","O(N) where N is the number of arguments to the command","8.0.0",CMD_DOC_NONE,NULL,NULL,"hash",COMMAND_GROUP_HASH,HTTL_History,0,HTTL_Tips,0,httlCommand,-4,CMD_READONLY|CMD_FAST,ACL_CATEGORY_HASH,HTTL_Keyspecs,1,NULL,3),.args=HTTL_Args},

diff --git a/src/commands/hgetf.json b/src/commands/hgetf.json
@@ -1,6 +1,6 @@
 {
     "HGETF": {
-        "summary": "For each specified field, returns its value and optionally set the field's remaining expiration time in seconds / milliseconds",
+        "summary": "For each specified field: get its value and optionally set the field's remaining time to live / UNIX expiration timestamp in seconds / milliseconds",
         "complexity": "O(N) where N is the number of arguments to the command",
         "group": "hash",
         "since": "8.0.0",

diff --git a/src/commands/hsetf.json b/src/commands/hsetf.json
@@ -1,6 +1,6 @@
 {
     "HSETF": {
-        "summary": "For each specified field, returns its value and optionally set the field's remaining expiration time in seconds / milliseconds",
+        "summary": "For each specified field value pair: set field to value and optionally set the field's remaining time to live / UNIX expiration timestamp in seconds / milliseconds",
         "complexity": "O(N) where N is the number of arguments to the command",
         "group": "hash",
         "since": "8.0.0",

diff --git a/src/t_hash.c b/src/t_hash.c
@@ -3391,7 +3391,7 @@ static int hgetfReplyValueAndSetExpiry(client *c, robj *o, sds field, int flag,
         if (vstr)
             addReplyBulkCBuffer(c, vstr, vlen);
         else
-            addReplyLongLong(c, vll);
+            addReplyBulkLongLong(c, vll);
     } else {
         serverPanic("Unknown encoding: %d", o->encoding);
     }
@@ -3528,7 +3528,7 @@ static void hsetfReplyFromListpack(client *c, unsigned char *vptr) {
         if (vstr)
             addReplyBulkCBuffer(c, vstr, vlen);
         else
-            addReplyLongLong(c, vll);
+            addReplyBulkLongLong(c, vll);
     }
 }
 

diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl
@@ -729,6 +729,17 @@ start_server {tags {"external:skip needs:debug"}} {
             assert_error {*invalid number of fields*} {r hgetf myhash fields -1 a}
         }
 
+        test "HGETF - Verify field value reply type is string ($type)" {
+            r del myhash
+            r hsetf myhash FVS 1 f1 1
+
+            r readraw 1
+            assert_equal [r hgetf myhash FIELDS 1 f1] {*1}
+            assert_equal [r read] {$1}
+            assert_equal [r read] {1}
+            r readraw 0
+        }
+
         test "HGETF - Test 'NX' flag ($type)" {
             r del myhash
             r hset myhash field1 value1 field2 value2 field3 value3
@@ -925,6 +936,24 @@ start_server {tags {"external:skip needs:debug"}} {
             assert_error {*invalid number of fvs count*} {r hsetf myhash fvs -1 a b}
         }
 
+        test "HSETF - Verify field value reply type is string ($type)" {
+            r del myhash
+            r hsetf myhash FVS 1 field 1
+            r readraw 1
+
+            # Test with GETOLD
+            assert_equal [r hsetf myhash GETOLD FVS 1 field 200] {*1}
+            assert_equal [r read] {$1}
+            assert_equal [r read] {1}
+
+            # Test with GETNEW.
+            assert_equal [r hsetf myhash DOF GETNEW FVS 1 field 300] {*1}
+            assert_equal [r read] {$3}
+            assert_equal [r read] {200}
+
+            r readraw 0
+        }
+
         test "HSETF - Test DC flag ($type)" {
             r del myhash
             # don't create key