From c940fd52298b70f69de0001c823945f33a39d2df Mon Sep 17 00:00:00 2001 From: Dylan Martin Date: Tue, 24 Sep 2024 19:06:22 -0400 Subject: [PATCH] feat(flags): rust implementation of a bunch of things (#24957) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- .gitignore | 4 +- rust/Cargo.lock | 33 +- rust/feature-flags/Cargo.toml | 1 + .../src/feature_flag_match_reason.rs | 101 ++ rust/feature-flags/src/flag_definitions.rs | 8 + rust/feature-flags/src/flag_matching.rs | 1497 +++++++++++++---- rust/feature-flags/src/lib.rs | 1 + rust/feature-flags/src/property_matching.rs | 9 +- .../tests/test_flag_matching_consistency.rs | 17 +- 9 files changed, 1309 insertions(+), 362 deletions(-) create mode 100644 rust/feature-flags/src/feature_flag_match_reason.rs diff --git a/.gitignore b/.gitignore index c3c2c8c374e96..a65e04a785832 100644 --- a/.gitignore +++ b/.gitignore @@ -66,4 +66,6 @@ plugin-transpiler/dist .dlt *.db # Ignore any log files that happen to be present -*.log \ No newline at end of file +*.log +# pyright config (keep this until we have a standardized one) +pyrightconfig.json \ No newline at end of file diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 9e14f8a558ce2..bd3e14f286803 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -471,7 +471,7 @@ version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "00c055ee2d014ae5981ce1016374e8213682aa14d9bf40e48ab48b5f3ef20eaa" dependencies = [ - "heck", + "heck 0.4.1", "proc-macro2", "quote", "syn 2.0.48", @@ -1184,6 +1184,7 @@ dependencies = [ "serde_json", "sha1", "sqlx", + "strum", "thiserror", "tokio", "tower", @@ -1553,6 +1554,12 @@ dependencies = [ "unicode-segmentation", ] +[[package]] +name = "heck" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" + [[package]] name = "hermit-abi" version = "0.3.9" @@ -3718,7 +3725,7 @@ dependencies = [ "atomic-write-file", "dotenvy", "either", - "heck", + "heck 0.4.1", "hex", "once_cell", "proc-macro2", @@ -3870,6 +3877,28 @@ dependencies = [ "unicode-normalization", ] +[[package]] +name = "strum" +version = "0.26.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8fec0f0aef304996cf250b31b5a10dee7980c85da9d759361292b8bca5a18f06" +dependencies = [ + "strum_macros", +] + +[[package]] +name = "strum_macros" +version = "0.26.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c6bee85a5a24955dc440386795aa378cd9cf82acd5f764469152d2270e581be" +dependencies = [ + "heck 0.5.0", + "proc-macro2", + "quote", + "rustversion", + "syn 2.0.48", +] + [[package]] name = "subtle" version = "2.5.0" diff --git a/rust/feature-flags/Cargo.toml b/rust/feature-flags/Cargo.toml index c6a1982d77d35..0cf96dfc6756b 100644 --- a/rust/feature-flags/Cargo.toml +++ b/rust/feature-flags/Cargo.toml @@ -34,6 +34,7 @@ uuid = { workspace = true } base64.workspace = true flate2.workspace = true common-alloc = { path = "../common/alloc" } +strum = { version = "0.26", features = ["derive"] } health = { path = "../common/health" } common-metrics = { path = "../common/metrics" } tower = { workspace = true } diff --git a/rust/feature-flags/src/feature_flag_match_reason.rs b/rust/feature-flags/src/feature_flag_match_reason.rs new file mode 100644 index 0000000000000..d6038c01ed159 --- /dev/null +++ b/rust/feature-flags/src/feature_flag_match_reason.rs @@ -0,0 +1,101 @@ +use std::cmp::Ordering; +use strum::EnumString; + +#[derive(Debug, Clone, PartialEq, Eq, EnumString)] +pub enum FeatureFlagMatchReason { + #[strum(serialize = "super_condition_value")] + SuperConditionValue, + #[strum(serialize = "condition_match")] + ConditionMatch, + #[strum(serialize = "no_condition_match")] + NoConditionMatch, + #[strum(serialize = "out_of_rollout_bound")] + OutOfRolloutBound, + #[strum(serialize = "no_group_type")] + NoGroupType, +} + +impl FeatureFlagMatchReason { + pub fn score(&self) -> i32 { + match self { + FeatureFlagMatchReason::SuperConditionValue => 4, + FeatureFlagMatchReason::ConditionMatch => 3, + FeatureFlagMatchReason::NoGroupType => 2, + FeatureFlagMatchReason::OutOfRolloutBound => 1, + FeatureFlagMatchReason::NoConditionMatch => 0, + } + } +} + +impl PartialOrd for FeatureFlagMatchReason { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for FeatureFlagMatchReason { + fn cmp(&self, other: &Self) -> Ordering { + self.score().cmp(&other.score()) + } +} + +impl std::fmt::Display for FeatureFlagMatchReason { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{}", + match self { + FeatureFlagMatchReason::SuperConditionValue => "super_condition_value", + FeatureFlagMatchReason::ConditionMatch => "condition_match", + FeatureFlagMatchReason::NoConditionMatch => "no_condition_match", + FeatureFlagMatchReason::OutOfRolloutBound => "out_of_rollout_bound", + FeatureFlagMatchReason::NoGroupType => "no_group_type", + } + ) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_ordering() { + let reasons = vec![ + FeatureFlagMatchReason::NoConditionMatch, + FeatureFlagMatchReason::OutOfRolloutBound, + FeatureFlagMatchReason::NoGroupType, + FeatureFlagMatchReason::ConditionMatch, + FeatureFlagMatchReason::SuperConditionValue, + ]; + + let mut sorted_reasons = reasons.clone(); + sorted_reasons.sort(); + + assert_eq!(sorted_reasons, reasons); + } + + #[test] + fn test_display() { + assert_eq!( + FeatureFlagMatchReason::SuperConditionValue.to_string(), + "super_condition_value" + ); + assert_eq!( + FeatureFlagMatchReason::ConditionMatch.to_string(), + "condition_match" + ); + assert_eq!( + FeatureFlagMatchReason::NoConditionMatch.to_string(), + "no_condition_match" + ); + assert_eq!( + FeatureFlagMatchReason::OutOfRolloutBound.to_string(), + "out_of_rollout_bound" + ); + assert_eq!( + FeatureFlagMatchReason::NoGroupType.to_string(), + "no_group_type" + ); + } +} diff --git a/rust/feature-flags/src/flag_definitions.rs b/rust/feature-flags/src/flag_definitions.rs index e61dabe2babbe..820d68a4d0250 100644 --- a/rust/feature-flags/src/flag_definitions.rs +++ b/rust/feature-flags/src/flag_definitions.rs @@ -110,6 +110,14 @@ impl FeatureFlag { .clone() .map_or(vec![], |m| m.variants) } + + pub fn get_payload(&self, match_val: &str) -> Option { + self.filters.payloads.as_ref().and_then(|payloads| { + payloads + .as_object() + .and_then(|obj| obj.get(match_val).cloned()) + }) + } } #[derive(Debug, Deserialize, Serialize)] diff --git a/rust/feature-flags/src/flag_matching.rs b/rust/feature-flags/src/flag_matching.rs index 4af88ed6a8770..e75580a6cbc90 100644 --- a/rust/feature-flags/src/flag_matching.rs +++ b/rust/feature-flags/src/flag_matching.rs @@ -1,6 +1,7 @@ use crate::{ api::{FlagError, FlagValue, FlagsResponse}, database::Client as DatabaseClient, + feature_flag_match_reason::FeatureFlagMatchReason, flag_definitions::{FeatureFlag, FeatureFlagList, FlagGroupType, PropertyFilter}, property_matching::match_property, }; @@ -17,10 +18,20 @@ type TeamId = i32; type DatabaseClientArc = Arc; type GroupTypeIndex = i32; +#[derive(Debug)] +struct SuperConditionEvaluation { + should_evaluate: bool, + is_match: bool, + reason: FeatureFlagMatchReason, +} + #[derive(Debug, PartialEq, Eq)] pub struct FeatureFlagMatch { pub matches: bool, pub variant: Option, + pub reason: FeatureFlagMatchReason, + pub condition_index: Option, + pub payload: Option, } #[derive(Debug, FromRow)] @@ -202,10 +213,12 @@ impl FeatureFlagMatcher { // Step 1: Evaluate flags that can be resolved with overrides for flag in &feature_flags.flags { + // Skip inactive or deleted flags if !flag.active || flag.deleted { continue; } + // Get any flag matches with overrides, assuming that these overrides can be computed locally match self .match_flag_with_overrides( flag, @@ -232,6 +245,9 @@ impl FeatureFlagMatcher { } } + // At this point, we have a list of flags that we couldn't locally evaluate (with overrides, or without), so + // we need to hit the DB to fetch the properties for these flags to continue our evaluation. + // Step 2: Fetch and cache properties for remaining flags if !flags_needing_db_properties.is_empty() { let group_type_indexes: HashSet = flags_needing_db_properties @@ -252,7 +268,8 @@ impl FeatureFlagMatcher { ) .await { - Ok(_) => {} + Ok(_) => {} // `fetch_and_locally_cache_all_properties` returns void on success, + // so at this point we know we've cached the properties, and can continue. Err(e) => { error_while_computing_flags = true; error!("Error fetching properties: {:?}", e); @@ -283,6 +300,12 @@ impl FeatureFlagMatcher { } } + /// Matches a feature flag with property overrides. + /// + /// This function attempts to match a feature flag using either group or person property overrides, + /// depending on whether the flag is group-based or person-based. It first collects all property + /// filters from the flag's conditions, then retrieves the appropriate overrides, and finally + /// attempts to match the flag using these overrides. async fn match_flag_with_overrides( &mut self, flag: &FeatureFlag, @@ -313,6 +336,11 @@ impl FeatureFlagMatcher { } } + /// Retrieves group overrides for a specific group type index. + /// + /// This function attempts to find and return property overrides for a given group type. + /// It first maps the group type index to a group type, then checks if there are any + /// overrides for that group type in the provided group property overrides. async fn get_group_overrides( &mut self, group_type_index: GroupTypeIndex, @@ -338,6 +366,11 @@ impl FeatureFlagMatcher { Ok(None) } + /// Retrieves person overrides for feature flag evaluation. + /// + /// This function attempts to find and return property overrides for a person. + /// It uses the provided person property overrides and filters them based on + /// the property filters defined in the feature flag. fn get_person_overrides( &self, person_property_overrides: &Option>, @@ -359,6 +392,19 @@ impl FeatureFlagMatcher { } } + /// Determines if a feature flag matches for the current context. + /// + /// This method evaluates the conditions of a feature flag to determine if it should be enabled, + /// and if so, which variant (if any) should be applied. It follows these steps: + /// + /// 1. Check if there's a valid hashed identifier for the flag. + /// 2. Evaluate any super conditions that might override normal conditions. + /// 3. Sort and evaluate each condition, prioritizing those with variant overrides. + /// 4. For each matching condition, determine the appropriate variant and payload. + /// 5. Return the result of the evaluation, including match status, variant, reason, and payload. + /// + /// The method also keeps track of the highest priority match reason and index, + /// which are used even if no conditions ultimately match. pub async fn get_match( &mut self, flag: &FeatureFlag, @@ -368,50 +414,117 @@ impl FeatureFlagMatcher { return Ok(FeatureFlagMatch { matches: false, variant: None, + reason: FeatureFlagMatchReason::NoGroupType, + condition_index: None, + payload: None, }); } - // TODO: super groups for early access - // TODO: Variant overrides condition sort + let mut highest_match = FeatureFlagMatchReason::NoConditionMatch; + let mut highest_index = None; + + // Evaluate any super conditions first + if let Some(super_groups) = &flag.filters.super_groups { + if !super_groups.is_empty() { + let super_condition_evaluation = self + .is_super_condition_match(flag, property_overrides.clone()) + .await?; + + if super_condition_evaluation.should_evaluate { + let payload = self.get_matching_payload(None, flag); + return Ok(FeatureFlagMatch { + matches: super_condition_evaluation.is_match, + variant: None, + reason: super_condition_evaluation.reason, + condition_index: Some(0), + payload, + }); + } // if no match, continue to normal conditions + } + } + + // Sort conditions with variant overrides to the top so that we can evaluate them first + let mut sorted_conditions: Vec<(usize, &FlagGroupType)> = + flag.get_conditions().iter().enumerate().collect(); + + sorted_conditions + .sort_by_key(|(_, condition)| if condition.variant.is_some() { 0 } else { 1 }); - for condition in flag.get_conditions().iter() { - let (is_match, _evaluation_reason) = self + for (index, condition) in sorted_conditions { + let (is_match, reason) = self .is_condition_match(flag, condition, property_overrides.clone()) .await?; + // Update highest_match and highest_index + let (new_highest_match, new_highest_index) = self + .get_highest_priority_match_evaluation( + highest_match.clone(), + highest_index, + reason.clone(), + Some(index), + ); + highest_match = new_highest_match; + highest_index = new_highest_index; + if is_match { - // TODO: this is a bit awkward, we should only handle variants when overrides exist - let variant = match condition.variant.clone() { - Some(variant_override) - if flag - .get_variants() - .iter() - .any(|v| v.key == variant_override) => - { - Some(variant_override) - } - _ => self.get_matching_variant(flag).await?, - }; + if highest_match == FeatureFlagMatchReason::SuperConditionValue { + break; // Exit early if we've found a super condition match + } + + let variant = self.get_matching_variant(flag).await?; + let payload = self.get_matching_payload(variant.as_deref(), flag); return Ok(FeatureFlagMatch { matches: true, variant, + reason: highest_match, + condition_index: highest_index, + payload, }); } } + // Return with the highest_match reason and index even if no conditions matched Ok(FeatureFlagMatch { matches: false, variant: None, + reason: highest_match, + condition_index: highest_index, + payload: None, }) } + /// This function determines the highest priority match evaluation for feature flag conditions. + /// It compares the current match reason with a new match reason and returns the higher priority one. + /// The priority is determined by the ordering of FeatureFlagMatchReason variants. + /// It's used to keep track of the most significant reason why a flag matched or didn't match, + /// which is especially useful when multiple conditions are evaluated. + fn get_highest_priority_match_evaluation( + &self, + current_match: FeatureFlagMatchReason, + current_index: Option, + new_match: FeatureFlagMatchReason, + new_index: Option, + ) -> (FeatureFlagMatchReason, Option) { + if current_match <= new_match { + (new_match, new_index) + } else { + (current_match, current_index) + } + } + + /// Check if a condition matches for a feature flag. + /// + /// This function evaluates a specific condition of a feature flag to determine if it should be enabled. + /// It first checks if the condition has any property filters. If not, it performs a rollout check. + /// Otherwise, it fetches the relevant properties and checks if they match the condition's filters. + /// The function returns a tuple indicating whether the condition matched and the reason for the match. async fn is_condition_match( &mut self, feature_flag: &FeatureFlag, condition: &FlagGroupType, property_overrides: Option>, - ) -> Result<(bool, String), FlagError> { + ) -> Result<(bool, FeatureFlagMatchReason), FlagError> { let rollout_percentage = condition.rollout_percentage.unwrap_or(100.0); if let Some(flag_property_filters) = &condition.properties { @@ -419,57 +532,151 @@ impl FeatureFlagMatcher { return self.check_rollout(feature_flag, rollout_percentage).await; } - let properties_to_check = - // Group-based flag - if let Some(group_type_index) = feature_flag.get_group_type_index() { - if let Some(local_overrides) = locally_computable_property_overrides( - &property_overrides.clone(), - flag_property_filters, - ) { - local_overrides - } else { - self.get_group_properties_from_cache_or_db(group_type_index) - .await? - } - } else { - // Person-based flag - if let Some(person_overrides) = property_overrides { - if let Some(local_overrides) = locally_computable_property_overrides( - &Some(person_overrides), - flag_property_filters, - ) { - local_overrides - } else { - self.get_person_properties_from_cache_or_db().await? - } - } else { - // We hit this block if there are no overrides AND we know it's not a group-based flag - self.get_person_properties_from_cache_or_db().await? - } - }; - - let properties_match = - all_properties_match(flag_property_filters, &properties_to_check); + // NB: we can only evaluate group or person properties, not both + let properties_to_check = self + .get_properties_to_check(feature_flag, property_overrides, flag_property_filters) + .await?; - if !properties_match { - return Ok((false, "NO_CONDITION_MATCH".to_string())); + if !all_properties_match(flag_property_filters, &properties_to_check) { + return Ok((false, FeatureFlagMatchReason::NoConditionMatch)); } } self.check_rollout(feature_flag, rollout_percentage).await } + /// Get properties to check for a feature flag. + /// + /// This function determines which properties to check based on the feature flag's group type index. + /// If the flag is group-based, it fetches group properties; otherwise, it fetches person properties. + async fn get_properties_to_check( + &mut self, + feature_flag: &FeatureFlag, + property_overrides: Option>, + flag_property_filters: &[PropertyFilter], + ) -> Result, FlagError> { + if let Some(group_type_index) = feature_flag.get_group_type_index() { + self.get_group_properties(group_type_index, property_overrides, flag_property_filters) + .await + } else { + self.get_person_properties(property_overrides, flag_property_filters) + .await + } + } + + /// Get group properties from cache or database. + /// + /// This function attempts to retrieve group properties either from a cache or directly from the database. + /// It first checks if there are any locally computable property overrides. If so, it returns those. + /// Otherwise, it fetches the properties from the cache or database and returns them. + async fn get_group_properties( + &mut self, + group_type_index: GroupTypeIndex, + property_overrides: Option>, + flag_property_filters: &[PropertyFilter], + ) -> Result, FlagError> { + if let Some(overrides) = + locally_computable_property_overrides(&property_overrides, flag_property_filters) + { + Ok(overrides) + } else { + self.get_group_properties_from_cache_or_db(group_type_index) + .await + } + } + + /// Get person properties from cache or database. + /// + /// This function attempts to retrieve person properties either from a cache or directly from the database. + /// It first checks if there are any locally computable property overrides. If so, it returns those. + /// Otherwise, it fetches the properties from the cache or database and returns them. + async fn get_person_properties( + &mut self, + property_overrides: Option>, + flag_property_filters: &[PropertyFilter], + ) -> Result, FlagError> { + if let Some(overrides) = + locally_computable_property_overrides(&property_overrides, flag_property_filters) + { + Ok(overrides) + } else { + self.get_person_properties_from_cache_or_db().await + } + } + + /// Check if a super condition matches for a feature flag. + /// + /// This function evaluates the super conditions of a feature flag to determine if any of them should be enabled. + /// It first checks if there are any super conditions. If so, it evaluates the first condition. + /// The function returns a struct indicating whether a super condition should be evaluated, + /// whether it matches if evaluated, and the reason for the match. + async fn is_super_condition_match( + &mut self, + feature_flag: &FeatureFlag, + property_overrides: Option>, + ) -> Result { + if let Some(first_condition) = feature_flag + .filters + .super_groups + .as_ref() + .and_then(|sc| sc.first()) + { + // Need to fetch person properties to check super conditions. If these properties are already locally computable, + // we don't need to fetch from the database, but if they aren't we need to fetch from the database and then we'll cache them. + let person_properties = self + .get_person_properties( + property_overrides, + first_condition.properties.as_deref().unwrap_or(&[]), + ) + .await?; + + let has_relevant_super_condition_properties = + first_condition.properties.as_ref().map_or(false, |props| { + props + .iter() + .any(|prop| person_properties.contains_key(&prop.key)) + }); + + let (is_match, _) = self + .is_condition_match(feature_flag, first_condition, Some(person_properties)) + .await?; + + if has_relevant_super_condition_properties { + return Ok(SuperConditionEvaluation { + should_evaluate: true, + is_match, + reason: FeatureFlagMatchReason::SuperConditionValue, + }); + // If there is a super condition evaluation, return early with those results. + // The reason is super condition value because we're not evaluating the rest of the conditions. + } + } + + Ok(SuperConditionEvaluation { + should_evaluate: false, + is_match: false, + reason: FeatureFlagMatchReason::NoConditionMatch, + }) + } + + /// Get group properties from cache or database. + /// + /// This function attempts to retrieve group properties either from a cache or directly from the database. + /// It first checks if the properties are already cached. If so, it returns those. + /// Otherwise, it fetches the properties from the database and caches them. async fn get_group_properties_from_cache_or_db( &mut self, group_type_index: GroupTypeIndex, ) -> Result, FlagError> { + // check if the properties are already cached, if so return them if let Some(properties) = self .properties_cache .group_properties .get(&group_type_index) - .cloned() { - return Ok(properties); + let mut result = HashMap::new(); + result.clone_from(properties); + return Ok(result); } let database_client = self.database_client.clone(); @@ -477,6 +684,7 @@ impl FeatureFlagMatcher { let db_properties = fetch_group_properties_from_db(database_client, team_id, group_type_index).await?; + // once the properties are fetched, cache them so we don't need to fetch again in a given request self.properties_cache .group_properties .insert(group_type_index, db_properties.clone()); @@ -484,11 +692,19 @@ impl FeatureFlagMatcher { Ok(db_properties) } + /// Get person properties from cache or database. + /// + /// This function attempts to retrieve person properties either from a cache or directly from the database. + /// It first checks if the properties are already cached. If so, it returns those. + /// Otherwise, it fetches the properties from the database and caches them. async fn get_person_properties_from_cache_or_db( &mut self, ) -> Result, FlagError> { + // check if the properties are already cached, if so return them if let Some(properties) = &self.properties_cache.person_properties { - return Ok(properties.clone()); + let mut result = HashMap::new(); + result.clone_from(properties); + return Ok(result); } let database_client = self.database_client.clone(); @@ -497,11 +713,16 @@ impl FeatureFlagMatcher { let db_properties = fetch_person_properties_from_db(database_client, distinct_id, team_id).await?; + // once the properties are fetched, cache them so we don't need to fetch again in a given request self.properties_cache.person_properties = Some(db_properties.clone()); Ok(db_properties) } + /// Get hashed identifier for a feature flag. + /// + /// This function generates a hashed identifier for a feature flag based on the feature flag's group type index. + /// If the feature flag is group-based, it fetches the group key; otherwise, it uses the distinct ID. async fn hashed_identifier(&mut self, feature_flag: &FeatureFlag) -> Result { // TODO: Use hash key overrides for experience continuity @@ -549,17 +770,22 @@ impl FeatureFlagMatcher { Ok(hash_val as f64 / LONG_SCALE as f64) } + /// Check if a feature flag should be shown based on its rollout percentage. + /// + /// This function determines if a feature flag should be shown to a user based on the flag's rollout percentage. + /// It first calculates a hash of the feature flag's identifier and compares it to the rollout percentage. + /// If the hash value is less than or equal to the rollout percentage, the flag is shown; otherwise, it is not. + /// The function returns a tuple indicating whether the flag matched and the reason for the match. async fn check_rollout( &mut self, feature_flag: &FeatureFlag, rollout_percentage: f64, - ) -> Result<(bool, String), FlagError> { - if rollout_percentage == 100.0 - || self.get_hash(feature_flag, "").await? <= (rollout_percentage / 100.0) - { - Ok((true, "CONDITION_MATCH".to_string())) // TODO enum, I'll implement this when I implement evaluation reasons + ) -> Result<(bool, FeatureFlagMatchReason), FlagError> { + let hash = self.get_hash(feature_flag, "").await?; + if rollout_percentage == 100.0 || hash <= (rollout_percentage / 100.0) { + Ok((true, FeatureFlagMatchReason::ConditionMatch)) } else { - Ok((false, "OUT_OF_ROLLOUT_BOUND".to_string())) // TODO enum, I'll implement this when I implement evaluation reasons + Ok((false, FeatureFlagMatchReason::OutOfRolloutBound)) } } @@ -579,8 +805,25 @@ impl FeatureFlagMatcher { } Ok(None) } + + /// Get matching payload for a feature flag. + /// + /// This function retrieves the payload associated with a matching variant of a feature flag. + /// It takes the matched variant key and the feature flag itself as inputs and returns the payload. + fn get_matching_payload( + &self, + match_variant: Option<&str>, + feature_flag: &FeatureFlag, + ) -> Option { + let variant = match_variant.unwrap_or("true"); + feature_flag.get_payload(variant) + } } +/// Fetch and locally cache all properties for a given distinct ID and team ID. +/// +/// This function fetches both person and group properties for a specified distinct ID and team ID. +/// It updates the properties cache with the fetched properties and returns the result. async fn fetch_and_locally_cache_all_properties( properties_cache: &mut PropertiesCache, database_client: DatabaseClientArc, @@ -651,6 +894,10 @@ async fn fetch_and_locally_cache_all_properties( Ok(()) } +/// Fetch person properties from the database for a given distinct ID and team ID. +/// +/// This function constructs and executes a SQL query to fetch the person properties for a specified distinct ID and team ID. +/// It returns the fetched properties as a HashMap. async fn fetch_person_properties_from_db( database_client: DatabaseClientArc, distinct_id: String, @@ -682,6 +929,10 @@ async fn fetch_person_properties_from_db( .collect()) } +/// Fetch group properties from the database for a given team ID and group type index. +/// +/// This function constructs and executes a SQL query to fetch the group properties for a specified team ID and group type index. +/// It returns the fetched properties as a HashMap. async fn fetch_group_properties_from_db( database_client: DatabaseClientArc, team_id: TeamId, @@ -757,15 +1008,24 @@ mod tests { test_utils::{insert_new_team_in_pg, insert_person_for_team_in_pg, setup_pg_client}, }; - fn create_test_flag(team_id: TeamId, properties: Vec) -> FeatureFlag { + fn create_test_flag( + id: Option, + team_id: Option, + name: Option, + key: Option, + filters: Option, + deleted: Option, + active: Option, + ensure_experience_continuity: Option, + ) -> FeatureFlag { FeatureFlag { - id: 1, - team_id, - name: Some("Test Flag".to_string()), - key: "test_flag".to_string(), - filters: FlagFilters { + id: id.unwrap_or(1), + team_id: team_id.unwrap_or(1), + name: name.or(Some("Test Flag".to_string())), + key: key.unwrap_or_else(|| "test_flag".to_string()), + filters: filters.unwrap_or_else(|| FlagFilters { groups: vec![FlagGroupType { - properties: Some(properties), + properties: Some(vec![]), rollout_percentage: Some(100.0), variant: None, }], @@ -773,10 +1033,10 @@ mod tests { aggregation_group_type_index: None, payloads: None, super_groups: None, - }, - deleted: false, - active: true, - ensure_experience_continuity: false, + }), + deleted: deleted.unwrap_or(false), + active: active.unwrap_or(true), + ensure_experience_continuity: ensure_experience_continuity.unwrap_or(false), } } @@ -872,14 +1132,30 @@ mod tests { .unwrap(); let flag = create_test_flag( - team.id, - vec![PropertyFilter { - key: "email".to_string(), - value: json!("override@example.com"), - operator: None, - prop_type: "person".to_string(), - group_type_index: None, - }], + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("override@example.com"), + operator: None, + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, ); let overrides = HashMap::from([("email".to_string(), json!("override@example.com"))]); @@ -899,7 +1175,6 @@ mod tests { let result = matcher .evaluate_feature_flags(flags, Some(overrides), None) .await; - assert!(!result.error_while_computing_flags); assert_eq!( result.feature_flags.get("test_flag"), @@ -909,26 +1184,39 @@ mod tests { #[tokio::test] async fn test_group_property_overrides() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); - let mut flag = create_test_flag( - team.id, - vec![PropertyFilter { - key: "industry".to_string(), - value: json!("tech"), - operator: None, - prop_type: "group".to_string(), - group_type_index: Some(1), - }], + let flag = create_test_flag( + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "industry".to_string(), + value: json!("tech"), + operator: None, + prop_type: "group".to_string(), + group_type_index: Some(1), + }]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: Some(1), + payloads: None, + super_groups: None, + }), + None, + None, + None, ); - flag.filters.aggregation_group_type_index = Some(1); - - let mut cache = GroupTypeMappingCache::new(team.id, database_client.clone()); - cache.group_types_to_indexes = [("organization".to_string(), 1)].into_iter().collect(); + let mut cache = GroupTypeMappingCache::new(team.id, client.clone()); + let group_types_to_indexes = [("organization".to_string(), 1)].into_iter().collect(); + cache.group_types_to_indexes = group_types_to_indexes; cache.group_indexes_to_types = [(1, "organization".to_string())].into_iter().collect(); let groups = HashMap::from([("organization".to_string(), json!("org_123"))]); @@ -944,7 +1232,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client, + client.clone(), Some(cache), None, Some(groups), @@ -982,7 +1270,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), 1, - database_client, + database_client.clone(), Some(cache), None, Some(groups), @@ -1007,7 +1295,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client, + database_client.clone(), None, None, None, @@ -1021,7 +1309,26 @@ mod tests { #[tokio::test] async fn test_is_condition_match_empty_properties() { let database_client = setup_pg_client(None).await; - let flag = create_test_flag(1, vec![]); + let flag = create_test_flag( + Some(1), + None, + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, + ); let condition = FlagGroupType { variant: None, @@ -1042,7 +1349,7 @@ mod tests { .await .unwrap(); assert!(is_match); - assert_eq!(reason, "CONDITION_MATCH"); + assert_eq!(reason, FeatureFlagMatchReason::ConditionMatch); } fn create_test_flag_with_variants(team_id: TeamId) -> FeatureFlag { @@ -1088,20 +1395,34 @@ mod tests { #[tokio::test] async fn test_overrides_avoid_db_lookups() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); let flag = create_test_flag( - team.id, - vec![PropertyFilter { - key: "email".to_string(), - value: json!("test@example.com"), - operator: Some(OperatorType::Exact), - prop_type: "person".to_string(), - group_type_index: None, - }], + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("test@example.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, ); let person_property_overrides = @@ -1110,7 +1431,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), None, None, None, @@ -1138,29 +1459,43 @@ mod tests { #[tokio::test] async fn test_fallback_to_db_when_overrides_insufficient() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); let flag = create_test_flag( - team.id, - vec![ - PropertyFilter { - key: "email".to_string(), - value: json!("test@example.com"), - operator: Some(OperatorType::Exact), - prop_type: "person".to_string(), - group_type_index: None, - }, - PropertyFilter { - key: "age".to_string(), - value: json!(25), - operator: Some(OperatorType::Gte), - prop_type: "person".to_string(), - group_type_index: None, - }, - ], + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![ + PropertyFilter { + key: "email".to_string(), + value: json!("test@example.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }, + PropertyFilter { + key: "age".to_string(), + value: json!(25), + operator: Some(OperatorType::Gte), + prop_type: "person".to_string(), + group_type_index: None, + }, + ]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, ); let person_property_overrides = Some(HashMap::from([( @@ -1169,7 +1504,7 @@ mod tests { )])); insert_person_for_team_in_pg( - database_client.clone(), + client.clone(), team.id, "test_user".to_string(), Some(json!({"email": "test@example.com", "age": 30})), @@ -1180,7 +1515,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), None, None, None, @@ -1203,14 +1538,12 @@ mod tests { #[tokio::test] async fn test_property_fetching_and_caching() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); let distinct_id = "test_user".to_string(); insert_person_for_team_in_pg( - database_client.clone(), + client.clone(), team.id, distinct_id.clone(), Some(json!({"email": "test@example.com", "age": 30})), @@ -1218,14 +1551,8 @@ mod tests { .await .unwrap(); - let mut matcher = FeatureFlagMatcher::new( - distinct_id, - team.id, - database_client.clone(), - None, - None, - None, - ); + let mut matcher = + FeatureFlagMatcher::new(distinct_id, team.id, client.clone(), None, None, None); let properties = matcher .get_person_properties_from_cache_or_db() @@ -1244,34 +1571,119 @@ mod tests { } #[tokio::test] - async fn test_overrides_locally_computable() { - let overrides = Some(HashMap::from([ - ("email".to_string(), json!("test@example.com")), - ("age".to_string(), json!(30)), - ])); - - let property_filters = vec![ - PropertyFilter { - key: "email".to_string(), - value: json!("test@example.com"), - operator: None, - prop_type: "person".to_string(), - group_type_index: None, - }, - PropertyFilter { - key: "age".to_string(), - value: json!(25), - operator: Some(OperatorType::Gte), - prop_type: "person".to_string(), - group_type_index: None, - }, - ]; - - let result = locally_computable_property_overrides(&overrides, &property_filters); - assert!(result.is_some()); + async fn test_property_caching() { + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); - let property_filters_with_cohort = vec![ - PropertyFilter { + let distinct_id = "test_user".to_string(); + insert_person_for_team_in_pg( + client.clone(), + team.id, + distinct_id.clone(), + Some(json!({"email": "test@example.com", "age": 30})), + ) + .await + .unwrap(); + + let mut matcher = FeatureFlagMatcher::new( + distinct_id.clone(), + team.id, + client.clone(), + None, + None, + None, + ); + + // First access should fetch from the database + let start = std::time::Instant::now(); + let properties = matcher + .get_person_properties_from_cache_or_db() + .await + .unwrap(); + let first_duration = start.elapsed(); + + // Second access should use the cache and be faster + let start = std::time::Instant::now(); + let cached_properties = matcher + .get_person_properties_from_cache_or_db() + .await + .unwrap(); + let second_duration = start.elapsed(); + + assert_eq!(properties, cached_properties); + assert!( + second_duration < first_duration, + "Second access should be faster due to caching" + ); + + // Create a new matcher to simulate a fresh state + let mut new_matcher = FeatureFlagMatcher::new( + distinct_id.clone(), + team.id, + client.clone(), + None, + None, + None, + ); + + // First access with new matcher should fetch from the database again + let start = std::time::Instant::now(); + let new_properties = new_matcher + .get_person_properties_from_cache_or_db() + .await + .unwrap(); + let new_first_duration = start.elapsed(); + + assert_eq!(properties, new_properties); + assert!( + new_first_duration > second_duration, + "First access with new matcher should be slower than cached access" + ); + + // Second access with new matcher should use the cache and be faster + let start = std::time::Instant::now(); + let new_cached_properties = new_matcher + .get_person_properties_from_cache_or_db() + .await + .unwrap(); + let new_second_duration = start.elapsed(); + + assert_eq!(properties, new_cached_properties); + assert!( + new_second_duration < new_first_duration, + "Second access with new matcher should be faster due to caching" + ); + } + + #[tokio::test] + async fn test_overrides_locally_computable() { + let overrides = Some(HashMap::from([ + ("email".to_string(), json!("test@example.com")), + ("age".to_string(), json!(30)), + ])); + + let property_filters = vec![ + PropertyFilter { + key: "email".to_string(), + value: json!("test@example.com"), + operator: None, + prop_type: "person".to_string(), + group_type_index: None, + }, + PropertyFilter { + key: "age".to_string(), + value: json!(25), + operator: Some(OperatorType::Gte), + prop_type: "person".to_string(), + group_type_index: None, + }, + ]; + + let result = locally_computable_property_overrides(&overrides, &property_filters); + assert!(result.is_some()); + + let property_filters_with_cohort = vec![ + PropertyFilter { key: "email".to_string(), value: json!("test@example.com"), operator: None, @@ -1294,21 +1706,38 @@ mod tests { #[tokio::test] async fn test_concurrent_flag_evaluation() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); - let flag = Arc::new(create_test_flag(team.id, vec![])); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); + let flag = Arc::new(create_test_flag( + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, + )); let mut handles = vec![]; for i in 0..100 { let flag_clone = flag.clone(); - let database_client_clone = database_client.clone(); + let client_clone = client.clone(); handles.push(tokio::spawn(async move { let mut matcher = FeatureFlagMatcher::new( format!("test_user_{}", i), team.id, - database_client_clone, + client_clone, None, None, None, @@ -1329,33 +1758,47 @@ mod tests { #[tokio::test] async fn test_property_operators() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); let flag = create_test_flag( - team.id, - vec![ - PropertyFilter { - key: "age".to_string(), - value: json!(25), - operator: Some(OperatorType::Gte), - prop_type: "person".to_string(), - group_type_index: None, - }, - PropertyFilter { - key: "email".to_string(), - value: json!("example@domain.com"), - operator: Some(OperatorType::Icontains), - prop_type: "person".to_string(), - group_type_index: None, - }, - ], + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![ + PropertyFilter { + key: "age".to_string(), + value: json!(25), + operator: Some(OperatorType::Gte), + prop_type: "person".to_string(), + group_type_index: None, + }, + PropertyFilter { + key: "email".to_string(), + value: json!("example@domain.com"), + operator: Some(OperatorType::Icontains), + prop_type: "person".to_string(), + group_type_index: None, + }, + ]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, ); insert_person_for_team_in_pg( - database_client.clone(), + client.clone(), team.id, "test_user".to_string(), Some(json!({"email": "user@example@domain.com", "age": 30})), @@ -1366,7 +1809,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), None, None, None, @@ -1379,11 +1822,30 @@ mod tests { #[tokio::test] async fn test_empty_hashed_identifier() { - let database_client = setup_pg_client(None).await; - let flag = create_test_flag(1, vec![]); + let client = setup_pg_client(None).await; - let mut matcher = - FeatureFlagMatcher::new("".to_string(), 1, database_client, None, None, None); + let flag = create_test_flag( + Some(1), + None, + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, + ); + + let mut matcher = FeatureFlagMatcher::new("".to_string(), 1, client, None, None, None); let result = matcher.get_match(&flag, None).await.unwrap(); @@ -1392,20 +1854,32 @@ mod tests { #[tokio::test] async fn test_rollout_percentage() { - let database_client = setup_pg_client(None).await; - let mut flag = create_test_flag(1, vec![]); - // Set the rollout percentage to 0% - flag.filters.groups[0].rollout_percentage = Some(0.0); + let client = setup_pg_client(None).await; - let mut matcher = FeatureFlagMatcher::new( - "test_user".to_string(), - 1, - database_client, + let mut flag = create_test_flag( + Some(1), + None, + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![]), + rollout_percentage: Some(0.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), None, None, None, ); + let mut matcher = + FeatureFlagMatcher::new("test_user".to_string(), 1, client, None, None, None); + let result = matcher.get_match(&flag, None).await.unwrap(); assert!(!result.matches); @@ -1420,7 +1894,8 @@ mod tests { #[tokio::test] async fn test_uneven_variant_distribution() { - let database_client = setup_pg_client(None).await; + let client = setup_pg_client(None).await; + let mut flag = create_test_flag_with_variants(1); // Adjust variant rollout percentages to be uneven @@ -1445,14 +1920,8 @@ mod tests { // Ensure the flag is person-based by setting aggregation_group_type_index to None flag.filters.aggregation_group_type_index = None; - let mut matcher = FeatureFlagMatcher::new( - "test_user".to_string(), - 1, - database_client, - None, - None, - None, - ); + let mut matcher = + FeatureFlagMatcher::new("test_user".to_string(), 1, client, None, None, None); let mut control_count = 0; let mut test_count = 0; @@ -1479,36 +1948,45 @@ mod tests { #[tokio::test] async fn test_missing_properties_in_db() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); // Insert a person without properties - insert_person_for_team_in_pg( - database_client.clone(), - team.id, - "test_user".to_string(), - None, - ) - .await - .unwrap(); + insert_person_for_team_in_pg(client.clone(), team.id, "test_user".to_string(), None) + .await + .unwrap(); let flag = create_test_flag( - team.id, - vec![PropertyFilter { - key: "email".to_string(), - value: json!("test@example.com"), - operator: None, - prop_type: "person".to_string(), - group_type_index: None, - }], + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("test@example.com"), + operator: None, + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, ); let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), None, None, None, @@ -1521,14 +1999,12 @@ mod tests { #[tokio::test] async fn test_malformed_property_data() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); // Insert a person with malformed properties insert_person_for_team_in_pg( - database_client.clone(), + client.clone(), team.id, "test_user".to_string(), Some(json!({"age": "not_a_number"})), @@ -1537,20 +2013,36 @@ mod tests { .unwrap(); let flag = create_test_flag( - team.id, - vec![PropertyFilter { - key: "age".to_string(), - value: json!(25), - operator: Some(OperatorType::Gte), - prop_type: "person".to_string(), - group_type_index: None, - }], + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "age".to_string(), + value: json!(25), + operator: Some(OperatorType::Gte), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), + None, + None, + None, ); let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), None, None, None, @@ -1563,73 +2055,44 @@ mod tests { } #[tokio::test] - async fn test_property_caching() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + async fn test_get_match_with_insufficient_overrides() { + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); - let distinct_id = "test_user".to_string(); - insert_person_for_team_in_pg( - database_client.clone(), - team.id, - distinct_id.clone(), - Some(json!({"email": "test@example.com", "age": 30})), - ) - .await - .unwrap(); - - let mut matcher = FeatureFlagMatcher::new( - distinct_id.clone(), - team.id, - database_client.clone(), + let flag = create_test_flag( + None, + Some(team.id), + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![ + PropertyFilter { + key: "email".to_string(), + value: json!("test@example.com"), + operator: None, + prop_type: "person".to_string(), + group_type_index: None, + }, + PropertyFilter { + key: "age".to_string(), + value: json!(25), + operator: Some(OperatorType::Gte), + prop_type: "person".to_string(), + group_type_index: None, + }, + ]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), None, None, None, - ); - - // First access should fetch from the database - let properties = matcher - .get_person_properties_from_cache_or_db() - .await - .unwrap(); - - assert!(matcher.properties_cache.person_properties.is_some()); - - // Second access should use the cache and not error out - let cached_properties = matcher - .get_person_properties_from_cache_or_db() - .await - .unwrap(); - - assert_eq!(properties, cached_properties); - } - - #[tokio::test] - async fn test_get_match_with_insufficient_overrides() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); - - let flag = create_test_flag( - team.id, - vec![ - PropertyFilter { - key: "email".to_string(), - value: json!("test@example.com"), - operator: None, - prop_type: "person".to_string(), - group_type_index: None, - }, - PropertyFilter { - key: "age".to_string(), - value: json!(25), - operator: Some(OperatorType::Gte), - prop_type: "person".to_string(), - group_type_index: None, - }, - ], ); let person_overrides = Some(HashMap::from([( @@ -1638,7 +2101,7 @@ mod tests { )])); insert_person_for_team_in_pg( - database_client.clone(), + client.clone(), team.id, "test_user".to_string(), Some(json!({"email": "test@example.com", "age": 30})), @@ -1649,7 +2112,7 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), None, None, None, @@ -1662,40 +2125,51 @@ mod tests { #[tokio::test] async fn test_evaluation_reasons() { - let database_client = setup_pg_client(None).await; - let flag = create_test_flag(1, vec![]); - - let mut matcher = FeatureFlagMatcher::new( - "test_user".to_string(), - 1, - database_client, + let client = setup_pg_client(None).await; + let flag = create_test_flag( + Some(1), + None, + None, + None, + Some(FlagFilters { + groups: vec![FlagGroupType { + properties: Some(vec![]), + rollout_percentage: Some(100.0), + variant: None, + }], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: None, + }), None, None, None, ); + let mut matcher = + FeatureFlagMatcher::new("test_user".to_string(), 1, client, None, None, None); + let (is_match, reason) = matcher .is_condition_match(&flag, &flag.filters.groups[0], None) .await .unwrap(); assert!(is_match); - assert_eq!(reason, "CONDITION_MATCH"); + assert_eq!(reason, FeatureFlagMatchReason::ConditionMatch); } #[tokio::test] async fn test_complex_conditions() { - let database_client = setup_pg_client(None).await; - let team = insert_new_team_in_pg(database_client.clone()) - .await - .unwrap(); + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); - let flag = FeatureFlag { - id: 1, - team_id: team.id, - name: Some("Complex Flag".to_string()), - key: "complex_flag".to_string(), - filters: FlagFilters { + let flag = create_test_flag( + Some(1), + Some(team.id), + Some("Complex Flag".to_string()), + Some("complex_flag".to_string()), + Some(FlagFilters { groups: vec![ FlagGroupType { properties: Some(vec![PropertyFilter { @@ -1724,14 +2198,14 @@ mod tests { aggregation_group_type_index: None, payloads: None, super_groups: None, - }, - deleted: false, - active: true, - ensure_experience_continuity: false, - }; + }), + Some(false), + Some(true), + Some(false), + ); insert_person_for_team_in_pg( - database_client.clone(), + client.clone(), team.id, "test_user".to_string(), Some(json!({"email": "user2@example.com", "age": 35})), @@ -1742,7 +2216,198 @@ mod tests { let mut matcher = FeatureFlagMatcher::new( "test_user".to_string(), team.id, - database_client.clone(), + client.clone(), + None, + None, + None, + ); + + let result = matcher.get_match(&flag, None).await.unwrap(); + + assert!(result.matches); + } + + #[tokio::test] + async fn test_super_condition_matches_boolean() { + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); + + let flag = create_test_flag( + Some(1), + Some(team.id), + Some("Super Condition Flag".to_string()), + Some("super_condition_flag".to_string()), + Some(FlagFilters { + groups: vec![ + FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("fake@posthog.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(0.0), + variant: None, + }, + FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("test@posthog.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }, + FlagGroupType { + properties: None, + rollout_percentage: Some(50.0), + variant: None, + }, + ], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: Some(vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "is_enabled".to_string(), + value: json!(["true"]), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }]), + }), + None, + None, + None, + ); + + insert_person_for_team_in_pg( + client.clone(), + team.id, + "test_id".to_string(), + Some(json!({"email": "test@posthog.com", "is_enabled": true})), + ) + .await + .unwrap(); + + let mut matcher_test_id = FeatureFlagMatcher::new( + "test_id".to_string(), + team.id, + client.clone(), + None, + None, + None, + ); + + let mut matcher_example_id = FeatureFlagMatcher::new( + "lil_id".to_string(), + team.id, + client.clone(), + None, + None, + None, + ); + + let mut matcher_another_id = FeatureFlagMatcher::new( + "another_id".to_string(), + team.id, + client.clone(), + None, + None, + None, + ); + + let result_test_id = matcher_test_id.get_match(&flag, None).await.unwrap(); + let result_example_id = matcher_example_id.get_match(&flag, None).await.unwrap(); + let result_another_id = matcher_another_id.get_match(&flag, None).await.unwrap(); + + assert!(result_test_id.matches); + assert!(result_test_id.reason == FeatureFlagMatchReason::SuperConditionValue); + assert!(result_example_id.matches); + assert!(result_example_id.reason == FeatureFlagMatchReason::ConditionMatch); + assert!(!result_another_id.matches); + assert!(result_another_id.reason == FeatureFlagMatchReason::OutOfRolloutBound); + } + + #[tokio::test] + async fn test_super_condition_matches_string() { + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); + + insert_person_for_team_in_pg( + client.clone(), + team.id, + "test_id".to_string(), + Some(json!({"email": "test@posthog.com", "is_enabled": "true"})), + ) + .await + .unwrap(); + + let flag = create_test_flag( + Some(1), + Some(team.id), + Some("Super Condition Flag".to_string()), + Some("super_condition_flag".to_string()), + Some(FlagFilters { + groups: vec![ + FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("fake@posthog.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(0.0), + variant: None, + }, + FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("test@posthog.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }, + FlagGroupType { + properties: None, + rollout_percentage: Some(50.0), + variant: None, + }, + ], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: Some(vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "is_enabled".to_string(), + value: json!("true"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }]), + }), + None, + None, + None, + ); + + let mut matcher = FeatureFlagMatcher::new( + "test_id".to_string(), + team.id, + client.clone(), None, None, None, @@ -1751,5 +2416,129 @@ mod tests { let result = matcher.get_match(&flag, None).await.unwrap(); assert!(result.matches); + assert_eq!(result.reason, FeatureFlagMatchReason::SuperConditionValue); + assert_eq!(result.condition_index, Some(0)); + } + + #[tokio::test] + async fn test_super_condition_matches_and_false() { + let client = setup_pg_client(None).await; + let team = insert_new_team_in_pg(client.clone()).await.unwrap(); + + insert_person_for_team_in_pg( + client.clone(), + team.id, + "test_id".to_string(), + Some(json!({"email": "test@posthog.com", "is_enabled": true})), + ) + .await + .unwrap(); + + let flag = create_test_flag( + Some(1), + Some(team.id), + Some("Super Condition Flag".to_string()), + Some("super_condition_flag".to_string()), + Some(FlagFilters { + groups: vec![ + FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("fake@posthog.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(0.0), + variant: None, + }, + FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "email".to_string(), + value: json!("test@posthog.com"), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }, + FlagGroupType { + properties: None, + rollout_percentage: Some(50.0), + variant: None, + }, + ], + multivariate: None, + aggregation_group_type_index: None, + payloads: None, + super_groups: Some(vec![FlagGroupType { + properties: Some(vec![PropertyFilter { + key: "is_enabled".to_string(), + value: json!(false), + operator: Some(OperatorType::Exact), + prop_type: "person".to_string(), + group_type_index: None, + }]), + rollout_percentage: Some(100.0), + variant: None, + }]), + }), + None, + None, + None, + ); + + let mut matcher_test_id = FeatureFlagMatcher::new( + "test_id".to_string(), + team.id, + client.clone(), + None, + None, + None, + ); + + let mut matcher_example_id = FeatureFlagMatcher::new( + "lil_id".to_string(), + team.id, + client.clone(), + None, + None, + None, + ); + + let mut matcher_another_id = FeatureFlagMatcher::new( + "another_id".to_string(), + team.id, + client.clone(), + None, + None, + None, + ); + + let result_test_id = matcher_test_id.get_match(&flag, None).await.unwrap(); + let result_example_id = matcher_example_id.get_match(&flag, None).await.unwrap(); + let result_another_id = matcher_another_id.get_match(&flag, None).await.unwrap(); + + assert!(!result_test_id.matches); + assert_eq!( + result_test_id.reason, + FeatureFlagMatchReason::SuperConditionValue + ); + assert_eq!(result_test_id.condition_index, Some(0)); + + assert!(result_example_id.matches); + assert_eq!( + result_example_id.reason, + FeatureFlagMatchReason::ConditionMatch + ); + assert_eq!(result_example_id.condition_index, Some(2)); + + assert!(!result_another_id.matches); + assert_eq!( + result_another_id.reason, + FeatureFlagMatchReason::OutOfRolloutBound + ); + assert_eq!(result_another_id.condition_index, Some(2)); } } diff --git a/rust/feature-flags/src/lib.rs b/rust/feature-flags/src/lib.rs index de5065723e45a..ed25ba7e318d6 100644 --- a/rust/feature-flags/src/lib.rs +++ b/rust/feature-flags/src/lib.rs @@ -1,6 +1,7 @@ pub mod api; pub mod config; pub mod database; +pub mod feature_flag_match_reason; pub mod flag_definitions; pub mod flag_matching; pub mod flag_request; diff --git a/rust/feature-flags/src/property_matching.rs b/rust/feature-flags/src/property_matching.rs index 3487c99dce113..8d12fe6ab5e9d 100644 --- a/rust/feature-flags/src/property_matching.rs +++ b/rust/feature-flags/src/property_matching.rs @@ -53,9 +53,12 @@ pub fn match_property( let compute_exact_match = |value: &Value, override_value: &Value| -> bool { if is_truthy_or_falsy_property_value(value) { // Do boolean handling, such that passing in "true" or "True" or "false" or "False" as matching value is equivalent - let truthy = is_truthy_property_value(value); - return override_value.to_string().to_lowercase() - == truthy.to_string().to_lowercase(); + let (truthy_value, truthy_override_value) = ( + is_truthy_property_value(value), + is_truthy_property_value(override_value), + ); + return truthy_override_value.to_string().to_lowercase() + == truthy_value.to_string().to_lowercase(); } if value.is_array() { diff --git a/rust/feature-flags/tests/test_flag_matching_consistency.rs b/rust/feature-flags/tests/test_flag_matching_consistency.rs index e8bff239cf901..5ce20cd55e89f 100644 --- a/rust/feature-flags/tests/test_flag_matching_consistency.rs +++ b/rust/feature-flags/tests/test_flag_matching_consistency.rs @@ -1,3 +1,4 @@ +use feature_flags::feature_flag_match_reason::FeatureFlagMatchReason; /// These tests are common between all libraries doing local evaluation of feature flags. /// This ensures there are no mismatches between implementations. use feature_flags::flag_matching::{FeatureFlagMatch, FeatureFlagMatcher}; @@ -121,6 +122,9 @@ async fn it_is_consistent_with_rollout_calculation_for_simple_flags() { FeatureFlagMatch { matches: true, variant: None, + reason: FeatureFlagMatchReason::ConditionMatch, + condition_index: Some(0), + payload: None, } ); } else { @@ -129,6 +133,9 @@ async fn it_is_consistent_with_rollout_calculation_for_simple_flags() { FeatureFlagMatch { matches: false, variant: None, + reason: FeatureFlagMatchReason::OutOfRolloutBound, + condition_index: Some(0), + payload: None, } ); } @@ -1199,12 +1206,15 @@ async fn it_is_consistent_with_rollout_calculation_for_multivariate_flags() { .await .unwrap(); - if result.is_some() { + if let Some(variant) = &result { assert_eq!( feature_flag_match, FeatureFlagMatch { matches: true, - variant: results[i].clone(), + variant: Some(variant.clone()), + reason: FeatureFlagMatchReason::ConditionMatch, + condition_index: Some(0), + payload: None, } ); } else { @@ -1213,6 +1223,9 @@ async fn it_is_consistent_with_rollout_calculation_for_multivariate_flags() { FeatureFlagMatch { matches: false, variant: None, + reason: FeatureFlagMatchReason::OutOfRolloutBound, + condition_index: Some(0), + payload: None, } ); }