diff --git a/Cargo.lock b/Cargo.lock index a2adc755c..ee79dacec 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2057,7 +2057,6 @@ dependencies = [ "anyhow", "bigdecimal", "blake3", - "cac_client", "chrono", "fred", "inventory", diff --git a/crates/context_aware_config/Cargo.toml b/crates/context_aware_config/Cargo.toml index aa349df59..ad2ac6956 100644 --- a/crates/context_aware_config/Cargo.toml +++ b/crates/context_aware_config/Cargo.toml @@ -13,7 +13,6 @@ actix-web = { workspace = true } anyhow = { workspace = true } bigdecimal = { workspace = true } blake3 = { workspace = true } -cac_client = { path = "../cac_client" } chrono = { workspace = true } diesel = { workspace = true, features = ["numeric"] } fred = { workspace = true, features = ["metrics"] } diff --git a/crates/context_aware_config/src/api/config/helpers.rs b/crates/context_aware_config/src/api/config/helpers.rs index 39b92fb85..b247fef43 100644 --- a/crates/context_aware_config/src/api/config/helpers.rs +++ b/crates/context_aware_config/src/api/config/helpers.rs @@ -4,7 +4,6 @@ use actix_web::{ HttpRequest, web::{Data, Header, Json}, }; -use cac_client::eval_cac; use chrono::{DateTime, Utc}; use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl, dsl::max}; use serde_json::{Map, Value}; @@ -16,9 +15,10 @@ use service_utils::{ }, service::types::{AppState, EncryptionKey, SchemaName, WorkspaceContext}, }; +use superposition_core::{eval_config, eval_with_key_sources}; use superposition_macros::{bad_argument, db_error, unexpected_error}; use superposition_types::{ - Condition, Config, ConfigFilter, DBConnection, PrefixList, + Config, ConfigFilter, Context, DBConnection, PrefixList, api::config::{ ContextPayload, DetailedResolvedConfigValue, DetailedResolvedConfiguration, ExplainKeyQuery, ExplainResolveQuery, Explanation, ExplanationTimelineItem, @@ -295,7 +295,7 @@ fn evaluate_resolved_config( merge_strategy: MergeStrategy, _show_reason: bool, // TODO: Remove this ) -> Map { - eval_cac(config, query_data.into_inner(), merge_strategy) + eval_config(config, query_data.into_inner(), merge_strategy, None, None) } fn fetch_default_config_metadata( @@ -338,33 +338,10 @@ fn fetch_default_config_metadata( .collect()) } -/// Condition and id of the last matching context that set each key. A key absent from the -/// map was never overridden, so its value came from the default config. -fn resolve_key_sources( - config: &Config, - context_data: &Map, -) -> HashMap { - let mut sources = HashMap::new(); - for context in config.contexts.iter() { - if !superposition_types::apply(&context.condition, context_data) { - continue; - } - let override_id = context.override_with_keys.get_key(); - let Some(overrides) = config.overrides.get(override_id) else { - continue; - }; - let condition = context.condition.clone(); - for key in overrides.keys() { - sources.insert(key.clone(), (condition.clone(), context.id.clone())); - } - } - sources -} - fn build_resolved_config( resolved_config: Map, metadata: &HashMap, - sources: &HashMap, + sources: &HashMap, ) -> DetailedResolvedConfiguration { resolved_config .into_iter() @@ -381,8 +358,8 @@ fn build_resolved_config( description, schema: schema.cloned(), value, - context: source.map(|(condition, _)| condition.clone()), - context_id: source.map(|(_, id)| id.clone()), + context: source.map(|context| context.condition.clone()), + context_id: source.map(|context| context.id.clone()), }, ) }) @@ -429,7 +406,6 @@ pub fn resolve_detailed( master_encryption_key: &Option, ) -> superposition::Result { let resolve_remote = resolve_options.resolve_remote; - let show_reason = resolve_options.show_reasoning.unwrap_or_default(); let resolution_config = apply_resolution_filters(config, resolve_options)?; let context_data = prepare_remote_query_data( @@ -442,17 +418,22 @@ pub fn resolve_detailed( )?; let merge_strategy = merge_strategy.into_inner(); - let evaluated_context = evaluate_local_cohorts( - &resolution_config.dimensions, - context_data.clone().into_inner(), - ); - let sources = resolve_key_sources(&resolution_config, &evaluated_context); - - let resolved_config = evaluate_resolved_config( - resolution_config, - context_data, + // a single pass yields both the values and the context that set each key + let Config { + default_configs, + contexts, + overrides, + dimensions, + } = resolution_config; + let (resolved_config, sources) = eval_with_key_sources( + default_configs, + &contexts, + &overrides, + &dimensions, + context_data.into_inner(), merge_strategy, - show_reason, + None, + None, ); let keys = resolved_config.keys().cloned().collect::>(); let metadata = diff --git a/crates/experimentation_platform/src/api/experiments/handlers.rs b/crates/experimentation_platform/src/api/experiments/handlers.rs index 42b420cab..19700b772 100644 --- a/crates/experimentation_platform/src/api/experiments/handlers.rs +++ b/crates/experimentation_platform/src/api/experiments/handlers.rs @@ -1031,10 +1031,14 @@ async fn get_applicable_variants_handler( (dimension_params.into_inner(), identifier) } else { let req_body = req_body.map(|body| body.into_inner()); - let (context, identifier) = req_body.map_or_else( - || (QueryMap::default(), String::default()), + let (context, body_identifier) = req_body.map_or_else( + || (QueryMap::default(), None), |body| (body.context.into(), body.identifier), ); + // the SDK sends the identifier as a query param, older clients in the body + let identifier = body_identifier + .or(query_data.identifier) + .unwrap_or_default(); (context, identifier) }; diff --git a/crates/superposition_core/src/config.rs b/crates/superposition_core/src/config.rs index 473415126..e0a0748a8 100644 --- a/crates/superposition_core/src/config.rs +++ b/crates/superposition_core/src/config.rs @@ -1,7 +1,10 @@ #[cfg(test)] mod tests; -use std::{borrow::Cow, collections::HashMap}; +use std::{ + borrow::Cow, + collections::{HashMap, HashSet}, +}; use serde_json::{json, Map, Value}; pub use superposition_types::api::config::MergeStrategy; @@ -101,6 +104,93 @@ pub fn eval( filter_prefixes: Option>, filter_exclude_prefixes: Option>, ) -> Map { + eval_inner( + default_configs, + contexts, + overrides, + dimensions, + query_data, + merge_strategy, + filter_prefixes, + filter_exclude_prefixes, + false, + ) + .0 +} + +/// Same as [`eval`], but also returns, for every resolved key that a context +/// overrode, the context that set it last and so decided its value. Keys left at +/// their default value have no entry. +/// +/// Both come out of the same pass over the contexts, so this costs one evaluation. +#[allow(clippy::too_many_arguments)] +pub fn eval_with_key_sources<'a>( + default_configs: ExtendedMap, + contexts: &'a [Context], + overrides: &'a HashMap, + dimensions: &'a HashMap, + query_data: Map, + merge_strategy: MergeStrategy, + filter_prefixes: Option>, + filter_exclude_prefixes: Option>, +) -> (Map, HashMap) { + eval_inner( + default_configs, + contexts, + overrides, + dimensions, + query_data, + merge_strategy, + filter_prefixes, + filter_exclude_prefixes, + true, + ) +} + +/// Same as [`eval`], but also returns the variants that actually took effect. +/// +/// `query_data["variantIds"]` lists every variant the caller is bucketed into, but a +/// variant whose context lost every key to a later context never reaches the resolved +/// config. Only the variants whose context is the source of at least one resolved key +/// (see [`eval_with_key_sources`]) are returned, in the order they appear in +/// `query_data["variantIds"]`. +#[allow(clippy::too_many_arguments)] +pub fn eval_with_applied_variants( + default_configs: ExtendedMap, + contexts: &[Context], + overrides: &HashMap, + dimensions: &HashMap, + query_data: Map, + merge_strategy: MergeStrategy, + filter_prefixes: Option>, + filter_exclude_prefixes: Option>, +) -> (Map, Vec) { + let variant_ids = query_data.get("variantIds").cloned(); + let (config, key_sources) = eval_with_key_sources( + default_configs, + contexts, + overrides, + dimensions, + query_data, + merge_strategy, + filter_prefixes, + filter_exclude_prefixes, + ); + (config, applied_variant_ids(variant_ids, &key_sources)) +} + +#[allow(clippy::too_many_arguments)] +fn eval_inner<'a>( + default_configs: ExtendedMap, + contexts: &'a [Context], + overrides: &'a HashMap, + dimensions: &'a HashMap, + query_data: Map, + merge_strategy: MergeStrategy, + filter_prefixes: Option>, + filter_exclude_prefixes: Option>, + track_sources: bool, +) -> (Map, HashMap) { // Create Config struct to use existing filtering logic let mut config = ConfigRef { default_configs, @@ -119,13 +209,19 @@ pub fn eval( let modified_query_data = evaluate_local_cohorts(config.dimensions, query_data); - let overrides_map: Map = get_overrides( + let mut key_sources = HashMap::new(); + let overrides_map: Map = merge_matching_overrides( &modified_query_data, config.contexts, config.overrides, &merge_strategy, + track_sources.then_some(&mut key_sources), ); + // overrides for keys missing from the default config are dropped while applying, + // so those keys have no source + key_sources.retain(|key, _| config.default_configs.contains_key(key)); + // Apply overrides to default config let result = apply_overrides_on_default_config( config.default_configs, @@ -133,7 +229,30 @@ pub fn eval( merge_strategy, ); - result.into_inner() + (result.into_inner(), key_sources) +} + +/// The ids in `variant_ids` whose context is the source of at least one key. +fn applied_variant_ids( + variant_ids: Option, + key_sources: &HashMap, +) -> Vec { + let applied: HashSet<&str> = key_sources + .values() + .filter_map(|context| context.condition.get("variantIds")) + .filter_map(Value::as_str) + .collect(); + + match variant_ids { + Some(Value::Array(ids)) => ids + .into_iter() + .filter_map(|id| match id { + Value::String(id) if applied.contains(id.as_str()) => Some(id), + _ => None, + }) + .collect(), + _ => Vec::new(), + } } // TODO: Move explain function to this file to avoid making this funcition public @@ -157,9 +276,21 @@ pub fn merge(doc: &mut Value, patch: Value) { /// contexts moves it out of `overrides` rather than cloning. fn get_overrides( query_data: &Map, - mut contexts: Vec<&Context>, + contexts: Vec<&Context>, + overrides: Cow<'_, HashMap>, + merge_strategy: &MergeStrategy, +) -> Map { + merge_matching_overrides(query_data, contexts, overrides, merge_strategy, None) +} + +/// [`get_overrides`], optionally recording in `key_sources` the context that last +/// wrote each key. +fn merge_matching_overrides<'c>( + query_data: &Map, + mut contexts: Vec<&'c Context>, mut overrides: Cow<'_, HashMap>, merge_strategy: &MergeStrategy, + mut key_sources: Option<&mut HashMap>, ) -> Map { // borrow the keys instead of allocating two Strings per matching context let mut final_consumer_context: HashMap<&str, &str> = HashMap::new(); @@ -192,6 +323,12 @@ fn get_overrides( continue; }; + if let Some(sources) = key_sources.as_deref_mut() { + for key in overriden_value.keys() { + sources.insert(key.clone(), context); + } + } + match merge_strategy { MergeStrategy::REPLACE => { if let Some(doc) = required_overrides.as_object_mut() { diff --git a/crates/superposition_core/src/config/tests.rs b/crates/superposition_core/src/config/tests.rs index 47ed61279..6b72e938e 100644 --- a/crates/superposition_core/src/config/tests.rs +++ b/crates/superposition_core/src/config/tests.rs @@ -897,6 +897,204 @@ fn eval_with_empty_config_returns_empty_map() { assert_eq!(resolved, Map::new()); } +// --------------------------------------------------------------------------- +// eval_with_applied_variants +// --------------------------------------------------------------------------- + +fn eval_variants( + defaults: ExtendedMap, + contexts: &[Context], + all_overrides: &HashMap, + query_data: Map, + filter_prefixes: Option>, +) -> (Map, Vec) { + eval_with_applied_variants( + defaults, + contexts, + all_overrides, + &HashMap::new(), + query_data, + MergeStrategy::REPLACE, + filter_prefixes, + None, + ) +} + +#[test] +fn applied_variants_are_the_last_writers_of_resolved_keys() { + let defaults = default_configs(vec![ + ("price", json!(100)), + ("color", json!("red")), + ("banner", json!(false)), + ]); + let contexts = [ + context("c0", vec![("variantIds", json!("v1"))], "o0"), + context("c1", vec![("variantIds", json!("v2"))], "o1"), + context("c2", vec![("variantIds", json!("v3"))], "o2"), + context("c3", vec![("city", json!("BLR"))], "o3"), + ]; + let all_overrides = overrides_map(vec![ + // v1 loses `price` to v2 but keeps `color`, so it still applied + ("o0", vec![("price", json!(200)), ("color", json!("blue"))]), + ("o1", vec![("price", json!(300))]), + // v3 loses its only key to the normal context after it + ("o2", vec![("banner", json!(true))]), + ("o3", vec![("banner", json!(false))]), + ]); + let query = value_map(vec![ + ("city", json!("BLR")), + ("variantIds", json!(["v3", "v2", "v1"])), + ]); + + let (resolved, applied) = eval_variants( + defaults.clone(), + &contexts, + &all_overrides, + query.clone(), + None, + ); + + assert_eq!(applied, vec!["v2", "v1"], "ordered as in the query"); + assert_eq!( + resolved, + value_map(vec![ + ("price", json!(300)), + ("color", json!("blue")), + ("banner", json!(false)), + ]) + ); + assert_eq!( + resolved, + eval( + defaults, + &contexts, + &all_overrides, + &HashMap::new(), + query, + MergeStrategy::REPLACE, + None, + None, + ), + "must resolve the same config as eval" + ); +} + +#[test] +fn key_sources_point_at_the_context_that_set_each_key_last() { + let defaults = default_configs(vec![ + ("price", json!(100)), + ("color", json!("red")), + ("banner", json!(false)), + ]); + let contexts = [ + context("c0", vec![("city", json!("BLR"))], "o0"), + context( + "c1", + vec![("city", json!("BLR")), ("os", json!("ios"))], + "o1", + ), + ]; + let all_overrides = overrides_map(vec![ + ("o0", vec![("price", json!(200)), ("color", json!("blue"))]), + ("o1", vec![("price", json!(300)), ("unknown", json!(1))]), + ]); + let query = value_map(vec![("city", json!("BLR")), ("os", json!("ios"))]); + let dimensions = HashMap::new(); + + let (resolved, sources) = eval_with_key_sources( + defaults.clone(), + &contexts, + &all_overrides, + &dimensions, + query.clone(), + MergeStrategy::REPLACE, + None, + None, + ); + + let source_ids: HashMap<&str, &str> = sources + .iter() + .map(|(key, context)| (key.as_str(), context.id.as_str())) + .collect(); + // `banner` kept its default and `unknown` is not a default config key, so + // neither has a source + assert_eq!( + source_ids, + HashMap::from([("price", "c1"), ("color", "c0")]) + ); + assert_eq!( + resolved, + eval( + defaults, + &contexts, + &all_overrides, + &HashMap::new(), + query, + MergeStrategy::REPLACE, + None, + None, + ) + ); +} + +#[test] +fn applied_variants_skip_keys_missing_from_default_config() { + let contexts = [context("c0", vec![("variantIds", json!("v1"))], "o0")]; + let all_overrides = overrides_map(vec![("o0", vec![("unknown", json!(1))])]); + + let (_, applied) = eval_variants( + default_configs(vec![("price", json!(100))]), + &contexts, + &all_overrides, + value_map(vec![("variantIds", json!(["v1"]))]), + None, + ); + + assert!(applied.is_empty(), "got {applied:?}"); +} + +#[test] +fn applied_variants_only_cover_keys_left_by_the_prefix_filter() { + let contexts = [ + context("c0", vec![("variantIds", json!("v1"))], "o0"), + context("c1", vec![("variantIds", json!("v2"))], "o1"), + ]; + let all_overrides = overrides_map(vec![ + ("o0", vec![("checkout.enabled", json!(true))]), + ("o1", vec![("search.enabled", json!(true))]), + ]); + + let (_, applied) = eval_variants( + default_configs(vec![ + ("checkout.enabled", json!(false)), + ("search.enabled", json!(false)), + ]), + &contexts, + &all_overrides, + value_map(vec![("variantIds", json!(["v1", "v2"]))]), + Some(vec!["checkout.".to_string()]), + ); + + assert_eq!(applied, vec!["v1"]); +} + +#[test] +fn applied_variants_are_empty_without_variant_ids_in_query() { + let contexts = [context("c0", vec![("city", json!("BLR"))], "o0")]; + let all_overrides = overrides_map(vec![("o0", vec![("price", json!(1))])]); + + let (resolved, applied) = eval_variants( + default_configs(vec![("price", json!(100))]), + &contexts, + &all_overrides, + value_map(vec![("city", json!("BLR"))]), + None, + ); + + assert!(applied.is_empty()); + assert_eq!(resolved, value_map(vec![("price", json!(1))])); +} + // --------------------------------------------------------------------------- // ConfigRef // --------------------------------------------------------------------------- diff --git a/crates/superposition_core/src/lib.rs b/crates/superposition_core/src/lib.rs index e6f00638b..f439b4c61 100644 --- a/crates/superposition_core/src/lib.rs +++ b/crates/superposition_core/src/lib.rs @@ -13,7 +13,10 @@ pub mod format; pub mod helpers; pub mod validations; // Re-export main config functions -pub use config::{eval, eval_config, merge, MergeStrategy}; +pub use config::{ + eval, eval_config, eval_with_applied_variants, eval_with_key_sources, merge, + MergeStrategy, +}; // Re-export experiment functions pub use experiment::{ diff --git a/crates/superposition_provider/src/conversions.rs b/crates/superposition_provider/src/conversions.rs index 2bcc1fe58..e5aa32902 100644 --- a/crates/superposition_provider/src/conversions.rs +++ b/crates/superposition_provider/src/conversions.rs @@ -99,6 +99,17 @@ pub fn evaluation_context_to_value(value: EvaluationContextFieldValue) -> Value EvaluationContextFieldValue::String(s) => Value::String(s), EvaluationContextFieldValue::DateTime(dt) => Value::String(dt.to_string()), EvaluationContextFieldValue::Struct(s) => { + // A struct field is `dyn Any`, so the concrete type has to be recovered by + // downcast. A plain `serde_json::Value` is accepted as-is, which is the only + // way a caller can pass an array (the enum has no Array variant), and + // `Vec` is accepted as the common case of that. + if let Some(value) = s.downcast_ref::() { + return value.clone(); + } + if let Some(list) = s.downcast_ref::>() { + return Value::Array(list.iter().cloned().map(Value::String).collect()); + } + // Convert struct to serde_json::Value let struct_map = s .downcast_ref::>() @@ -324,3 +335,57 @@ mod tests { assert!(value_to_struct(json!(10)).is_err()); } } + +#[cfg(test)] +mod conversion_tests { + use super::*; + use std::sync::Arc; + + #[test] + fn a_json_value_struct_field_survives_conversion() { + let arr = + Value::Array(vec![Value::String("v1".into()), Value::String("v2".into())]); + let field = EvaluationContextFieldValue::Struct(Arc::new(arr.clone())); + assert_eq!(evaluation_context_to_value(field), arr); + } + + #[test] + fn a_vec_of_strings_becomes_a_json_array() { + let field = EvaluationContextFieldValue::Struct(Arc::new(vec![ + "v1".to_string(), + "v2".to_string(), + ])); + assert_eq!( + evaluation_context_to_value(field), + Value::Array(vec![Value::String("v1".into()), Value::String("v2".into())]) + ); + } + + #[test] + fn a_map_struct_still_converts_as_before() { + let mut m = HashMap::new(); + m.insert( + "k".to_string(), + EvaluationContextFieldValue::String("v".into()), + ); + let field = EvaluationContextFieldValue::Struct(Arc::new(m)); + let out = evaluation_context_to_value(field); + assert_eq!(out.get("k"), Some(&Value::String("v".into()))); + } + + #[test] + fn variant_ids_reach_the_query_map_as_an_array() { + let ctx = EvaluationContext::default() + .with_custom_field("shopId", EvaluationContextFieldValue::String("s1".into())) + .with_custom_field( + "variantIds", + EvaluationContextFieldValue::Struct(Arc::new(vec!["exp-1".to_string()])), + ); + let (query, targeting) = evaluation_context_to_query(ctx); + assert_eq!(targeting, None); + assert_eq!( + query.get("variantIds"), + Some(&Value::Array(vec![Value::String("exp-1".into())])) + ); + } +} diff --git a/crates/superposition_provider/src/local_provider.rs b/crates/superposition_provider/src/local_provider.rs index 207ace211..5e795653a 100644 --- a/crates/superposition_provider/src/local_provider.rs +++ b/crates/superposition_provider/src/local_provider.rs @@ -11,7 +11,8 @@ use open_feature::{EvaluationContext, EvaluationResult, StructValue}; use serde_json::{Map, Value}; use superposition_core::experiment::{filter_experiments_by_context, FfiExperimentGroup}; use superposition_core::{ - eval, get_applicable_variants, get_satisfied_experiments, MergeStrategy, + eval, eval_with_applied_variants, get_applicable_variants, get_satisfied_experiments, + MergeStrategy, }; use superposition_types::experimental::Experimental; use superposition_types::{ConfigFilter, DimensionInfo, PrefixList}; @@ -531,22 +532,29 @@ impl LocalResolutionProvider { context: EvaluationContext, prefix_filter: Option>, exclude_prefix_filter: Option>, - ) -> Result> { + track_variants: bool, + ) -> Result { self.ensure_fresh_data().await?; let (mut query_data, targeting_key) = self.get_merged_context(context).await; let dimensions_info = self.get_dimensions_info().await; - // If experiments are cached, get applicable variants and inject variantIds + // Variants are only computed when there is an identifier to evaluate them + // against and experiments are cached. Otherwise a `variantIds` already on the + // context is the caller's own assignment and is left untouched — injecting here + // would overwrite it with an empty list. { let cached_exp = self.cached_experiments.read().await; - if let Some(exp_data) = cached_exp.as_ref() { + if let (Some(key), Some(exp_data)) = ( + targeting_key.filter(|key| !key.is_empty()), + cached_exp.as_ref(), + ) { let variant_ids = get_applicable_variants( &dimensions_info, exp_data.data.experiments.clone(), &exp_data.data.experiment_groups, query_data.clone(), - &targeting_key.unwrap_or_default(), + &key, prefix_filter.clone(), exclude_prefix_filter.clone(), ); @@ -560,21 +568,42 @@ impl LocalResolutionProvider { // Evaluate config using cached data let cached = self.cached_config.read().await; - match cached.as_ref() { - Some(config_data) => Ok(eval( - config_data.data.default_configs.clone(), - &config_data.data.contexts, - &config_data.data.overrides, - &config_data.data.dimensions, + let Some(config_data) = cached.as_ref() else { + return Err(SuperpositionError::ProviderError( + "Provider not initialized: no cached config available".into(), + )); + }; + let data = &config_data.data; + + // Report only the variants that took effect, not every applicable one: a + // variant whose keys were all overridden by later contexts was never served. + if track_variants { + let (value, variant_ids) = eval_with_applied_variants( + data.default_configs.clone(), + &data.contexts, + &data.overrides, + &data.dimensions, query_data, - MergeStrategy::MERGE, + MergeStrategy::REPLACE, prefix_filter, exclude_prefix_filter, - )), - None => Err(SuperpositionError::ProviderError( - "Provider not initialized: no cached config available".into(), - )), + ); + return Ok(AllFeaturesResolutionDetails { value, variant_ids }); } + + Ok(AllFeaturesResolutionDetails { + value: eval( + data.default_configs.clone(), + &data.contexts, + &data.overrides, + &data.dimensions, + query_data, + MergeStrategy::REPLACE, + prefix_filter, + exclude_prefix_filter, + ), + variant_ids: Vec::new(), + }) } } @@ -586,7 +615,18 @@ impl AllFeatureProvider for LocalResolutionProvider { prefix_filter: Option>, exclude_prefix_filter: Option>, ) -> Result> { - self.eval_with_context(context, prefix_filter, exclude_prefix_filter) + self.eval_with_context(context, prefix_filter, exclude_prefix_filter, false) + .await + .map(|details| details.value) + } + + async fn resolve_all_features_with_filter_details( + &self, + context: EvaluationContext, + prefix_filter: Option>, + exclude_prefix_filter: Option>, + ) -> Result { + self.eval_with_context(context, prefix_filter, exclude_prefix_filter, true) .await } } diff --git a/crates/superposition_provider/src/remote_provider.rs b/crates/superposition_provider/src/remote_provider.rs index e0f3c7f81..70450829a 100644 --- a/crates/superposition_provider/src/remote_provider.rs +++ b/crates/superposition_provider/src/remote_provider.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use async_trait::async_trait; use aws_smithy_types::Document; @@ -48,21 +48,20 @@ impl SuperpositionAPIProvider { async fn resolve_remote( &self, - context: EvaluationContext, + query_data: HashMap, + identifier: Option, prefix_filter: Option>, exclude_prefix_filter: Option>, ) -> Result> { // TODO: Check if we need to add a separte check to verify the status of provider before doing stuff - let (query_data, targeting_key) = self.get_merged_context(context).await; - let response = self .client .get_resolved_config_with_identifier() .workspace_id(&self.options.workspace_id) .org_id(&self.options.org_id) .set_context(Some(query_data)) - .set_identifier(targeting_key) + .set_identifier(identifier) .set_prefix(prefix_filter) .set_exclude_prefix(exclude_prefix_filter) .send() @@ -89,38 +88,53 @@ impl SuperpositionAPIProvider { Ok(result) } -} -#[async_trait] -impl AllFeatureProvider for SuperpositionAPIProvider { - async fn resolve_all_features_with_filter( + /// Resolves through `/config/resolve/detailed`, whose entries carry the condition of + /// the context that last set each key alongside its value. + async fn resolve_remote_detailed( &self, - context: EvaluationContext, + query_data: HashMap, prefix_filter: Option>, exclude_prefix_filter: Option>, ) -> Result> { - self.resolve_remote(context, prefix_filter, exclude_prefix_filter) + let response = self + .client + .get_detailed_resolved_config() + .workspace_id(&self.options.workspace_id) + .org_id(&self.options.org_id) + .set_context(Some(query_data)) + .set_prefix(prefix_filter) + .set_exclude_prefix(exclude_prefix_filter) + .send() .await + .map_err(|e| { + SuperpositionError::NetworkError(format!( + "Failed to get detailed resolved config: {e}" + )) + })?; + + match conversions::document_to_value(response.config) { + Value::Object(map) => Ok(map), + other => Err(SuperpositionError::SerializationError(format!( + "Detailed resolved config is not an object: {other:?}" + ))), + } } -} -#[async_trait] -impl FeatureExperimentMeta for SuperpositionAPIProvider { - async fn get_applicable_variants( + async fn fetch_applicable_variants( &self, - context: EvaluationContext, + query_data: HashMap, + identifier: String, prefix_filter: Option>, exclude_prefix_filter: Option>, ) -> Result> { - let (query_data, targeting_key) = self.get_merged_context(context).await; - let applicable_variants = self .client .applicable_variants() .workspace_id(&self.options.workspace_id) .org_id(&self.options.org_id) .set_context(Some(query_data)) - .identifier(targeting_key.unwrap_or_default()) + .identifier(identifier) .set_prefix(prefix_filter) .set_exclude_prefix(exclude_prefix_filter) .send() @@ -135,6 +149,116 @@ impl FeatureExperimentMeta for SuperpositionAPIProvider { } } +#[async_trait] +impl AllFeatureProvider for SuperpositionAPIProvider { + async fn resolve_all_features_with_filter( + &self, + context: EvaluationContext, + prefix_filter: Option>, + exclude_prefix_filter: Option>, + ) -> Result> { + let (query_data, targeting_key) = self.get_merged_context(context).await; + self.resolve_remote( + query_data, + targeting_key, + prefix_filter, + exclude_prefix_filter, + ) + .await + } + + async fn resolve_all_features_with_filter_details( + &self, + context: EvaluationContext, + prefix_filter: Option>, + exclude_prefix_filter: Option>, + ) -> Result { + let (mut query_data, targeting_key) = self.get_merged_context(context).await; + + // Without an identifier, a `variantIds` already on the context is the caller's + // own assignment and is resolved as given. + if let Some(identifier) = targeting_key { + let variant_ids = self + .fetch_applicable_variants( + query_data.clone(), + identifier, + prefix_filter.clone(), + exclude_prefix_filter.clone(), + ) + .await?; + query_data.insert( + "variantIds".to_string(), + Document::Array(variant_ids.into_iter().map(Document::String).collect()), + ); + } + let variant_ids: Vec = match query_data.get("variantIds") { + Some(Document::Array(ids)) => ids + .iter() + .filter_map(|id| match id { + Document::String(id) => Some(id.clone()), + _ => None, + }) + .collect(), + _ => Vec::new(), + }; + + let detailed = self + .resolve_remote_detailed(query_data, prefix_filter, exclude_prefix_filter) + .await?; + + // Report only the variants that took effect, not every applicable one: the + // detailed response names the context that last set each key, so a variant + // whose keys were all overridden by later contexts never shows up there. + let mut applied = HashSet::new(); + let value = detailed + .into_iter() + .map(|(key, entry)| { + let variant_id = entry + .get("context") + .and_then(|condition| condition.get("variantIds")) + .and_then(Value::as_str); + if let Some(variant_id) = variant_id { + applied.insert(variant_id.to_string()); + } + let value = match entry { + Value::Object(mut entry) => { + entry.remove("value").unwrap_or(Value::Null) + } + other => other, + }; + (key, value) + }) + .collect(); + + Ok(AllFeaturesResolutionDetails { + value, + variant_ids: variant_ids + .into_iter() + .filter(|id| applied.contains(id)) + .collect(), + }) + } +} + +#[async_trait] +impl FeatureExperimentMeta for SuperpositionAPIProvider { + async fn get_applicable_variants( + &self, + context: EvaluationContext, + prefix_filter: Option>, + exclude_prefix_filter: Option>, + ) -> Result> { + let (query_data, targeting_key) = self.get_merged_context(context).await; + self.fetch_applicable_variants( + query_data, + targeting_key.unwrap_or_default(), + prefix_filter, + exclude_prefix_filter, + ) + .await + } +} + #[async_trait] impl FeatureProvider for SuperpositionAPIProvider { async fn initialize(&mut self, context: &EvaluationContext) { diff --git a/crates/superposition_provider/src/traits.rs b/crates/superposition_provider/src/traits.rs index 1518143a9..04b8d8713 100644 --- a/crates/superposition_provider/src/traits.rs +++ b/crates/superposition_provider/src/traits.rs @@ -5,7 +5,10 @@ use open_feature::{ }; use serde_json::{Map, Value}; -use crate::{conversions, types::Result}; +use crate::{ + conversions, + types::{AllFeaturesResolutionDetails, Result}, +}; /// Trait for experiment variant resolution. /// @@ -46,6 +49,21 @@ pub trait AllFeatureProvider: Send + Sync { exclude_prefix_filter: Option>, ) -> Result>; + async fn resolve_all_features_details( + &self, + context: EvaluationContext, + ) -> Result { + self.resolve_all_features_with_filter_details(context, None, None) + .await + } + + async fn resolve_all_features_with_filter_details( + &self, + context: EvaluationContext, + prefix_filter: Option>, + exclude_prefix_filter: Option>, + ) -> Result; + /// Resolve a flag and extract it as `T`. /// /// Error reasons need no handling here: the failure paths return `EvaluationError`, which has diff --git a/crates/superposition_provider/src/types.rs b/crates/superposition_provider/src/types.rs index 8b41cc2b4..6e015d6f8 100644 --- a/crates/superposition_provider/src/types.rs +++ b/crates/superposition_provider/src/types.rs @@ -317,6 +317,12 @@ impl SuperpositionProviderOptions { } } +#[derive(Debug, Clone, Default, PartialEq)] +pub struct AllFeaturesResolutionDetails { + pub value: serde_json::Map, + pub variant_ids: Vec, +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/superposition_provider/tests/integration_test.rs b/crates/superposition_provider/tests/integration_test.rs index 0b8156716..31d98f29a 100644 --- a/crates/superposition_provider/tests/integration_test.rs +++ b/crates/superposition_provider/tests/integration_test.rs @@ -375,6 +375,76 @@ async fn setup_with_sdk(org_id: &str, workspace_id: &str) { println!("\n=== Setup complete ===\n"); } +async fn check_resolution_details(provider: &impl AllFeatureProvider) { + println!("Test: resolve_all_features_details reports applied variants"); + + let ctx = EvaluationContext::default().with_custom_field("city", "Kolkata"); + let details = provider + .resolve_all_features_details(ctx.clone()) + .await + .unwrap(); + assert!( + details.variant_ids.is_empty(), + "No variants should apply without a targeting key, got {:?}", + details.variant_ids + ); + assert_eq!( + details.value, + provider.resolve_all_features(ctx).await.unwrap(), + "Details should carry the same config as resolve_all_features" + ); + + let ctx = EvaluationContext::default() + .with_custom_field("city", "Kolkata") + .with_targeting_key("test"); + let details = provider + .resolve_all_features_details(ctx.clone()) + .await + .unwrap(); + println!(" Applied variants: {:?}", details.variant_ids); + let [variant_id] = details.variant_ids.as_slice() else { + panic!( + "Exactly one Kolkata experiment variant should apply, got {:?}", + details.variant_ids + ); + }; + let expected_price = if variant_id.ends_with("-control") { + 8000.0 + } else { + assert!( + variant_id.ends_with("-Experimental"), + "Unexpected variant id {variant_id}" + ); + 7000.0 + }; + assert_eq!( + details.value.get("price").and_then(Value::as_f64), + Some(expected_price), + "Price should come from the reported variant {variant_id}" + ); + assert_eq!( + details.value, + provider.resolve_all_features(ctx.clone()).await.unwrap(), + "Details should carry the same config as resolve_all_features" + ); + + let filtered = provider + .resolve_all_features_with_filter_details( + ctx, + Some(vec!["price".to_string()]), + None, + ) + .await + .unwrap(); + assert_eq!( + filtered.value.keys().collect::>(), + vec!["price"], + "Only price should survive the prefix filter" + ); + assert_eq!(filtered.variant_ids, details.variant_ids); + println!(" ✓ Test passed\n"); +} + async fn run_provider_tests(org_id: &str, workspace_id: &str) { println!("\n=== Starting OpenFeature provider tests ===\n"); @@ -424,6 +494,8 @@ async fn run_provider_tests(org_id: &str, workspace_id: &str) { "Currency should be default Rupee" ); println!(" ✓ Test passed\n"); + + check_resolution_details(&provider_clone).await; } // Set provider as the global provider @@ -614,6 +686,8 @@ async fn run_provider_tests(org_id: &str, workspace_id: &str) { } { println!("Testing SuperpositionAPIProvider"); + check_resolution_details(&SuperpositionAPIProvider::new(http_options.clone())) + .await; let provider = SuperpositionAPIProvider::new(http_options); // Set provider as the global provider let mut api = OpenFeature::singleton_mut().await; diff --git a/crates/superposition_types/src/api/experiments.rs b/crates/superposition_types/src/api/experiments.rs index b956f4816..14f1c837f 100644 --- a/crates/superposition_types/src/api/experiments.rs +++ b/crates/superposition_types/src/api/experiments.rs @@ -187,18 +187,20 @@ pub struct ApplicableVariantsQuery { #[derive(Debug, Serialize, Deserialize)] pub struct ApplicableVariantsRequest { pub context: Map, - // TODO: remove this once services start using newer sdk - #[serde(alias = "toss", deserialize_with = "deserialize_identifier")] - pub identifier: String, + // TODO: remove this once services start using newer sdk, which sends the + // identifier as the `identifier` query param instead + #[serde(default, alias = "toss", deserialize_with = "deserialize_identifier")] + pub identifier: Option, } -fn deserialize_identifier<'de, D>(deserializer: D) -> Result +fn deserialize_identifier<'de, D>(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, { match Value::deserialize(deserializer)? { - Value::Number(toss) => Ok(toss.to_string()), - Value::String(identifier) => Ok(identifier), + Value::Null => Ok(None), + Value::Number(toss) => Ok(Some(toss.to_string())), + Value::String(identifier) => Ok(Some(identifier)), _ => Err(serde::de::Error::custom("identifier must be a string")), } }