Skip to content

Commit

Permalink
Merge pull request #144 from KarthikSubbarao/main
Browse files Browse the repository at this point in the history 
Add a non breaking feature for module configs to support validation & rejection of config sets
  • Loading branch information
@dmitrypol
dmitrypol authored Dec 6, 2024
2 parents 5ff3a8d + 809af26 commit 0a1fb4e
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 13 deletions.
62 changes: 60 additions & 2 deletions examples/configuration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ use lazy_static::lazy_static;
use valkey_module::alloc::ValkeyAlloc;
use valkey_module::{
configuration::{ConfigurationContext, ConfigurationFlags},
enum_configuration, valkey_module, ConfigurationValue, Context, ValkeyGILGuard, ValkeyResult,
ValkeyString, ValkeyValue,
enum_configuration, valkey_module, ConfigurationValue, Context, ValkeyError, ValkeyGILGuard,
ValkeyResult, ValkeyString, ValkeyValue,
};

enum_configuration! {
#[derive(PartialEq)]
enum EnumConfiguration {
Val1 = 1,
Val2 = 2,
Expand All @@ -21,15 +22,21 @@ enum_configuration! {
lazy_static! {
static ref NUM_OF_CONFIGURATION_CHANGES: ValkeyGILGuard<i64> = ValkeyGILGuard::default();
static ref CONFIGURATION_I64: ValkeyGILGuard<i64> = ValkeyGILGuard::default();
static ref CONFIGURATION_REJECT_I64: ValkeyGILGuard<i64> = ValkeyGILGuard::default();
static ref CONFIGURATION_ATOMIC_I64: AtomicI64 = AtomicI64::new(1);
static ref CONFIGURATION_VALKEY_STRING: ValkeyGILGuard<ValkeyString> =
ValkeyGILGuard::new(ValkeyString::create(None, "default"));
static ref CONFIGURATION_REJECT_VALKEY_STRING: ValkeyGILGuard<ValkeyString> =
ValkeyGILGuard::new(ValkeyString::create(None, "default"));
static ref CONFIGURATION_STRING: ValkeyGILGuard<String> = ValkeyGILGuard::new("default".into());
static ref CONFIGURATION_MUTEX_STRING: Mutex<String> = Mutex::new("default".into());
static ref CONFIGURATION_ATOMIC_BOOL: AtomicBool = AtomicBool::default();
static ref CONFIGURATION_BOOL: ValkeyGILGuard<bool> = ValkeyGILGuard::default();
static ref CONFIGURATION_REJECT_BOOL: ValkeyGILGuard<bool> = ValkeyGILGuard::default();
static ref CONFIGURATION_ENUM: ValkeyGILGuard<EnumConfiguration> =
ValkeyGILGuard::new(EnumConfiguration::Val1);
static ref CONFIGURATION_REJECT_ENUM: ValkeyGILGuard<EnumConfiguration> =
ValkeyGILGuard::new(EnumConfiguration::Val1);
static ref CONFIGURATION_MUTEX_ENUM: Mutex<EnumConfiguration> =
Mutex::new(EnumConfiguration::Val1);
}
Expand All @@ -43,6 +50,53 @@ fn on_configuration_changed<G, T: ConfigurationValue<G>>(
*val += 1
}

// Custom on_set handlers to add validation and conditionally
// reject upon config change.
fn on_string_config_set<G, T: ConfigurationValue<ValkeyString>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v.to_string_lossy().contains("rejectvalue") {
return Err(ValkeyError::Str("Rejected from custom string validation."));
}
Ok(())
}
fn on_i64_config_set<G, T: ConfigurationValue<i64>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v == 123 {
return Err(ValkeyError::Str("Rejected from custom i64 validation."));
}
Ok(())
}
fn on_bool_config_set<G, T: ConfigurationValue<bool>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v == false {
return Err(ValkeyError::Str("Rejected from custom bool validation."));
}
Ok(())
}
fn on_enum_config_set<G, T: ConfigurationValue<EnumConfiguration>>(
config_ctx: &ConfigurationContext,
_name: &str,
val: &'static T,
) -> Result<(), ValkeyError> {
let v = val.get(config_ctx);
if v == EnumConfiguration::Val2 {
return Err(ValkeyError::Str("Rejected from custom enum validation."));
}
Ok(())
}

