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

Fix regression not aborting transaction on error, and re-edit some error responses (#10612)

1. Disk error and slave count checks didn't flag the transactions or counted correctly in command stats (regression from #10372  , 7.0 RC3)
2. RM_Call will reply the same way Redis does, in case of non-exisitng command or arity error
3. RM_WrongArtiy will consider the full command name
4. Use lowercase 'u' in "unknonw subcommand" (to align with "unknown command")

Followup work of #10127
parent 21e39ec4
...@@ -2659,9 +2659,7 @@ void RM_TrimStringAllocation(RedisModuleString *str) { ...@@ -2659,9 +2659,7 @@ void RM_TrimStringAllocation(RedisModuleString *str) {
* if (argc != 3) return RedisModule_WrongArity(ctx); * if (argc != 3) return RedisModule_WrongArity(ctx);
*/ */
int RM_WrongArity(RedisModuleCtx *ctx) { int RM_WrongArity(RedisModuleCtx *ctx) {
addReplyErrorFormat(ctx->client, addReplyErrorArity(ctx->client);
"wrong number of arguments for '%s' command",
(char*)ctx->client->argv[0]->ptr);
return REDISMODULE_OK; return REDISMODULE_OK;
} }
...@@ -5759,26 +5757,18 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch ...@@ -5759,26 +5757,18 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
/* Lookup command now, after filters had a chance to make modifications /* Lookup command now, after filters had a chance to make modifications
* if necessary. * if necessary.
*/ */
cmd = lookupCommand(c->argv,c->argc); cmd = c->cmd = c->lastcmd = c->realcmd = lookupCommand(c->argv,c->argc);
if (!cmd) { sds err;
if (!commandCheckExistence(c, error_as_call_replies? &err : NULL)) {
errno = ENOENT; errno = ENOENT;
if (error_as_call_replies) { if (error_as_call_replies)
sds msg = sdscatfmt(sdsempty(),"Unknown Redis " reply = callReplyCreateError(err, ctx);
"command '%S'.",c->argv[0]->ptr);
reply = callReplyCreateError(msg, ctx);
}
goto cleanup; goto cleanup;
} }
c->cmd = c->lastcmd = c->realcmd = cmd; if (!commandCheckArity(c, error_as_call_replies? &err : NULL)) {
/* Basic arity checks. */
if ((cmd->arity > 0 && cmd->arity != argc) || (argc < -cmd->arity)) {
errno = EINVAL; errno = EINVAL;
if (error_as_call_replies) { if (error_as_call_replies)
sds msg = sdscatfmt(sdsempty(), "Wrong number of " reply = callReplyCreateError(err, ctx);
"args calling Redis command '%S'.", c->cmd->fullname);
reply = callReplyCreateError(msg, ctx);
}
goto cleanup; goto cleanup;
} }
......
...@@ -1078,7 +1078,7 @@ void addReplySubcommandSyntaxError(client *c) { ...@@ -1078,7 +1078,7 @@ void addReplySubcommandSyntaxError(client *c) {
sds cmd = sdsnew((char*) c->argv[0]->ptr); sds cmd = sdsnew((char*) c->argv[0]->ptr);
sdstoupper(cmd); sdstoupper(cmd);
addReplyErrorFormat(c, addReplyErrorFormat(c,
"Unknown subcommand or wrong number of arguments for '%.128s'. Try %s HELP.", "unknown subcommand or wrong number of arguments for '%.128s'. Try %s HELP.",
(char*)c->argv[1]->ptr,cmd); (char*)c->argv[1]->ptr,cmd);
sdsfree(cmd); sdsfree(cmd);
} }
......
...@@ -3429,6 +3429,8 @@ void rejectCommand(client *c, robj *reply) { ...@@ -3429,6 +3429,8 @@ void rejectCommand(client *c, robj *reply) {
} }
void rejectCommandSds(client *c, sds s) { void rejectCommandSds(client *c, sds s) {
flagTransaction(c);
if (c->cmd) c->cmd->rejected_calls++;
if (c->cmd && c->cmd->proc == execCommand) { if (c->cmd && c->cmd->proc == execCommand) {
execCommandAbort(c, s); execCommandAbort(c, s);
sdsfree(s); sdsfree(s);
...@@ -3439,8 +3441,6 @@ void rejectCommandSds(client *c, sds s) { ...@@ -3439,8 +3441,6 @@ void rejectCommandSds(client *c, sds s) {
} }
void rejectCommandFormat(client *c, const char *fmt, ...) { void rejectCommandFormat(client *c, const char *fmt, ...) {
if (c->cmd) c->cmd->rejected_calls++;
flagTransaction(c);
va_list ap; va_list ap;
va_start(ap,fmt); va_start(ap,fmt);
sds s = sdscatvprintf(sdsempty(),fmt,ap); sds s = sdscatvprintf(sdsempty(),fmt,ap);
...@@ -3494,6 +3494,51 @@ void populateCommandMovableKeys(struct redisCommand *cmd) { ...@@ -3494,6 +3494,51 @@ void populateCommandMovableKeys(struct redisCommand *cmd) {
cmd->flags |= CMD_MOVABLE_KEYS; cmd->flags |= CMD_MOVABLE_KEYS;
} }
/* Check if c->cmd exists, fills `err` with deatils in case it doesn't
* Return 1 if exists. */
int commandCheckExistence(client *c, sds *err) {
if (c->cmd)
return 1;
if (!err)
return 0;
if (isContainerCommandBySds(c->argv[0]->ptr)) {
/* If we can't find the command but argv[0] by itself is a command
* it means we're dealing with an invalid subcommand. Print Help. */
sds cmd = sdsnew((char *)c->argv[0]->ptr);
sdstoupper(cmd);
*err = sdsnew(NULL);
*err = sdscatprintf(*err, "unknown subcommand '%.128s'. Try %s HELP.",
(char *)c->argv[1]->ptr, cmd);
sdsfree(cmd);
} else {
sds args = sdsempty();
int i;
for (i=1; i < c->argc && sdslen(args) < 128; i++)
args = sdscatprintf(args, "'%.*s' ", 128-(int)sdslen(args), (char*)c->argv[i]->ptr);
*err = sdsnew(NULL);
*err = sdscatprintf(*err, "unknown command '%s', with args beginning with: %s",
(char*)c->argv[0]->ptr, args);
sdsfree(args);
}
return 0;
}
/* Check if c->argc is valid for c->cmd, fills `err` with deatils in case it isn't
* Return 1 if valid. */
int commandCheckArity(client *c, sds *err) {
if ((c->cmd->arity > 0 && c->cmd->arity != c->argc) ||
(c->argc < -c->cmd->arity))
{
if (err) {
*err = sdsnew(NULL);
*err = sdscatprintf(*err, "wrong number of arguments for '%s' command", c->cmd->fullname);
}
return 0;
}
return 1;
}
/* If this function gets called we already read a whole /* If this function gets called we already read a whole
* command, arguments are in the client argv/argc fields. * command, arguments are in the client argv/argc fields.
* processCommand() execute the command or prepare the * processCommand() execute the command or prepare the
...@@ -3533,29 +3578,13 @@ int processCommand(client *c) { ...@@ -3533,29 +3578,13 @@ int processCommand(client *c) {
/* Now lookup the command and check ASAP about trivial error conditions /* Now lookup the command and check ASAP about trivial error conditions
* such as wrong arity, bad command name and so forth. */ * such as wrong arity, bad command name and so forth. */
c->cmd = c->lastcmd = c->realcmd = lookupCommand(c->argv,c->argc); c->cmd = c->lastcmd = c->realcmd = lookupCommand(c->argv,c->argc);
if (!c->cmd) { sds err;
if (isContainerCommandBySds(c->argv[0]->ptr)) { if (!commandCheckExistence(c, &err)) {
/* If we can't find the command but argv[0] by itself is a command rejectCommandSds(c, err);
* it means we're dealing with an invalid subcommand. Print Help. */
sds cmd = sdsnew((char *)c->argv[0]->ptr);
sdstoupper(cmd);
rejectCommandFormat(c, "Unknown subcommand '%.128s'. Try %s HELP.",
(char *)c->argv[1]->ptr, cmd);
sdsfree(cmd);
return C_OK;
}
sds args = sdsempty();
int i;
for (i=1; i < c->argc && sdslen(args) < 128; i++)
args = sdscatprintf(args, "'%.*s' ", 128-(int)sdslen(args), (char*)c->argv[i]->ptr);
rejectCommandFormat(c,"unknown command '%s', with args beginning with: %s",
(char*)c->argv[0]->ptr, args);
sdsfree(args);
return C_OK; return C_OK;
} else if ((c->cmd->arity > 0 && c->cmd->arity != c->argc) || }
(c->argc < -c->cmd->arity)) if (!commandCheckArity(c, &err)) {
{ rejectCommandSds(c, err);
rejectCommandFormat(c,"wrong number of arguments for '%s' command", c->cmd->fullname);
return C_OK; return C_OK;
} }
......
...@@ -2864,6 +2864,8 @@ struct redisCommand *lookupCommandBySds(sds s); ...@@ -2864,6 +2864,8 @@ struct redisCommand *lookupCommandBySds(sds s);
struct redisCommand *lookupCommandByCStringLogic(dict *commands, const char *s); struct redisCommand *lookupCommandByCStringLogic(dict *commands, const char *s);
struct redisCommand *lookupCommandByCString(const char *s); struct redisCommand *lookupCommandByCString(const char *s);
struct redisCommand *lookupCommandOrOriginal(robj **argv, int argc); struct redisCommand *lookupCommandOrOriginal(robj **argv, int argc);
int commandCheckExistence(client *c, sds *err);
int commandCheckArity(client *c, sds *err);
void startCommandExecution(); void startCommandExecution();
int incrCommandStatsOnError(struct redisCommand *cmd, int flags); int incrCommandStatsOnError(struct redisCommand *cmd, int flags);
void call(client *c, int flags); void call(client *c, int flags);
......
...@@ -866,10 +866,10 @@ int TestBasics(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { ...@@ -866,10 +866,10 @@ int TestBasics(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
if (!TestAssertStringReply(ctx,RedisModule_CallReplyArrayElement(reply, 1),"1234",4)) goto fail; if (!TestAssertStringReply(ctx,RedisModule_CallReplyArrayElement(reply, 1),"1234",4)) goto fail;
T("foo", "E"); T("foo", "E");
if (!TestAssertErrorReply(ctx,reply,"ERR Unknown Redis command 'foo'.",32)) goto fail; if (!TestAssertErrorReply(ctx,reply,"ERR unknown command 'foo', with args beginning with: ",53)) goto fail;
T("set", "Ec", "x"); T("set", "Ec", "x");
if (!TestAssertErrorReply(ctx,reply,"ERR Wrong number of args calling Redis command 'set'.",53)) goto fail; if (!TestAssertErrorReply(ctx,reply,"ERR wrong number of arguments for 'set' command",47)) goto fail;
T("shutdown", "SE"); T("shutdown", "SE");
if (!TestAssertErrorReply(ctx,reply,"ERR command 'shutdown' is not allowed on script mode",52)) goto fail; if (!TestAssertErrorReply(ctx,reply,"ERR command 'shutdown' is not allowed on script mode",52)) goto fail;
......
...@@ -11,7 +11,10 @@ int cmd_set(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { ...@@ -11,7 +11,10 @@ int cmd_set(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
int cmd_get(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { int cmd_get(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) {
UNUSED(argv); UNUSED(argv);
UNUSED(argc);
if (argc > 4) /* For testing */
return RedisModule_WrongArity(ctx);
RedisModule_ReplyWithSimpleString(ctx, "OK"); RedisModule_ReplyWithSimpleString(ctx, "OK");
return REDISMODULE_OK; return REDISMODULE_OK;
} }
......
...@@ -511,7 +511,7 @@ start_server {tags {"acl external:skip"}} { ...@@ -511,7 +511,7 @@ start_server {tags {"acl external:skip"}} {
test "ACL CAT with illegal arguments" { test "ACL CAT with illegal arguments" {
assert_error {*Unknown category 'NON_EXISTS'} {r ACL CAT NON_EXISTS} assert_error {*Unknown category 'NON_EXISTS'} {r ACL CAT NON_EXISTS}
assert_error {*Unknown subcommand or wrong number of arguments for 'CAT'*} {r ACL CAT NON_EXISTS NON_EXISTS2} assert_error {*unknown subcommand or wrong number of arguments for 'CAT'*} {r ACL CAT NON_EXISTS NON_EXISTS2}
} }
test "ACL CAT without category - list all categories" { test "ACL CAT without category - list all categories" {
......
...@@ -117,7 +117,7 @@ start_server {tags {"scripting"}} { ...@@ -117,7 +117,7 @@ start_server {tags {"scripting"}} {
r function bad_subcommand r function bad_subcommand
} e } e
set _ $e set _ $e
} {*Unknown subcommand*} } {*unknown subcommand*}
test {FUNCTION - test loading from rdb} { test {FUNCTION - test loading from rdb} {
r debug reload r debug reload
...@@ -205,7 +205,7 @@ start_server {tags {"scripting"}} { ...@@ -205,7 +205,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
} {*Unknown subcommand or wrong number of arguments for 'restore'. Try FUNCTION HELP.} } {*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 REPLACE [get_no_writes_function_code lua test test {return redis.call('set', 'x', '1')}] r function load REPLACE [get_no_writes_function_code lua test test {return redis.call('set', 'x', '1')}]
...@@ -298,7 +298,7 @@ start_server {tags {"scripting"}} { ...@@ -298,7 +298,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 {*Unknown subcommand or wrong number of arguments for 'flush'. Try FUNCTION HELP.} $e assert_match {*unknown subcommand or wrong number of arguments for 'flush'. Try FUNCTION HELP.} $e
} }
} }
......
...@@ -203,9 +203,13 @@ foreach call_type {nested normal} { ...@@ -203,9 +203,13 @@ foreach call_type {nested normal} {
r config resetstat r config resetstat
# simple module command that replies with string error # simple module command that replies with string error
assert_error "ERR Unknown Redis command 'hgetalllll'." {r do_rm_call hgetalllll} assert_error "ERR unknown command 'hgetalllll', with args beginning with:" {r do_rm_call hgetalllll}
assert_equal [errorrstat ERR r] {count=1} assert_equal [errorrstat ERR r] {count=1}
# simple module command that replies with string error
assert_error "ERR unknown subcommand 'bla'. Try CONFIG HELP." {r do_rm_call config bla}
assert_equal [errorrstat ERR r] {count=2}
# module command that replies with string error from bg thread # module command that replies with string error from bg thread
assert_error "NULL reply returned" {r do_bg_rm_call hgetalllll} assert_error "NULL reply returned" {r do_bg_rm_call hgetalllll}
assert_equal [errorrstat NULL r] {count=1} assert_equal [errorrstat NULL r] {count=1}
...@@ -213,7 +217,7 @@ foreach call_type {nested normal} { ...@@ -213,7 +217,7 @@ foreach call_type {nested normal} {
# module command that returns an arity error # module command that returns an arity error
r do_rm_call set x x r do_rm_call set x x
assert_error "ERR wrong number of arguments for 'do_rm_call' command" {r do_rm_call} assert_error "ERR wrong number of arguments for 'do_rm_call' command" {r do_rm_call}
assert_equal [errorrstat ERR r] {count=2} assert_equal [errorrstat ERR r] {count=3}
# RM_Call that propagates an error # RM_Call that propagates an error
assert_error "WRONGTYPE*" {r do_rm_call hgetall x} assert_error "WRONGTYPE*" {r do_rm_call hgetall x}
...@@ -225,8 +229,8 @@ foreach call_type {nested normal} { ...@@ -225,8 +229,8 @@ foreach call_type {nested normal} {
assert_equal [errorrstat WRONGTYPE r] {count=2} assert_equal [errorrstat WRONGTYPE r] {count=2}
assert_match {*calls=2,*,rejected_calls=0,failed_calls=2} [cmdrstat hgetall r] assert_match {*calls=2,*,rejected_calls=0,failed_calls=2} [cmdrstat hgetall r]
assert_equal [s total_error_replies] 5 assert_equal [s total_error_replies] 6
assert_match {*calls=4,*,rejected_calls=0,failed_calls=3} [cmdrstat do_rm_call r] assert_match {*calls=5,*,rejected_calls=0,failed_calls=4} [cmdrstat do_rm_call r]
assert_match {*calls=2,*,rejected_calls=0,failed_calls=2} [cmdrstat do_bg_rm_call r] assert_match {*calls=2,*,rejected_calls=0,failed_calls=2} [cmdrstat do_bg_rm_call r]
} }
......
...@@ -25,6 +25,10 @@ start_server {tags {"modules"}} { ...@@ -25,6 +25,10 @@ start_server {tags {"modules"}} {
# Subcommands can be called # Subcommands can be called
assert_equal [r subcommands.bitarray get k1] {OK} assert_equal [r subcommands.bitarray get k1] {OK}
# Subcommand arity error
catch {r subcommands.bitarray get k1 8 90} e
assert_match {*wrong number of arguments for 'subcommands.bitarray|get' command} $e
} }
test "Module get current command fullname" { test "Module get current command fullname" {
......
...@@ -311,7 +311,7 @@ start_server {tags {"other"}} { ...@@ -311,7 +311,7 @@ start_server {tags {"other"}} {
assert_error {*unknown command*} {r GET|SET} assert_error {*unknown command*} {r GET|SET}
assert_error {*unknown command*} {r GET|SET|OTHER} assert_error {*unknown command*} {r GET|SET|OTHER}
assert_error {*unknown command*} {r CONFIG|GET GET_XX} assert_error {*unknown command*} {r CONFIG|GET GET_XX}
assert_error {*Unknown subcommand*} {r CONFIG GET_XX} assert_error {*unknown subcommand*} {r CONFIG GET_XX}
} }
} }
......
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