From 01205ef920974440217af2f4c096521fb94109dd Mon Sep 17 00:00:00 2001 From: "Javier G. Sogo" Date: Mon, 20 Jan 2025 14:29:56 +0100 Subject: [PATCH] Isolate FeatureSnapshot and PropertySnapshot from crate::models types Signed-off-by: Javier G. Sogo --- src/client/feature_snapshot.rs | 90 +++++++++++------------ src/client/property_snapshot.rs | 49 ++++++------- src/segment_evaluation/mod.rs | 126 +++++++++++++++++++++++++++----- 3 files changed, 174 insertions(+), 91 deletions(-) diff --git a/src/client/feature_snapshot.rs b/src/client/feature_snapshot.rs index 92c217b..98ff42d 100644 --- a/src/client/feature_snapshot.rs +++ b/src/client/feature_snapshot.rs @@ -18,15 +18,20 @@ use crate::Feature; use std::collections::HashMap; use super::feature_proxy::random_value; -use crate::segment_evaluation::find_applicable_segment_rule_for_entity; +use crate::segment_evaluation::SegmentRules; use crate::errors::Result; /// Provides a snapshot of a [`Feature`]. #[derive(Debug)] pub struct FeatureSnapshot { - feature: crate::models::Feature, - segments: HashMap, + enabled: bool, + enabled_value: Value, + disabled_value: Value, + rollout_percentage: u32, + name: String, + feature_id: String, + segment_rules: SegmentRules, } impl FeatureSnapshot { @@ -34,50 +39,46 @@ impl FeatureSnapshot { feature: crate::models::Feature, segments: HashMap, ) -> Self { - Self { feature, segments } + let segment_rules = SegmentRules::new(segments, feature.segment_rules, feature.kind); + Self { + enabled: feature.enabled, // TODO: Based on this enabled value we can create two different implementations, as the disabled one is much more simpler. + enabled_value: (feature.kind, feature.enabled_value) + .try_into() + .expect("TODO: Handle this error"), + disabled_value: (feature.kind, feature.disabled_value) + .try_into() + .expect("TODO: Handle this error"), + rollout_percentage: feature.rollout_percentage, + segment_rules, + name: feature.name, + feature_id: feature.feature_id, + } } - fn evaluate_feature_for_entity( - &self, - entity: &impl Entity, - ) -> Result { - if !self.feature.enabled { - return Ok(self.feature.disabled_value.clone()); + fn evaluate_feature_for_entity(&self, entity: &impl Entity) -> Result { + if !self.enabled { + return Ok(self.disabled_value.clone()); } - if self.feature.segment_rules.is_empty() || entity.get_attributes().is_empty() { + if self.segment_rules.is_empty() || entity.get_attributes().is_empty() { // No match possible. Do not consider segment rules: return self.use_rollout_percentage_to_get_value_from_feature_directly(entity); } - match find_applicable_segment_rule_for_entity( - &self.segments, - &self.feature.segment_rules, - entity, - )? { + match self + .segment_rules + .find_applicable_segment_rule_for_entity(entity)? + { Some(segment_rule) => { // Get rollout percentage - let rollout_percentage = match &segment_rule.rollout_percentage { - Some(value) => { - if value.is_default() { - self.feature.rollout_percentage - } else { - u32::try_from(value.as_u64().expect("Rollout value is not u64.")) - .expect("Invalid rollout value. Could not convert to u32.") - } - } - None => panic!("Rollout value is missing."), - }; + let rollout_percentage = + segment_rule.rollout_percentage(self.rollout_percentage)?; // Should rollout? - if Self::should_rollout(rollout_percentage, entity, &self.feature.feature_id) { - if segment_rule.value.is_default() { - Ok(self.feature.enabled_value.clone()) - } else { - Ok(segment_rule.value.clone()) - } + if Self::should_rollout(rollout_percentage, entity, &self.feature_id) { + segment_rule.value(&self.enabled_value) } else { - Ok(self.feature.disabled_value.clone()) + Ok(self.disabled_value.clone()) } } None => self.use_rollout_percentage_to_get_value_from_feature_directly(entity), @@ -92,28 +93,27 @@ impl FeatureSnapshot { fn use_rollout_percentage_to_get_value_from_feature_directly( &self, entity: &impl Entity, - ) -> Result { - let rollout_percentage = self.feature.rollout_percentage; - if Self::should_rollout(rollout_percentage, entity, &self.feature.feature_id) { - Ok(self.feature.enabled_value.clone()) + ) -> Result { + let rollout_percentage = self.rollout_percentage; + if Self::should_rollout(rollout_percentage, entity, &self.feature_id) { + Ok(self.enabled_value.clone()) } else { - Ok(self.feature.disabled_value.clone()) + Ok(self.disabled_value.clone()) } } } impl Feature for FeatureSnapshot { fn get_name(&self) -> Result { - Ok(self.feature.name.clone()) + Ok(self.name.clone()) } fn is_enabled(&self) -> Result { - Ok(self.feature.enabled) + Ok(self.enabled) } fn get_value(&self, entity: &impl Entity) -> Result { - let model_value = self.evaluate_feature_for_entity(entity)?; - (self.feature.kind, model_value).try_into() + self.evaluate_feature_for_entity(entity) } fn get_value_into>( @@ -186,7 +186,7 @@ pub mod tests { attributes: entity_attributes.clone(), }; assert_eq!( - random_value(format!("{}:{}", entity.id, feature.feature.feature_id).as_str()), + random_value(format!("{}:{}", entity.id, feature.feature_id).as_str()), 68 ); let value = feature.get_value(&entity).unwrap(); @@ -198,7 +198,7 @@ pub mod tests { attributes: entity_attributes, }; assert_eq!( - random_value(format!("{}:{}", entity.id, feature.feature.feature_id).as_str()), + random_value(format!("{}:{}", entity.id, feature.feature_id).as_str()), 29 ); let value = feature.get_value(&entity).unwrap(); diff --git a/src/client/property_snapshot.rs b/src/client/property_snapshot.rs index 8e0b274..5fbf202 100644 --- a/src/client/property_snapshot.rs +++ b/src/client/property_snapshot.rs @@ -18,13 +18,14 @@ use crate::Property; use std::collections::HashMap; use crate::errors::Result; -use crate::segment_evaluation::find_applicable_segment_rule_for_entity; +use crate::segment_evaluation::SegmentRules; /// Provides a snapshot of a [`Property`]. #[derive(Debug)] pub struct PropertySnapshot { - property: crate::models::Property, - segments: HashMap, + value: Value, + segment_rules: SegmentRules, + name: String, } impl PropertySnapshot { @@ -32,45 +33,41 @@ impl PropertySnapshot { property: crate::models::Property, segments: HashMap, ) -> Self { - Self { property, segments } + let segment_rules = SegmentRules::new(segments, property.segment_rules, property.kind); + Self { + value: (property.kind, property.value) + .try_into() + .expect("TODO: Handle error here"), + segment_rules, + name: property.name, + } } - fn evaluate_feature_for_entity( - &self, - entity: &impl Entity, - ) -> Result { - if self.property.segment_rules.is_empty() || entity.get_attributes().is_empty() { + fn evaluate_feature_for_entity(&self, entity: &impl Entity) -> Result { + if self.segment_rules.is_empty() || entity.get_attributes().is_empty() { // TODO: this makes only sense if there can be a rule which matches // even on empty attributes // No match possible. Do not consider segment rules: - return Ok(self.property.value.clone()); + return Ok(self.value.clone()); } - match find_applicable_segment_rule_for_entity( - &self.segments, - &self.property.segment_rules, - entity, - )? { - Some(segment_rule) => { - if segment_rule.value.is_default() { - Ok(self.property.value.clone()) - } else { - Ok(segment_rule.value.clone()) - } - } - None => Ok(self.property.value.clone()), + match self + .segment_rules + .find_applicable_segment_rule_for_entity(entity)? + { + Some(segment_rule) => segment_rule.value(&self.value), + None => Ok(self.value.clone()), } } } impl Property for PropertySnapshot { fn get_name(&self) -> Result { - Ok(self.property.name.clone()) + Ok(self.name.clone()) } fn get_value(&self, entity: &impl Entity) -> Result { - let model_value = self.evaluate_feature_for_entity(entity)?; - (self.property.kind, model_value).try_into() + self.evaluate_feature_for_entity(entity) } fn get_value_into>( diff --git a/src/segment_evaluation/mod.rs b/src/segment_evaluation/mod.rs index 8a326a2..c67805c 100644 --- a/src/segment_evaluation/mod.rs +++ b/src/segment_evaluation/mod.rs @@ -17,23 +17,104 @@ pub(crate) mod errors; use std::collections::HashMap; use crate::entity::Entity; +use crate::errors::Error; use crate::errors::Result; use crate::models::Segment; use crate::models::TargetingRule; +use crate::models::ValueKind; use crate::Value; use errors::{CheckOperatorErrorDetail, SegmentEvaluationError}; -pub(crate) fn find_applicable_segment_rule_for_entity<'a>( - segments: &HashMap, - segment_rules: &'a [TargetingRule], - entity: &impl Entity, -) -> Result> { - for targeting_rule in segment_rules.iter() { - if targeting_rule_applies_to_entity(segments, targeting_rule, entity)? { - return Ok(Some(targeting_rule)); +#[derive(Debug)] +pub(crate) struct SegmentRules { + targeting_rules: Vec, + segments: HashMap, + kind: ValueKind, +} + +impl SegmentRules { + pub(crate) fn new( + segments: HashMap, + targeting_rules: Vec, + kind: ValueKind, + ) -> Self { + Self { + segments, + targeting_rules, + kind, + } + } + + pub(crate) fn is_empty(&self) -> bool { + self.targeting_rules.is_empty() + } + + pub(crate) fn find_applicable_segment_rule_for_entity( + &self, + entity: &impl Entity, + ) -> Result> { + for targeting_rule in self.targeting_rules.iter() { + if targeting_rule_applies_to_entity(&self.segments, targeting_rule, entity)? { + return Ok(Some(SegmentRule { + targeting_rule, + kind: self.kind, + })); + } + } + Ok(None) + } +} + +#[derive(Debug)] +pub(crate) struct SegmentRule<'a> { + targeting_rule: &'a TargetingRule, + kind: ValueKind, +} + +impl SegmentRule<'_> { + fn is_default(&self) -> bool { + self.targeting_rule.value.is_default() + } + + /// Returns the rollout percentage using the following logic: + /// * If there is not rollout percentage in the [`TargetingRule`] it returns an error + /// * If the rollout value in the [`TargetingRule`] is equal to "$default" it will return + /// the given `default` argument. + /// * Otherwise it will return the rollout value from the [`TargetingRule`] converted to u32 + pub(crate) fn rollout_percentage(&self, default: u32) -> Result { + self.targeting_rule + .rollout_percentage + .as_ref() + .map(|v| { + if v.is_default() { + Ok(default) + } else { + let value: u64 = v.as_u64().ok_or(Error::ProtocolError( + "Rollout value is not u64.".to_string(), + ))?; + + value.try_into().map_err(|e| { + Error::ProtocolError(format!( + "Invalid rollout value. Could not convert to u32: {e}" + )) + }) + } + }) + .transpose()? + .ok_or(Error::ProtocolError("Rollout is missing".to_string())) + } + + /// Returns the value using the following logic: + /// * If the value in the [`TargetingRule`] is equal to "$default" it will return + /// the given `default` argument. + /// * Otherwise it will return the value from the [`TargetingRule`] + pub(crate) fn value(&self, default: &Value) -> Result { + if self.is_default() { + Ok(default.clone()) + } else { + (self.kind, self.targeting_rule.value.clone()).try_into() } } - Ok(None) } fn targeting_rule_applies_to_entity( @@ -213,11 +294,12 @@ pub mod tests { segments: HashMap, segment_rules: Vec, ) { + let segment_rules = SegmentRules::new(segments, segment_rules, ValueKind::String); let entity = crate::tests::GenericEntity { id: "a2".into(), attributes: HashMap::from([("name2".into(), Value::from("heinz".to_string()))]), }; - let rule = find_applicable_segment_rule_for_entity(&segments, &segment_rules, &entity); + let rule = segment_rules.find_applicable_segment_rule_for_entity(&entity); // Segment evaluation should not fail: let rule = rule.unwrap(); // But no segment should be found: @@ -233,15 +315,18 @@ pub mod tests { id: "a2".into(), attributes: HashMap::from([("name".into(), Value::from(42.0))]), }; - let segment_rules = vec![TargetingRule { - rules: vec![Segments { - segments: vec!["non_existing_segment_id".into()], - }], - value: ConfigValue(serde_json::Value::Number((-48).into())), - order: 0, - rollout_percentage: Some(ConfigValue(serde_json::Value::Number((100).into()))), - }]; - let rule = find_applicable_segment_rule_for_entity(&segments, &segment_rules, &entity); + let segment_rules = { + let targeting_rules = vec![TargetingRule { + rules: vec![Segments { + segments: vec!["non_existing_segment_id".into()], + }], + value: ConfigValue(serde_json::Value::Number((-48).into())), + order: 0, + rollout_percentage: Some(ConfigValue(serde_json::Value::Number((100).into()))), + }]; + SegmentRules::new(segments, targeting_rules, ValueKind::String) + }; + let rule = segment_rules.find_applicable_segment_rule_for_entity(&entity); // Error message should look something like this: // Failed to evaluate entity: Failed to evaluate entity 'a2' against targeting rule '0'. // Caused by: Segment 'non_existing_segment_id' not found. @@ -261,11 +346,12 @@ pub mod tests { // We can mark this as failure and return error. #[rstest] fn test_operator_failed(segments: HashMap, segment_rules: Vec) { + let segment_rules = SegmentRules::new(segments, segment_rules, ValueKind::String); let entity = crate::tests::GenericEntity { id: "a2".into(), attributes: HashMap::from([("name".into(), Value::from(42.0))]), }; - let rule = find_applicable_segment_rule_for_entity(&segments, &segment_rules, &entity); + let rule = segment_rules.find_applicable_segment_rule_for_entity(&entity); let e = rule.unwrap_err(); assert!(matches!(e, Error::EntityEvaluationError(_))); let Error::EntityEvaluationError(EntityEvaluationError(