fn num_changes(ctx: &Context, _: Vec<ValkeyString>) -> ValkeyResult {
let val = NUM_OF_CONFIGURATION_CHANGES.lock(ctx);
Ok(ValkeyValue::Integer(*val))
Expand All @@ -61,19 +115,23 @@ valkey_module! {
configurations: [
i64: [
["i64", &*CONFIGURATION_I64, 10, 0, 1000, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject_i64", &*CONFIGURATION_REJECT_I64, 10, 0, 1000, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_i64_config_set::<ValkeyString, ValkeyGILGuard<i64>>))],
["atomic_i64", &*CONFIGURATION_ATOMIC_I64, 10, 0, 1000, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
],
string: [
["valkey_string", &*CONFIGURATION_VALKEY_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject_valkey_string", &*CONFIGURATION_REJECT_VALKEY_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_string_config_set::<ValkeyString, ValkeyGILGuard<ValkeyString>>))],
["string", &*CONFIGURATION_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed::<String, _>))],
["mutex_string", &*CONFIGURATION_MUTEX_STRING, "default", ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed::<String, _>))],
],
bool: [
["atomic_bool", &*CONFIGURATION_ATOMIC_BOOL, true, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["bool", &*CONFIGURATION_BOOL, true, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject_bool", &*CONFIGURATION_REJECT_BOOL, true, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_bool_config_set::<ValkeyString, ValkeyGILGuard<bool>>))],
],
enum: [
["enum", &*CONFIGURATION_ENUM, EnumConfiguration::Val1, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
["reject_enum", &*CONFIGURATION_REJECT_ENUM, EnumConfiguration::Val1, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed)), Some(Box::new(on_enum_config_set::<ValkeyString, ValkeyGILGuard<EnumConfiguration>>))],
["enum_mutex", &*CONFIGURATION_MUTEX_ENUM, EnumConfiguration::Val1, ConfigurationFlags::DEFAULT, Some(Box::new(on_configuration_changed))],
],
module_args_as_configuration: true,
Expand Down
26 changes: 25 additions & 1 deletion src/configuration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,13 @@ impl ConfigurationValue<bool> for AtomicBool {

type OnUpdatedCallback<T> = Box<dyn Fn(&ConfigurationContext, &str, &'static T)>;

type OnSetCallback<T> =
Box<dyn Fn(&ConfigurationContext, &str, &'static T) -> Result<(), ValkeyError>>;

struct ConfigrationPrivateData<G, T: ConfigurationValue<G> + 'static> {
variable: &'static T,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
phantom: PhantomData<G>,
}

Expand All @@ -191,12 +195,24 @@ impl<G, T: ConfigurationValue<G> + 'static> ConfigrationPrivateData<G, T> {
return raw::REDISMODULE_ERR as i32;
}
let c_str_name = unsafe { CStr::from_ptr(name) };
if let Some(v) = self.on_set.as_ref() {
let result = v(
&configuration_ctx,
c_str_name.to_str().unwrap(),
self.variable,
);
if let Err(e) = result {
let error_msg = ValkeyString::create(None, e.to_string().as_str());
unsafe { *err = error_msg.take() };
return raw::REDISMODULE_ERR as i32;
}
}
if let Some(v) = self.on_changed.as_ref() {
v(
&configuration_ctx,
c_str_name.to_str().unwrap(),
self.variable,
)
);
}
raw::REDISMODULE_OK as i32
}
Expand Down Expand Up @@ -233,11 +249,13 @@ pub fn register_i64_configuration<T: ConfigurationValue<i64>>(
max: i64,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<i64>,
};
unsafe {
Expand Down Expand Up @@ -304,12 +322,14 @@ pub fn register_string_configuration<T: ConfigurationValue<ValkeyString>>(
default: &str,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let default = CString::new(default).unwrap();
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<ValkeyString>,
};
unsafe {
Expand Down Expand Up @@ -359,11 +379,13 @@ pub fn register_bool_configuration<T: ConfigurationValue<bool>>(
default: bool,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<bool>,
};
unsafe {
Expand Down Expand Up @@ -427,6 +449,7 @@ pub fn register_enum_configuration<G: EnumConfigurationValue, T: ConfigurationVa
default: G,
flags: ConfigurationFlags,
on_changed: Option<OnUpdatedCallback<T>>,
on_set: Option<OnSetCallback<T>>,
) {
let name = CString::new(name).unwrap();
let (names, vals) = default.get_options();
Expand All @@ -438,6 +461,7 @@ pub fn register_enum_configuration<G: EnumConfigurationValue, T: ConfigurationVa
let config_private_data = ConfigrationPrivateData {
variable,
on_changed,
on_set,
phantom: PhantomData::<G>,
};
unsafe {
Expand Down
44 changes: 36 additions & 8 deletions src/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,28 +148,28 @@ macro_rules! valkey_module {
$i64_min:expr,
$i64_max:expr,
$i64_flags_options:expr,
$i64_on_changed:expr
$i64_on_changed:expr $(, $i64_on_set:expr)?
]),* $(,)*],)?
$(string:[$([
$string_configuration_name:expr,
$string_configuration_val:expr,
$string_default:expr,
$string_flags_options:expr,
$string_on_changed:expr
$string_on_changed:expr $(, $string_on_set:expr)?
]),* $(,)*],)?
$(bool:[$([
$bool_configuration_name:expr,
$bool_configuration_val:expr,
$bool_default:expr,
$bool_flags_options:expr,
$bool_on_changed:expr
$bool_on_changed:expr $(, $bool_on_set:expr)?
]),* $(,)*],)?
$(enum:[$([
$enum_configuration_name:expr,
$enum_configuration_val:expr,
$enum_default:expr,
$enum_flags_options:expr,
$enum_on_changed:expr
$enum_on_changed:expr $(, $enum_on_set:expr)?
]),* $(,)*],)?
$(module_args_as_configuration:$use_module_args:expr,)?
$(module_config_get:$module_config_get_command:expr,)?
Expand Down Expand Up @@ -290,7 +290,14 @@ macro_rules! valkey_module {
} else {
$i64_default
};
register_i64_configuration(&context, $i64_configuration_name, $i64_configuration_val, default, $i64_min, $i64_max, $i64_flags_options, $i64_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_i64_configuration(&context, $i64_configuration_name, $i64_configuration_val, default, $i64_min, $i64_max, $i64_flags_options, $i64_on_changed, $i64_on_set);
)?
if (use_fallback) {
register_i64_configuration(&context, $i64_configuration_name, $i64_configuration_val, default, $i64_min, $i64_max, $i64_flags_options, $i64_on_changed, None);
}
)*
)?
$(
Expand All @@ -306,7 +313,14 @@ macro_rules! valkey_module {
} else {
$string_default
};
register_string_configuration(&context, $string_configuration_name, $string_configuration_val, default, $string_flags_options, $string_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_string_configuration(&context, $string_configuration_name, $string_configuration_val, default, $string_flags_options, $string_on_changed, $string_on_set);
)?
if (use_fallback) {
register_string_configuration(&context, $string_configuration_name, $string_configuration_val, default, $string_flags_options, $string_on_changed, None);
}
)*
)?
$(
Expand All @@ -322,7 +336,14 @@ macro_rules! valkey_module {
} else {
$bool_default
};
register_bool_configuration(&context, $bool_configuration_name, $bool_configuration_val, default, $bool_flags_options, $bool_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_bool_configuration(&context, $bool_configuration_name, $bool_configuration_val, default, $bool_flags_options, $bool_on_changed, $bool_on_set);
)?
if (use_fallback) {
register_bool_configuration(&context, $bool_configuration_name, $bool_configuration_val, default, $bool_flags_options, $bool_on_changed, None);
}
)*
)?
$(
Expand All @@ -338,7 +359,14 @@ macro_rules! valkey_module {
} else {
$enum_default
};
register_enum_configuration(&context, $enum_configuration_name, $enum_configuration_val, default, $enum_flags_options, $enum_on_changed);
let mut use_fallback = true;
$(
use_fallback = false;
register_enum_configuration(&context, $enum_configuration_name, $enum_configuration_val, default.clone(), $enum_flags_options, $enum_on_changed, $enum_on_set);
)?
if (use_fallback) {
register_enum_configuration(&context, $enum_configuration_name, $enum_configuration_val, default.clone(), $enum_flags_options, $enum_on_changed, None);
}
)*
)?
raw::RedisModule_LoadConfigs.unwrap()(ctx);
Expand Down
48 changes: 46 additions & 2 deletions tests/integration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ fn test_configuration() -> Result<()> {
let res: String = redis::cmd("config")
.arg(&["set", config, val])
.query(&mut con)
.with_context(|| "failed to run config set")?;
.map_err(|e| anyhow::anyhow!("Failed to run config set: {}", e))?;
assert_eq!(res, "OK");
Ok(())
};
Expand Down Expand Up @@ -521,11 +521,55 @@ fn test_configuration() -> Result<()> {
config_set("configuration.enum_mutex", "Val2")?;
assert_eq!(config_get("configuration.enum_mutex")?, "Val2");

// Validate that configs can be rejected
let value = config_set("configuration.reject_valkey_string", "rejectvalue");
assert!(value
.unwrap_err()
.to_string()
.contains("Rejected from custom string validation"));
let value = config_set("configuration.reject_i64", "123");
assert!(value
.unwrap_err()
.to_string()
.contains("Rejected from custom i64 validation"));
let value = config_set("configuration.reject_bool", "no");
assert!(value
.unwrap_err()
.to_string()
.contains("Rejected from custom bool validation"));
let value = config_set("configuration.reject_enum", "Val2");
assert!(value
.unwrap_err()
.to_string()
.contains("Rejected from custom enum validation"));

let mut con = get_valkey_connection(port).with_context(|| FAILED_TO_CONNECT_TO_SERVER)?;
let res: i64 = redis::cmd("configuration.num_changes")
.query(&mut con)
.with_context(|| "failed to run configuration.num_changes")?;
assert_eq!(res, 26); // the first configuration initialisation is counted as well, so we will get 22 changes.

// Validate that configs with logic to reject values can also succeed
assert_eq!(config_get("configuration.reject_valkey_string")?, "default");
config_set("configuration.reject_valkey_string", "validvalue")?;
assert_eq!(
config_get("configuration.reject_valkey_string")?,
"validvalue"
);
assert_eq!(config_get("configuration.reject_i64")?, "10");
config_set("configuration.reject_i64", "11")?;
assert_eq!(config_get("configuration.reject_i64")?, "11");
assert_eq!(config_get("configuration.reject_bool")?, "yes");
config_set("configuration.reject_bool", "yes")?;
assert_eq!(config_get("configuration.reject_bool")?, "yes");
assert_eq!(config_get("configuration.reject_enum")?, "Val1");
config_set("configuration.reject_enum", "Val1")?;
assert_eq!(config_get("configuration.reject_enum")?, "Val1");
let mut con = get_valkey_connection(port).with_context(|| FAILED_TO_CONNECT_TO_SERVER)?;
let res: i64 = redis::cmd("configuration.num_changes")
.query(&mut con)
.with_context(|| "failed to run configuration.num_changes")?;
assert_eq!(res, 18); // the first configuration initialisation is counted as well, so we will get 18 changes.
assert_eq!(res, 28);

Ok(())
}
Expand Down

0 comments on commit 0a1fb4e

Please sign in to comment.