diff --git a/src/module.c b/src/module.c index 5b3f217cf..472c3eea7 100644 --- a/src/module.c +++ b/src/module.c @@ -439,7 +439,7 @@ typedef int (*RedisModuleConfigApplyFunc)(RedisModuleCtx *ctx, void *privdata, R struct ModuleConfig { sds name; /* Fullname of the config (as it appears in the config file) */ sds alias; /* Optional alias for the configuration. NULL if none exists */ - + int defaultWasSet; /* Indicates if the default value was set for the configuration */ int unprefixedFlag; /* Indicates if the REDISMODULE_CONFIG_UNPREFIXED flag was set. * If the configuration name was prefixed,during get_fn/set_fn * callbacks, it should be reported without the prefix */ @@ -12499,8 +12499,8 @@ int moduleLoad(const char *path, void **module_argv, int module_argc, int is_loa ctx.module->onload = 0; int post_load_err = 0; - if (listLength(ctx.module->module_configs) && !ctx.module->configs_initialized) { - serverLogRaw(LL_WARNING, "Module Configurations were not set, likely a missing LoadConfigs call. Unloading the module."); + if (listLength(ctx.module->module_configs) && !(ctx.module->configs_initialized & MODULE_ONLOAD_CONFIG)) { + serverLogRaw(LL_WARNING, "Module Configurations were not set, missing LoadConfigs call. Unloading the module."); post_load_err = 1; } @@ -12890,37 +12890,60 @@ long long getModuleNumericConfig(ModuleConfig *module_config) { return module_config->get_fn.get_numeric(rname, module_config->privdata); } -/* This function takes a module and a list of configs stored as sds NAME VALUE pairs. - * It attempts to call set on each of these configs. */ -int loadModuleConfigs(RedisModule *module) { +int loadModuleSingleConfig(dictEntry *config_queue_entry, ModuleConfig *module_config, bool set_default_if_missing) { + const char *err = NULL; + if (config_queue_entry) { + if (!performModuleConfigSetFromName(dictGetKey(config_queue_entry), dictGetVal(config_queue_entry), &err)) { + serverLog(LL_WARNING, "Issue during loading of configuration %s : %s", (sds) dictGetKey(config_queue_entry), err); + dictFreeUnlinkedEntry(server.module_configs_queue, config_queue_entry); + dictEmpty(server.module_configs_queue, NULL); + return REDISMODULE_ERR; + } + dictFreeUnlinkedEntry(server.module_configs_queue, config_queue_entry); + } else if (set_default_if_missing) { + if (!performModuleConfigSetDefaultFromName(module_config->name, &err)) { + serverLog(LL_WARNING, "Issue attempting to set default value of configuration %s : %s", module_config->name, err); + dictEmpty(server.module_configs_queue, NULL); + return REDISMODULE_ERR; + } + } + return REDISMODULE_OK; +} + +int loadModuleDefaultConfigs(RedisModule *module) { listIter li; listNode *ln; const char *err = NULL; listRewind(module->module_configs, &li); + while ((ln = listNext(&li))) { + ModuleConfig *module_config = listNodeValue(ln); + if (loadModuleSingleConfig(NULL, module_config, true) != REDISMODULE_OK) { + return REDISMODULE_ERR; + } + } + module->configs_initialized |= MODULE_DEFAULT_CONFIG; + return REDISMODULE_OK; +} + +/* This function takes a module and a list of configs stored as sds NAME VALUE pairs. + * It attempts to call set on each of these configs. */ +int loadModuleConfigs(RedisModule *module) { + listIter li; + listNode *ln; + listRewind(module->module_configs, &li); + const bool set_default_if_missing = !(module->configs_initialized & MODULE_DEFAULT_CONFIG); while ((ln = listNext(&li))) { ModuleConfig *module_config = listNodeValue(ln); dictEntry *de = dictUnlink(server.module_configs_queue, module_config->name); if ((!de) && (module_config->alias)) de = dictUnlink(server.module_configs_queue, module_config->alias); - - /* If found in the queue, set the value. Otherwise, set the default value. */ - if (de) { - if (!performModuleConfigSetFromName(dictGetKey(de), dictGetVal(de), &err)) { - serverLog(LL_WARNING, "Issue during loading of configuration %s : %s", (sds) dictGetKey(de), err); - dictFreeUnlinkedEntry(server.module_configs_queue, de); - dictEmpty(server.module_configs_queue, NULL); - return REDISMODULE_ERR; - } - dictFreeUnlinkedEntry(server.module_configs_queue, de); - } else { - if (!performModuleConfigSetDefaultFromName(module_config->name, &err)) { - serverLog(LL_WARNING, "Issue attempting to set default value of configuration %s : %s", module_config->name, err); - dictEmpty(server.module_configs_queue, NULL); - return REDISMODULE_ERR; - } + + /* If found in the queue, set the value. Otherwise, set the default value if set_default_if_missing is true. */ + if (loadModuleSingleConfig(de, module_config, set_default_if_missing) != REDISMODULE_OK) { + return REDISMODULE_ERR; } } - module->configs_initialized = 1; + module->configs_initialized |= (MODULE_DEFAULT_CONFIG | MODULE_ONLOAD_CONFIG); return REDISMODULE_OK; } @@ -13266,6 +13289,15 @@ int RM_RegisterNumericConfig(RedisModuleCtx *ctx, const char *name, long long de return REDISMODULE_OK; } +int RM_LoadDefaultConfigs(RedisModuleCtx *ctx) { + if (!ctx || !ctx->module || !ctx->module->onload) { + return REDISMODULE_ERR; + } + RedisModule *module = ctx->module; + /* Load configs from conf file or arguments from loadex */ + return loadModuleDefaultConfigs(module); +} + /* Applies all pending configurations on the module load. This should be called * after all of the configurations have been registered for the module inside of RedisModule_OnLoad. * This will return REDISMODULE_ERR if it is called outside RedisModule_OnLoad. @@ -13277,8 +13309,7 @@ int RM_LoadConfigs(RedisModuleCtx *ctx) { } RedisModule *module = ctx->module; /* Load configs from conf file or arguments from loadex */ - if (loadModuleConfigs(module)) return REDISMODULE_ERR; - return REDISMODULE_OK; + return loadModuleConfigs(module); } /* -------------------------------------------------------------------------- diff --git a/src/redismodule.h b/src/redismodule.h index 7105fc2dc..8d5e94e26 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -1323,6 +1323,7 @@ REDISMODULE_API int (*RedisModule_RegisterNumericConfig)(RedisModuleCtx *ctx, co REDISMODULE_API int (*RedisModule_RegisterStringConfig)(RedisModuleCtx *ctx, const char *name, const char *default_val, unsigned int flags, RedisModuleConfigGetStringFunc getfn, RedisModuleConfigSetStringFunc setfn, RedisModuleConfigApplyFunc applyfn, void *privdata) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_RegisterEnumConfig)(RedisModuleCtx *ctx, const char *name, int default_val, unsigned int flags, const char **enum_values, const int *int_values, int num_enum_vals, RedisModuleConfigGetEnumFunc getfn, RedisModuleConfigSetEnumFunc setfn, RedisModuleConfigApplyFunc applyfn, void *privdata) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_LoadConfigs)(RedisModuleCtx *ctx) REDISMODULE_ATTR; +REDISMODULE_API int (*RedisModule_LoadDefaultConfigs)(RedisModuleCtx *ctx) REDISMODULE_ATTR; REDISMODULE_API RedisModuleRdbStream *(*RedisModule_RdbStreamCreateFromFile)(const char *filename) REDISMODULE_ATTR; REDISMODULE_API void (*RedisModule_RdbStreamFree)(RedisModuleRdbStream *stream) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_RdbLoad)(RedisModuleCtx *ctx, RedisModuleRdbStream *stream, int flags) REDISMODULE_ATTR; diff --git a/src/server.h b/src/server.h index 66b69c448..fd20890d7 100644 --- a/src/server.h +++ b/src/server.h @@ -866,6 +866,12 @@ typedef struct moduleValue { void *value; } moduleValue; +typedef enum { + MODULE_DEFAULT_CONFIG = 0x1, + MODULE_ONLOAD_CONFIG = 0x2, + MODULE_CONFIGURED = MODULE_DEFAULT_CONFIG | MODULE_ONLOAD_CONFIG +} ModuleConfigFlags; + /* This structure represents a module inside the system. */ struct RedisModule { void *handle; /* Module dlopen() handle. */ @@ -877,7 +883,7 @@ struct RedisModule { list *using; /* List of modules we use some APIs of. */ list *filters; /* List of filters the module has registered. */ list *module_configs; /* List of configurations the module has registered */ - int configs_initialized; /* Have the module configurations been initialized? */ + ModuleConfigFlags configs_initialized; /* Have the module configurations been initialized? */ int in_call; /* RM_Call() nesting level */ int in_hook; /* Hooks callback nesting level for this module (0 or 1). */ int options; /* Module options and capabilities. */ diff --git a/tests/modules/moduleconfigs.c b/tests/modules/moduleconfigs.c index 504a7e291..bcfbc4769 100644 --- a/tests/modules/moduleconfigs.c +++ b/tests/modules/moduleconfigs.c @@ -226,6 +226,8 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) size_t len; if (argc && !strcasecmp(RedisModule_StringPtrLen(argv[0], &len), "noload")) { return REDISMODULE_OK; + } else if (RedisModule_LoadDefaultConfigs(ctx) == REDISMODULE_ERR) { + return REDISMODULE_ERR; } else if (RedisModule_LoadConfigs(ctx) == REDISMODULE_ERR) { if (strval) { RedisModule_FreeString(ctx, strval); diff --git a/tests/unit/moduleapi/moduleconfigs.tcl b/tests/unit/moduleapi/moduleconfigs.tcl index 609732b58..46d218d44 100644 --- a/tests/unit/moduleapi/moduleconfigs.tcl +++ b/tests/unit/moduleapi/moduleconfigs.tcl @@ -342,5 +342,49 @@ start_server {tags {"modules"}} { assert_equal [r config get unprefix-enum-alias] "unprefix-enum-alias one" } } + + test {loadmodule CONFIG values take precedence over loadmoduleex ARGS values} { + # Load module with conflicting CONFIG and ARGS values + r module loadex $testmodule \ + CONFIG moduleconfigs.mutable_bool yes \ + CONFIG moduleconfigs.memory_numeric 2mb \ + ARGS moduleconfigs.mutable_bool no \ + ARGS moduleconfigs.memory_numeric 4mb + + # Verify CONFIG values took precedence + assert_equal [r config get moduleconfigs.mutable_bool] "moduleconfigs.mutable_bool yes" + assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 2097152" + + r module unload moduleconfigs + } + + # Test: Ensure that modified configuration values from ARGS are correctly written to the config file + test {Modified ARGS values are persisted after config rewrite when set through CONFIG commands} { + # Load module with non-default ARGS values + r module loadex $testmodule \ + ARGS moduleconfigs.memory_numeric 4mb \ + ARGS moduleconfigs.string_setting "custom_value" + + # Verify the initial ARGS values + assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 4194304" + assert_equal [r config get moduleconfigs.string_setting] "moduleconfigs.string_setting custom_value" + + # Set new values to simulate user configuration changes + r config set moduleconfigs.memory_numeric 8mb + r config set moduleconfigs.string_setting "modified_value" + + # Verify that the changes took effect + assert_equal [r config get moduleconfigs.memory_numeric] "moduleconfigs.memory_numeric 8388608" + assert_equal [r config get moduleconfigs.string_setting] "moduleconfigs.string_setting modified_value" + + # Perform a config rewrite + r config rewrite + + # Read and verify config file contents to check for persistence + set config_contents [file read [config_path]] + assert_contains "moduleconfigs.memory_numeric 8388608" $config_contents + assert_contains "moduleconfigs.string_setting modified_value" $config_contents + r module unload moduleconfigs + } }