From a92db5a1a77b1adbdf294b0c7991327dd5b6ea9d Mon Sep 17 00:00:00 2001 From: Oleksii Shmalko Date: Tue, 17 Sep 2024 20:12:52 +0300 Subject: [PATCH] FF-3204 perf: compile regex during parsing (#31) * chore: add regex benchmark * perf: compile regexes during configuration parsing --- eppo_core/benches/evaluation_details.rs | 29 ++ eppo_core/src/attributes.rs | 11 + eppo_core/src/eval/eval_details.rs | 4 +- eppo_core/src/eval/eval_details_builder.rs | 18 +- eppo_core/src/eval/eval_rules.rs | 485 ++++++++++++--------- eppo_core/src/eval/eval_visitor.rs | 25 +- eppo_core/src/ufc/models.rs | 242 +++++++++- 7 files changed, 598 insertions(+), 216 deletions(-) diff --git a/eppo_core/benches/evaluation_details.rs b/eppo_core/benches/evaluation_details.rs index 471c2ba6..62b1ffd8 100644 --- a/eppo_core/benches/evaluation_details.rs +++ b/eppo_core/benches/evaluation_details.rs @@ -128,6 +128,35 @@ fn criterion_benchmark(c: &mut Criterion) { }); group.finish(); } + + { + let mut group = c.benchmark_group("regex-flag"); + group.throughput(Throughput::Elements(1)); + let attributes = [("email".into(), "test@gmail.com".into())].into(); + group.bench_function("get_assignment", |b| { + b.iter(|| { + get_assignment( + black_box(Some(&configuration)), + black_box("regex-flag"), + black_box("subject1"), + black_box(&attributes), + black_box(None), + ) + }) + }); + group.bench_function("get_assignment_details", |b| { + b.iter(|| { + get_assignment_details( + black_box(Some(&configuration)), + black_box("regex-flag"), + black_box("subject1"), + black_box(&attributes), + black_box(None), + ) + }) + }); + group.finish(); + } } criterion_group!( diff --git a/eppo_core/src/attributes.rs b/eppo_core/src/attributes.rs index 11008f35..44d99d72 100644 --- a/eppo_core/src/attributes.rs +++ b/eppo_core/src/attributes.rs @@ -41,6 +41,17 @@ pub enum AttributeValue { /// A null value or absence of value. Null, } + +impl AttributeValue { + pub fn as_str(&self) -> Option<&str> { + if let AttributeValue::String(s) = self { + Some(s.as_str()) + } else { + None + } + } +} + impl From<&str> for AttributeValue { fn from(value: &str) -> Self { Self::String(value.to_owned()) diff --git a/eppo_core/src/eval/eval_details.rs b/eppo_core/src/eval/eval_details.rs index f2fcb2ba..5d6e7426 100644 --- a/eppo_core/src/eval/eval_details.rs +++ b/eppo_core/src/eval/eval_details.rs @@ -3,7 +3,7 @@ use serde::{Deserialize, Serialize}; use crate::{ error::{EvaluationError, EvaluationFailure}, - ufc::{Condition, Shard, Value}, + ufc::{ConditionWire, Shard, Value}, AttributeValue, Attributes, }; @@ -135,7 +135,7 @@ pub struct RuleEvaluationDetails { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct ConditionEvaluationDetails { - pub condition: Condition, + pub condition: ConditionWire, pub attribute_value: Option, pub matched: bool, } diff --git a/eppo_core/src/eval/eval_details_builder.rs b/eppo_core/src/eval/eval_details_builder.rs index 543e9c63..8d7cca62 100644 --- a/eppo_core/src/eval/eval_details_builder.rs +++ b/eppo_core/src/eval/eval_details_builder.rs @@ -4,7 +4,7 @@ use chrono::{DateTime, Utc}; use crate::{ error::EvaluationFailure, - ufc::{Allocation, Assignment, Condition, Flag, Rule, Shard, Split, Value, Variation}, + ufc::{Allocation, Assignment, Condition, ConditionWire, Flag, Rule, Shard, Split, Value, Variation}, AttributeValue, Attributes, Configuration, EvaluationError, }; @@ -384,11 +384,25 @@ impl<'a> EvalRuleVisitor for EvalRuleDetailsBuilder<'a> { .conditions .push(ConditionEvaluationDetails { matched: result, - condition: condition.clone(), + condition: condition.clone().into(), attribute_value: attribute_value.cloned(), }); } + fn on_condition_skip( + &mut self, + condition: &serde_json::Value, + ) { + let condition = match serde_json::from_value::(condition.clone()) { + Ok(condition_wire) => condition_wire, + Err(err) => { + log::warn!("condition cannot be parsed: {err:?}"); + return; + } + }; + self.rule_details.conditions.push(ConditionEvaluationDetails { condition, attribute_value: None, matched: false }); + } + fn on_result(&mut self, result: bool) { self.rule_details.matched = result; } diff --git a/eppo_core/src/eval/eval_rules.rs b/eppo_core/src/eval/eval_rules.rs index 2866f8ea..48b389fa 100644 --- a/eppo_core/src/eval/eval_rules.rs +++ b/eppo_core/src/eval/eval_rules.rs @@ -1,8 +1,9 @@ -use regex::Regex; +use std::borrow::Cow; + use semver::Version; use crate::{ - ufc::{Condition, ConditionOperator, ConditionValue, Rule, Value}, + ufc::{Comparand, ComparisonOperator, Condition, ConditionCheck, Rule, TryParse}, AttributeValue, Attributes, }; @@ -14,128 +15,97 @@ impl Rule { visitor: &mut V, attributes: &Attributes, ) -> bool { - self.conditions - .iter() - .all(|condition| condition.eval(visitor, attributes)) + self.conditions.iter().all(|condition| match condition { + TryParse::Parsed(condition) => condition.eval(visitor, attributes), + TryParse::ParseFailed(raw_condition) => { + visitor.on_condition_skip(raw_condition); + false + } + }) } } impl Condition { fn eval(&self, visitor: &mut V, attributes: &Attributes) -> bool { - let attribute = attributes.get(&self.attribute); - let result = self.operator.eval(attribute, &self.value); + let attribute = attributes.get(self.attribute.as_ref()); + let result = self.check.eval(attribute); visitor.on_condition_eval(self, attribute, result); result } } -impl ConditionOperator { - /// Applying `Operator` to the values. Returns `false` if the operator cannot be applied or - /// there's a misconfiguration. - fn eval(&self, attribute: Option<&AttributeValue>, condition_value: &ConditionValue) -> bool { - self.try_eval(attribute, condition_value).unwrap_or(false) +impl ConditionCheck { + /// Check if `attribute` matches. + fn eval(&self, attribute: Option<&AttributeValue>) -> bool { + self.try_eval(attribute).unwrap_or(false) } /// Try applying `Operator` to the values, returning `None` if the operator cannot be applied. - fn try_eval( - &self, - attribute: Option<&AttributeValue>, - condition_value: &ConditionValue, - ) -> Option { - match self { - Self::Matches | Self::NotMatches => { - let s = match attribute { - Some(AttributeValue::String(s)) => s.clone(), - Some(AttributeValue::Boolean(b)) => b.to_string(), - _ => return None, - }; - let regex = match condition_value { - ConditionValue::Single(Value::String(s)) => Regex::new(s).ok()?, - _ => return None, + fn try_eval(&self, attribute: Option<&AttributeValue>) -> Option { + let result = match self { + ConditionCheck::Comparison { + operator, + comparand, + } => { + let attribute = attribute?; + let ordering = match comparand { + Comparand::Version(comparand) => { + let attribute = Version::parse(attribute.as_str()?).ok()?; + attribute.cmp(comparand) + } + Comparand::Number(comparand) => { + let attribute = match attribute { + AttributeValue::Number(n) => *n, + AttributeValue::String(s) => s.parse().ok()?, + _ => return None, + }; + attribute.partial_cmp(comparand)? + } }; - let matches = regex.is_match(&s); - Some(if matches!(self, Self::Matches) { - matches - } else { - !matches - }) + match operator { + ComparisonOperator::Gte => ordering.is_gt() || ordering.is_eq(), + ComparisonOperator::Gt => ordering.is_gt(), + ComparisonOperator::Lte => ordering.is_lt() || ordering.is_eq(), + ComparisonOperator::Lt => ordering.is_lt(), + } } - - Self::OneOf | Self::NotOneOf => { - let s = match attribute { - Some(AttributeValue::String(s)) => s.clone(), - Some(AttributeValue::Number(n)) => n.to_string(), - Some(AttributeValue::Boolean(b)) => b.to_string(), + ConditionCheck::Regex { + expected_match, + regex, + } => { + let s = match attribute? { + AttributeValue::String(s) => s.as_str(), + AttributeValue::Boolean(v) => { + if *v { + "true" + } else { + "false" + } + } _ => return None, }; - let values = match condition_value { - ConditionValue::Multiple(v) => v, + regex.is_match(s) == *expected_match + } + ConditionCheck::Membership { + expected_membership, + values, + } => { + let s = match attribute? { + AttributeValue::String(s) => Cow::Borrowed(s.as_str()), + AttributeValue::Number(n) => Cow::Owned(n.to_string()), + AttributeValue::Boolean(b) => Cow::Borrowed(if *b { "true" } else { "false" }), _ => return None, }; - let is_one_of = values.iter().any(|v| v == &s); - let has_to_be_one_of = *self == Self::OneOf; - Some(is_one_of == has_to_be_one_of) + let s = s.as_ref(); + values.into_iter().any(|it| it.as_ref() == s) == *expected_membership } - - Self::IsNull => { + ConditionCheck::Null { expected_null } => { let is_null = attribute.is_none() || attribute == Some(&AttributeValue::Null); - let ConditionValue::Single(Value::Boolean(expected_null)) = condition_value else { - return None; - }; - Some(is_null == *expected_null) + is_null == *expected_null } + }; - Self::Gte | Self::Gt | Self::Lte | Self::Lt => { - let condition_version = match condition_value { - ConditionValue::Single(Value::String(s)) => Version::parse(s).ok(), - _ => None, - }; - - if let Some(condition_version) = condition_version { - // semver comparison - - let attribute_version = match attribute { - Some(AttributeValue::String(s)) => Version::parse(s).ok(), - _ => None, - }?; - - Some(match self { - Self::Gt => attribute_version > condition_version, - Self::Gte => attribute_version >= condition_version, - Self::Lt => attribute_version < condition_version, - Self::Lte => attribute_version <= condition_version, - _ => { - // unreachable - return None; - } - }) - } else { - // numeric comparison - let condition_value = match condition_value { - ConditionValue::Single(Value::Number(n)) => *n, - ConditionValue::Single(Value::String(s)) => s.parse().ok()?, - _ => return None, - }; - - let attribute_value = match attribute { - Some(AttributeValue::Number(n)) => *n, - Some(AttributeValue::String(s)) => s.parse().ok()?, - _ => return None, - }; - - Some(match self { - Self::Gt => attribute_value > condition_value, - Self::Gte => attribute_value >= condition_value, - Self::Lt => attribute_value < condition_value, - Self::Lte => attribute_value <= condition_value, - _ => { - // unreachable - return None; - } - }) - } - } - } + Some(result) } } @@ -145,154 +115,255 @@ mod tests { use crate::{ eval::eval_visitor::NoopEvalVisitor, - ufc::{Condition, ConditionOperator, Rule}, + ufc::{Comparand, ComparisonOperator, Condition, ConditionCheck, Rule}, }; #[test] fn matches_regex() { - assert!( - ConditionOperator::Matches.eval(Some(&"test@example.com".into()), &"^test.*".into()) - ); - assert!( - !ConditionOperator::Matches.eval(Some(&"example@test.com".into()), &"^test.*".into()) - ); + let check = ConditionCheck::Regex { + expected_match: true, + regex: "^test.*".try_into().unwrap(), + }; + assert!(check.eval(Some(&"test@example.com".into()))); + assert!(!check.eval(Some(&"example@test.com".into()))); + assert!(!check.eval(None)); } #[test] fn not_matches_regex() { - assert!(!ConditionOperator::NotMatches - .eval(Some(&"test@example.com".into()), &"^test.*".into())); - assert!(!ConditionOperator::NotMatches.eval(None, &"^test.*".into())); - assert!( - ConditionOperator::NotMatches.eval(Some(&"example@test.com".into()), &"^test.*".into()) - ); + let check = ConditionCheck::Regex { + expected_match: false, + regex: "^test.*".try_into().unwrap(), + }; + assert!(!check.eval(Some(&"test@example.com".into()))); + assert!(check.eval(Some(&"example@test.com".into()))); + assert!(!check.eval(None)); } #[test] fn one_of() { - assert!(ConditionOperator::OneOf.eval( - Some(&"alice".into()), - &vec![String::from("alice"), String::from("bob")].into() - )); - assert!(ConditionOperator::OneOf.eval( - Some(&"bob".into()), - &vec![String::from("alice"), String::from("bob")].into() - )); - assert!(!ConditionOperator::OneOf.eval( - Some(&"charlie".into()), - &vec![String::from("alice"), String::from("bob")].into() - )); + let check = ConditionCheck::Membership { + expected_membership: true, + values: ["alice".into(), "bob".into()].into(), + }; + assert!(check.eval(Some(&"alice".into()))); + assert!(check.eval(Some(&"bob".into()))); + assert!(!check.eval(Some(&"charlie".into()))); } #[test] fn not_one_of() { - assert!(!ConditionOperator::NotOneOf.eval( - Some(&"alice".into()), - &vec![String::from("alice"), String::from("bob")].into() - )); - assert!(!ConditionOperator::NotOneOf.eval( - Some(&"bob".into()), - &vec![String::from("alice"), String::from("bob")].into() - )); - assert!(ConditionOperator::NotOneOf.eval( - Some(&"charlie".into()), - &vec![String::from("alice"), String::from("bob")].into() - )); + let check = ConditionCheck::Membership { + expected_membership: false, + values: ["alice".into(), "bob".into()].into(), + }; + assert!(!check.eval(Some(&"alice".into()))); + assert!(!check.eval(Some(&"bob".into()))); + assert!(check.eval(Some(&"charlie".into()))); // NOT_ONE_OF fails when attribute is not specified - assert!(!ConditionOperator::NotOneOf.eval( - None, - &vec![String::from("alice"), String::from("bob")].into() - )); + assert!(!check.eval(None)); } #[test] fn one_of_int() { - assert!(ConditionOperator::OneOf.eval(Some(&42.0.into()), &vec![String::from("42")].into())); + assert!(ConditionCheck::Membership { + expected_membership: true, + values: ["42".into()].into() + } + .eval(Some(&42.0.into()))); } #[test] fn one_of_bool() { - assert!( - ConditionOperator::OneOf.eval(Some(&true.into()), &vec![String::from("true")].into()) - ); - assert!( - ConditionOperator::OneOf.eval(Some(&false.into()), &vec![String::from("false")].into()) - ); - assert!( - !ConditionOperator::OneOf.eval(Some(&1.0.into()), &vec![String::from("true")].into()) - ); - assert!( - !ConditionOperator::OneOf.eval(Some(&0.0.into()), &vec![String::from("false")].into()) - ); - assert!(!ConditionOperator::OneOf.eval(None, &vec![String::from("true")].into())); - assert!(!ConditionOperator::OneOf.eval(None, &vec![String::from("false")].into())); + let true_check = ConditionCheck::Membership { + expected_membership: true, + values: ["true".into()].into(), + }; + let false_check = ConditionCheck::Membership { + expected_membership: true, + values: ["false".into()].into(), + }; + assert!(true_check.eval(Some(&true.into()))); + assert!(false_check.eval(Some(&false.into()))); + assert!(!true_check.eval(Some(&1.0.into()))); + assert!(!false_check.eval(Some(&0.0.into()))); + assert!(!true_check.eval(None)); + assert!(!false_check.eval(None)); } #[test] fn is_null() { - assert!(ConditionOperator::IsNull.eval(None, &true.into())); - assert!(!ConditionOperator::IsNull.eval(Some(&10.0.into()), &true.into())); + assert!(ConditionCheck::Null { + expected_null: true + } + .eval(None)); + assert!(!ConditionCheck::Null { + expected_null: true + } + .eval(Some(&10.0.into()))); } #[test] fn is_not_null() { - assert!(!ConditionOperator::IsNull.eval(None, &false.into())); - assert!(ConditionOperator::IsNull.eval(Some(&10.0.into()), &false.into())); + assert!(!ConditionCheck::Null { + expected_null: false + } + .eval(None)); + assert!(ConditionCheck::Null { + expected_null: false + } + .eval(Some(&10.0.into()))); } #[test] fn gte() { - assert!(ConditionOperator::Gte.eval(Some(&18.0.into()), &18.0.into())); - assert!(!ConditionOperator::Gte.eval(Some(&17.0.into()), &18.0.into())); + let check = ConditionCheck::Comparison { + operator: ComparisonOperator::Gte, + comparand: Comparand::Number(18.0), + }; + assert!(check.eval(Some(&18.0.into()))); + assert!(!check.eval(Some(&17.0.into()))); } #[test] fn gt() { - assert!(ConditionOperator::Gt.eval(Some(&19.0.into()), &18.0.into())); - assert!(!ConditionOperator::Gt.eval(Some(&18.0.into()), &18.0.into())); + let check = ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: Comparand::Number(18.0), + }; + assert!(check.eval(Some(&19.0.into()))); + assert!(!check.eval(Some(&18.0.into()))); } #[test] fn lte() { - assert!(ConditionOperator::Lte.eval(Some(&18.0.into()), &18.0.into())); - assert!(!ConditionOperator::Lte.eval(Some(&19.0.into()), &18.0.into())); + let check = ConditionCheck::Comparison { + operator: ComparisonOperator::Lte, + comparand: Comparand::Number(18.0), + }; + assert!(check.eval(Some(&18.0.into()))); + assert!(!check.eval(Some(&19.0.into()))); } #[test] fn lt() { - assert!(ConditionOperator::Lt.eval(Some(&17.0.into()), &18.0.into())); - assert!(!ConditionOperator::Lt.eval(Some(&18.0.into()), &18.0.into())); + let check = ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: Comparand::Number(18.0), + }; + assert!(check.eval(Some(&17.0.into()))); + assert!(!check.eval(Some(&18.0.into()))); } #[test] fn semver_gte() { - assert!(ConditionOperator::Gte.eval(Some(&"1.0.1".into()), &"1.0.0".into())); - assert!(ConditionOperator::Gte.eval(Some(&"1.0.0".into()), &"1.0.0".into())); - assert!(!ConditionOperator::Gte.eval(Some(&"1.2.0".into()), &"1.10.0".into())); - assert!(ConditionOperator::Gte.eval(Some(&"1.13.0".into()), &"1.5.0".into())); - assert!(!ConditionOperator::Gte.eval(Some(&"0.9.9".into()), &"1.0.0".into())); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Gte, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.1".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Gte, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.0".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Gte, + comparand: Comparand::Version("1.10.0".parse().unwrap()) + } + .eval(Some(&"1.2.0".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Gte, + comparand: Comparand::Version("1.5.0".parse().unwrap()) + } + .eval(Some(&"1.13.0".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Gte, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"0.9.9".into()))); } #[test] fn semver_gt() { - assert!(ConditionOperator::Gt.eval(Some(&"1.0.1".into()), &"1.0.0".into())); - assert!(!ConditionOperator::Gt.eval(Some(&"1.0.0".into()), &"1.0.0".into())); - assert!(!ConditionOperator::Gt.eval(Some(&"1.2.0".into()), &"1.10.0".into())); - assert!(ConditionOperator::Gt.eval(Some(&"1.13.0".into()), &"1.5.0".into())); - assert!(!ConditionOperator::Gt.eval(Some(&"0.9.9".into()), &"1.0.0".into())); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.1".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.0".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: Comparand::Version("1.10.0".parse().unwrap()) + } + .eval(Some(&"1.2.0".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: Comparand::Version("1.5.0".parse().unwrap()) + } + .eval(Some(&"1.13.0".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"0.9.9".into()))); } #[test] fn semver_lte() { - assert!(!ConditionOperator::Lte.eval(Some(&"1.0.1".into()), &"1.0.0".into())); - assert!(ConditionOperator::Lte.eval(Some(&"1.0.0".into()), &"1.0.0".into())); - assert!(ConditionOperator::Lte.eval(Some(&"1.2.0".into()), &"1.10.0".into())); - assert!(!ConditionOperator::Lte.eval(Some(&"1.13.0".into()), &"1.5.0".into())); - assert!(ConditionOperator::Lte.eval(Some(&"0.9.9".into()), &"1.0.0".into())); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Lte, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.1".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Lte, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.0".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Lte, + comparand: Comparand::Version("1.10.0".parse().unwrap()) + } + .eval(Some(&"1.2.0".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Lte, + comparand: Comparand::Version("1.5.0".parse().unwrap()) + } + .eval(Some(&"1.13.0".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Lte, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"0.9.9".into()))); } #[test] fn semver_lt() { - assert!(!ConditionOperator::Lt.eval(Some(&"1.0.1".into()), &"1.0.0".into())); - assert!(!ConditionOperator::Lt.eval(Some(&"1.0.0".into()), &"1.0.0".into())); - assert!(ConditionOperator::Lt.eval(Some(&"1.2.0".into()), &"1.10.0".into())); - assert!(!ConditionOperator::Lt.eval(Some(&"1.13.0".into()), &"1.5.0".into())); - assert!(ConditionOperator::Lt.eval(Some(&"0.9.9".into()), &"1.0.0".into())); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.1".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"1.0.0".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: Comparand::Version("1.10.0".parse().unwrap()) + } + .eval(Some(&"1.2.0".into()))); + assert!(!ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: Comparand::Version("1.5.0".parse().unwrap()) + } + .eval(Some(&"1.13.0".into()))); + assert!(ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: Comparand::Version("1.0.0".parse().unwrap()) + } + .eval(Some(&"0.9.9".into()))); } #[test] @@ -306,9 +377,12 @@ mod tests { let rule = Rule { conditions: vec![Condition { attribute: "age".into(), - operator: ConditionOperator::Gt, - value: 10.0.into(), - }], + check: ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: 10.0.into(), + }, + } + .into()], }; assert!(rule.eval( &mut NoopEvalVisitor, @@ -322,14 +396,20 @@ mod tests { conditions: vec![ Condition { attribute: "age".into(), - operator: ConditionOperator::Gt, - value: 18.0.into(), - }, + check: ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: 18.0.into(), + }, + } + .into(), Condition { attribute: "age".into(), - operator: ConditionOperator::Lt, - value: 100.0.into(), - }, + check: ConditionCheck::Comparison { + operator: ComparisonOperator::Lt, + comparand: 100.0.into(), + }, + } + .into(), ], }; assert!(rule.eval( @@ -351,9 +431,12 @@ mod tests { let rule = Rule { conditions: vec![Condition { attribute: "age".into(), - operator: ConditionOperator::Gt, - value: 10.0.into(), - }], + check: ConditionCheck::Comparison { + operator: ComparisonOperator::Gt, + comparand: 10.0.into(), + }, + } + .into()], }; assert!(!rule.eval( &mut NoopEvalVisitor, diff --git a/eppo_core/src/eval/eval_visitor.rs b/eppo_core/src/eval/eval_visitor.rs index bd7bbc1d..2d71639a 100644 --- a/eppo_core/src/eval/eval_visitor.rs +++ b/eppo_core/src/eval/eval_visitor.rs @@ -80,15 +80,18 @@ pub(super) trait EvalAllocationVisitor { } pub(super) trait EvalRuleVisitor { - #[allow(unused_variables)] - #[inline] + /// Called when condition is skipped due to being invalid (e.g., regex cannot be compiled or + /// server response is ill-formatted). + /// + /// `condition` is original server response. + fn on_condition_skip(&mut self, condition: &serde_json::Value); + fn on_condition_eval( &mut self, condition: &Condition, attribute_value: Option<&AttributeValue>, result: bool, - ) { - } + ); #[allow(unused_variables)] #[inline] @@ -153,6 +156,18 @@ impl EvalAllocationVisitor for NoopEvalVisitor { } } -impl EvalRuleVisitor for NoopEvalVisitor {} +impl EvalRuleVisitor for NoopEvalVisitor { + #[inline] + fn on_condition_skip(&mut self, _condition: &serde_json::Value) {} + + #[inline] + fn on_condition_eval( + &mut self, + _condition: &Condition, + _attribute_value: Option<&AttributeValue>, + _result: bool, + ) { + } +} impl EvalSplitVisitor for NoopEvalVisitor {} diff --git a/eppo_core/src/ufc/models.rs b/eppo_core/src/ufc/models.rs index a22e622c..62afe43f 100644 --- a/eppo_core/src/ufc/models.rs +++ b/eppo_core/src/ufc/models.rs @@ -1,8 +1,12 @@ use std::collections::HashMap; use derive_more::From; +use regex::Regex; +use semver::Version; use serde::{Deserialize, Serialize}; +use crate::{Error, EvaluationError}; + use super::AssignmentValue; #[allow(missing_docs)] @@ -45,8 +49,16 @@ pub enum TryParse { /// Successfully parsed. Parsed(T), /// Parsing failed. + /// + /// This holds the generic JSON value, so even if parsing originally failed, we could serialize + /// the value back to JSON. ParseFailed(serde_json::Value), } +impl From for TryParse { + fn from(value: T) -> TryParse { + TryParse::Parsed(value) + } +} impl From> for Result { fn from(value: TryParse) -> Self { match value { @@ -194,24 +206,242 @@ fn default_do_log() -> bool { true } -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] #[allow(missing_docs)] pub struct Rule { - pub conditions: Vec, + pub conditions: Vec>, } /// `Condition` is a check that given user `attribute` matches the condition `value` under the given /// `operator`. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(try_from = "ConditionWire", into = "ConditionWire")] +pub struct Condition { + pub attribute: Box, + pub check: ConditionCheck, +} + +#[derive(Debug, Clone)] +pub enum ConditionCheck { + Comparison { + operator: ComparisonOperator, + comparand: Comparand, + }, + Regex { + expected_match: bool, + // As regex is supplied by user, we allow regex parse failure to not fail parsing and + // evaluation. Invalid regexes are simply ignored. + regex: Regex, + }, + Membership { + expected_membership: bool, + values: Box<[Box]>, + }, + Null { + expected_null: bool, + }, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub enum ComparisonOperator { + Gte, + Gt, + Lte, + Lt, +} + +impl From for ConditionOperator { + fn from(value: ComparisonOperator) -> ConditionOperator { + match value { + ComparisonOperator::Gte => ConditionOperator::Gte, + ComparisonOperator::Gt => ConditionOperator::Gt, + ComparisonOperator::Lte => ConditionOperator::Lte, + ComparisonOperator::Lt => ConditionOperator::Lt, + } + } +} + +#[derive(Debug, Clone, PartialEq, PartialOrd, From)] +pub enum Comparand { + Version(Version), + Number(f64), +} + +impl From for ConditionValue { + fn from(value: Comparand) -> ConditionValue { + let s = match value { + Comparand::Version(v) => v.to_string(), + Comparand::Number(n) => n.to_string(), + }; + ConditionValue::Single(s.into()) + } +} + +/// Wire (JSON) format for the `Condition`. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] #[allow(missing_docs)] -pub struct Condition { +pub struct ConditionWire { + pub attribute: Box, pub operator: ConditionOperator, - pub attribute: String, pub value: ConditionValue, } +impl From for ConditionWire { + fn from(condition: Condition) -> Self { + let (operator, value) = match condition.check { + ConditionCheck::Comparison { + operator, + comparand, + } => (operator.into(), comparand.into()), + ConditionCheck::Regex { + expected_match, + regex, + } => ( + if expected_match { + ConditionOperator::Matches + } else { + ConditionOperator::NotMatches + }, + regex.as_str().into(), + ), + ConditionCheck::Membership { + expected_membership, + values, + } => ( + if expected_membership { + ConditionOperator::OneOf + } else { + ConditionOperator::NotOneOf + }, + ConditionValue::Multiple(values), + ), + ConditionCheck::Null { expected_null } => { + (ConditionOperator::IsNull, expected_null.into()) + } + }; + ConditionWire { + attribute: condition.attribute, + operator, + value, + } + } +} + +impl From for Option { + fn from(value: ConditionWire) -> Self { + Condition::try_from(value).ok() + } +} + +impl TryFrom for Condition { + type Error = Error; + + fn try_from(condition: ConditionWire) -> Result { + let attribute = condition.attribute; + let check = match condition.operator { + ConditionOperator::Matches | ConditionOperator::NotMatches => { + let expected_match = condition.operator == ConditionOperator::Matches; + + let regex_string = match condition.value { + ConditionValue::Single(Value::String(s)) => s, + _ => { + log::warn!( + "failed to parse condition: {:?} condition with non-string condition value", + condition.operator + ); + return Err(Error::EvaluationError( + EvaluationError::UnexpectedConfigurationParseError, + )); + } + }; + let regex = match Regex::new(®ex_string) { + Ok(regex) => regex, + Err(err) => { + log::warn!("failed to parse condition: failed to compile regex {regex_string:?}: {err:?}"); + return Err(Error::EvaluationError( + EvaluationError::UnexpectedConfigurationParseError, + )); + } + }; + + ConditionCheck::Regex { + expected_match, + regex, + } + } + ConditionOperator::Gte + | ConditionOperator::Gt + | ConditionOperator::Lte + | ConditionOperator::Lt => { + let operator = match condition.operator { + ConditionOperator::Gte => ComparisonOperator::Gte, + ConditionOperator::Gt => ComparisonOperator::Gt, + ConditionOperator::Lte => ComparisonOperator::Lte, + ConditionOperator::Lt => ComparisonOperator::Lt, + _ => unreachable!(), + }; + + let condition_version = match &condition.value { + ConditionValue::Single(Value::String(s)) => Version::parse(s).ok(), + _ => None, + }; + + if let Some(condition_version) = condition_version { + ConditionCheck::Comparison { + operator, + comparand: Comparand::Version(condition_version), + } + } else { + // numeric comparison + let condition_value = match &condition.value { + ConditionValue::Single(Value::Number(n)) => Some(*n), + ConditionValue::Single(Value::String(s)) => s.parse().ok(), + _ => None, + }; + let Some(condition_value) = condition_value else { + log::warn!("failed to parse condition: comparision value is neither regex, nor number: {:?}", condition.value); + return Err(Error::EvaluationError( + EvaluationError::UnexpectedConfigurationParseError, + )); + }; + ConditionCheck::Comparison { + operator, + comparand: Comparand::Number(condition_value), + } + } + } + ConditionOperator::OneOf | ConditionOperator::NotOneOf => { + let expected_membership = condition.operator == ConditionOperator::OneOf; + let values = match condition.value { + ConditionValue::Multiple(v) => v, + _ => { + log::warn!("failed to parse condition: membership condition with non-array value: {:?}", condition.value); + return Err(Error::EvaluationError( + EvaluationError::UnexpectedConfigurationParseError, + )); + } + }; + ConditionCheck::Membership { + expected_membership, + values, + } + } + ConditionOperator::IsNull => { + let ConditionValue::Single(Value::Boolean(expected_null)) = condition.value else { + log::warn!("failed to parse condition: IS_NULL condition with non-boolean condition value"); + return Err(Error::EvaluationError( + EvaluationError::UnexpectedConfigurationParseError, + )); + }; + ConditionCheck::Null { expected_null } + } + }; + Ok(Condition { attribute, check }) + } +} + /// Possible condition types. #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Clone, Copy)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] @@ -249,7 +479,7 @@ pub enum ConditionOperator { pub enum ConditionValue { Single(Value), // Only string arrays are currently supported. - Multiple(Vec), + Multiple(Box<[Box]>), } impl> From for ConditionValue { @@ -259,7 +489,7 @@ impl> From for ConditionValue { } impl From> for ConditionValue { fn from(value: Vec) -> Self { - Self::Multiple(value) + Self::Multiple(value.into_iter().map(|it| it.into()).collect()) } }