Unverified Commit 23325c13 authored by Binbin's avatar Binbin Committed by GitHub
Browse files

sub-command support for ACL CAT and COMMAND LIST. redisCommand always stores fullname (#10127)



Summary of changes:
1. Rename `redisCommand->name` to `redisCommand->declared_name`, it is a
  const char * for native commands and SDS for module commands.
2. Store the [sub]command fullname in `redisCommand->fullname` (sds).
3. List subcommands in `ACL CAT`
4. List subcommands in `COMMAND LIST`
5. `moduleUnregisterCommands` now will also free the module subcommands.
6. RM_GetCurrentCommandName returns full command name

Other changes:
1. Add `addReplyErrorArity` and `addReplyErrorExpireTime`
2. Remove `getFullCommandName` function that now is useless.
3. Some cleanups about `fullname` since now it is SDS.
4. Delete `populateSingleCommand` function from server.h that is useless.
5. Added tests to cover this change.
6. Add some module unload tests and fix the leaks
7. Make error messages uniform, make sure they always contain the full command
  name and that it's quoted.
7. Fixes some typos

see the history in #9504, fixes #10124
Co-authored-by: default avatarOran Agra <oran@redislabs.com>
Co-authored-by: default avatarguybe7 <guy.benoish@redislabs.com>
parent a6fd2a46
...@@ -244,35 +244,35 @@ start_server {tags {"expire"}} { ...@@ -244,35 +244,35 @@ start_server {tags {"expire"}} {
test {SET with EX with big integer should report an error} { test {SET with EX with big integer should report an error} {
catch {r set foo bar EX 10000000000000000} e catch {r set foo bar EX 10000000000000000} e
set e set e
} {ERR invalid expire time in set} } {ERR invalid expire time in 'set' command}
test {SET with EX with smallest integer should report an error} { test {SET with EX with smallest integer should report an error} {
catch {r SET foo bar EX -9999999999999999} e catch {r SET foo bar EX -9999999999999999} e
set e set e
} {ERR invalid expire time in set} } {ERR invalid expire time in 'set' command}
test {GETEX with big integer should report an error} { test {GETEX with big integer should report an error} {
r set foo bar r set foo bar
catch {r GETEX foo EX 10000000000000000} e catch {r GETEX foo EX 10000000000000000} e
set e set e
} {ERR invalid expire time in getex} } {ERR invalid expire time in 'getex' command}
test {GETEX with smallest integer should report an error} { test {GETEX with smallest integer should report an error} {
r set foo bar r set foo bar
catch {r GETEX foo EX -9999999999999999} e catch {r GETEX foo EX -9999999999999999} e
set e set e
} {ERR invalid expire time in getex} } {ERR invalid expire time in 'getex' command}
test {EXPIRE with big integer overflows when converted to milliseconds} { test {EXPIRE with big integer overflows when converted to milliseconds} {
r set foo bar r set foo bar
# Hit `when > LLONG_MAX - basetime` # Hit `when > LLONG_MAX - basetime`
assert_error "ERR invalid expire time in expire" {r EXPIRE foo 9223370399119966} assert_error "ERR invalid expire time in 'expire' command" {r EXPIRE foo 9223370399119966}
# Hit `when > LLONG_MAX / 1000` # Hit `when > LLONG_MAX / 1000`
assert_error "ERR invalid expire time in expire" {r EXPIRE foo 9223372036854776} assert_error "ERR invalid expire time in 'expire' command" {r EXPIRE foo 9223372036854776}
assert_error "ERR invalid expire time in expire" {r EXPIRE foo 10000000000000000} assert_error "ERR invalid expire time in 'expire' command" {r EXPIRE foo 10000000000000000}
assert_error "ERR invalid expire time in expire" {r EXPIRE foo 18446744073709561} assert_error "ERR invalid expire time in 'expire' command" {r EXPIRE foo 18446744073709561}
assert_equal {-1} [r ttl foo] assert_equal {-1} [r ttl foo]
} }
...@@ -281,14 +281,14 @@ start_server {tags {"expire"}} { ...@@ -281,14 +281,14 @@ start_server {tags {"expire"}} {
r set foo bar r set foo bar
catch {r PEXPIRE foo 9223372036854770000} e catch {r PEXPIRE foo 9223372036854770000} e
set e set e
} {ERR invalid expire time in pexpire} } {ERR invalid expire time in 'pexpire' command}
test {EXPIRE with big negative integer} { test {EXPIRE with big negative integer} {
r set foo bar r set foo bar
# Hit `when < LLONG_MIN / 1000` # Hit `when < LLONG_MIN / 1000`
assert_error "ERR invalid expire time in expire" {r EXPIRE foo -9223372036854776} assert_error "ERR invalid expire time in 'expire' command" {r EXPIRE foo -9223372036854776}
assert_error "ERR invalid expire time in expire" {r EXPIRE foo -9999999999999999} assert_error "ERR invalid expire time in 'expire' command" {r EXPIRE foo -9999999999999999}
r ttl foo r ttl foo
} {-1} } {-1}
......
...@@ -209,7 +209,7 @@ start_server {tags {"scripting"}} { ...@@ -209,7 +209,7 @@ start_server {tags {"scripting"}} {
test {FUNCTION - test function restore with wrong number of arguments} { test {FUNCTION - test function restore with wrong number of arguments} {
catch {r function restore arg1 args2 arg3} e catch {r function restore arg1 args2 arg3} e
set _ $e set _ $e
} {*wrong number of arguments*} } {*Unknown subcommand or wrong number of arguments for 'restore'. Try FUNCTION HELP.}
test {FUNCTION - test fcall_ro with write command} { test {FUNCTION - test fcall_ro with write command} {
r function load lua test REPLACE [get_no_writes_function_code test {return redis.call('set', 'x', '1')}] r function load lua test REPLACE [get_no_writes_function_code test {return redis.call('set', 'x', '1')}]
...@@ -302,7 +302,7 @@ start_server {tags {"scripting"}} { ...@@ -302,7 +302,7 @@ start_server {tags {"scripting"}} {
assert_match {*only supports SYNC|ASYNC*} $e assert_match {*only supports SYNC|ASYNC*} $e
catch {r function flush sync extra_arg} e catch {r function flush sync extra_arg} e
assert_match {*wrong number of arguments*} $e assert_match {*Unknown subcommand or wrong number of arguments for 'flush'. Try FUNCTION HELP.} $e
} }
} }
......
...@@ -197,7 +197,7 @@ start_server {tags {"info" "external:skip"}} { ...@@ -197,7 +197,7 @@ start_server {tags {"info" "external:skip"}} {
assert_match {} [errorstat ERR] assert_match {} [errorstat ERR]
r multi r multi
catch {r set} e catch {r set} e
assert_match {ERR wrong number of arguments*} $e assert_match {ERR wrong number of arguments for 'set' command} $e
catch {r exec} e catch {r exec} e
assert_match {EXECABORT*} $e assert_match {EXECABORT*} $e
assert_match {*count=1*} [errorstat ERR] assert_match {*count=1*} [errorstat ERR]
...@@ -216,7 +216,7 @@ start_server {tags {"info" "external:skip"}} { ...@@ -216,7 +216,7 @@ start_server {tags {"info" "external:skip"}} {
assert_equal [s total_error_replies] 0 assert_equal [s total_error_replies] 0
assert_match {} [errorstat ERR] assert_match {} [errorstat ERR]
catch {r set k} e catch {r set k} e
assert_match {ERR wrong number of arguments*} $e assert_match {ERR wrong number of arguments for 'set' command} $e
assert_match {*count=1*} [errorstat ERR] assert_match {*count=1*} [errorstat ERR]
assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat set] assert_match {*calls=0,*,rejected_calls=1,failed_calls=0} [cmdstat set]
# ensure that after a rejected command, valid ones are counted properly # ensure that after a rejected command, valid ones are counted properly
......
...@@ -101,14 +101,59 @@ start_server {tags {"introspection"}} { ...@@ -101,14 +101,59 @@ start_server {tags {"introspection"}} {
assert_equal {key1 key2} [r command getkeys lcs key1 key2] assert_equal {key1 key2} [r command getkeys lcs key1 key2]
} }
test "COMMAND LIST FILTERBY ACLCAT" { test "COMMAND LIST syntax error" {
set reply [r command list filterby aclcat hyperloglog] assert_error "ERR syntax error*" {r command list bad_arg}
assert_equal [lsort $reply] {pfadd pfcount pfdebug pfmerge pfselftest} assert_error "ERR syntax error*" {r command list filterby bad_arg}
assert_error "ERR syntax error*" {r command list filterby bad_arg bad_arg2}
} }
test "COMMAND LIST FILTERBY PATTERN" { test "COMMAND LIST WITHOUT FILTERBY" {
set reply [r command list filterby pattern pf*] set commands [r command list]
assert_equal [lsort $reply] {pfadd pfcount pfdebug pfmerge pfselftest} assert_not_equal [lsearch $commands "set"] -1
assert_not_equal [lsearch $commands "client|list"] -1
}
test "COMMAND LIST FILTERBY ACLCAT against non existing category" {
assert_equal {} [r command list filterby aclcat non_existing_category]
}
test "COMMAND LIST FILTERBY ACLCAT - list all commands/subcommands" {
set commands [r command list filterby aclcat scripting]
assert_not_equal [lsearch $commands "eval"] -1
assert_not_equal [lsearch $commands "script|kill"] -1
# Negative check, a command that should not be here
assert_equal [lsearch $commands "set"] -1
}
test "COMMAND LIST FILTERBY PATTERN - list all commands/subcommands" {
# Exact command match.
assert_equal {set} [r command list filterby pattern set]
assert_equal {get} [r command list filterby pattern get]
# Return the parent command and all the subcommands below it.
set commands [r command list filterby pattern config*]
assert_not_equal [lsearch $commands "config"] -1
assert_not_equal [lsearch $commands "config|get"] -1
# We can filter subcommands under a parent command.
set commands [r command list filterby pattern config|*re*]
assert_not_equal [lsearch $commands "config|resetstat"] -1
assert_not_equal [lsearch $commands "config|rewrite"] -1
# We can filter subcommands across parent commands.
set commands [r command list filterby pattern cl*help]
assert_not_equal [lsearch $commands "client|help"] -1
assert_not_equal [lsearch $commands "cluster|help"] -1
# Negative check, command that doesn't exist.
assert_equal {} [r command list filterby pattern non_exists]
assert_equal {} [r command list filterby pattern non_exists*]
}
test "COMMAND LIST FILTERBY MODULE against non existing module" {
# This should be empty, the real one is in subcommands.tcl
assert_equal {} [r command list filterby module non_existing_module]
} }
test {COMMAND INFO of invalid subcommands} { test {COMMAND INFO of invalid subcommands} {
......
start_server {tags {"introspection"}} { start_server {tags {"introspection"}} {
test "PING" {
assert_equal {PONG} [r ping]
assert_equal {redis} [r ping redis]
assert_error {*wrong number of arguments for 'ping' command} {r ping hello redis}
}
test {CLIENT LIST} { test {CLIENT LIST} {
r client list r client list
} {*addr=*:* fd=* age=* idle=* flags=N db=* sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=* argv-mem=* obl=0 oll=0 omem=0 tot-mem=* events=r cmd=client*} } {id=* addr=*:* laddr=*:* fd=* name=* age=* idle=* flags=N db=* sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=* argv-mem=* multi-mem=0 obl=0 oll=0 omem=0 tot-mem=* events=r cmd=client|list user=* redir=-1 resp=2*}
test {CLIENT LIST with IDs} { test {CLIENT LIST with IDs} {
set myid [r client id] set myid [r client id]
set cl [split [r client list id $myid] "\r\n"] set cl [split [r client list id $myid] "\r\n"]
assert_match "id=$myid*" [lindex $cl 0] assert_match "id=$myid * cmd=client|list *" [lindex $cl 0]
} }
test {CLIENT INFO} { test {CLIENT INFO} {
r client info r client info
} {*addr=*:* fd=* age=* idle=* flags=N db=* sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=* argv-mem=* obl=0 oll=0 omem=0 tot-mem=* events=r cmd=client*} } {id=* addr=*:* laddr=*:* fd=* name=* age=* idle=* flags=N db=* sub=0 psub=0 multi=-1 qbuf=26 qbuf-free=* argv-mem=* multi-mem=0 obl=0 oll=0 omem=0 tot-mem=* events=r cmd=client|info user=* redir=-1 resp=2*}
test {CLIENT KILL with illegal arguments} { test {CLIENT KILL with illegal arguments} {
assert_error "ERR wrong number*" {r client kill} assert_error "ERR wrong number of arguments for 'client|kill' command" {r client kill}
assert_error "ERR syntax error*" {r client kill id 10 wrong_arg} assert_error "ERR syntax error*" {r client kill id 10 wrong_arg}
assert_error "ERR*greater than 0*" {r client kill id str} assert_error "ERR*greater than 0*" {r client kill id str}
......
...@@ -8,6 +8,7 @@ start_server {tags {"latency-monitor needs:latency"}} { ...@@ -8,6 +8,7 @@ start_server {tags {"latency-monitor needs:latency"}} {
set histo [dict create {*}[r latency histogram]] set histo [dict create {*}[r latency histogram]]
# Config resetstat is recorded # Config resetstat is recorded
assert_equal [dict size $histo] 1 assert_equal [dict size $histo] 1
assert_match {*config|resetstat*} $histo
} }
test {LATENCY HISTOGRAM all commands} { test {LATENCY HISTOGRAM all commands} {
...@@ -141,6 +142,6 @@ start_server {tags {"latency-monitor needs:latency"}} { ...@@ -141,6 +142,6 @@ start_server {tags {"latency-monitor needs:latency"}} {
test {LATENCY HELP should not have unexpected options} { test {LATENCY HELP should not have unexpected options} {
catch {r LATENCY help xxx} e catch {r LATENCY help xxx} e
assert_match "*wrong number of arguments*" $e assert_match "*wrong number of arguments for 'latency|help' command" $e
} }
} }
...@@ -72,4 +72,8 @@ start_server {tags {"modules acl"}} { ...@@ -72,4 +72,8 @@ start_server {tags {"modules acl"}} {
assert {[dict get $entry context] eq {module}} assert {[dict get $entry context] eq {module}}
assert {[dict get $entry object] eq {set}} assert {[dict get $entry object] eq {set}}
} }
test "Unload the module - aclcheck" {
assert_equal {OK} [r module unload aclcheck]
}
} }
...@@ -68,4 +68,7 @@ start_server {tags {"modules"}} { ...@@ -68,4 +68,7 @@ start_server {tags {"modules"}} {
assert_equal [r acl whoami] "default" assert_equal [r acl whoami] "default"
} }
} test "Unload the module - testacl" {
\ No newline at end of file assert_equal {OK} [r module unload testacl]
}
}
set testmodule [file normalize tests/modules/basics.so] set testmodule [file normalize tests/modules/basics.so]
start_server {tags {"modules"}} { start_server {tags {"modules"}} {
r module load $testmodule r module load $testmodule
...@@ -30,7 +29,9 @@ start_server {tags {"modules"}} { ...@@ -30,7 +29,9 @@ start_server {tags {"modules"}} {
assert_equal $reply 3 assert_equal $reply 3
} }
r module unload test test "Unload the module - test" {
assert_equal {OK} [r module unload test]
}
} }
start_server {tags {"modules external:skip"} overrides {enable-module-command no}} { start_server {tags {"modules external:skip"} overrides {enable-module-command no}} {
......
# source tests/support/util.tcl
set testmodule [file normalize tests/modules/blockedclient.so] set testmodule [file normalize tests/modules/blockedclient.so]
start_server {tags {"modules"}} { start_server {tags {"modules"}} {
...@@ -181,4 +179,8 @@ start_server {tags {"modules"}} { ...@@ -181,4 +179,8 @@ start_server {tags {"modules"}} {
set clients [r client list] set clients [r client list]
assert_no_match "*name=myclient*" $clients assert_no_match "*name=myclient*" $clients
} }
test "Unload the module - blockedclient" {
assert_equal {OK} [r module unload blockedclient]
}
} }
...@@ -91,4 +91,8 @@ start_server {tags {"modules"}} { ...@@ -91,4 +91,8 @@ start_server {tags {"modules"}} {
r eval "redis.call('commandfilter.ping')" 0 r eval "redis.call('commandfilter.ping')" 0
assert_equal {} [r lrange log-key 0 -1] assert_equal {} [r lrange log-key 0 -1]
} }
test "Unload the module - commandfilter" {
assert_equal {OK} [r module unload commandfilter]
}
} }
...@@ -21,4 +21,8 @@ start_server {tags {"modules"}} { ...@@ -21,4 +21,8 @@ start_server {tags {"modules"}} {
test "Module eventloop oneshot" { test "Module eventloop oneshot" {
r test.oneshot r test.oneshot
} }
test "Unload the module - eventloop" {
assert_equal {OK} [r module unload eventloop]
}
} }
...@@ -39,4 +39,7 @@ start_server {tags {"modules"}} { ...@@ -39,4 +39,7 @@ start_server {tags {"modules"}} {
assert {[count_log_message "Can't fork for module: File exists"] eq "1"} assert {[count_log_message "Can't fork for module: File exists"] eq "1"}
} }
test "Unload the module - fork" {
assert_equal {OK} [r module unload fork]
}
} }
...@@ -41,4 +41,8 @@ start_server {tags {"modules"}} { ...@@ -41,4 +41,8 @@ start_server {tags {"modules"}} {
catch {r getkeys.introspect set key} e catch {r getkeys.introspect set key} e
set _ $e set _ $e
} {*EINVAL*} } {*EINVAL*}
test "Unload the module - getkeys" {
assert_equal {OK} [r module unload getkeys]
}
} }
...@@ -20,4 +20,8 @@ start_server {tags {"modules"}} { ...@@ -20,4 +20,8 @@ start_server {tags {"modules"}} {
assert_equal 1 [r hash.set k "" sushi :delete: none :delete:] assert_equal 1 [r hash.set k "" sushi :delete: none :delete:]
r hgetall k r hgetall k
} {squirrel ofcourse banana no what nothing something nice} } {squirrel ofcourse banana no what nothing something nice}
test "Unload the module - hash" {
assert_equal {OK} [r module unload hash]
}
} }
...@@ -90,5 +90,9 @@ start_server {tags {"modules"}} { ...@@ -90,5 +90,9 @@ start_server {tags {"modules"}} {
assert_match {*infotest_unsafe_field:value=1*} $info assert_match {*infotest_unsafe_field:value=1*} $info
} }
test "Unload the module - infotest" {
assert_equal {OK} [r module unload infotest]
}
# TODO: test crash report. # TODO: test crash report.
} }
...@@ -12,11 +12,11 @@ test {modules config rewrite} { ...@@ -12,11 +12,11 @@ test {modules config rewrite} {
assert_equal [lindex [lindex [r module list] 0] 1] infotest assert_equal [lindex [lindex [r module list] 0] 1] infotest
r module unload infotest assert_equal {OK} [r module unload infotest]
r config rewrite r config rewrite
restart_server 0 true false restart_server 0 true false
assert_equal [llength [r module list]] 0 assert_equal [llength [r module list]] 0
} }
} }
...@@ -82,5 +82,9 @@ tags "modules" { ...@@ -82,5 +82,9 @@ tags "modules" {
assert_equal {pmessage * __keyspace@9__:x notify} [$rd1 read] assert_equal {pmessage * __keyspace@9__:x notify} [$rd1 read]
$rd1 close $rd1 close
} }
}
test "Unload the module - testkeyspace" {
assert_equal {OK} [r module unload testkeyspace]
}
}
} }
...@@ -48,4 +48,8 @@ start_server {tags {"modules"}} { ...@@ -48,4 +48,8 @@ start_server {tags {"modules"}} {
set reply [r command list filterby module keyspecs] set reply [r command list filterby module keyspecs]
assert_equal [lsort $reply] {kspec.complex1 kspec.complex2 kspec.legacy} assert_equal [lsort $reply] {kspec.complex1 kspec.complex2 kspec.legacy}
} }
test "Unload the module - keyspecs" {
assert_equal {OK} [r module unload keyspecs]
}
} }
...@@ -63,4 +63,8 @@ start_server {tags {"modules"}} { ...@@ -63,4 +63,8 @@ start_server {tags {"modules"}} {
r list.edit k reverse rkr foo bar r list.edit k reverse rkr foo bar
r list.getall k r list.getall k
} {bar y foo} } {bar y foo}
test "Unload the module - list" {
assert_equal {OK} [r module unload list]
}
} }
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