From b139d141ef576bbf8f2d31b419b2daab32eb199a Mon Sep 17 00:00:00 2001 From: Justin Harris Date: Fri, 28 Nov 2025 21:36:55 -0500 Subject: [PATCH 1/3] [Rust] Merge specific config --- rust/optify/README.md | 3 + rust/optify/src/provider/provider_impl.rs | 111 ++++++++++++++++------ rust/optify/tests/test_provider.rs | 48 ++++++++++ 3 files changed, 134 insertions(+), 28 deletions(-) diff --git a/rust/optify/README.md b/rust/optify/README.md index e3c08d51..522f105b 100644 --- a/rust/optify/README.md +++ b/rust/optify/README.md @@ -38,6 +38,9 @@ Run: ```shell cargo build --release cargo bench + +# Open one of the reports +open target/criterion/*/report/index.html ``` ## Publishing diff --git a/rust/optify/src/provider/provider_impl.rs b/rust/optify/src/provider/provider_impl.rs index 9ebf5e48..ccbf8864 100644 --- a/rust/optify/src/provider/provider_impl.rs +++ b/rust/optify/src/provider/provider_impl.rs @@ -76,22 +76,37 @@ impl OptionsProvider { } }; - let mut result = serde_json::Value::Object(serde_json::Map::new()); - - for canonical_feature_name in feature_names { - let source = match self.sources.get(canonical_feature_name) { - Some(src) => src, + let mut result = if feature_names.len() == 1 { + // Avoid merging to an empty object. + let feature_name = &feature_names[0]; + match self.sources.get(feature_name) { + Some(src) => src.clone(), None => { - // Should not happen. - // All canonical feature names are included as keys in the sources map. - // It could happen in the future if we allow aliases to be added directly, but we should try to validate them when the provider is built. return Err(format!( - "Feature name {canonical_feature_name:?} is not a known feature." + "Feature name {feature_name:?} is not a known feature." )); } - }; - merge_json_value(&mut result, source); - } + } + } else { + let mut result = serde_json::Value::Object(serde_json::Map::new()); + + for canonical_feature_name in feature_names { + let source = match self.sources.get(canonical_feature_name) { + Some(src) => src, + None => { + // Should not happen. + // All canonical feature names are included as keys in the sources map. + // It could happen in the future if we allow aliases to be added directly, but we should try to validate them when the provider is built. + return Err(format!( + "Feature name {canonical_feature_name:?} is not a known feature." + )); + } + }; + merge_json_value(&mut result, source); + } + + result + }; if let Some(preferences) = preferences { if let Some(overrides) = &preferences.overrides_json { @@ -112,6 +127,61 @@ impl OptionsProvider { Ok(result) } + /// Get options for a specific key by extracting and merging only that key from each source. + /// This avoids building the entire config when only one key is needed. + fn get_options_for_key( + &self, + key: &str, + feature_names: &[String], + preferences: Option<&GetOptionsPreferences>, + ) -> Result { + let mut result = if feature_names.len() == 1 { + // Avoid merging to an empty object. + let feature_name = &feature_names[0]; + let source = self + .sources + .get(feature_name) + .ok_or_else(|| format!("Feature name {feature_name:?} is not a known feature."))?; + source.get(key).cloned() + } else { + let mut result = None; + for canonical_feature_name in feature_names { + let source = self.sources.get(canonical_feature_name).ok_or_else(|| { + format!("Feature name {canonical_feature_name:?} is not a known feature.") + })?; + + if let Some(source_value) = source.get(key) { + match &mut result { + Some(existing) => merge_json_value(existing, source_value), + None => result = Some(source_value.clone()), + } + } + } + result + }; + + // Apply overrides for this specific key. + if let Some(preferences) = preferences { + if let Some(overrides) = &preferences.overrides_json { + let overrides_value: serde_json::Value = serde_json::from_str(overrides) + .map_err(|e| format!("Failed to parse overrides JSON: {e}"))?; + if let Some(override_for_key) = overrides_value.get(key) { + match &mut result { + Some(existing) => merge_json_value(existing, override_for_key), + None => result = Some(override_for_key.clone()), + } + } + } + } + + result.ok_or_else(|| { + format!( + "Error getting options with features {:?}: configuration property \"{}\" not found", + feature_names, key + ) + }) + } + fn get_entire_config_from_cache( &self, feature_names: &[String], @@ -386,22 +456,7 @@ impl OptionsRegistry for OptionsProvider { } let filtered_feature_names = self.get_filtered_feature_names(feature_names, preferences)?; - // TODO Optimization: Don't build entire config, just get what is needed from each one. - // It might be tricky to use the overrides, but it should be much faster overall. - // It was done this way to just get something working adequately - // because it was assumed that each language can optimize - // by caching results including conversion to immutable types. - let config = self.get_entire_config(&filtered_feature_names, cache_options, preferences)?; - let mut value = match config.get(key) { - Some(v) => v.clone(), - None => { - return Err(format!( - "Error getting options with features {:?}: configuration property \"{}\" not found", - feature_names.iter().map(|f| f.as_ref()).collect::>(), - key - )) - } - }; + let mut value = self.get_options_for_key(key, &filtered_feature_names, preferences)?; self.process_configurable_strings(&mut value, Some(key), preferences)?; if cache_options.is_some() { diff --git a/rust/optify/tests/test_provider.rs b/rust/optify/tests/test_provider.rs index b46105a9..ef6cc705 100644 --- a/rust/optify/tests/test_provider.rs +++ b/rust/optify/tests/test_provider.rs @@ -98,6 +98,54 @@ fn test_provider_get_features_and_aliases() -> Result<(), Box Result<(), Box> { + let key = "does not exist"; + let feature_names = vec!["a"]; + let provider = get_provider(); + let opts = provider.get_options(key, &feature_names); + assert!(opts.is_err()); + assert_eq!(opts.unwrap_err(), "Error getting options with features [\"feature_A\"]: configuration property \"does not exist\" not found"); + + let mut preferences = GetOptionsPreferences::new(); + preferences.overrides_json = Some( + serde_json::json!({ + "does not exist": 42 + }) + .to_string(), + ); + let opts = provider.get_options_with_preferences(key, &feature_names, None, Some(&preferences)); + let value = opts.expect("should be able to get options"); + assert_eq!(value, serde_json::json!(42)); + + Ok(()) +} + +#[test] +fn test_provider_get_options_no_features() -> Result<(), Box> { + let key = "wtv"; + let provider = get_provider(); + let feature_names: Vec<&str> = vec![]; + let opts = provider.get_options(key, &feature_names); + assert!(opts.is_err()); + assert_eq!( + opts.unwrap_err(), + "Error getting options with features []: configuration property \"wtv\" not found" + ); + + let mut preferences = GetOptionsPreferences::new(); + preferences.overrides_json = Some( + serde_json::json!({ + key: 42 + }) + .to_string(), + ); + let opts = provider.get_options_with_preferences(key, &feature_names, None, Some(&preferences)); + let value = opts.expect("should be able to get options"); + assert_eq!(value, serde_json::json!(42)); + Ok(()) +} + #[test] fn test_provider_get_options_with_overrides() -> Result<(), Box> { let provider = get_provider(); From e977be302720a1b60492e1ee60567e56c6697c1e Mon Sep 17 00:00:00 2001 From: Justin Harris Date: Fri, 28 Nov 2025 21:53:42 -0500 Subject: [PATCH 2/3] overrides as serde_json::Value --- rust/optify/benches/get_options_benchmark.rs | 23 +++---- .../src/provider/get_options_preferences.rs | 21 ++----- rust/optify/src/provider/provider_impl.rs | 14 ++--- rust/optify/tests/test_cache.rs | 2 +- rust/optify/tests/test_provider.rs | 62 ++++++++----------- 5 files changed, 45 insertions(+), 77 deletions(-) diff --git a/rust/optify/benches/get_options_benchmark.rs b/rust/optify/benches/get_options_benchmark.rs index af99141e..864757e2 100644 --- a/rust/optify/benches/get_options_benchmark.rs +++ b/rust/optify/benches/get_options_benchmark.rs @@ -215,7 +215,7 @@ fn benchmark_with_overrides(c: &mut Criterion) { // With small override group.bench_function("small_override", |b| { let mut preferences = GetOptionsPreferences::new(); - preferences.overrides_json = Some(r#"{"config1": {"extra": "value"}}"#.to_string()); + preferences.overrides = Some(serde_json::json!({"config1": {"extra": "value"}})); b.iter(|| { provider @@ -232,20 +232,17 @@ fn benchmark_with_overrides(c: &mut Criterion) { // With larger override group.bench_function("larger_override", |b| { let mut preferences = GetOptionsPreferences::new(); - preferences.overrides_json = Some( - r#"{ - "config1": { - "level1": { - "level2": { - "key1": "overridden value 1", - "key2": "overridden value 2", - "newKey": "brand new value" - } + preferences.overrides = Some(serde_json::json!({ + "config1": { + "level1": { + "level2": { + "key1": "overridden value 1", + "key2": "overridden value 2", + "newKey": "brand new value" } } - }"# - .to_string(), - ); + } + })); b.iter(|| { provider diff --git a/rust/optify/src/provider/get_options_preferences.rs b/rust/optify/src/provider/get_options_preferences.rs index 84fc5bc5..aa56387f 100644 --- a/rust/optify/src/provider/get_options_preferences.rs +++ b/rust/optify/src/provider/get_options_preferences.rs @@ -1,6 +1,6 @@ -use crate::provider::constraints::Constraints; +use crate::provider::{constraints::Constraints, SourceValue}; -#[derive(Hash, PartialEq, Eq)] +#[derive(Clone, Hash, PartialEq, Eq)] pub struct GetOptionsPreferences { /// Allows resolving configurable strings. /// Defaults to false: no configurable strings will be resolved. @@ -8,25 +8,12 @@ pub struct GetOptionsPreferences { pub are_configurable_strings_enabled: bool, pub constraints: Option, /// Overrides to apply after the built configuration. - /// A string is used because it makes it easier to pass to the `config` library, but this may change in the future. - /// It also makes it simpler and maybe faster to get from other programming languages. - pub overrides_json: Option, + pub overrides: Option, /// Determines if the feature names should be converted to canonical feature names. /// Defaults to false: given features names will be converted to canonical feature names before looking for features or options. pub skip_feature_name_conversion: bool, } -impl Clone for GetOptionsPreferences { - fn clone(&self) -> Self { - Self { - are_configurable_strings_enabled: self.are_configurable_strings_enabled, - constraints: self.constraints.clone(), - overrides_json: self.overrides_json.clone(), - skip_feature_name_conversion: self.skip_feature_name_conversion, - } - } -} - impl Default for GetOptionsPreferences { fn default() -> Self { Self::new() @@ -38,7 +25,7 @@ impl GetOptionsPreferences { Self { are_configurable_strings_enabled: false, constraints: None, - overrides_json: None, + overrides: None, skip_feature_name_conversion: false, } } diff --git a/rust/optify/src/provider/provider_impl.rs b/rust/optify/src/provider/provider_impl.rs index ccbf8864..2877d9a4 100644 --- a/rust/optify/src/provider/provider_impl.rs +++ b/rust/optify/src/provider/provider_impl.rs @@ -109,10 +109,8 @@ impl OptionsProvider { }; if let Some(preferences) = preferences { - if let Some(overrides) = &preferences.overrides_json { - let overrides_value: serde_json::Value = serde_json::from_str(overrides) - .map_err(|e| format!("Failed to parse overrides JSON: {e}"))?; - merge_json_value(&mut result, &overrides_value); + if let Some(overrides) = &preferences.overrides { + merge_json_value(&mut result, overrides); } } @@ -162,10 +160,8 @@ impl OptionsProvider { // Apply overrides for this specific key. if let Some(preferences) = preferences { - if let Some(overrides) = &preferences.overrides_json { - let overrides_value: serde_json::Value = serde_json::from_str(overrides) - .map_err(|e| format!("Failed to parse overrides JSON: {e}"))?; - if let Some(override_for_key) = overrides_value.get(key) { + if let Some(overrides) = &preferences.overrides { + if let Some(override_for_key) = overrides.get(key) { match &mut result { Some(existing) => merge_json_value(existing, override_for_key), None => result = Some(override_for_key.clone()), @@ -188,7 +184,7 @@ impl OptionsProvider { preferences: Option<&GetOptionsPreferences>, ) -> Result, String> { if let Some(preferences) = preferences { - if preferences.overrides_json.is_some() { + if preferences.overrides.is_some() { return Err("Caching when overrides are given is not supported.".to_owned()); } } diff --git a/rust/optify/tests/test_cache.rs b/rust/optify/tests/test_cache.rs index d03940e7..1ac764c4 100644 --- a/rust/optify/tests/test_cache.rs +++ b/rust/optify/tests/test_cache.rs @@ -87,7 +87,7 @@ fn test_cache_with_overrides() -> Result<(), Box> { let cache_options = CacheOptions {}; let mut preferences = GetOptionsPreferences::new(); - preferences.overrides_json = Some(r#"{"test": "override"}"#.to_string()); + preferences.overrides = Some(serde_json::json!({"test": "override"})); let result = provider.get_all_options(&["a"], Some(&cache_options), Some(&preferences)); diff --git a/rust/optify/tests/test_provider.rs b/rust/optify/tests/test_provider.rs index ef6cc705..7bebcfe8 100644 --- a/rust/optify/tests/test_provider.rs +++ b/rust/optify/tests/test_provider.rs @@ -108,12 +108,9 @@ fn test_provider_get_options_missing_key() -> Result<(), Box Result<(), Box Result<(), Box Result<(), Box> { let provider = get_provider(); let mut preferences = GetOptionsPreferences::new(); - preferences.overrides_json = Some( - serde_json::json!({ - "myConfig": { - "new key": 33, - "rootString": "new string", - "myObject": { - "one": 1321, - "something new for test_provider_get_options_with_overrides": "hello" - } + preferences.overrides = Some(serde_json::json!({ + "myConfig": { + "new key": 33, + "rootString": "new string", + "myObject": { + "one": 1321, + "something new for test_provider_get_options_with_overrides": "hello" } - }) - .to_string(), - ); + } + })); let opts = provider.get_options_with_preferences("myConfig", &["a"], None, Some(&preferences)); let expected = serde_json::json!({ @@ -311,20 +302,17 @@ fn test_configurable_values_get_all_options_with_overrides( let provider = get_configurable_values_provider(); let mut preferences = GetOptionsPreferences::new(); preferences.are_configurable_strings_enabled = true; - preferences.overrides_json = Some( - serde_json::json!({ - "message": { - "$type": "Optify.ConfigurableString", - "base": { - "liquid": "Hello {{ name }}!" - }, - "arguments": { - "name": "from the test" - } + preferences.overrides = Some(serde_json::json!({ + "message": { + "$type": "Optify.ConfigurableString", + "base": { + "liquid": "Hello {{ name }}!" + }, + "arguments": { + "name": "from the test" } - }) - .to_string(), - ); + } + })); let features: Vec<&str> = vec![]; let opts = provider.get_all_options(&features, None, Some(&preferences))?; From 7fc667b997652646b336b5023672b8b0e0df75b3 Mon Sep 17 00:00:00 2001 From: Justin Harris Date: Fri, 28 Nov 2025 22:05:34 -0500 Subject: [PATCH 3/3] Correct other langs --- js/optify-config/src/preferences.rs | 3 ++- ruby/optify/ext/optify_ruby/src/preferences.rs | 11 ++++++++--- rust/optify/src/provider/provider_impl.rs | 18 ++++++++++++------ rust/optify/tests/test_provider.rs | 2 +- 4 files changed, 23 insertions(+), 11 deletions(-) diff --git a/js/optify-config/src/preferences.rs b/js/optify-config/src/preferences.rs index 21ef88af..95c2ef9e 100644 --- a/js/optify-config/src/preferences.rs +++ b/js/optify-config/src/preferences.rs @@ -40,7 +40,8 @@ impl JsGetOptionsPreferences { #[napi] pub fn set_overrides_json(&mut self, overrides_json: Option) { - self.inner.overrides_json = overrides_json; + self.inner.overrides = + overrides_json.map(|s| serde_json::from_str(&s).expect("overrides should be valid JSON")); } #[napi] diff --git a/ruby/optify/ext/optify_ruby/src/preferences.rs b/ruby/optify/ext/optify_ruby/src/preferences.rs index 1883af24..bca10819 100644 --- a/ruby/optify/ext/optify_ruby/src/preferences.rs +++ b/ruby/optify/ext/optify_ruby/src/preferences.rs @@ -47,15 +47,20 @@ impl MutGetOptionsPreferences { // Overrides Section pub fn has_overrides(&self) -> bool { - self.0.borrow().overrides_json.is_some() + self.0.borrow().overrides.is_some() } pub fn set_overrides_json(&self, overrides: Option) { - self.0.borrow_mut().overrides_json = overrides; + self.0.borrow_mut().overrides = + overrides.map(|s| serde_json::from_str(&s).expect("overrides should be valid JSON")); } pub fn get_overrides_json(&self) -> Option { - self.0.borrow().overrides_json.clone() + self.0 + .borrow() + .overrides + .as_ref() + .map(|o| serde_json::to_string(&o).unwrap()) } // Skip Feature Name Conversion Section diff --git a/rust/optify/src/provider/provider_impl.rs b/rust/optify/src/provider/provider_impl.rs index 2877d9a4..963c4d73 100644 --- a/rust/optify/src/provider/provider_impl.rs +++ b/rust/optify/src/provider/provider_impl.rs @@ -130,12 +130,13 @@ impl OptionsProvider { fn get_options_for_key( &self, key: &str, - feature_names: &[String], + filtered_feature_names: &[String], + original_feature_names: &[impl AsRef], preferences: Option<&GetOptionsPreferences>, ) -> Result { - let mut result = if feature_names.len() == 1 { + let mut result = if filtered_feature_names.len() == 1 { // Avoid merging to an empty object. - let feature_name = &feature_names[0]; + let feature_name = &filtered_feature_names[0]; let source = self .sources .get(feature_name) @@ -143,7 +144,7 @@ impl OptionsProvider { source.get(key).cloned() } else { let mut result = None; - for canonical_feature_name in feature_names { + for canonical_feature_name in filtered_feature_names { let source = self.sources.get(canonical_feature_name).ok_or_else(|| { format!("Feature name {canonical_feature_name:?} is not a known feature.") })?; @@ -173,7 +174,11 @@ impl OptionsProvider { result.ok_or_else(|| { format!( "Error getting options with features {:?}: configuration property \"{}\" not found", - feature_names, key + original_feature_names + .iter() + .map(|f| f.as_ref()) + .collect::>(), + key ) }) } @@ -452,7 +457,8 @@ impl OptionsRegistry for OptionsProvider { } let filtered_feature_names = self.get_filtered_feature_names(feature_names, preferences)?; - let mut value = self.get_options_for_key(key, &filtered_feature_names, preferences)?; + let mut value = + self.get_options_for_key(key, &filtered_feature_names, feature_names, preferences)?; self.process_configurable_strings(&mut value, Some(key), preferences)?; if cache_options.is_some() { diff --git a/rust/optify/tests/test_provider.rs b/rust/optify/tests/test_provider.rs index 7bebcfe8..55a86e9b 100644 --- a/rust/optify/tests/test_provider.rs +++ b/rust/optify/tests/test_provider.rs @@ -105,7 +105,7 @@ fn test_provider_get_options_missing_key() -> Result<(), Box