HSETEX with FXX should not create an object if it does not exist (#2716)

When the hash object does not exist FXX should simply fail the check
without creating the object while FNX should be trivial and succeed.

Note - also fix a potential compilation warning on some COMPILERS doing
constant folding of variable length array when size is const expression.

Signed-off-by: Ran Shidlansik <[email protected]>
(cherry picked from commit 8182f4a)
Signed-off-by: cherukum-amazon <[email protected]>

Author: ranshid

src/db.c

@@ -1971,7 +1971,7 @@ void propagateDeletion(serverDb *db, robj *key, int lazy, int slot) {
     server.replication_allowed = prev_replication_allowed;
 }
 
-static const size_t EXPIRE_BULK_LIMIT = 1024; /* Maximum number of fields to active-expire (per replicated HDEL command */
+#define EXPIRE_BULK_LIMIT ((size_t)1024) /* Maximum number of fields to active-expire (per replicated HDEL command */
 
 /* Propagate HDEL commands for deleted hash fields to AOF and replicas.
  *

src/t_hash.c

@@ -1209,13 +1209,6 @@ void hsetexCommand(client *c) {
     if (checkType(c, o, OBJ_HASH))
         return;
 
-    if (o == NULL) {
-        o = createHashObject();
-        dbAdd(c->db, c->argv[1], &o);
-    }
-
-    bool has_volatile_fields = hashTypeHasVolatileFields(o);
-
     /* Handle parsing and calculating the expiration time. */
     if (flags & ARGS_KEEPTTL)
         set_flags |= HASH_SET_KEEP_EXPIRY;
@@ -1230,17 +1223,35 @@ void hsetexCommand(client *c) {
         }
     }
 
-    /* Check for all fields condition */
+    /* Check FNX/FXX field-level conditions */
     if (flags & (ARGS_SET_FNX | ARGS_SET_FXX)) {
-        for (i = fields_index; i < c->argc; i += 2) {
-            if (((flags & ARGS_SET_FNX) && hashTypeExists(o, c->argv[i]->ptr)) ||
-                ((flags & ARGS_SET_FXX) && !hashTypeExists(o, c->argv[i]->ptr))) {
+        if (o) {
+            /* Key exists: check fields normally */
+            for (i = fields_index; i < c->argc; i += 2) {
+                if (((flags & ARGS_SET_FNX) && hashTypeExists(o, c->argv[i]->ptr)) ||
+                    ((flags & ARGS_SET_FXX) && !hashTypeExists(o, c->argv[i]->ptr))) {
+                    addReply(c, shared.czero);
+                    return;
+                }
+            }
+        } else {
+            /* Key does not exist */
+            if (flags & ARGS_SET_FXX) {
+                /* Any FXX fails because no fields exist */
                 addReply(c, shared.czero);
                 return;
             }
+            /* FNX automatically passes if key doesn't exist, nothing to check */
         }
     }
 
+    if (o == NULL) {
+        o = createHashObject();
+        dbAdd(c->db, c->argv[1], &o);
+    }
+
+    bool has_volatile_fields = hashTypeHasVolatileFields(o);
+
     /* In case we are expiring all the elements prepare a new argv since we are going to delete all the expired fields. */
     if (set_expired) {
         new_argv = zmalloc(sizeof(robj *) * (num_fields + 2));

tests/unit/hashexpire.tcl

@@ -684,6 +684,12 @@ start_server {tags {"hashexpire"}} {
         set e
     } {ERR *}
 
+     test {HSETEX EX - FXX does not create object in case key does not exist} {
+        r FLUSHALL
+        assert_equal 0 [r HSETEX myhash EX 10 FXX FIELDS 1 x y]
+        assert_equal 0 [r EXISTS myhash]
+    }
+
     ###### Test EXPIRE #############