Unverified Commit 3330ea18 authored by guybe7's avatar guybe7 Committed by GitHub
Browse files

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.
parent c0725abf
...@@ -1899,14 +1899,6 @@ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc, ...@@ -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 */ /* The command has at least one key-spec marked as VARIABLE_FLAGS */
int has_varflags = (getAllKeySpecsFlags(cmd, 0) & CMD_KEY_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. */ /* We prefer key-specs if there are any, and their flags are reliable. */
if (has_keyspec && !has_varflags) { if (has_keyspec && !has_varflags) {
int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result); int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result);
...@@ -1917,7 +1909,7 @@ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc, ...@@ -1917,7 +1909,7 @@ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc,
} }
/* Resort to getkeys callback methods. */ /* Resort to getkeys callback methods. */
if (has_module_getkeys) if (cmd->flags & CMD_MODULE_GETKEYS)
return moduleGetCommandKeysViaAPI(cmd,argv,argc,result); return moduleGetCommandKeysViaAPI(cmd,argv,argc,result);
/* We use native getkeys as a last resort, since not all these native getkeys provide /* We use native getkeys as a last resort, since not all these native getkeys provide
......
...@@ -1155,7 +1155,9 @@ RedisModuleCommand *moduleCreateCommandProxy(struct RedisModule *module, sds dec ...@@ -1155,7 +1155,9 @@ RedisModuleCommand *moduleCreateCommandProxy(struct RedisModule *module, sds dec
cp->rediscmd->key_specs = cp->rediscmd->key_specs_static; cp->rediscmd->key_specs = cp->rediscmd->key_specs_static;
if (firstkey != 0) { if (firstkey != 0) {
cp->rediscmd->key_specs_num = 1; 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].begin_search_type = KSPEC_BS_INDEX;
cp->rediscmd->key_specs[0].bs.index.pos = firstkey; cp->rediscmd->key_specs[0].bs.index.pos = firstkey;
cp->rediscmd->key_specs[0].find_keys_type = KSPEC_FK_RANGE; cp->rediscmd->key_specs[0].find_keys_type = KSPEC_FK_RANGE;
......
...@@ -7,6 +7,15 @@ ...@@ -7,6 +7,15 @@
int kspec_impl(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { int kspec_impl(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
UNUSED(argv); UNUSED(argv);
UNUSED(argc); 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"); RedisModule_ReplyWithSimpleString(ctx, "OK");
return REDISMODULE_OK; return REDISMODULE_OK;
} }
......
...@@ -13,10 +13,24 @@ start_server {tags {"modules"}} { ...@@ -13,10 +13,24 @@ start_server {tags {"modules"}} {
# Verify key-spec auto-generated from the legacy triple # Verify key-spec auto-generated from the legacy triple
set keyspecs [lindex $reply 8] set keyspecs [lindex $reply 8]
assert_equal [llength $keyspecs] 1 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} 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" { test "Module key specs: Two ranges" {
set reply [lindex [r command info kspec.tworanges] 0] set reply [lindex [r command info kspec.tworanges] 0]
# Verify (first, last, step) and not movablekeys # Verify (first, last, step) and not movablekeys
...@@ -99,7 +113,11 @@ start_server {tags {"modules"}} { ...@@ -99,7 +113,11 @@ start_server {tags {"modules"}} {
test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec without flags} { test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec without flags} {
r command getkeysandflags kspec.none key1 val1 key2 val2 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} { test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec flags} {
r command getkeysandflags kspec.keyword keys key1 key2 key3 r command getkeysandflags kspec.keyword keys key1 key2 key3
......
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