diff options
author | Binbin <binloveplay1314@qq.com> | 2022-06-12 13:22:18 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-06-12 08:22:18 +0300 |
commit | 92fb4f4f61bc430aec30105e24d1dc148a4d6466 (patch) | |
tree | aa56a2db3c730f31f73254a9fb24e9d9a3aa44e8 /tests | |
parent | 62ac1ab007b9ba25588671fdefd0025028030504 (diff) | |
download | redis-92fb4f4f61bc430aec30105e24d1dc148a4d6466.tar.gz |
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>
Diffstat (limited to 'tests')
-rw-r--r-- | tests/modules/keyspecs.c | 42 | ||||
-rw-r--r-- | tests/unit/introspection-2.tcl | 15 | ||||
-rw-r--r-- | tests/unit/moduleapi/keyspecs.tcl | 30 |
3 files changed, 86 insertions, 1 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; diff --git a/tests/unit/introspection-2.tcl b/tests/unit/introspection-2.tcl index 46dac50b7..dab8008e8 100644 --- a/tests/unit/introspection-2.tcl +++ b/tests/unit/introspection-2.tcl @@ -176,4 +176,19 @@ start_server {tags {"introspection"}} { assert_equal {{}} [r command info get|key] assert_equal {{}} [r command info config|get|key] } + + foreach cmd {SET GET MSET BITFIELD LMOVE LPOP BLPOP PING MEMORY MEMORY|USAGE RENAME GEORADIUS_RO} { + test "$cmd command will not be marked with movablekeys" { + set info [lindex [r command info $cmd] 0] + assert_no_match {*movablekeys*} [lindex $info 2] + } + } + + foreach cmd {ZUNIONSTORE XREAD EVAL SORT SORT_RO MIGRATE GEORADIUS} { + test "$cmd command is marked with movablekeys" { + set info [lindex [r command info $cmd] 0] + assert_match {*movablekeys*} [lindex $info 2] + } + } + } diff --git a/tests/unit/moduleapi/keyspecs.tcl b/tests/unit/moduleapi/keyspecs.tcl index 60d3fe5d3..ef5b92334 100644 --- a/tests/unit/moduleapi/keyspecs.tcl +++ b/tests/unit/moduleapi/keyspecs.tcl @@ -31,6 +31,20 @@ start_server {tags {"modules"}} { assert_equal [r command getkeys kspec.tworanges foo bar baz quux] {foo bar} } + test "Module key specs: Two ranges with gap" { + set reply [lindex [r command info kspec.tworangeswithgap] 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] 1 + # Verify key-specs + set keyspecs [lindex $reply 8] + assert_equal [lindex $keyspecs 0] {flags {RO access} begin_search {type index spec {index 1}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}} + assert_equal [lindex $keyspecs 1] {flags {RW update} begin_search {type index spec {index 3}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}} + assert_equal [r command getkeys kspec.tworangeswithgap foo bar baz quux] {foo baz} + } + test "Module key specs: Keyword-only spec clears the legacy triple" { set reply [lindex [r command info kspec.keyword] 0] # Verify (first, last, step) and movablekeys @@ -79,7 +93,7 @@ start_server {tags {"modules"}} { test "Module command list filtering" { ;# Note: we piggyback this tcl file to test the general functionality of command list filtering set reply [r command list filterby module keyspecs] - assert_equal [lsort $reply] {kspec.complex1 kspec.complex2 kspec.keyword kspec.none kspec.tworanges} + assert_equal [lsort $reply] {kspec.complex1 kspec.complex2 kspec.keyword kspec.none kspec.nonewithgetkeys kspec.tworanges kspec.tworangeswithgap} assert_equal [r command getkeys kspec.complex2 foo bar 2 baz quux banana STORE dst dummy MOREKEYS hey ho] {dst foo bar baz quux hey ho} } @@ -108,6 +122,20 @@ start_server {tags {"modules"}} { assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser kspec.tworanges write rw] } + foreach cmd {kspec.none kspec.tworanges} { + test "$cmd command will not be marked with movablekeys" { + set info [lindex [r command info $cmd] 0] + assert_no_match {*movablekeys*} [lindex $info 2] + } + } + + foreach cmd {kspec.keyword kspec.complex1 kspec.complex2 kspec.nonewithgetkeys} { + test "$cmd command is marked with movablekeys" { + set info [lindex [r command info $cmd] 0] + assert_match {*movablekeys*} [lindex $info 2] + } + } + test "Unload the module - keyspecs" { assert_equal {OK} [r module unload keyspecs] } |