diff --git a/.gitmodules b/.gitmodules index c62bff8..e1bc1d5 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,4 +1,4 @@ [submodule "tests/engine_tests/engine-test-data"] path = tests/engine_tests/engine-test-data url = https://github.com/flagsmith/engine-test-data.git - branch = v3.7.0 + branch = v3.12.1 diff --git a/Cargo.toml b/Cargo.toml index 303f09e..1bd2baa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,8 @@ maintenance = { status = "actively-developed" } [dependencies] serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -serde_json_path = "0.7" +jsonpath-rust = "1.0" +indexmap = { version = "2.14", features = ["serde"] } chrono = { version = "0.4", features = ["serde"] } md-5 = "0.10.1" num-bigint = "0.4" diff --git a/src/engine.rs b/src/engine.rs index d78e691..8d26a5f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -1,13 +1,16 @@ use crate::engine_eval::context::{EngineEvaluationContext, FeatureContext}; use crate::engine_eval::result::{EvaluationResult, FlagResult, SegmentResult}; -use crate::engine_eval::segment_evaluator::SegmentEvaluator; +use crate::engine_eval::segment_evaluator::{ResolvedFlags, SegmentEvaluator}; use crate::utils::hashing; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; -/// Holds a feature context with its associated segment name for priority comparison -struct FeatureContextWithSegment { - feature_context: FeatureContext, - segment_name: String, +/// Reason given to flags whose evaluation depends on themselves. +pub const CIRCULAR_DEPENDENCY_REASON: &str = "ERROR; code=CIRCULAR_DEPENDENCY"; + +/// A segment override, with the name of the segment it belongs to +pub(crate) struct SegmentOverride<'a> { + pub(crate) feature_context: &'a FeatureContext, + pub(crate) segment_name: &'a str, } /// Helper to get priority or default @@ -15,26 +18,39 @@ fn get_priority_or_default(priority: Option) -> f64 { priority.unwrap_or(f64::INFINITY) // Weakest possible priority } +/// Whether a segment override takes precedence over the one held so far; the first seen wins a tie +pub(crate) fn wins_over(candidate: &FeatureContext, incumbent: Option<&FeatureContext>) -> bool { + incumbent.is_none_or(|incumbent| { + get_priority_or_default(candidate.priority) < get_priority_or_default(incumbent.priority) + }) +} + +/// Gets the reason for a flag evaluated to its environment default: +/// `DEFAULT` if segments override the feature, `STATIC` if none does. +pub(crate) fn get_fallback_reason(is_targeted: bool) -> String { + if is_targeted { "DEFAULT" } else { "STATIC" }.to_string() +} + +/// Matching segments, the winning override of each feature they override, and every feature any segment overrides +struct SegmentEvaluation<'a> { + segments: Vec, + segment_overrides: HashMap<&'a str, SegmentOverride<'a>>, + targeted_feature_names: HashSet<&'a str>, +} + /// Gets matching segments and their overrides -fn get_matching_segments_and_overrides( - ec: &EngineEvaluationContext, -) -> ( - Vec, - HashMap, -) { +fn evaluate_segments<'a>( + ec: &'a EngineEvaluationContext, + evaluator: &SegmentEvaluator<'a>, +) -> SegmentEvaluation<'a> { let mut segments = Vec::new(); - let mut segment_feature_contexts: HashMap = HashMap::new(); - let evaluator = SegmentEvaluator::new(ec); - - // Sort segment keys for deterministic ordering - let mut segment_keys: Vec<_> = ec.segments.keys().collect(); - segment_keys.sort(); + let mut segment_overrides: HashMap<&str, SegmentOverride> = HashMap::new(); + let mut targeted_feature_names = HashSet::new(); - // Process segments in sorted order - for segment_key in segment_keys { - let segment_context = &ec.segments[segment_key]; + for (segment_key, segment_context) in &ec.segments { + targeted_feature_names.extend(segment_context.overrides.iter().map(|o| o.name.as_str())); - if !evaluator.is_context_in_segment(segment_context) { + if !evaluator.matches_segment_key(segment_key) { continue; } @@ -46,39 +62,62 @@ fn get_matching_segments_and_overrides( // Process segment overrides for override_fc in &segment_context.overrides { - let feature_name = &override_fc.name; - - // Check if we should update the segment feature context - let should_update = if let Some(existing) = segment_feature_contexts.get(feature_name) { - let existing_priority = get_priority_or_default(existing.feature_context.priority); - let override_priority = get_priority_or_default(override_fc.priority); - override_priority < existing_priority - } else { - true - }; - - if should_update { - segment_feature_contexts.insert( - feature_name.clone(), - FeatureContextWithSegment { - feature_context: override_fc.clone(), - segment_name: segment_context.name.clone(), + let incumbent = segment_overrides + .get(override_fc.name.as_str()) + .map(|o| o.feature_context); + if wins_over(override_fc, incumbent) { + segment_overrides.insert( + &override_fc.name, + SegmentOverride { + feature_context: override_fc, + segment_name: &segment_context.name, }, ); } } } - (segments, segment_feature_contexts) + SegmentEvaluation { + segments, + segment_overrides, + targeted_feature_names, + } } /// Gets flag results from feature contexts and segment overrides fn get_flag_results( ec: &EngineEvaluationContext, - segment_feature_contexts: &HashMap, + segment_evaluation: &SegmentEvaluation, + identity_key: Option<&String>, ) -> HashMap { let mut flags = HashMap::new(); + // Process all features + for feature_context in ec.features.values() { + let feature_name = feature_context.name.as_str(); + let flag_result = match segment_evaluation.segment_overrides.get(feature_name) { + Some(segment_override) => get_flag_result_from_feature_context( + segment_override.feature_context, + identity_key, + format!("TARGETING_MATCH; segment={}", segment_override.segment_name), + ), + None => get_flag_result_from_feature_context( + feature_context, + identity_key, + get_fallback_reason( + segment_evaluation + .targeted_feature_names + .contains(feature_name), + ), + ), + }; + flags.insert(feature_context.name.clone(), flag_result); + } + + flags +} + +pub fn get_evaluation_result(ec: &EngineEvaluationContext) -> EvaluationResult { // Get identity key if identity exists // If identity key is not provided, construct it from environment key and identifier let identity_key: Option = ec.identity.as_ref().map(|i| { @@ -89,54 +128,49 @@ fn get_flag_results( } }); - // Process all features - for feature_context in ec.features.values() { - // Check if we have a segment override for this feature - if let Some(segment_fc) = segment_feature_contexts.get(&feature_context.name) { - // Use segment override with multivariate evaluation - let fc = &segment_fc.feature_context; - let reason = format!("TARGETING_MATCH; segment={}", segment_fc.segment_name); - let flag_result = - get_flag_result_from_feature_context(fc, identity_key.as_ref(), reason); - flags.insert(feature_context.name.clone(), flag_result); - } else { - // Use default feature context - let flag_result = get_flag_result_from_feature_context( - feature_context, - identity_key.as_ref(), - "DEFAULT".to_string(), - ); - flags.insert(feature_context.name.clone(), flag_result); + let evaluator = SegmentEvaluator::with_flag_dependencies(ec, identity_key.as_ref()); + let segment_evaluation = evaluate_segments(ec, &evaluator); + let mut flags = get_flag_results(ec, &segment_evaluation, identity_key.as_ref()); + + // Flags read by segment conditions take precedence, as only they were resolved guarding against cycles + let ResolvedFlags { + flags: resolved_flags, + cyclic, + } = evaluator.into_resolved_flags(); + flags.extend( + resolved_flags + .into_iter() + .map(|(feature_name, flag)| (feature_name.to_string(), flag)), + ); + for feature_name in cyclic { + if let Some(flag) = flags.get_mut(feature_name) { + flag.reason = CIRCULAR_DEPENDENCY_REASON.to_string(); } } - flags -} - -pub fn get_evaluation_result(ec: &EngineEvaluationContext) -> EvaluationResult { - // Process segments - let (segments, segment_feature_contexts) = get_matching_segments_and_overrides(ec); - - // Get flag results - let flags = get_flag_results(ec, &segment_feature_contexts); - - EvaluationResult { flags, segments } + EvaluationResult { + flags, + segments: segment_evaluation.segments, + } } /// Creates a FlagResult from a FeatureContext -fn get_flag_result_from_feature_context( +pub(crate) fn get_flag_result_from_feature_context( feature_context: &FeatureContext, identity_key: Option<&String>, default_reason: String, ) -> FlagResult { let mut reason = default_reason; let mut value = feature_context.value.clone(); + let mut variant = None; // Handle multivariate features if !feature_context.variants.is_empty() && identity_key.is_some() && !feature_context.key.is_empty() { + variant = Some("control".to_string()); // Unless a variant is selected below + // Sort variants by priority (lower priority value = higher priority) let mut sorted_variants = feature_context.variants.clone(); sorted_variants.sort_by(|a, b| { @@ -151,11 +185,12 @@ fn get_flag_result_from_feature_context( // Select variant based on weighted distribution let mut cumulative_weight = 0.0; - for variant in &sorted_variants { - cumulative_weight += variant.weight; + for feature_value in &sorted_variants { + cumulative_weight += feature_value.weight; if (hash_percentage as f64) <= cumulative_weight { - value = variant.value.clone(); - reason = format!("SPLIT; weight={}", variant.weight); + value = feature_value.value.clone(); + variant = feature_value.key.clone(); + reason = format!("SPLIT; weight={}", feature_value.weight); break; } } @@ -165,6 +200,7 @@ fn get_flag_result_from_feature_context( enabled: feature_context.enabled, name: feature_context.name.clone(), value, + variant, reason, metadata: feature_context.metadata.clone(), } @@ -189,7 +225,7 @@ mod tests { name: "test".to_string(), }, features: HashMap::new(), - segments: HashMap::new(), + segments: Default::default(), identity: None, }; diff --git a/src/engine_eval/context.rs b/src/engine_eval/context.rs index 6ec41b4..ac5c9ac 100644 --- a/src/engine_eval/context.rs +++ b/src/engine_eval/context.rs @@ -1,4 +1,5 @@ use crate::types::FlagsmithValue; +use indexmap::IndexMap; use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -29,6 +30,9 @@ fn default_feature_type() -> String { /// Represents a multivariate value for a feature flag. #[derive(Clone, Debug, Serialize, Deserialize)] pub struct FeatureValue { + /// The key identifying this variant in flag results. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub key: Option, /// The value of the feature. pub value: FlagsmithValue, /// The weight of the feature value variant, as a percentage number (i.e. 100.0). @@ -257,9 +261,9 @@ pub struct EngineEvaluationContext { #[serde(default)] pub features: HashMap, - /// Segment contexts indexed by segment key. + /// Segment contexts indexed by segment key, in evaluation order. #[serde(default)] - pub segments: HashMap, + pub segments: IndexMap, /// Optional identity context for evaluation. #[serde(skip_serializing_if = "Option::is_none")] diff --git a/src/engine_eval/mappers.rs b/src/engine_eval/mappers.rs index c075f57..cb8835d 100644 --- a/src/engine_eval/mappers.rs +++ b/src/engine_eval/mappers.rs @@ -7,6 +7,7 @@ use crate::environments::Environment; use crate::features::{FeatureState, MultivariateFeatureStateValue}; use crate::identities::{Identity, Trait}; use crate::segments::{Segment, SegmentRule as OldSegmentRule}; +use indexmap::IndexMap; use sha2::{Digest, Sha256}; use std::collections::HashMap; @@ -24,7 +25,7 @@ pub fn environment_to_context(environment: Environment) -> EngineEvaluationConte name: environment.name.clone(), }, features: HashMap::new(), - segments: HashMap::new(), + segments: IndexMap::new(), identity: None, }; @@ -91,6 +92,7 @@ fn map_multivariate_values_to_variants( mv_values .iter() .map(|mv| FeatureValue { + key: None, value: mv.multivariate_feature_option.value.clone(), weight: mv.percentage_allocation as f64, priority: None, @@ -340,3 +342,36 @@ pub fn add_identity_to_context( new_context.identity = Some(identity); new_context } + +/// Gets the context to evaluate environment flags against +/// +/// # Arguments +/// * `context` - The context to evaluate environment flags for +/// +/// # Returns +/// A new context without identity, keeping only segments whose every condition reads the environment or other flags +pub fn remove_identity_from_context(context: &EngineEvaluationContext) -> EngineEvaluationContext { + EngineEvaluationContext { + environment: context.environment.clone(), + features: context.features.clone(), + segments: context + .segments + .iter() + .filter(|(_, segment)| is_identity_free(&segment.rules)) + .map(|(key, segment)| (key.clone(), segment.clone())) + .collect(), + identity: None, + } +} + +fn is_identity_free(rules: &[SegmentRule]) -> bool { + const IDENTITY_FREE_PROPERTY_PREFIXES: [&str; 4] = + ["$.environment.", "$.environment[", "$.flags.", "$.flags["]; + rules.iter().all(|rule| { + rule.conditions.iter().all(|condition| { + IDENTITY_FREE_PROPERTY_PREFIXES + .iter() + .any(|prefix| condition.property.starts_with(prefix)) + }) && is_identity_free(&rule.rules) + }) +} diff --git a/src/engine_eval/mod.rs b/src/engine_eval/mod.rs index 95f7ff6..bc217d3 100644 --- a/src/engine_eval/mod.rs +++ b/src/engine_eval/mod.rs @@ -12,6 +12,6 @@ pub mod mappers; // Re-export commonly used types for convenience pub use context::{EngineEvaluationContext, FeatureContext, FeatureMetadata, SegmentSource}; -pub use mappers::{add_identity_to_context, environment_to_context}; +pub use mappers::{add_identity_to_context, environment_to_context, remove_identity_from_context}; pub use result::{EvaluationResult, FlagResult, SegmentResult}; pub use segment_evaluator::is_context_in_segment; diff --git a/src/engine_eval/result.rs b/src/engine_eval/result.rs index 8f65635..af5a8df 100644 --- a/src/engine_eval/result.rs +++ b/src/engine_eval/result.rs @@ -23,12 +23,16 @@ pub struct FlagResult { /// The name of the feature. pub name: String, - /// The reason for this evaluation result (e.g., "DEFAULT", "TARGETING_MATCH; segment=name", "SPLIT; weight=50"). + /// The reason for this evaluation result (e.g., "STATIC", "DEFAULT", "TARGETING_MATCH; segment=name", "SPLIT; weight=50", "ERROR; code=CIRCULAR_DEPENDENCY"). pub reason: String, /// The value of the feature flag. pub value: FlagsmithValue, + /// The key of the selected multivariate variant, "control" when none was selected, or `None` when no split applies. + #[serde(default)] + pub variant: Option, + /// Metadata about the feature. #[serde(default)] pub metadata: FeatureMetadata, diff --git a/src/engine_eval/segment_evaluator.rs b/src/engine_eval/segment_evaluator.rs index 793f83f..028ba2a 100644 --- a/src/engine_eval/segment_evaluator.rs +++ b/src/engine_eval/segment_evaluator.rs @@ -2,13 +2,20 @@ use super::context::{ Condition, ConditionOperator, EngineEvaluationContext, SegmentContext, SegmentRule, SegmentRuleType, }; -use crate::types::FlagsmithValue; +use super::result::FlagResult; +use crate::engine::{ + get_fallback_reason, get_flag_result_from_feature_context, wins_over, SegmentOverride, +}; +use crate::types::{FlagsmithValue, FlagsmithValueType}; use crate::utils::hashing; +use jsonpath_rust::parser::model::{JpQuery, Segment, Selector}; +use jsonpath_rust::parser::parse_json_path; +use jsonpath_rust::query::js_path_process; use regex::Regex; use semver::Version; -use serde_json_path::JsonPath; -use std::cell::{OnceCell, RefCell}; -use std::collections::HashMap; +use std::cell::{Cell, OnceCell, RefCell}; +use std::collections::{HashMap, HashSet}; +use std::rc::Rc; /// Evaluates segments against a single evaluation context. /// @@ -17,11 +24,47 @@ use std::collections::HashMap; /// segments and conditions: /// - the context is serialised to JSON at most once, and only if a JSONPath /// property is evaluated; -/// - each JSONPath property is parsed and resolved at most once. +/// - each JSONPath property is parsed at most once, and resolved at most once +/// unless it reads a flag. +/// +/// Built [`with_flag_dependencies`](Self::with_flag_dependencies), conditions +/// can also read other flags through `$.flags.` paths, each flag +/// resolved on first read. pub(crate) struct SegmentEvaluator<'a> { ec: &'a EngineEvaluationContext, context_json: OnceCell>, json_path_values: RefCell>>, + flag_dependencies: Option>, +} + +/// The flags read by segment conditions, resolved on first read and memoised +struct FlagDependencies<'a> { + identity_key: Option<&'a String>, + /// Keys of the segments overriding each feature, in evaluation order + segment_keys_by_feature_name: OnceCell>>, + segment_matches: RefCell>, + flag_paths: RefCell>>>, + /// Features being resolved, outermost first + resolving: RefCell>, + resolved: RefCell>, + cyclic: RefCell>, + /// Incremented whenever a cycle is cut, so that verdicts resting on one can be told apart + cycle_hits: Cell, + /// Whether a condition has read a flag, after which segment matches are memoised + used: Cell, +} + +/// A JSONPath reading the result of the feature it names, e.g. `$.flags.my_feature.enabled` +struct FlagPath { + feature_name: String, + json_path: JpQuery, +} + +/// Flags read by segment conditions, and the features found in a dependency cycle +#[derive(Default)] +pub(crate) struct ResolvedFlags<'a> { + pub(crate) flags: HashMap<&'a str, FlagResult>, + pub(crate) cyclic: HashSet<&'a str>, } impl<'a> SegmentEvaluator<'a> { @@ -30,9 +73,46 @@ impl<'a> SegmentEvaluator<'a> { ec, context_json: OnceCell::new(), json_path_values: RefCell::new(HashMap::new()), + flag_dependencies: None, } } + pub(crate) fn with_flag_dependencies( + ec: &'a EngineEvaluationContext, + identity_key: Option<&'a String>, + ) -> Self { + Self { + flag_dependencies: Some(FlagDependencies { + identity_key, + segment_keys_by_feature_name: OnceCell::new(), + segment_matches: RefCell::new(HashMap::new()), + flag_paths: RefCell::new(HashMap::new()), + resolving: RefCell::new(Vec::new()), + resolved: RefCell::new(HashMap::new()), + cyclic: RefCell::new(HashSet::new()), + cycle_hits: Cell::new(0), + used: Cell::new(false), + }), + ..Self::new(ec) + } + } + + /// Whether the context belongs to a segment; one matching, or not, on a flag in a dependency cycle does not + pub(crate) fn matches_segment_key(&self, segment_key: &'a str) -> bool { + match &self.flag_dependencies { + Some(dependencies) => self.matches_segment(dependencies, segment_key), + None => self.is_context_in_segment(&self.ec.segments[segment_key]), + } + } + + pub(crate) fn into_resolved_flags(self) -> ResolvedFlags<'a> { + self.flag_dependencies + .map_or_else(ResolvedFlags::default, |dependencies| ResolvedFlags { + flags: dependencies.resolved.into_inner(), + cyclic: dependencies.cyclic.into_inner(), + }) + } + /// Determines if the evaluation context matches the segment rules pub(crate) fn is_context_in_segment(&self, segment: &SegmentContext) -> bool { if segment.rules.is_empty() { @@ -66,39 +146,279 @@ impl<'a> SegmentEvaluator<'a> { fn resolve_jsonpath(&self, path: &str) -> Option { // Parse the JSONPath expression - let json_path = JsonPath::parse(path).ok()?; + let json_path = parse_json_path(path).ok()?; // Query the JSON using the path - let result = json_path.query(self.context_json()?); + query_first_value(&json_path, self.context_json()?) + } - // Get the first match (if any) - let value = *result.all().first()?; + /// Gets a value by member names straight from the context, or `None` for members only JSONPath can read + fn get_member_value(&self, names: &[&str]) -> Option> { + let string_value = |value: &String| FlagsmithValue { + value: value.clone(), + value_type: FlagsmithValueType::String, + }; + let identity = self.ec.identity.as_ref(); + Some(match names { + ["environment", "key"] => Some(string_value(&self.ec.environment.key)), + ["environment", "name"] => Some(string_value(&self.ec.environment.name)), + ["identity", "identifier"] => identity.map(|i| string_value(&i.identifier)), + ["identity", "key"] => identity.map(|i| string_value(&i.key)), + ["identity", "traits", trait_key] => identity + .and_then(|i| i.traits.get(*trait_key)) + .filter(|value| value.value_type != FlagsmithValueType::None) + .cloned(), + ["flags", feature_name, field @ ("enabled" | "value" | "variant")] => { + let dependencies = self.flag_dependencies.as_ref()?; + let Some((feature_name, _)) = self.ec.features.get_key_value(*feature_name) else { + return Some(None); + }; + self.read_flag(dependencies, feature_name, |flag| match *field { + "enabled" => Some(FlagsmithValue { + value: flag.enabled.to_string(), + value_type: FlagsmithValueType::Bool, + }), + "value" => Some(flag.value.clone()) + .filter(|value| value.value_type != FlagsmithValueType::None), + _ => flag.variant.as_ref().map(string_value), + }) + .flatten() + } + _ => return None, + }) + } + + /// Gets a value from the result of the feature a `$.flags` JSONPath names, resolving it on first read + fn get_value_from_flags( + &self, + dependencies: &FlagDependencies<'a>, + path: &str, + ) -> Option { + let Some(flag_path) = self.get_flag_path(dependencies, path) else { + return self.get_value_from_jsonpath(path); // Reads no flag, as the context holds none + }; + let (feature_name, _) = self.ec.features.get_key_value(&flag_path.feature_name)?; + let flag = self + .read_flag(dependencies, feature_name, |flag| { + serde_json::to_value(flag) + })? + .ok()?; + let flags = serde_json::Map::from_iter([(feature_name.clone(), flag)]); + let flags_json = serde_json::Value::Object(serde_json::Map::from_iter([( + "flags".to_string(), + serde_json::Value::Object(flags), + )])); + query_first_value(&flag_path.json_path, &flags_json) + } + + fn get_flag_path( + &self, + dependencies: &FlagDependencies<'a>, + path: &str, + ) -> Option> { + if let Some(flag_path) = dependencies.flag_paths.borrow().get(path) { + return flag_path.clone(); + } + let flag_path = parse_flag_path(path).map(Rc::new); + dependencies + .flag_paths + .borrow_mut() + .insert(path.to_string(), flag_path.clone()); + flag_path + } + + fn read_flag( + &self, + dependencies: &FlagDependencies<'a>, + feature_name: &'a str, + read: impl FnOnce(&FlagResult) -> T, + ) -> Option { + dependencies.used.set(true); + if !dependencies.resolved.borrow().contains_key(feature_name) { + self.resolve_feature(dependencies, feature_name); + } + dependencies.resolved.borrow().get(feature_name).map(read) + } - // Convert to FlagsmithValue based on the JSON type - match value { - serde_json::Value::String(s) => Some(FlagsmithValue { - value: s.clone(), - value_type: crate::types::FlagsmithValueType::String, - }), - serde_json::Value::Number(n) => { - if n.is_f64() { - Some(FlagsmithValue { - value: n.to_string(), - value_type: crate::types::FlagsmithValueType::Float, - }) - } else { - Some(FlagsmithValue { - value: n.to_string(), - value_type: crate::types::FlagsmithValueType::Integer, - }) + /// Resolves a flag, unless it depends on itself, or is absent from the context + fn resolve_feature(&self, dependencies: &FlagDependencies<'a>, feature_name: &'a str) { + let cycle_start = dependencies + .resolving + .borrow() + .iter() + .position(|name| *name == feature_name); + if let Some(cycle_start) = cycle_start { + let resolving = dependencies.resolving.borrow(); + dependencies + .cyclic + .borrow_mut() + .extend(&resolving[cycle_start..]); + dependencies + .cycle_hits + .set(dependencies.cycle_hits.get() + 1); + return; + } + + let Some(feature_context) = self.ec.features.get(feature_name) else { + return; + }; + + dependencies.resolving.borrow_mut().push(feature_name); + let segment_override = self.get_segment_override(dependencies, feature_name); + dependencies.resolving.borrow_mut().pop(); + + if dependencies.cyclic.borrow().contains(feature_name) { + return; // Not a result conditions may match on + } + + let flag_result = match segment_override { + Some(segment_override) => get_flag_result_from_feature_context( + segment_override.feature_context, + dependencies.identity_key, + format!("TARGETING_MATCH; segment={}", segment_override.segment_name), + ), + None => get_flag_result_from_feature_context( + feature_context, + dependencies.identity_key, + get_fallback_reason( + self.get_segment_keys_by_feature_name(dependencies) + .contains_key(feature_name), + ), + ), + }; + dependencies + .resolved + .borrow_mut() + .insert(feature_name, flag_result); + } + + fn get_segment_override( + &self, + dependencies: &FlagDependencies<'a>, + feature_name: &str, + ) -> Option> { + let mut segment_override: Option> = None; + + let segment_keys = self + .get_segment_keys_by_feature_name(dependencies) + .get(feature_name) + .map(Vec::as_slice) + .unwrap_or_default(); + for &segment_key in segment_keys { + if !self.matches_segment(dependencies, segment_key) { + continue; + } + let segment_context = &self.ec.segments[segment_key]; + for override_fc in &segment_context.overrides { + if override_fc.name == feature_name + && wins_over( + override_fc, + segment_override.as_ref().map(|o| o.feature_context), + ) + { + segment_override = Some(SegmentOverride { + feature_context: override_fc, + segment_name: &segment_context.name, + }); } } - serde_json::Value::Bool(b) => Some(FlagsmithValue { - value: b.to_string(), - value_type: crate::types::FlagsmithValueType::Bool, - }), - _ => None, } + + segment_override + } + + fn matches_segment(&self, dependencies: &FlagDependencies<'a>, segment_key: &'a str) -> bool { + if let Some(&matches) = dependencies.segment_matches.borrow().get(segment_key) { + return matches; + } + + let cycle_hits = dependencies.cycle_hits.get(); + let matches = self.is_context_in_segment(&self.ec.segments[segment_key]); + if dependencies.cycle_hits.get() != cycle_hits { + return false; // The verdict rests on a flag left unresolved by a cycle + } + + if dependencies.used.get() { + dependencies + .segment_matches + .borrow_mut() + .insert(segment_key, matches); + } + matches + } + + fn get_segment_keys_by_feature_name<'d>( + &self, + dependencies: &'d FlagDependencies<'a>, + ) -> &'d HashMap<&'a str, Vec<&'a str>> { + dependencies.segment_keys_by_feature_name.get_or_init(|| { + let mut segment_keys_by_feature_name: HashMap<&str, Vec<&str>> = HashMap::new(); + for (segment_key, segment_context) in &self.ec.segments { + for override_fc in &segment_context.overrides { + segment_keys_by_feature_name + .entry(override_fc.name.as_str()) + .or_default() + .push(segment_key); + } + } + segment_keys_by_feature_name + }) + } +} + +/// Parses a JSONPath reading the result of a feature, i.e. one selecting a single feature by name right after `$.flags` +fn parse_flag_path(path: &str) -> Option { + if !path.starts_with("$.flags") { + return None; // e.g. `$['flags']`, which may name a trait + } + let json_path = parse_json_path(path).ok()?; + let [Segment::Selector(Selector::Name(flags)), Segment::Selector(Selector::Name(feature_name)), ..] = + json_path.segments.as_slice() + else { + return None; + }; + if flags != "flags" { + return None; + } + Some(FlagPath { + feature_name: feature_name.clone(), + json_path, + }) +} + +/// Gets the first value a JSONPath selects, if it is a scalar +fn query_first_value(json_path: &JpQuery, json: &serde_json::Value) -> Option { + // Get the first match (if any) + let value = js_path_process(json_path, json) + .ok()? + .into_iter() + .next()? + .val(); + + // Convert to FlagsmithValue based on the JSON type + match value { + serde_json::Value::String(s) => Some(FlagsmithValue { + value: s.clone(), + value_type: crate::types::FlagsmithValueType::String, + }), + serde_json::Value::Number(n) => { + if n.is_f64() { + Some(FlagsmithValue { + value: n.to_string(), + value_type: crate::types::FlagsmithValueType::Float, + }) + } else { + Some(FlagsmithValue { + value: n.to_string(), + value_type: crate::types::FlagsmithValueType::Integer, + }) + } + } + serde_json::Value::Bool(b) => Some(FlagsmithValue { + value: b.to_string(), + value_type: crate::types::FlagsmithValueType::Bool, + }), + _ => None, } } @@ -214,11 +534,35 @@ fn context_matches_condition( } } +/// Splits a JSONPath made of member names only, e.g. `$.identity.identifier`, into those names +fn get_member_names(path: &str) -> Option> { + let names: Vec<&str> = path.strip_prefix("$.")?.split('.').collect(); + names + .iter() + .all(|name| { + let mut chars = name.chars(); + chars + .next() + .is_some_and(|c| c.is_ascii_alphabetic() || c == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') + }) + .then_some(names) +} + /// Gets a value from the context by property name or JSONPath fn get_context_value(evaluator: &SegmentEvaluator, property: &str) -> Option { // If property starts with $., try to parse it as a JSONPath expression if property.starts_with("$.") { - if let Some(value) = evaluator.get_value_from_jsonpath(property) { + let member_value = + get_member_names(property).and_then(|names| evaluator.get_member_value(&names)); + let value = match (member_value, &evaluator.flag_dependencies) { + (Some(value), _) => value, + (None, Some(dependencies)) if property.starts_with("$.flags") => { + evaluator.get_value_from_flags(dependencies, property) + } + (None, _) => evaluator.get_value_from_jsonpath(property), + }; + if let Some(value) = value { return Some(value); } // If JSONPath parsing fails, fall through to treat it as a trait name @@ -486,7 +830,7 @@ mod tests { rule_type: SegmentRuleType::All, conditions: vec![Condition { operator: ConditionOperator::In, - property: "$.identity.identifier".to_string(), + property: "$.identity['identifier']".to_string(), value: super::super::context::ConditionValue::Multiple( identifiers.iter().map(|i| i.to_string()).collect(), ), @@ -508,19 +852,28 @@ mod tests { let json_path_values = evaluator.json_path_values.borrow(); assert_eq!(json_path_values.len(), 1); assert_eq!( - json_path_values["$.identity.identifier"] + json_path_values["$.identity['identifier']"] .as_ref() .map(|v| v.value.as_str()), Some("user-1") ); } - #[test] - fn test_segment_evaluator_skips_serialisation_without_jsonpath() { + #[rstest::rstest] + #[case("plan", "pro")] + #[case("$.identity.traits.plan", "pro")] + #[case("$.identity.identifier", "user-1")] + #[case("$.identity.key", "env-key_user-1")] + #[case("$.environment.name", "Environment")] + #[case("$.environment.key", "env-key")] + fn test_segment_evaluator_skips_serialisation_without_jsonpath( + #[case] property: &str, + #[case] value: &str, + ) { // Given let ec: EngineEvaluationContext = serde_json::from_value(serde_json::json!({ "environment": {"key": "env-key", "name": "Environment"}, - "identity": {"identifier": "user-1", "traits": {"plan": "pro"}}, + "identity": {"identifier": "user-1", "key": "env-key_user-1", "traits": {"plan": "pro"}}, })) .unwrap(); let segment = SegmentContext { @@ -532,8 +885,8 @@ mod tests { rule_type: SegmentRuleType::All, conditions: vec![Condition { operator: ConditionOperator::Equal, - property: "plan".to_string(), - value: super::super::context::ConditionValue::Single("pro".to_string()), + property: property.to_string(), + value: super::super::context::ConditionValue::Single(value.to_string()), }], rules: vec![], }], diff --git a/tests/engine_eval_test.rs b/tests/engine_eval_test.rs index acf76c3..6a7b473 100644 --- a/tests/engine_eval_test.rs +++ b/tests/engine_eval_test.rs @@ -2,10 +2,16 @@ use flagsmith_flag_engine::engine::get_evaluation_result; use flagsmith_flag_engine::engine_eval::{EngineEvaluationContext, EvaluationResult}; use json_comments::StripComments; use rstest::*; -use serde_json; +use serde::Deserialize; use std::fs; use std::io::Read; +#[derive(Deserialize)] +struct TestCase { + context: EngineEvaluationContext, + result: EvaluationResult, +} + #[rstest] fn test_engine_evaluation() { // Get all test files @@ -42,26 +48,16 @@ fn test_engine_evaluation() { test_data }; - // Parse the JSON - let test_json: serde_json::Value = serde_json::from_str(&json_string) - .expect(&format!("Failed to parse JSON for: {}", test_name)); - - // Deserialize the context - let context: EngineEvaluationContext = serde_json::from_value(test_json["context"].clone()) - .unwrap_or_else(|e| panic!("Failed to deserialize context for {}: {:?}", test_name, e)); + // Deserialize the context and the expected result, keeping segments in context order + let TestCase { + context, + result: expected, + } = serde_json::from_str(&json_string) + .unwrap_or_else(|e| panic!("Failed to deserialize {}: {:?}", test_name, e)); // Get the evaluation result let result = get_evaluation_result(&context); - // Deserialize the expected result - let expected: EvaluationResult = serde_json::from_value(test_json["result"].clone()) - .unwrap_or_else(|e| { - panic!( - "Failed to deserialize expected result for {}: {:?}", - test_name, e - ) - }); - // Compare results - panic immediately on mismatch assert_eq!( result.flags, expected.flags, diff --git a/tests/engine_tests/engine-test-data b/tests/engine_tests/engine-test-data index 4b29dc7..cb06740 160000 --- a/tests/engine_tests/engine-test-data +++ b/tests/engine_tests/engine-test-data @@ -1 +1 @@ -Subproject commit 4b29dc772a764364af2dd504ecefbdf74cf5473f +Subproject commit cb067401093766e55c3354c1de7c73d9331334df diff --git a/tests/mappers_test.rs b/tests/mappers_test.rs index b4f6870..2b7ab03 100644 --- a/tests/mappers_test.rs +++ b/tests/mappers_test.rs @@ -1,8 +1,24 @@ +#![allow(non_snake_case)] + use flagsmith_flag_engine::engine_eval::{ context::{ConditionOperator, ConditionValue, SegmentRuleType, SegmentSource}, - environment_to_context, + environment_to_context, remove_identity_from_context, EngineEvaluationContext, }; use flagsmith_flag_engine::environments::Environment; +use rstest::*; +use serde_json::json; + +#[fixture] +fn identity_context(#[default(json!([]))] rules: serde_json::Value) -> EngineEvaluationContext { + serde_json::from_value(json!({ + "environment": {"key": "key", "name": "Production"}, + "identity": {"identifier": "user", "traits": {"plan": "pro"}}, + "segments": { + "targeting": {"key": "targeting", "name": "targeting", "rules": rules} + } + })) + .unwrap() +} fn get_environment_fixture() -> &'static str { r#"{ @@ -309,3 +325,50 @@ fn test_environment_to_context_produces_evaluation_context() { assert_eq!(mv_feature_without_ids.variants[2].value.value, "option_z"); assert_eq!(mv_feature_without_ids.variants[2].weight, 25.0); } + +#[rstest] +#[case("$.environment.name")] +#[case("$.environment['name']")] +#[case("$.flags.banner.enabled")] +#[case("$.flags[\"banner\"].enabled")] +fn test_remove_identity_from_context__segment_reading_environment_or_flags__keeps_segment( + #[case] property: &str, +) { + // Given + let context = identity_context(json!([{ + "type": "ALL", + "conditions": [{"operator": "IS_SET", "property": property, "value": null}] + }])); + + // When + let environment_context = remove_identity_from_context(&context); + + // Then + assert!(environment_context.identity.is_none()); + assert!(environment_context.segments.contains_key("targeting")); +} + +#[rstest] +#[case(json!([{"type": "ALL", "conditions": [{"operator": "IS_NOT_SET", "property": "plan", "value": null}]}]))] +#[case(json!([{"type": "ALL", "conditions": [{"operator": "EQUAL", "property": "$.identity.identifier", "value": "user"}]}]))] +#[case(json!([{"type": "ALL", "conditions": [{"operator": "PERCENTAGE_SPLIT", "property": "", "value": "100"}]}]))] +#[case(json!([{"type": "ALL", "conditions": [ + {"operator": "EQUAL", "property": "$.environment.name", "value": "Production"}, + {"operator": "EQUAL", "property": "plan", "value": "pro"} +]}]))] +#[case(json!([{"type": "ALL", "rules": [ + {"type": "NONE", "conditions": [{"operator": "EQUAL", "property": "plan", "value": "free"}]} +]}]))] +fn test_remove_identity_from_context__segment_needing_identity__drops_segment( + #[case] rules: serde_json::Value, +) { + // Given + let context = identity_context(rules); + + // When + let environment_context = remove_identity_from_context(&context); + + // Then + assert!(environment_context.identity.is_none()); + assert!(environment_context.segments.is_empty()); +}