Fixed SET and BITFIELD commands being wrongly marked movablekeys (#10837)

The SET and BITFIELD command were added `get_keys_function` in #10148, causing
them to be wrongly marked movablekeys in `populateCommandMovableKeys`.

This was an unintended side effect introduced in #10148 (7.0 RC1)
which could cause some clients an extra round trip for these commands in cluster mode.

Since we define movablekeys as a way to determine if the legacy range [first, last, step]
doesn't find all keys, then we need a completely different approach.

The right approach should be to check if the legacy range covers all key-specs,
and if none of the key-specs have the INCOMPLETE flag. 
This way, we don't need to look at getkeys_proc of VARIABLE_FLAG at all.
Probably with the exception of modules, who may still not be using key-specs.

In this PR, we removed `populateCommandMovableKeys` and put its logic in
`populateCommandLegacyRangeSpec`.
In order to properly serve both old and new modules, we must probably keep relying
CMD_MODULE_GETKEYS, but do that only for modules that don't declare key-specs. 
For ones that do, we need to take the same approach we take with native redis commands.

This approach was proposed by Oran. Fixes #10833

Co-authored-by: Oran Agra <oran@redislabs.com>

1 files changed, 42 insertions, 0 deletions

diff --git a/tests/modules/keyspecs.c b/tests/modules/keyspecs.c
index 32a6bebaa..d2ae9fd6c 100644
--- a/tests/modules/keyspecs.c
+++ b/tests/modules/keyspecs.c
@@ -18,6 +18,13 @@ int createKspecNone(RedisModuleCtx *ctx) {
     return REDISMODULE_OK;
 }
 
+int createKspecNoneWithGetkeys(RedisModuleCtx *ctx) {
+    /* A command without keyspecs; only the legacy (first,last,step) triple (MSET like spec), but also has a getkeys callback */
+    if (RedisModule_CreateCommand(ctx,"kspec.nonewithgetkeys",kspec_impl,"getkeys-api",1,-1,2) == REDISMODULE_ERR)
+        return REDISMODULE_ERR;
+    return REDISMODULE_OK;
+}
+
 int createKspecTwoRanges(RedisModuleCtx *ctx) {
     /* Test that two position/range-based key specs are combined to produce the
      * legacy (first,last,step) values representing both keys. */
@@ -51,6 +58,39 @@ int createKspecTwoRanges(RedisModuleCtx *ctx) {
     return REDISMODULE_OK;
 }
 
+int createKspecTwoRangesWithGap(RedisModuleCtx *ctx) {
+    /* Test that two position/range-based key specs are combined to produce the
+     * legacy (first,last,step) values representing just one key. */
+    if (RedisModule_CreateCommand(ctx,"kspec.tworangeswithgap",kspec_impl,"",0,0,0) == REDISMODULE_ERR)
+        return REDISMODULE_ERR;
+
+    RedisModuleCommand *command = RedisModule_GetCommand(ctx,"kspec.tworangeswithgap");
+    RedisModuleCommandInfo info = {
+        .version = REDISMODULE_COMMAND_INFO_VERSION,
+        .arity = -2,
+        .key_specs = (RedisModuleCommandKeySpec[]){
+            {
+                .flags = REDISMODULE_CMD_KEY_RO | REDISMODULE_CMD_KEY_ACCESS,
+                .begin_search_type = REDISMODULE_KSPEC_BS_INDEX,
+                .bs.index.pos = 1,
+                .find_keys_type = REDISMODULE_KSPEC_FK_RANGE,
+                .fk.range = {0,1,0}
+            },
+            {
+                .flags = REDISMODULE_CMD_KEY_RW | REDISMODULE_CMD_KEY_UPDATE,
+                .begin_search_type = REDISMODULE_KSPEC_BS_INDEX,
+                .bs.index.pos = 3,
+                /* Omitted find_keys_type is shorthand for RANGE {0,1,0} */
+            },
+            {0}
+        }
+    };
+    if (RedisModule_SetCommandInfo(command, &info) == REDISMODULE_ERR)
+        return REDISMODULE_ERR;
+
+    return REDISMODULE_OK;
+}
+
 int createKspecKeyword(RedisModuleCtx *ctx) {
     /* Only keyword-based specs. The legacy triple is wiped and set to (0,0,0). */
     if (RedisModule_CreateCommand(ctx,"kspec.keyword",kspec_impl,"",3,-1,1) == REDISMODULE_ERR)
@@ -177,7 +217,9 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
         return REDISMODULE_ERR;
 
     if (createKspecNone(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
+    if (createKspecNoneWithGetkeys(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
     if (createKspecTwoRanges(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
+    if (createKspecTwoRangesWithGap(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
     if (createKspecKeyword(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
     if (createKspecComplex1(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;
     if (createKspecComplex2(ctx) == REDISMODULE_ERR) return REDISMODULE_ERR;