Unverified Commit 4aa25d04 authored by Ozan Tezcan's avatar Ozan Tezcan Committed by GitHub
Browse files

Reply with array of return codes if the key does not exist for HFE commands (#13343)

Currently, HFE commands reply with empty array if the key does not
exist. Though, non-existing key and empty key is the same thing. 
It means fields given in the command do not exist in the empty key. 
So, replying with an array of 'no field' error codes (-2) suits better 
to Redis logic. e.g. Similarly, `hmget` returns array of nulls if the 
key does not exist.

After this PR:
```
127.0.0.1:6379> hpersist missingkey fields 2 a b
1) (integer) -2
2) (integer) -2
```
parent 871c9859
...@@ -2811,8 +2811,9 @@ static void httlGenericCommand(client *c, const char *cmd, long long basetime, i ...@@ -2811,8 +2811,9 @@ static void httlGenericCommand(client *c, const char *cmd, long long basetime, i
long numFields = 0, numFieldsAt = 3; long numFields = 0, numFieldsAt = 3;
/* Read the hash object */ /* Read the hash object */
if ((hashObj = lookupKeyReadOrReply(c, c->argv[1], shared.emptyarray)) == NULL || hashObj = lookupKeyRead(c->db, c->argv[1]);
checkType(c, hashObj, OBJ_HASH)) return; if (checkType(c, hashObj, OBJ_HASH))
return;
if (strcasecmp(c->argv[numFieldsAt-1]->ptr, "FIELDS")) { if (strcasecmp(c->argv[numFieldsAt-1]->ptr, "FIELDS")) {
addReplyError(c, "Mandatory argument FIELDS is missing or not at the right position"); addReplyError(c, "Mandatory argument FIELDS is missing or not at the right position");
...@@ -2830,6 +2831,16 @@ static void httlGenericCommand(client *c, const char *cmd, long long basetime, i ...@@ -2830,6 +2831,16 @@ static void httlGenericCommand(client *c, const char *cmd, long long basetime, i
return; return;
} }
/* Non-existing keys and empty hashes are the same thing. It also means
* fields in the command don't exist in the hash key. */
if (!hashObj) {
addReplyArrayLen(c, numFields);
for (int i = 0; i < numFields; i++) {
addReplyLongLong(c, HFE_GET_NO_FIELD);
}
return;
}
if (hashObj->encoding == OBJ_ENCODING_LISTPACK) { if (hashObj->encoding == OBJ_ENCODING_LISTPACK) {
void *lp = hashObj->ptr; void *lp = hashObj->ptr;
...@@ -2934,8 +2945,9 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime ...@@ -2934,8 +2945,9 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime
robj *hashObj, *keyArg = c->argv[1], *expireArg = c->argv[2]; robj *hashObj, *keyArg = c->argv[1], *expireArg = c->argv[2];
/* Read the hash object */ /* Read the hash object */
if ((hashObj = lookupKeyWriteOrReply(c, keyArg, shared.emptyarray)) == NULL || hashObj = lookupKeyWrite(c->db, keyArg);
checkType(c, hashObj, OBJ_HASH)) return; if (checkType(c, hashObj, OBJ_HASH))
return;
/* Read the expiry time from command */ /* Read the expiry time from command */
if (getLongLongFromObjectOrReply(c, expireArg, &expire, NULL) != C_OK) if (getLongLongFromObjectOrReply(c, expireArg, &expire, NULL) != C_OK)
...@@ -2988,6 +3000,16 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime ...@@ -2988,6 +3000,16 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime
return; return;
} }
/* Non-existing keys and empty hashes are the same thing. It also means
* fields in the command don't exist in the hash key. */
if (!hashObj) {
addReplyArrayLen(c, numFields);
for (int i = 0; i < numFields; i++) {
addReplyLongLong(c, HSETEX_NO_FIELD);
}
return;
}
HashTypeSetEx exCtx; HashTypeSetEx exCtx;
hashTypeSetExInit(keyArg, hashObj, c, c->db, cmd, expireSetCond, &exCtx); hashTypeSetExInit(keyArg, hashObj, c, c->db, cmd, expireSetCond, &exCtx);
addReplyArrayLen(c, numFields); addReplyArrayLen(c, numFields);
...@@ -3062,8 +3084,9 @@ void hpersistCommand(client *c) { ...@@ -3062,8 +3084,9 @@ void hpersistCommand(client *c) {
int changed = 0; /* Used to determine whether to send a notification. */ int changed = 0; /* Used to determine whether to send a notification. */
/* Read the hash object */ /* Read the hash object */
if ((hashObj = lookupKeyWriteOrReply(c, c->argv[1], shared.emptyarray)) == NULL || hashObj = lookupKeyWrite(c->db, c->argv[1]);
checkType(c, hashObj, OBJ_HASH)) return; if (checkType(c, hashObj, OBJ_HASH))
return;
if (strcasecmp(c->argv[numFieldsAt-1]->ptr, "FIELDS")) { if (strcasecmp(c->argv[numFieldsAt-1]->ptr, "FIELDS")) {
addReplyError(c, "Mandatory argument FIELDS is missing or not at the right position"); addReplyError(c, "Mandatory argument FIELDS is missing or not at the right position");
...@@ -3081,6 +3104,16 @@ void hpersistCommand(client *c) { ...@@ -3081,6 +3104,16 @@ void hpersistCommand(client *c) {
return; return;
} }
/* Non-existing keys and empty hashes are the same thing. It also means
* fields in the command don't exist in the hash key. */
if (!hashObj) {
addReplyArrayLen(c, numFields);
for (int i = 0; i < numFields; i++) {
addReplyLongLong(c, HFE_PERSIST_NO_FIELD);
}
return;
}
if (hashObj->encoding == OBJ_ENCODING_LISTPACK) { if (hashObj->encoding == OBJ_ENCODING_LISTPACK) {
addReplyArrayLen(c, numFields); addReplyArrayLen(c, numFields);
for (int i = 0 ; i < numFields ; i++) { for (int i = 0 ; i < numFields ; i++) {
......
...@@ -117,15 +117,12 @@ start_server {tags {"external:skip needs:debug"}} { ...@@ -117,15 +117,12 @@ start_server {tags {"external:skip needs:debug"}} {
r config set hash-max-listpack-entries 512 r config set hash-max-listpack-entries 512
} }
test "HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT - Returns empty array if key does not exist" { test "HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT - Returns array if the key does not exist" {
r del myhash r del myhash
# Make sure we can distinguish between an empty array and a null response assert_equal [r HEXPIRE myhash 1000 FIELDS 1 a] [list $E_NO_FIELD]
r readraw 1 assert_equal [r HEXPIREAT myhash 1000 FIELDS 1 a] [list $E_NO_FIELD]
assert_equal {*0} [r HEXPIRE myhash 1000 FIELDS 1 a] assert_equal [r HPEXPIRE myhash 1000 FIELDS 2 a b] [list $E_NO_FIELD $E_NO_FIELD]
assert_equal {*0} [r HEXPIREAT myhash 1000 FIELDS 1 a] assert_equal [r HPEXPIREAT myhash 1000 FIELDS 2 a b] [list $E_NO_FIELD $E_NO_FIELD]
assert_equal {*0} [r HPEXPIRE myhash 1000 FIELDS 1 a]
assert_equal {*0} [r HPEXPIREAT myhash 1000 FIELDS 1 a]
r readraw 0
} }
test "HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT - Verify that the expire time does not overflow" { test "HEXPIRE/HEXPIREAT/HPEXPIRE/HPEXPIREAT - Verify that the expire time does not overflow" {
...@@ -305,13 +302,10 @@ start_server {tags {"external:skip needs:debug"}} { ...@@ -305,13 +302,10 @@ start_server {tags {"external:skip needs:debug"}} {
r flushall async r flushall async
} }
test "HTTL/HPTTL - Returns empty array if key does not exist" { test "HTTL/HPTTL - Returns array if the key does not exist" {
r del myhash r del myhash
# Make sure we can distinguish between an empty array and a null response assert_equal [r HTTL myhash FIELDS 1 a] [list $T_NO_FIELD]
r readraw 1 assert_equal [r HPTTL myhash FIELDS 2 a b] [list $T_NO_FIELD $T_NO_FIELD]
assert_equal {*0} [r HTTL myhash FIELDS 1 a]
assert_equal {*0} [r HPTTL myhash FIELDS 1 a]
r readraw 0
} }
test "HTTL/HPTTL - Input validation gets failed on nonexists field or field without expire ($type)" { test "HTTL/HPTTL - Input validation gets failed on nonexists field or field without expire ($type)" {
...@@ -320,7 +314,6 @@ start_server {tags {"external:skip needs:debug"}} { ...@@ -320,7 +314,6 @@ start_server {tags {"external:skip needs:debug"}} {
r HPEXPIRE myhash 1000 NX FIELDS 1 field1 r HPEXPIRE myhash 1000 NX FIELDS 1 field1
foreach cmd {HTTL HPTTL} { foreach cmd {HTTL HPTTL} {
assert_equal [r $cmd non_exists_key FIELDS 1 f] {}
assert_equal [r $cmd myhash FIELDS 2 field2 non_exists_field] "$T_NO_EXPIRY $T_NO_FIELD" assert_equal [r $cmd myhash FIELDS 2 field2 non_exists_field] "$T_NO_EXPIRY $T_NO_FIELD"
# Set numFields less than actual number of fields. Fine. # Set numFields less than actual number of fields. Fine.
assert_equal [r $cmd myhash FIELDS 1 non_exists_field1 non_exists_field2] "$T_NO_FIELD" assert_equal [r $cmd myhash FIELDS 1 non_exists_field1 non_exists_field2] "$T_NO_FIELD"
...@@ -337,13 +330,10 @@ start_server {tags {"external:skip needs:debug"}} { ...@@ -337,13 +330,10 @@ start_server {tags {"external:skip needs:debug"}} {
assert_range $ttl 1000 2000 assert_range $ttl 1000 2000
} }
test "HEXPIRETIME/HPEXPIRETIME - Returns empty array if key does not exist" { test "HEXPIRETIME/HPEXPIRETIME - Returns array if the key does not exist" {
r del myhash r del myhash
# Make sure we can distinguish between an empty array and a null response assert_equal [r HEXPIRETIME myhash FIELDS 1 a] [list $T_NO_FIELD]
r readraw 1 assert_equal [r HPEXPIRETIME myhash FIELDS 2 a b] [list $T_NO_FIELD $T_NO_FIELD]
assert_equal {*0} [r HEXPIRETIME myhash FIELDS 1 a]
assert_equal {*0} [r HPEXPIRETIME myhash FIELDS 1 a]
r readraw 0
} }
test "HEXPIRETIME - returns TTL in Unix timestamp ($type)" { test "HEXPIRETIME - returns TTL in Unix timestamp ($type)" {
...@@ -711,12 +701,10 @@ start_server {tags {"external:skip needs:debug"}} { ...@@ -711,12 +701,10 @@ start_server {tags {"external:skip needs:debug"}} {
r debug set-active-expire 1 r debug set-active-expire 1
} }
test "HPERSIST - Returns empty array if key does not exist ($type)" { test "HPERSIST - Returns array if the key does not exist ($type)" {
r del myhash r del myhash
# Make sure we can distinguish between an empty array and a null response assert_equal [r HPERSIST myhash FIELDS 1 a] [list $P_NO_FIELD]
r readraw 1 assert_equal [r HPERSIST myhash FIELDS 2 a b] [list $P_NO_FIELD $P_NO_FIELD]
assert_equal {*0} [r HPERSIST myhash FIELDS 1 a]
r readraw 0
} }
test "HPERSIST - input validation ($type)" { test "HPERSIST - input validation ($type)" {
...@@ -726,7 +714,6 @@ start_server {tags {"external:skip needs:debug"}} { ...@@ -726,7 +714,6 @@ start_server {tags {"external:skip needs:debug"}} {
r hexpire myhash 1000 NX FIELDS 1 f1 r hexpire myhash 1000 NX FIELDS 1 f1
assert_error {*wrong number of arguments*} {r hpersist myhash} assert_error {*wrong number of arguments*} {r hpersist myhash}
assert_error {*wrong number of arguments*} {r hpersist myhash FIELDS 1} assert_error {*wrong number of arguments*} {r hpersist myhash FIELDS 1}
assert_equal [r hpersist not-exists-key FIELDS 1 f1] {}
assert_equal [r hpersist myhash FIELDS 2 f1 not-exists-field] "$P_OK $P_NO_FIELD" assert_equal [r hpersist myhash FIELDS 2 f1 not-exists-field] "$P_OK $P_NO_FIELD"
assert_equal [r hpersist myhash FIELDS 1 f2] "$P_NO_EXPIRY" assert_equal [r hpersist myhash FIELDS 1 f2] "$P_NO_EXPIRY"
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment