From 66be30f7fc2f7f6378eedc8d7b219db18addbc06 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Tue, 8 Feb 2022 10:01:35 +0200 Subject: [PATCH] Handle key-spec flags with modules (#10237) - add COMMAND GETKEYSANDFLAGS sub-command - add RM_KeyAtPosWithFlags and GetCommandKeysWithFlags - RM_KeyAtPos and RM_CreateCommand set flags requiring full access for keys - RM_CreateCommand set VARIABLE_FLAGS - expose `variable_flags` flag in COMMAND INFO key-specs - getKeysFromCommandWithSpecs prefers key-specs over getkeys-api - add tests for all of these --- src/commands.c | 11 +- src/commands/command-getkeysandflags.json | 18 ++++ src/db.c | 50 ++++++--- src/module.c | 120 +++++++++++++++------- src/redismodule.h | 4 + src/server.c | 31 ++++-- src/server.h | 4 + tests/modules/getkeys.c | 71 +++++++++++-- tests/modules/keyspecs.c | 4 +- tests/unit/introspection-2.tcl | 7 ++ tests/unit/moduleapi/getkeys.tcl | 44 ++++++-- tests/unit/moduleapi/keyspecs.tcl | 35 ++++++- 12 files changed, 319 insertions(+), 80 deletions(-) create mode 100644 src/commands/command-getkeysandflags.json diff --git a/src/commands.c b/src/commands.c index 3a9df2f84..700c810eb 100644 --- a/src/commands.c +++ b/src/commands.c @@ -3315,7 +3315,7 @@ NULL /* FUNCTION DELETE argument table */ struct redisCommandArg FUNCTION_DELETE_Args[] = { -{"function-name",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE}, +{"library-name",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE}, {0} }; @@ -4033,6 +4033,14 @@ struct redisCommandArg COMMAND_DOCS_Args[] = { /* COMMAND GETKEYS tips */ #define COMMAND_GETKEYS_tips NULL +/********** COMMAND GETKEYSANDFLAGS ********************/ + +/* COMMAND GETKEYSANDFLAGS history */ +#define COMMAND_GETKEYSANDFLAGS_History NULL + +/* COMMAND GETKEYSANDFLAGS tips */ +#define COMMAND_GETKEYSANDFLAGS_tips NULL + /********** COMMAND HELP ********************/ /* COMMAND HELP history */ @@ -4085,6 +4093,7 @@ struct redisCommand COMMAND_Subcommands[] = { {"count","Get total number of Redis commands","O(1)","2.8.13",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_COUNT_History,COMMAND_COUNT_tips,commandCountCommand,2,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION}, {"docs","Get array of specific Redis command documentation","O(N) where N is the number of commands to look up","7.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_DOCS_History,COMMAND_DOCS_tips,commandDocsCommand,-2,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION,.args=COMMAND_DOCS_Args}, {"getkeys","Extract keys given a full Redis command","O(N) where N is the number of arguments to the command","2.8.13",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_GETKEYS_History,COMMAND_GETKEYS_tips,commandGetKeysCommand,-4,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION}, +{"getkeysandflags","Extract keys given a full Redis command","O(N) where N is the number of arguments to the command","7.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_GETKEYSANDFLAGS_History,COMMAND_GETKEYSANDFLAGS_tips,commandGetKeysAndFlagsCommand,-4,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION}, {"help","Show helpful text about the different subcommands","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_HELP_History,COMMAND_HELP_tips,commandHelpCommand,2,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION}, {"info","Get array of specific Redis command details, or all when no argument is given.","O(N) where N is the number of commands to look up","2.8.13",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_INFO_History,COMMAND_INFO_tips,commandInfoCommand,-2,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION,.args=COMMAND_INFO_Args}, {"list","Get an array of Redis command names","O(N) where N is the total number of Redis commands","7.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,COMMAND_LIST_History,COMMAND_LIST_tips,commandListCommand,-2,CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION,.args=COMMAND_LIST_Args}, diff --git a/src/commands/command-getkeysandflags.json b/src/commands/command-getkeysandflags.json new file mode 100644 index 000000000..1ac8e990d --- /dev/null +++ b/src/commands/command-getkeysandflags.json @@ -0,0 +1,18 @@ +{ + "GETKEYSANDFLAGS": { + "summary": "Extract keys given a full Redis command", + "complexity": "O(N) where N is the number of arguments to the command", + "group": "server", + "since": "7.0.0", + "arity": -4, + "container": "COMMAND", + "function": "commandGetKeysAndFlagsCommand", + "command_flags": [ + "LOADING", + "STALE" + ], + "acl_categories": [ + "CONNECTION" + ] + } +} diff --git a/src/db.c b/src/db.c index 218eaf567..11665fa1d 100644 --- a/src/db.c +++ b/src/db.c @@ -1821,31 +1821,47 @@ invalid_spec: * associated with how Redis will access the key. * * 'cmd' must be point to the corresponding entry into the redisCommand - * table, according to the command name in argv[0]. - * - * This function uses the command's key specs, which contain the key-spec flags, - * (e.g. RO / RW) and only resorts to the command-specific helper function if - * any of the keys-specs are marked as INCOMPLETE. */ + * table, according to the command name in argv[0]. */ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc, int search_flags, getKeysResult *result) { - if (cmd->flags & CMD_MODULE_GETKEYS) { - return moduleGetCommandKeysViaAPI(cmd,argv,argc,result); - } else { - if (!(getAllKeySpecsFlags(cmd, 0) & CMD_KEY_VARIABLE_FLAGS)) { - int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result); - if (ret >= 0) - return ret; - } - if (!(cmd->flags & CMD_MODULE) && cmd->getkeys_proc) - return cmd->getkeys_proc(cmd,argv,argc,result); - return 0; + /* The command has at least one key-spec not marked as CHANNEL */ + int has_keyspec = (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_CHANNEL); + /* The command has at least one key-spec marked as 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; + int has_native_getkeys = !(cmd->flags & CMD_MODULE) && cmd->getkeys_proc; + + /* 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. */ + if (has_keyspec && !has_varflags) { + int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result); + if (ret >= 0) + return ret; + /* If the specs returned with an error (probably an INVALID or INCOMPLETE spec), + * fallback to the callback method. */ } + + /* Resort to getkeys callback methods. */ + if (has_module_getkeys) + return moduleGetCommandKeysViaAPI(cmd,argv,argc,result); + + /* We use native getkeys as a last resort, since not all these native getkeys provide + * flags properly (only the ones that correspond to INVALID, INCOMPLETE or VARIABLE_FLAGS do.*/ + if (has_native_getkeys) + return cmd->getkeys_proc(cmd,argv,argc,result); + return 0; } /* This function returns a sanity check if the command may have keys. */ int doesCommandHaveKeys(struct redisCommand *cmd) { return (!(cmd->flags & CMD_MODULE) && cmd->getkeys_proc) || /* has getkeys_proc (non modules) */ (cmd->flags & CMD_MODULE_GETKEYS) || /* module with GETKEYS */ - (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_CHANNEL); /* has at least one key-spec not marked as CHANNEL */ + (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_CHANNEL); /* has at least one key-spec not marked as CHANNEL */ } /* The base case is to use the keys position as given in the command table diff --git a/src/module.c b/src/module.c index dc93e0c9b..62bbd35e7 100644 --- a/src/module.c +++ b/src/module.c @@ -390,6 +390,7 @@ typedef struct RedisModuleKeyOptCtx { In most cases, only 'from_dbid' is valid, but in callbacks such as `copy2`, 'from_dbid' and 'to_dbid' are both valid. */ } RedisModuleKeyOptCtx; + /* -------------------------------------------------------------------------- * Prototypes * -------------------------------------------------------------------------- */ @@ -404,6 +405,16 @@ static void moduleInitKeyTypeSpecific(RedisModuleKey *key); void RM_FreeDict(RedisModuleCtx *ctx, RedisModuleDict *d); void RM_FreeServerInfo(RedisModuleCtx *ctx, RedisModuleServerInfoData *data); +/* Helpers for RM_SetCommandInfo. */ +static int moduleValidateCommandInfo(const RedisModuleCommandInfo *info); +static int64_t moduleConvertKeySpecsFlags(int64_t flags, int from_api); +static int moduleValidateCommandArgs(RedisModuleCommandArg *args, + const RedisModuleCommandInfoVersion *version); +static struct redisCommandArg *moduleCopyCommandArgs(RedisModuleCommandArg *args, + const RedisModuleCommandInfoVersion *version); +static redisCommandArgType moduleConvertArgType(RedisModuleCommandArgType type, int *error); +static int moduleConvertArgFlags(int flags); + /* -------------------------------------------------------------------------- * ## Heap allocation raw functions * @@ -789,17 +800,23 @@ int RM_IsKeysPositionRequest(RedisModuleCtx *ctx) { * keys, since it was flagged as "getkeys-api" during the registration, * the command implementation checks for this special call using the * RedisModule_IsKeysPositionRequest() API and uses this function in - * order to report keys, like in the following example: + * order to report keys. + * + * The supported flags are the ones used by RM_SetCommandInfo, see REDISMODULE_CMD_KEY_*. + * + * + * The following is an example of how it could be used: * * if (RedisModule_IsKeysPositionRequest(ctx)) { - * RedisModule_KeyAtPos(ctx,1); - * RedisModule_KeyAtPos(ctx,2); + * RedisModule_KeyAtPosWithFlags(ctx, 2, REDISMODULE_CMD_KEY_RO | REDISMODULE_CMD_KEY_ACCESS); + * RedisModule_KeyAtPosWithFlags(ctx, 1, REDISMODULE_CMD_KEY_RW | REDISMODULE_CMD_KEY_UPDATE | REDISMODULE_CMD_KEY_ACCESS); * } * - * Note: in the example below the get keys API would not be needed since - * keys are at fixed positions. This interface is only used for commands - * with a more complex structure. */ -void RM_KeyAtPos(RedisModuleCtx *ctx, int pos) { + * Note: in the example above the get keys API could have been handled by key-specs (preferred). + * Implementing the getkeys-api is required only when is it not possible to declare key-specs that cover all keys. + * + */ +void RM_KeyAtPosWithFlags(RedisModuleCtx *ctx, int pos, int flags) { if (!(ctx->flags & REDISMODULE_CTX_KEYS_POS_REQUEST) || !ctx->keys_result) return; if (pos <= 0) return; @@ -811,7 +828,18 @@ void RM_KeyAtPos(RedisModuleCtx *ctx, int pos) { getKeysPrepareResult(res, newsize); } - res->keys[res->numkeys++].pos = pos; + res->keys[res->numkeys].pos = pos; + res->keys[res->numkeys].flags = moduleConvertKeySpecsFlags(flags, 1); + res->numkeys++; +} + +/* This API existed before RM_KeyAtPosWithFlags was added, now deprecated and + * can be used for compatibility with older versions, before key-specs and flags + * were introduced. */ +void RM_KeyAtPos(RedisModuleCtx *ctx, int pos) { + /* Default flags require full access */ + int flags = moduleConvertKeySpecsFlags(CMD_KEY_FULL_ACCESS, 0); + RM_KeyAtPosWithFlags(ctx, pos, flags); } /* Helper for RM_CreateCommand(). Turns a string representing command @@ -991,7 +1019,7 @@ RedisModuleCommand *moduleCreateCommandProxy(struct RedisModule *module, sds dec cp->rediscmd->key_specs = cp->rediscmd->key_specs_static; if (firstkey != 0) { cp->rediscmd->key_specs_num = 1; - cp->rediscmd->key_specs[0].flags = 0; + cp->rediscmd->key_specs[0].flags = CMD_KEY_FULL_ACCESS | CMD_KEY_VARIABLE_FLAGS; 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].find_keys_type = KSPEC_FK_RANGE; @@ -1092,16 +1120,6 @@ int RM_CreateSubcommand(RedisModuleCommand *parent, const char *name, RedisModul return REDISMODULE_OK; } -/* Helpers for RM_SetCommandInfo. */ -static int moduleValidateCommandInfo(const RedisModuleCommandInfo *info); -static int64_t moduleConvertKeySpecsFlags(int64_t flags); -static int moduleValidateCommandArgs(RedisModuleCommandArg *args, - const RedisModuleCommandInfoVersion *version); -static struct redisCommandArg *moduleCopyCommandArgs(RedisModuleCommandArg *args, - const RedisModuleCommandInfoVersion *version); -static redisCommandArgType moduleConvertArgType(RedisModuleCommandArgType type, int *error); -static int moduleConvertArgFlags(int flags); - /* Accessors of array elements of structs where the element size is stored * separately in the version struct. */ static RedisModuleCommandHistoryEntry * @@ -1193,8 +1211,9 @@ moduleCmdArgAt(const RedisModuleCommandInfoVersion *version, * versions where RM_SetCommandInfo is not available. * * Note that key-specs don't fully replace the "getkeys-api" (see - * RM_CreateCommand, RM_IsKeysPositionRequest and RM_KeyAtPos) so it may be - * a good idea to supply both key-specs and a implement the getkeys-api. + * RM_CreateCommand, RM_IsKeysPositionRequest and RM_KeyAtPosWithFlags) so + * it may be a good idea to supply both key-specs and implement the + * getkeys-api. * * A key-spec has the following structure: * @@ -1503,7 +1522,7 @@ int RM_SetCommandInfo(RedisModuleCommand *command, const RedisModuleCommandInfo RedisModuleCommandKeySpec *spec = moduleCmdKeySpecAt(version, info->key_specs, j); cmd->key_specs[j].notes = spec->notes ? zstrdup(spec->notes) : NULL; - cmd->key_specs[j].flags = moduleConvertKeySpecsFlags(spec->flags); + cmd->key_specs[j].flags = moduleConvertKeySpecsFlags(spec->flags, 1); switch (spec->begin_search_type) { case REDISMODULE_KSPEC_BS_UNKNOWN: cmd->key_specs[j].begin_search_type = KSPEC_BS_UNKNOWN; @@ -1671,21 +1690,28 @@ static int moduleValidateCommandInfo(const RedisModuleCommandInfo *info) { return moduleValidateCommandArgs(info->args, version); } -/* Converts from REDISMODULE_CMD_KEY_* flags to CMD_KEY_* flags. */ -static int64_t moduleConvertKeySpecsFlags(int64_t flags) { - int64_t realflags = 0; - if (flags & REDISMODULE_CMD_KEY_RO) realflags |= CMD_KEY_RO; - if (flags & REDISMODULE_CMD_KEY_RW) realflags |= CMD_KEY_RW; - if (flags & REDISMODULE_CMD_KEY_OW) realflags |= CMD_KEY_OW; - if (flags & REDISMODULE_CMD_KEY_RM) realflags |= CMD_KEY_RM; - if (flags & REDISMODULE_CMD_KEY_ACCESS) realflags |= CMD_KEY_ACCESS; - if (flags & REDISMODULE_CMD_KEY_INSERT) realflags |= CMD_KEY_INSERT; - if (flags & REDISMODULE_CMD_KEY_UPDATE) realflags |= CMD_KEY_UPDATE; - if (flags & REDISMODULE_CMD_KEY_DELETE) realflags |= CMD_KEY_DELETE; - if (flags & REDISMODULE_CMD_KEY_CHANNEL) realflags |= CMD_KEY_CHANNEL; - if (flags & REDISMODULE_CMD_KEY_INCOMPLETE) realflags |= CMD_KEY_INCOMPLETE; - if (flags & REDISMODULE_CMD_KEY_VARIABLE_FLAGS) realflags |= CMD_KEY_VARIABLE_FLAGS; - return realflags; +/* When from_api is true, converts from REDISMODULE_CMD_KEY_* flags to CMD_KEY_* flags. + * When from_api is false, converts from CMD_KEY_* flags to REDISMODULE_CMD_KEY_* flags. */ +static int64_t moduleConvertKeySpecsFlags(int64_t flags, int from_api) { + int64_t out = 0; + int64_t map[][2] = { + {REDISMODULE_CMD_KEY_RO, CMD_KEY_RO}, + {REDISMODULE_CMD_KEY_RW, CMD_KEY_RW}, + {REDISMODULE_CMD_KEY_OW, CMD_KEY_OW}, + {REDISMODULE_CMD_KEY_RM, CMD_KEY_RM}, + {REDISMODULE_CMD_KEY_ACCESS, CMD_KEY_ACCESS}, + {REDISMODULE_CMD_KEY_INSERT, CMD_KEY_INSERT}, + {REDISMODULE_CMD_KEY_UPDATE, CMD_KEY_UPDATE}, + {REDISMODULE_CMD_KEY_DELETE, CMD_KEY_DELETE}, + {REDISMODULE_CMD_KEY_CHANNEL, CMD_KEY_CHANNEL}, + {REDISMODULE_CMD_KEY_INCOMPLETE, CMD_KEY_INCOMPLETE}, + {REDISMODULE_CMD_KEY_VARIABLE_FLAGS, CMD_KEY_VARIABLE_FLAGS}, + {0,0}}; + + int from_idx = from_api ? 0 : 1, to_idx = !from_idx; + for (int i=0; map[i][0]; i++) + if (flags & map[i][from_idx]) out |= map[i][to_idx]; + return out; } /* Validates an array of RedisModuleCommandArg. Returns 1 if it's valid and 0 if @@ -11036,6 +11062,10 @@ int RM_ModuleTypeReplaceValue(RedisModuleKey *key, moduleType *mt, void *new_val * contains the indexes of all key name arguments. This function is * essentially a more efficient way to do `COMMAND GETKEYS`. * + * The out_flags argument is optional, and can be set to NULL. + * When provided it is filled with REDISMODULE_CMD_KEY_ flags in matching + * indexes with the key indexes of the returned array. + * * A NULL return value indicates the specified command has no keys, or * an error condition. Error conditions are indicated by setting errno * as follows: @@ -11045,9 +11075,10 @@ int RM_ModuleTypeReplaceValue(RedisModuleKey *key, moduleType *mt, void *new_val * * NOTE: The returned array is not a Redis Module object so it does not * get automatically freed even when auto-memory is used. The caller - * must explicitly call RM_Free() to free it. + * must explicitly call RM_Free() to free it, same as the out_flags pointer if + * used. */ -int *RM_GetCommandKeys(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, int *num_keys) { +int *RM_GetCommandKeysWithFlags(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, int *num_keys, int **out_flags) { UNUSED(ctx); struct redisCommand *cmd; int *res = NULL; @@ -11082,13 +11113,22 @@ int *RM_GetCommandKeys(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, /* The return value here expects an array of key positions */ unsigned long int size = sizeof(int) * result.numkeys; res = zmalloc(size); + if (out_flags) + *out_flags = zmalloc(size); for (int i = 0; i < result.numkeys; i++) { res[i] = result.keys[i].pos; + if (out_flags) + (*out_flags)[i] = moduleConvertKeySpecsFlags(result.keys[i].flags, 0); } return res; } +/* Identinal to RM_GetCommandKeysWithFlags when flags are not needed. */ +int *RM_GetCommandKeys(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, int *num_keys) { + return RM_GetCommandKeysWithFlags(ctx, argv, argc, num_keys, NULL); +} + /* Return the name of the command currently running */ const char *RM_GetCurrentCommandName(RedisModuleCtx *ctx) { if (!ctx || !ctx->client || !ctx->client->cmd) @@ -11439,6 +11479,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(StreamTrimByID); REGISTER_API(IsKeysPositionRequest); REGISTER_API(KeyAtPos); + REGISTER_API(KeyAtPosWithFlags); REGISTER_API(GetClientId); REGISTER_API(GetClientUserNameById); REGISTER_API(GetContextFlags); @@ -11616,6 +11657,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(GetServerVersion); REGISTER_API(GetClientCertificate); REGISTER_API(GetCommandKeys); + REGISTER_API(GetCommandKeysWithFlags); REGISTER_API(GetCurrentCommandName); REGISTER_API(GetTypeMethodVersion); REGISTER_API(RegisterDefragFunc); diff --git a/src/redismodule.h b/src/redismodule.h index 6438abdf0..3b125a4ec 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -945,6 +945,7 @@ REDISMODULE_API long long (*RedisModule_StreamTrimByLength)(RedisModuleKey *key, REDISMODULE_API long long (*RedisModule_StreamTrimByID)(RedisModuleKey *key, int flags, RedisModuleStreamID *id) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_IsKeysPositionRequest)(RedisModuleCtx *ctx) REDISMODULE_ATTR; REDISMODULE_API void (*RedisModule_KeyAtPos)(RedisModuleCtx *ctx, int pos) REDISMODULE_ATTR; +REDISMODULE_API void (*RedisModule_KeyAtPosWithFlags)(RedisModuleCtx *ctx, int pos, int flags) REDISMODULE_ATTR; REDISMODULE_API unsigned long long (*RedisModule_GetClientId)(RedisModuleCtx *ctx) REDISMODULE_ATTR; REDISMODULE_API RedisModuleString * (*RedisModule_GetClientUserNameById)(RedisModuleCtx *ctx, uint64_t id) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_GetClientInfoById)(void *ci, uint64_t id) REDISMODULE_ATTR; @@ -1122,6 +1123,7 @@ REDISMODULE_API int (*RedisModule_AuthenticateClientWithUser)(RedisModuleCtx *ct REDISMODULE_API int (*RedisModule_DeauthenticateAndCloseClient)(RedisModuleCtx *ctx, uint64_t client_id) REDISMODULE_ATTR; REDISMODULE_API RedisModuleString * (*RedisModule_GetClientCertificate)(RedisModuleCtx *ctx, uint64_t id) REDISMODULE_ATTR; REDISMODULE_API int *(*RedisModule_GetCommandKeys)(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, int *num_keys) REDISMODULE_ATTR; +REDISMODULE_API int *(*RedisModule_GetCommandKeysWithFlags)(RedisModuleCtx *ctx, RedisModuleString **argv, int argc, int *num_keys, int **out_flags) REDISMODULE_ATTR; REDISMODULE_API const char *(*RedisModule_GetCurrentCommandName)(RedisModuleCtx *ctx) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_RegisterDefragFunc)(RedisModuleCtx *ctx, RedisModuleDefragFunc func) REDISMODULE_ATTR; REDISMODULE_API void *(*RedisModule_DefragAlloc)(RedisModuleDefragCtx *ctx, void *ptr) REDISMODULE_ATTR; @@ -1264,6 +1266,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(StreamTrimByID); REDISMODULE_GET_API(IsKeysPositionRequest); REDISMODULE_GET_API(KeyAtPos); + REDISMODULE_GET_API(KeyAtPosWithFlags); REDISMODULE_GET_API(GetClientId); REDISMODULE_GET_API(GetClientUserNameById); REDISMODULE_GET_API(GetContextFlags); @@ -1441,6 +1444,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(AuthenticateClientWithUser); REDISMODULE_GET_API(GetClientCertificate); REDISMODULE_GET_API(GetCommandKeys); + REDISMODULE_GET_API(GetCommandKeysWithFlags); REDISMODULE_GET_API(GetCurrentCommandName); REDISMODULE_GET_API(RegisterDefragFunc); REDISMODULE_GET_API(DefragAlloc); diff --git a/src/server.c b/src/server.c index d709d1d18..314e0b301 100644 --- a/src/server.c +++ b/src/server.c @@ -4150,6 +4150,7 @@ void addReplyFlagsForKeyArgs(client *c, uint64_t flags) { {CMD_KEY_DELETE, "delete"}, {CMD_KEY_CHANNEL, "channel"}, {CMD_KEY_INCOMPLETE, "incomplete"}, + {CMD_KEY_VARIABLE_FLAGS, "variable_flags"}, {0,NULL} }; addReplyCommandFlags(c, flags, docFlagNames); @@ -4492,10 +4493,8 @@ void addReplyCommandDocs(client *c, struct redisCommand *cmd) { } } -/* Helper for COMMAND(S) command - * - * COMMAND(S) GETKEYS cmd arg1 arg2 ... */ -void getKeysSubcommand(client *c) { +/* Helper for COMMAND GETKEYS and GETKEYSANDFLAGS */ +void getKeysSubcommandImpl(client *c, int with_flags) { struct redisCommand *cmd = lookupCommand(c->argv+2,c->argc-2); getKeysResult result = GETKEYS_RESULT_INIT; int j; @@ -4513,7 +4512,7 @@ void getKeysSubcommand(client *c) { return; } - if (!getKeysFromCommand(cmd,c->argv+2,c->argc-2,&result)) { + if (!getKeysFromCommandWithSpecs(cmd,c->argv+2,c->argc-2,GET_KEYSPEC_DEFAULT,&result)) { if (cmd->flags & CMD_NO_MANDATORY_KEYS) { addReplyArrayLen(c,0); } else { @@ -4521,11 +4520,29 @@ void getKeysSubcommand(client *c) { } } else { addReplyArrayLen(c,result.numkeys); - for (j = 0; j < result.numkeys; j++) addReplyBulk(c,c->argv[result.keys[j].pos+2]); + for (j = 0; j < result.numkeys; j++) { + if (!with_flags) { + addReplyBulk(c,c->argv[result.keys[j].pos+2]); + } else { + addReplyArrayLen(c,2); + addReplyBulk(c,c->argv[result.keys[j].pos+2]); + addReplyFlagsForKeyArgs(c,result.keys[j].flags); + } + } } getKeysFreeResult(&result); } +/* COMMAND GETKEYSANDFLAGS cmd arg1 arg2 ... */ +void commandGetKeysAndFlagsCommand(client *c) { + getKeysSubcommandImpl(c, 1); +} + +/* COMMAND GETKEYS cmd arg1 arg2 ... */ +void getKeysSubcommand(client *c) { + getKeysSubcommandImpl(c, 0); +} + /* COMMAND (no args) */ void commandCommand(client *c) { dictIterator *di; @@ -4742,6 +4759,8 @@ void commandHelpCommand(client *c) { " commands are returned.", "GETKEYS ", " Return the keys from a full Redis command.", +"GETKEYSANDFLAGS ", +" Return the keys and the access flags from a full Redis command.", NULL }; diff --git a/src/server.h b/src/server.h index 0b1904d4e..e4f7246c8 100644 --- a/src/server.h +++ b/src/server.h @@ -268,6 +268,9 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT]; #define CMD_KEY_VARIABLE_FLAGS (1ULL<<10) /* Means that some keys might have * different flags depending on arguments */ +/* Key flags for when access type is unknown */ +#define CMD_KEY_FULL_ACCESS (CMD_KEY_RW | CMD_KEY_ACCESS | CMD_KEY_UPDATE) + /* AOF states */ #define AOF_OFF 0 /* AOF is off */ #define AOF_ON 1 /* AOF is on */ @@ -3127,6 +3130,7 @@ void commandCountCommand(client *c); void commandListCommand(client *c); void commandInfoCommand(client *c); void commandGetKeysCommand(client *c); +void commandGetKeysAndFlagsCommand(client *c); void commandHelpCommand(client *c); void commandDocsCommand(client *c); void setCommand(client *c); diff --git a/tests/modules/getkeys.c b/tests/modules/getkeys.c index acb8a1295..cee3b3e13 100644 --- a/tests/modules/getkeys.c +++ b/tests/modules/getkeys.c @@ -44,6 +44,40 @@ int getkeys_command(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) return REDISMODULE_OK; } +int getkeys_command_with_flags(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) +{ + int i; + int count = 0; + + /* Handle getkeys-api introspection */ + if (RedisModule_IsKeysPositionRequest(ctx)) { + for (i = 0; i < argc; i++) { + size_t len; + const char *str = RedisModule_StringPtrLen(argv[i], &len); + + if (len == 3 && !strncasecmp(str, "key", 3) && i + 1 < argc) + RedisModule_KeyAtPosWithFlags(ctx, i + 1, REDISMODULE_CMD_KEY_RO | REDISMODULE_CMD_KEY_ACCESS); + } + + return REDISMODULE_OK; + } + + /* Handle real command invocation */ + RedisModule_ReplyWithArray(ctx, REDISMODULE_POSTPONED_LEN); + for (i = 0; i < argc; i++) { + size_t len; + const char *str = RedisModule_StringPtrLen(argv[i], &len); + + if (len == 3 && !strncasecmp(str, "key", 3) && i + 1 < argc) { + RedisModule_ReplyWithString(ctx, argv[i+1]); + count++; + } + } + RedisModule_ReplySetArrayLength(ctx, count); + + return REDISMODULE_OK; +} + int getkeys_fixed(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { int i; @@ -57,19 +91,22 @@ int getkeys_fixed(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) /* Introspect a command using RM_GetCommandKeys() and returns the list * of keys. Essentially this is COMMAND GETKEYS implemented in a module. + * INTROSPECT */ int getkeys_introspect(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { - UNUSED(argv); - UNUSED(argc); + long long with_flags = 0; - if (argc < 3) { + if (argc < 4) { RedisModule_WrongArity(ctx); return REDISMODULE_OK; } - int num_keys; - int *keyidx = RedisModule_GetCommandKeys(ctx, &argv[1], argc - 1, &num_keys); + if (RedisModule_StringToLongLong(argv[1],&with_flags) != REDISMODULE_OK) + return RedisModule_ReplyWithError(ctx,"ERR invalid integer"); + + int num_keys, *keyflags = NULL; + int *keyidx = RedisModule_GetCommandKeysWithFlags(ctx, &argv[2], argc - 2, &num_keys, with_flags ? &keyflags : NULL); if (!keyidx) { if (!errno) @@ -93,10 +130,27 @@ int getkeys_introspect(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) int i; RedisModule_ReplyWithArray(ctx, num_keys); - for (i = 0; i < num_keys; i++) - RedisModule_ReplyWithString(ctx, argv[1 + keyidx[i]]); + for (i = 0; i < num_keys; i++) { + if (!with_flags) { + RedisModule_ReplyWithString(ctx, argv[2 + keyidx[i]]); + continue; + } + RedisModule_ReplyWithArray(ctx, 2); + RedisModule_ReplyWithString(ctx, argv[2 + keyidx[i]]); + char* sflags = ""; + if (keyflags[i] & REDISMODULE_CMD_KEY_RO) + sflags = "RO"; + else if (keyflags[i] & REDISMODULE_CMD_KEY_RW) + sflags = "RW"; + else if (keyflags[i] & REDISMODULE_CMD_KEY_OW) + sflags = "OW"; + else if (keyflags[i] & REDISMODULE_CMD_KEY_RM) + sflags = "RM"; + RedisModule_ReplyWithCString(ctx, sflags); + } RedisModule_Free(keyidx); + RedisModule_Free(keyflags); } return REDISMODULE_OK; @@ -111,6 +165,9 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) if (RedisModule_CreateCommand(ctx,"getkeys.command", getkeys_command,"getkeys-api",0,0,0) == REDISMODULE_ERR) return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"getkeys.command_with_flags", getkeys_command_with_flags,"getkeys-api",0,0,0) == REDISMODULE_ERR) + return REDISMODULE_ERR; + if (RedisModule_CreateCommand(ctx,"getkeys.fixed", getkeys_fixed,"",2,4,1) == REDISMODULE_ERR) return REDISMODULE_ERR; diff --git a/tests/modules/keyspecs.c b/tests/modules/keyspecs.c index 5789b8cff..32a6bebaa 100644 --- a/tests/modules/keyspecs.c +++ b/tests/modules/keyspecs.c @@ -12,8 +12,8 @@ int kspec_impl(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { } int createKspecNone(RedisModuleCtx *ctx) { - /* A command without keyspecs; only the legacy (first,last,step) triple. */ - if (RedisModule_CreateCommand(ctx,"kspec.none",kspec_impl,"",1,3,2) == REDISMODULE_ERR) + /* A command without keyspecs; only the legacy (first,last,step) triple (MSET like spec). */ + if (RedisModule_CreateCommand(ctx,"kspec.none",kspec_impl,"",1,-1,2) == REDISMODULE_ERR) return REDISMODULE_ERR; return REDISMODULE_OK; } diff --git a/tests/unit/introspection-2.tcl b/tests/unit/introspection-2.tcl index 40124e035..52ed54a29 100644 --- a/tests/unit/introspection-2.tcl +++ b/tests/unit/introspection-2.tcl @@ -81,6 +81,13 @@ start_server {tags {"introspection"}} { assert_equal {key} [r command getkeys get key] } + test {COMMAND GETKEYSANDFLAGS} { + assert_equal {{k1 {OW update}}} [r command getkeysandflags set k1 v1] + assert_equal {{k1 {OW update}} {k2 {OW update}}} [r command getkeysandflags mset k1 v1 k2 v2] + assert_equal {{k1 {RW access delete}} {k2 {RW insert}}} [r command getkeysandflags LMOVE k1 k2 left right] + assert_equal {{k1 {RO access}} {k2 {OW update}}} [r command getkeysandflags sort k1 store k2] + } + test {COMMAND GETKEYS MEMORY USAGE} { assert_equal {key} [r command getkeys memory usage key] } diff --git a/tests/unit/moduleapi/getkeys.tcl b/tests/unit/moduleapi/getkeys.tcl index 6061fe8cf..734c55fa2 100644 --- a/tests/unit/moduleapi/getkeys.tcl +++ b/tests/unit/moduleapi/getkeys.tcl @@ -16,32 +16,64 @@ start_server {tags {"modules"}} { r command getkeys getkeys.command arg1 arg2 key key1 arg3 key key2 key key3 } {key1 key2 key3} + test {COMMAND GETKEYS correctly reports a movable keys module command using flags} { + r command getkeys getkeys.command_with_flags arg1 arg2 key key1 arg3 key key2 key key3 + } {key1 key2 key3} + + test {COMMAND GETKEYSANDFLAGS correctly reports a movable keys module command not using flags} { + r command getkeysandflags getkeys.command arg1 arg2 key key1 arg3 key key2 + } {{key1 {RW access update}} {key2 {RW access update}}} + + test {COMMAND GETKEYSANDFLAGS correctly reports a movable keys module command using flags} { + r command getkeysandflags getkeys.command_with_flags arg1 arg2 key key1 arg3 key key2 key key3 + } {{key1 {RO access}} {key2 {RO access}} {key3 {RO access}}} + test {RM_GetCommandKeys on non-existing command} { - catch {r getkeys.introspect non-command key1 key2} e + catch {r getkeys.introspect 0 non-command key1 key2} e set _ $e } {*ENOENT*} test {RM_GetCommandKeys on built-in fixed keys command} { - r getkeys.introspect set key1 value1 + r getkeys.introspect 0 set key1 value1 } {key1} + test {RM_GetCommandKeys on built-in fixed keys command with flags} { + r getkeys.introspect 1 set key1 value1 + } {{key1 OW}} + test {RM_GetCommandKeys on EVAL} { - r getkeys.introspect eval "" 4 key1 key2 key3 key4 arg1 arg2 + r getkeys.introspect 0 eval "" 4 key1 key2 key3 key4 arg1 arg2 } {key1 key2 key3 key4} test {RM_GetCommandKeys on a movable keys module command} { - r getkeys.introspect getkeys.command arg1 arg2 key key1 arg3 key key2 key key3 + r getkeys.introspect 0 getkeys.command arg1 arg2 key key1 arg3 key key2 key key3 } {key1 key2 key3} test {RM_GetCommandKeys on a non-movable module command} { - r getkeys.introspect getkeys.fixed arg1 key1 key2 key3 arg2 + r getkeys.introspect 0 getkeys.fixed arg1 key1 key2 key3 arg2 } {key1 key2 key3} test {RM_GetCommandKeys with bad arity} { - catch {r getkeys.introspect set key} e + catch {r getkeys.introspect 0 set key} e set _ $e } {*EINVAL*} + # user that can only read from "read" keys, write to "write" keys, and read+write to "RW" keys + r ACL setuser testuser +@all %R~read* %W~write* %RW~rw* + + test "module getkeys-api - ACL" { + # legacy triple didn't provide flags, so they require both read and write + assert_equal "OK" [r ACL DRYRUN testuser getkeys.command key rw] + assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN testuser getkeys.command key read] + assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser getkeys.command key write] + } + + test "module getkeys-api with flags - ACL" { + assert_equal "OK" [r ACL DRYRUN testuser getkeys.command_with_flags key rw] + assert_equal "OK" [r ACL DRYRUN testuser getkeys.command_with_flags key read] + assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser getkeys.command_with_flags key write] + } + test "Unload the module - getkeys" { assert_equal {OK} [r module unload getkeys] } diff --git a/tests/unit/moduleapi/keyspecs.tcl b/tests/unit/moduleapi/keyspecs.tcl index e1d42572d..60d3fe5d3 100644 --- a/tests/unit/moduleapi/keyspecs.tcl +++ b/tests/unit/moduleapi/keyspecs.tcl @@ -8,12 +8,13 @@ start_server {tags {"modules"}} { # Verify (first, last, step) and not movablekeys assert_equal [lindex $reply 2] {module} assert_equal [lindex $reply 3] 1 - assert_equal [lindex $reply 4] 3 + 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 {} begin_search {type index spec {index 1}} find_keys {type range spec {lastkey 2 keystep 2 limit 0}}} + 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.none key1 val1 key2 val2] {key1 key2} } test "Module key specs: Two ranges" { @@ -27,6 +28,7 @@ start_server {tags {"modules"}} { 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 2}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}} + assert_equal [r command getkeys kspec.tworanges foo bar baz quux] {foo bar} } test "Module key specs: Keyword-only spec clears the legacy triple" { @@ -39,6 +41,7 @@ start_server {tags {"modules"}} { # Verify key-specs set keyspecs [lindex $reply 8] assert_equal [lindex $keyspecs 0] {flags {RO access} begin_search {type keyword spec {keyword KEYS startfrom 1}} find_keys {type range spec {lastkey -1 keystep 1 limit 0}}} + assert_equal [r command getkeys kspec.keyword foo KEYS bar baz] {bar baz} } test "Module key specs: Complex specs, case 1" { @@ -53,6 +56,7 @@ start_server {tags {"modules"}} { assert_equal [lindex $keyspecs 0] {flags RO 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 keyword spec {keyword STORE startfrom 2}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}} assert_equal [lindex $keyspecs 2] {flags {RO access} begin_search {type keyword spec {keyword KEYS startfrom 2}} find_keys {type keynum spec {keynumidx 0 firstkey 1 keystep 1}}} + assert_equal [r command getkeys kspec.complex1 foo dummy KEYS 1 bar baz STORE quux] {foo quux bar} } test "Module key specs: Complex specs, case 2" { @@ -69,12 +73,39 @@ start_server {tags {"modules"}} { assert_equal [lindex $keyspecs 2] {flags {RO access} begin_search {type index spec {index 2}} find_keys {type range spec {lastkey 0 keystep 1 limit 0}}} assert_equal [lindex $keyspecs 3] {flags {RW update} begin_search {type index spec {index 3}} find_keys {type keynum spec {keynumidx 0 firstkey 1 keystep 1}}} assert_equal [lindex $keyspecs 4] {flags {RW update} begin_search {type keyword spec {keyword MOREKEYS startfrom 5}} find_keys {type range spec {lastkey -1 keystep 1 limit 0}}} + 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} } 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 [r command getkeys kspec.complex2 foo bar 2 baz quux banana STORE dst dummy MOREKEYS hey ho] {dst foo bar baz quux hey ho} + } + + test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec without flags} { + r command getkeysandflags kspec.none key1 val1 key2 val2 + } {{key1 {RW access update variable_flags}} {key2 {RW access update variable_flags}}} + + test {COMMAND GETKEYSANDFLAGS correctly reports module key-spec flags} { + r command getkeysandflags kspec.keyword keys key1 key2 key3 + } {{key1 {RO access}} {key2 {RO access}} {key3 {RO access}}} + + # user that can only read from "read" keys, write to "write" keys, and read+write to "RW" keys + r ACL setuser testuser +@all %R~read* %W~write* %RW~rw* + + test "Module key specs: No spec, only legacy triple - ACL" { + # legacy triple didn't provide flags, so they require both read and write + assert_equal "OK" [r ACL DRYRUN testuser kspec.none rw val1] + assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN testuser kspec.none read val1] + assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser kspec.none write val1] + } + + test "Module key specs: tworanges - ACL" { + assert_equal "OK" [r ACL DRYRUN testuser kspec.tworanges read write] + assert_equal "OK" [r ACL DRYRUN testuser kspec.tworanges rw rw] + assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN testuser kspec.tworanges rw read] + assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser kspec.tworanges write rw] } test "Unload the module - keyspecs" {