From 013130990c1c3890dbb29af4dd93107903a63c84 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 30 Oct 2026 15:57:35 -0300 Subject: [PATCH 1/4] refactor: Query JSONPath conditions with jsonpath-rust serde_json_path keeps parsed paths private, while jsonpath-rust exposes them, so conditions can tell which flag a path reads. Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.toml | 2 +- src/engine_eval/segment_evaluator.rs | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 303f09e..ca93718 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,7 @@ maintenance = { status = "actively-developed" } [dependencies] serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" -serde_json_path = "0.7" +jsonpath-rust = "1.0" chrono = { version = "0.4", features = ["serde"] } md-5 = "0.10.1" num-bigint = "0.4" diff --git a/src/engine_eval/segment_evaluator.rs b/src/engine_eval/segment_evaluator.rs index 793f83f..537d1c7 100644 --- a/src/engine_eval/segment_evaluator.rs +++ b/src/engine_eval/segment_evaluator.rs @@ -4,9 +4,10 @@ use super::context::{ }; use crate::types::FlagsmithValue; use crate::utils::hashing; +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; @@ -66,13 +67,13 @@ 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()?); + let result = js_path_process(&json_path, self.context_json()?).ok()?; // Get the first match (if any) - let value = *result.all().first()?; + let value = result.first()?.clone().val(); // Convert to FlagsmithValue based on the JSON type match value { From 51052f5eeaee60e222296b6a52a534b266576fe6 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 30 Oct 2026 16:41:34 -0300 Subject: [PATCH 2/4] feat: Evaluate flags that depend on other flags Segment conditions can read another flag's `enabled`, `value` or `variant` through `$.flags.`, as in flagsmith-engine. Flags in a dependency cycle get `ERROR; code=CIRCULAR_DEPENDENCY`. Also, as engine-test-data v3.12.1 expects: - Flag results report the selected multivariate variant. - Untargeted flags report `STATIC`, and targeted ones left at their default `DEFAULT`. - Segments evaluate in context order, which also breaks override priority ties. Co-Authored-By: Claude Opus 5.5 (1M context) --- .gitmodules | 2 +- Cargo.toml | 1 + src/engine.rs | 192 +++++++++------ src/engine_eval/context.rs | 8 +- src/engine_eval/mappers.rs | 4 +- src/engine_eval/result.rs | 6 +- src/engine_eval/segment_evaluator.rs | 336 ++++++++++++++++++++++++--- tests/engine_eval_test.rs | 30 ++- tests/engine_tests/engine-test-data | 2 +- 9 files changed, 452 insertions(+), 129 deletions(-) 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 ca93718..1bd2baa 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,7 @@ maintenance = { status = "actively-developed" } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" 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..2ff2363 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,27 +18,42 @@ 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_context in ec.segments.values() { + targeted_feature_names.extend(segment_context.overrides.iter().map(|o| o.name.as_str())); - if !evaluator.is_context_in_segment(segment_context) { - continue; + let cycle_hits = evaluator.cycle_hits(); + if !evaluator.is_context_in_segment(segment_context) || evaluator.cycle_hits() != cycle_hits + { + continue; // Unmatched, or the verdict rests on a flag left unresolved by a cycle } // Add segment to results @@ -46,39 +64,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 +130,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 +187,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 +202,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 +227,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..7403ef5 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, 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 537d1c7..7548a87 100644 --- a/src/engine_eval/segment_evaluator.rs +++ b/src/engine_eval/segment_evaluator.rs @@ -2,14 +2,20 @@ use super::context::{ Condition, ConditionOperator, EngineEvaluationContext, SegmentContext, SegmentRule, SegmentRuleType, }; +use super::result::FlagResult; +use crate::engine::{ + get_fallback_reason, get_flag_result_from_feature_context, wins_over, SegmentOverride, +}; use crate::types::FlagsmithValue; 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 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. /// @@ -18,11 +24,45 @@ 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, +} + +/// 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> { @@ -31,9 +71,44 @@ 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), + }), + ..Self::new(ec) + } + } + + /// How many times a dependency cycle has been cut so far + pub(crate) fn cycle_hits(&self) -> usize { + self.flag_dependencies + .as_ref() + .map_or(0, |dependencies| dependencies.cycle_hits.get()) + } + + 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() { @@ -70,36 +145,233 @@ impl<'a> SegmentEvaluator<'a> { let json_path = parse_json_path(path).ok()?; // Query the JSON using the path - let result = js_path_process(&json_path, self.context_json()?).ok()?; + query_first_value(&json_path, self.context_json()?) + } - // Get the first match (if any) - let value = result.first()?.clone().val(); + /// 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)?; + 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 + } - // 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, - }) + fn read_flag( + &self, + dependencies: &FlagDependencies<'a>, + feature_name: &'a str, + ) -> Option { + if !dependencies.resolved.borrow().contains_key(feature_name) { + self.resolve_feature(dependencies, feature_name); + } + let resolved = dependencies.resolved.borrow(); + serde_json::to_value(resolved.get(feature_name)?).ok() + } + + /// 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 + } + + 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, } } @@ -219,7 +491,13 @@ fn context_matches_condition( 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 value = match &evaluator.flag_dependencies { + Some(dependencies) if property.starts_with("$.flags") => { + evaluator.get_value_from_flags(dependencies, property) + } + _ => 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 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 From 0552139222db80ea1ec16ec5a6a7e4ae5912bcc5 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 30 Oct 2026 16:41:34 -0300 Subject: [PATCH 3/4] feat: Get the context environment flags evaluate against Environment flags can't resolve segments that need an identity, so `remove_identity_from_context` keeps only those reading the environment or other flags, as Flagsmith does for `/api/v1/flags`. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/engine_eval/mappers.rs | 33 +++++++++++++++++++ src/engine_eval/mod.rs | 2 +- tests/mappers_test.rs | 65 +++++++++++++++++++++++++++++++++++++- 3 files changed, 98 insertions(+), 2 deletions(-) diff --git a/src/engine_eval/mappers.rs b/src/engine_eval/mappers.rs index 7403ef5..cb8835d 100644 --- a/src/engine_eval/mappers.rs +++ b/src/engine_eval/mappers.rs @@ -342,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/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()); +} From 92e19652b656f489769bdd62187a440f594a3c73 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 30 Oct 2026 18:53:24 -0300 Subject: [PATCH 4/4] perf(Flag Dependency): Avoid repeated work when evaluating segments Read JSONPaths made of member names only, such as `$.identity.identifier` or `$.flags..enabled`, straight from the context, without serialising it or evaluating JSONPath. Other paths are still evaluated by jsonpath-rust. Reuse the segment matches memoised while resolving flag dependencies when evaluating segments, so segments read through a dependency are evaluated once. Memoise only once a condition has read a flag. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/engine.rs | 8 +- src/engine_eval/segment_evaluator.rs | 124 +++++++++++++++++++++------ 2 files changed, 102 insertions(+), 30 deletions(-) diff --git a/src/engine.rs b/src/engine.rs index 2ff2363..8d26a5f 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -47,13 +47,11 @@ fn evaluate_segments<'a>( let mut segment_overrides: HashMap<&str, SegmentOverride> = HashMap::new(); let mut targeted_feature_names = HashSet::new(); - for segment_context in ec.segments.values() { + for (segment_key, segment_context) in &ec.segments { targeted_feature_names.extend(segment_context.overrides.iter().map(|o| o.name.as_str())); - let cycle_hits = evaluator.cycle_hits(); - if !evaluator.is_context_in_segment(segment_context) || evaluator.cycle_hits() != cycle_hits - { - continue; // Unmatched, or the verdict rests on a flag left unresolved by a cycle + if !evaluator.matches_segment_key(segment_key) { + continue; } // Add segment to results diff --git a/src/engine_eval/segment_evaluator.rs b/src/engine_eval/segment_evaluator.rs index 7548a87..028ba2a 100644 --- a/src/engine_eval/segment_evaluator.rs +++ b/src/engine_eval/segment_evaluator.rs @@ -6,7 +6,7 @@ use super::result::FlagResult; use crate::engine::{ get_fallback_reason, get_flag_result_from_feature_context, wins_over, SegmentOverride, }; -use crate::types::FlagsmithValue; +use crate::types::{FlagsmithValue, FlagsmithValueType}; use crate::utils::hashing; use jsonpath_rust::parser::model::{JpQuery, Segment, Selector}; use jsonpath_rust::parser::parse_json_path; @@ -50,6 +50,8 @@ struct FlagDependencies<'a> { 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` @@ -89,16 +91,18 @@ impl<'a> SegmentEvaluator<'a> { resolved: RefCell::new(HashMap::new()), cyclic: RefCell::new(HashSet::new()), cycle_hits: Cell::new(0), + used: Cell::new(false), }), ..Self::new(ec) } } - /// How many times a dependency cycle has been cut so far - pub(crate) fn cycle_hits(&self) -> usize { - self.flag_dependencies - .as_ref() - .map_or(0, |dependencies| dependencies.cycle_hits.get()) + /// 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> { @@ -148,6 +152,42 @@ impl<'a> SegmentEvaluator<'a> { query_first_value(&json_path, self.context_json()?) } + /// 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, @@ -158,7 +198,11 @@ impl<'a> SegmentEvaluator<'a> { 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)?; + 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(), @@ -183,16 +227,17 @@ impl<'a> SegmentEvaluator<'a> { flag_path } - fn read_flag( + fn read_flag( &self, dependencies: &FlagDependencies<'a>, feature_name: &'a str, - ) -> Option { + read: impl FnOnce(&FlagResult) -> T, + ) -> Option { + dependencies.used.set(true); if !dependencies.resolved.borrow().contains_key(feature_name) { self.resolve_feature(dependencies, feature_name); } - let resolved = dependencies.resolved.borrow(); - serde_json::to_value(resolved.get(feature_name)?).ok() + dependencies.resolved.borrow().get(feature_name).map(read) } /// Resolves a flag, unless it depends on itself, or is absent from the context @@ -293,10 +338,12 @@ impl<'a> SegmentEvaluator<'a> { return false; // The verdict rests on a flag left unresolved by a cycle } - dependencies - .segment_matches - .borrow_mut() - .insert(segment_key, matches); + if dependencies.used.get() { + dependencies + .segment_matches + .borrow_mut() + .insert(segment_key, matches); + } matches } @@ -487,15 +534,33 @@ 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("$.") { - let value = match &evaluator.flag_dependencies { - Some(dependencies) if property.starts_with("$.flags") => { + 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) } - _ => evaluator.get_value_from_jsonpath(property), + (None, _) => evaluator.get_value_from_jsonpath(property), }; if let Some(value) = value { return Some(value); @@ -765,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(), ), @@ -787,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 { @@ -811,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![], }],