RM_CreateCommand should not set CMD_KEY_VARIABLE_FLAGS automatically (#11320)

The original idea behind auto-setting the default (first,last,step) spec was to use
the most "open" flags when the user didn't provide any key-spec flags information.

While the above idea is a good approach, it really makes no sense to set
CMD_KEY_VARIABLE_FLAGS if the user didn't provide the getkeys-api flag:
in this case there's not way to retrieve these variable flags, so what's the point?

Internally in redis there was code to ignore this already, so this fix doesn't change
redis's behavior, it only affects the output of COMMAND command.

4 files changed, 33 insertions, 12 deletions

diff --git a/src/db.c b/src/db.c
index 3fd4938af..7ebf57ac4 100644
--- a/src/db.c
+++ b/src/db.c
@@ -1899,14 +1899,6 @@ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc,
     /* The command has at least one key-spec marked as VARIABLE_FLAGS */
     int has_varflags = (getAllKeySpecsFlags(cmd, 0) & CMD_KEY_VARIABLE_FLAGS);
 
-    /* Flags indicating that we have a getkeys callback */
-    int has_module_getkeys = cmd->flags & CMD_MODULE_GETKEYS;
-
-    /* The key-spec that's auto generated by RM_CreateCommand sets VARIABLE_FLAGS since no flags are given.
-     * If the module provides getkeys callback, we'll prefer it, but if it didn't, we'll use key-spec anyway. */
-    if ((cmd->flags & CMD_MODULE) && has_varflags && !has_module_getkeys)
-        has_varflags = 0;
-
     /* We prefer key-specs if there are any, and their flags are reliable. */
     if (has_keyspec && !has_varflags) {
         int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result);
@@ -1917,7 +1909,7 @@ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc,
     }
 
     /* Resort to getkeys callback methods. */
-    if (has_module_getkeys)
+    if (cmd->flags & CMD_MODULE_GETKEYS)
         return moduleGetCommandKeysViaAPI(cmd,argv,argc,result);
 
     /* We use native getkeys as a last resort, since not all these native getkeys provide
diff --git a/src/module.c b/src/module.c
index 9e67359df..28d2cad55 100644
--- a/src/module.c
+++ b/src/module.c
@@ -1155,7 +1155,9 @@ RedisModuleCommand *moduleCreateCommandProxy(struct RedisModule *module, sds dec
     cp->rediscmd->key_specs = cp->rediscmd->key_specs_static;
     if (firstkey != 0) {
         cp->rediscmd->key_specs_num = 1;
-        cp->rediscmd->key_specs[0].flags = CMD_KEY_FULL_ACCESS | CMD_KEY_VARIABLE_FLAGS;
+        cp->rediscmd->key_specs[0].flags = CMD_KEY_FULL_ACCESS;
+        if (flags & CMD_MODULE_GETKEYS)
+            cp->rediscmd->key_specs[0].flags |= CMD_KEY_VARIABLE_FLAGS;
         cp->rediscmd->key_specs[0].begin_search_type = KSPEC_BS_INDEX;
         cp->rediscmd->key_specs[0].bs.index.pos = firstkey;
         cp->rediscmd->key_specs[0].find_keys_type = KSPEC_FK_RANGE;
diff --git a/tests/modules/keyspecs.c b/tests/modules/keyspecs.c
index d2ae9fd6c..0a70de814 100644
--- a/tests/modules/keyspecs.c
+++ b/tests/modules/keyspecs.c
@@ -7,6 +7,15 @@
 int kspec_impl(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
     UNUSED(argv);
     UNUSED(argc);
+
+    /* Handle getkeys-api introspection (for "kspec.nonewithgetkeys")  */
+    if (RedisModule_IsKeysPositionRequest(ctx)) {
+        for (int i = 1; i < argc; i += 2)
+            RedisModule_KeyAtPosWithFlags(ctx, i, REDISMODULE_CMD_KEY_RO | REDISMODULE_CMD_KEY_ACCESS);
+
+        return REDISMODULE_OK;
+    }
+
     RedisModule_ReplyWithSimpleString(ctx, "OK");
     return REDISMODULE_OK;
 }
diff --git a/tests/unit/moduleapi/keyspecs.tcl b/tests/unit/moduleapi/keyspecs.tcl
index ef5b92334..8491bc19e 100644
--- a/tests/unit/moduleapi/keyspecs.tcl
+++ b/tests/unit/moduleapi/keyspecs.tcl
@@ -13,10 +13,24 @@ start_server {tags {"modules"}} {
         # Verify key-spec auto-generated from the legacy triple
         set keyspecs [lindex $reply 8]
         assert_equal [llength $keyspecs] 1
-        assert_equal [lindex $keyspecs 0] {flags {RW access update variable_flags} begin_search {type index spec {index 1}} find_keys {type range spec {lastkey -1 keystep 2 limit 0}}}
+        assert_equal [lindex $keyspecs 0] {flags {RW access update} begin_search {type index spec {index 1}} find_keys {type range spec {lastkey -1 keystep 2 limit 0}}}
         assert_equal [r command getkeys kspec.none key1 val1 key2 val2] {key1 key2}
     }
 
+    test "Module key specs: No spec, only legacy triple with getkeys-api" {
+        set reply [lindex [r command info kspec.nonewithgetkeys] 0]
+        # Verify (first, last, step) and movablekeys
+        assert_equal [lindex $reply 2] {module movablekeys}
+        assert_equal [lindex $reply 3] 1
+        assert_equal [lindex $reply 4] -1
+        assert_equal [lindex $reply 5] 2
+        # Verify key-spec auto-generated from the legacy triple
+        set keyspecs [lindex $reply 8]
+        assert_equal [llength $keyspecs] 1
+        assert_equal [lindex $keyspecs 0] {flags {RW access update variable_flags} begin_search {type index spec {index 1}} find_keys {type range spec {lastkey -1 keystep 2 limit 0}}}
+        assert_equal [r command getkeys kspec.nonewithgetkeys key1 val1 key2 val2] {key1 key2}
+    }
+
     test "Module key specs: Two ranges" {
         set reply [lindex [r command info kspec.tworanges] 0]
         # Verify (first, last, step) and not movablekeys
@@ -99,7 +113,11 @@ start_server {tags {"modules"}} {
 
     test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec without flags} {
         r command getkeysandflags kspec.none key1 val1 key2 val2
-    } {{key1 {RW access update variable_flags}} {key2 {RW access update variable_flags}}}
+    } {{key1 {RW access update}} {key2 {RW access update}}}
+
+    test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec with flags} {
+        r command getkeysandflags kspec.nonewithgetkeys key1 val1 key2 val2
+    } {{key1 {RO access}} {key2 {RO access}}}
 
     test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec flags} {
         r command getkeysandflags kspec.keyword keys key1 key2 key3