feat: Add evaluation context #23

gagantrivedi · 2025-10-28T08:44:41Z

No description provided.

khvn26

Looks good overall. Left a handful of comments.

src/engine_eval/context.rs

khvn26 · 2025-11-05T19:26:23Z

src/engine_eval/mappers.rs

Note: Shipping the mapper layer for with the engine for a while makes sense to me. That's what we did with the Python engine at first, and it was easier to review and integrate. This also explains why the SDK metadata structs belong to here.

khvn26 · 2025-11-05T19:41:51Z

src/engine_eval/mappers.rs

+                conditions: vec![Condition {
+                    operator: ConditionOperator::In,
+                    property: "$.identity.identifier".to_string(),
+                    value: identifiers.join(","),


If we stick with the full serde route, I suggest serialising to a JSON list to support identifiers containing the , separator:

Suggested change

value: identifiers.join(","),

value: serde_json::to_string(identifiers.join),

Otherwise, I'd prefer to avoid extra marshalling:

Suggested change

value: identifiers.join(","),

value: ConditionValue::Values(identifiers),

khvn26 · 2025-11-05T20:01:03Z

src/engine_eval/context.rs

+// Helper function to deserialize value that can be a string or array
+fn deserialize_condition_value<'de, D>(deserializer: D) -> Result<String, D::Error>
+where
+    D: serde::Deserializer<'de>,
+{
+    use serde_json::Value;
+    let value: Value = serde::Deserialize::deserialize(deserializer)?;
+    Ok(match value {
+        Value::String(s) => s,
+        Value::Array(_) | Value::Object(_) | Value::Number(_) | Value::Bool(_) => {
+            // Serialize non-string values back to JSON string
+            serde_json::to_string(&value).unwrap_or_default()
+        }
+        Value::Null => String::new(),
+    })
+}
+
+/// Represents a condition for segment rule evaluation.
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct Condition {
+    /// The operator for this condition.
+    pub operator: ConditionOperator,
+    /// The property to evaluate (can be a JSONPath expression starting with $.).
+    pub property: String,
+    /// The value to compare against (can be a string or serialized JSON).
+    #[serde(deserialize_with = "deserialize_condition_value")]
+    pub value: String,
+}


Consider adding an untagged enum to support native vectors instead:

Suggested change

// Helper function to deserialize value that can be a string or array

fn deserialize_condition_value<'de, D>(deserializer: D) -> Result<String, D::Error>

where

D: serde::Deserializer<'de>,

{

use serde_json::Value;

let value: Value = serde::Deserialize::deserialize(deserializer)?;

Ok(match value {

Value::String(s) => s,

Value::Array(_) | Value::Object(_) | Value::Number(_) | Value::Bool(_) => {

// Serialize non-string values back to JSON string

serde_json::to_string(&value).unwrap_or_default()

}

Value::Null => String::new(),

})

}

/// Represents a condition for segment rule evaluation.

#[derive(Clone, Debug, Serialize, Deserialize)]

pub struct Condition {

/// The operator for this condition.

pub operator: ConditionOperator,

/// The property to evaluate (can be a JSONPath expression starting with $.).

pub property: String,

/// The value to compare against (can be a string or serialized JSON).

#[serde(deserialize_with = "deserialize_condition_value")]

pub value: String,

}

// Represents a segment condition value that can be a string (including a JSON-encoded array), or a native vector of strings.

#[derive(Clone, Debug, Serialize, Deserialize)]

#[serde(untagged)]

enum ConditionValue {

Value(String),

Values(Vec<String>),

}

/// Represents a condition for segment rule evaluation.

#[derive(Clone, Debug, Serialize, Deserialize)]

pub struct Condition {

/// The operator for this condition.

pub operator: ConditionOperator,

/// The property to evaluate (can be a JSONPath expression starting with $.).

pub property: String,

/// The value to compare against.

pub value: ConditionValue,

}

khvn26 · 2025-11-05T20:06:36Z

tests/engine_eval_test.rs

+        let entry = entry.expect("Failed to read directory entry");
+        let path = entry.path();
+
+        // Only process JSON and JSONC files
+        let extension = path.extension().and_then(|s| s.to_str());
+        if extension != Some("json") && extension != Some("jsonc") {
+            continue;
+        }
+
+        let test_name = path
+            .file_name()
+            .and_then(|n| n.to_str())
+            .unwrap_or("unknown");
+
+        test_count += 1;
+
+        // Read the test file
+        let test_data =
+            fs::read_to_string(&path).expect(&format!("Failed to read test file: {:?}", path));
+
+        // Strip comments if it's a JSONC file
+        let json_string = if extension == Some("jsonc") {
+            let mut stripped = String::new();
+            StripComments::new(test_data.as_bytes())
+                .read_to_string(&mut stripped)
+                .expect(&format!("Failed to strip comments from: {}", test_name));
+            stripped
+        } else {
+            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_result: Result<EngineEvaluationContext, _> =
+            serde_json::from_value(test_json["context"].clone());
+
+        if context_result.is_err() {
+            println!(
+                "FAIL {}: Failed to deserialize context: {:?}",
+                test_name,
+                context_result.err()
+            );
+            failed += 1;
+            failed_tests.push(test_name.to_string());
+            continue;
+        }
+
+        let context = context_result.unwrap();


nit: For cleaner test discovery, this could've been a macro, or a build script.

khvn26 · 2025-11-05T20:08:56Z

tests/engine_eval_test.rs

+                if actual_flag.enabled != expected_flag.enabled {
+                    println!(
+                        "FAIL {}: Flag '{}' enabled mismatch - got {}, expected {}",
+                        test_name, flag_name, actual_flag.enabled, expected_flag.enabled
+                    );
+                    success = false;
+                }
+
+                if actual_flag.value != expected_flag.value {
+                    println!(
+                        "FAIL {}: Flag '{}' value mismatch - got {:?}, expected {:?}",
+                        test_name, flag_name, actual_flag.value, expected_flag.value
+                    );
+                    success = false;


It's important to compare reasons as well:

Suggested change

if actual_flag.enabled != expected_flag.enabled {

println!(

"FAIL {}: Flag '{}' enabled mismatch - got {}, expected {}",

test_name, flag_name, actual_flag.enabled, expected_flag.enabled

);

success = false;

}

if actual_flag.value != expected_flag.value {

println!(

"FAIL {}: Flag '{}' value mismatch - got {:?}, expected {:?}",

test_name, flag_name, actual_flag.value, expected_flag.value

);

success = false;

if actual_flag.enabled != expected_flag.enabled {

println!(

"FAIL {}: Flag '{}' enabled mismatch - got {}, expected {}",

test_name, flag_name, actual_flag.enabled, expected_flag.enabled

);

success = false;

}

if actual_flag.value != expected_flag.value {

println!(

"FAIL {}: Flag '{}' value mismatch - got {:?}, expected {:?}",

test_name, flag_name, actual_flag.value, expected_flag.value

);

success = false;

if actual_flag.reason != expected_flag.reason {

println!(

"FAIL {}: Flag '{}' reason mismatch - got {:?}, expected {:?}",

test_name, flag_name, actual_flag.reason, expected_flag.reason

);

success = false;

khvn26 · 2025-11-05T20:09:41Z

tests/engine_eval_test.rs

+        );
+        success = false;
+    }
+


What about segment names?

- Implement ConditionValue enum to handle string or array values - Supports Single(String) and Multiple(Vec<String>) variants - Custom deserializer handles JSON arrays, JSON array strings, and comma-separated strings - Helper methods: as_string(), as_vec(), contains_string() - Simplify IN operator implementation - Use ConditionValue's contains_string() for string matching - Remove redundant JSON array parsing logic

- Fix collapsible_if warning: collapse nested if statements - Fix manual_strip warning: use strip_suffix instead of manual slicing - Fix unwrap_or_default warning: use or_default() instead of or_insert_with(Vec::new)

gagantrivedi force-pushed the feat/engine-eval-ctx branch 7 times, most recently from e9ab450 to c6b5a56 Compare October 28, 2025 10:26

wip: Add evaluation context

4f0a0fb

gagantrivedi force-pushed the feat/engine-eval-ctx branch from c6b5a56 to 4f0a0fb Compare October 30, 2025 08:57

gagantrivedi changed the title ~~wip: Add evaluation context~~ feat: Add evaluation context Oct 30, 2025

gagantrivedi marked this pull request as ready for review October 30, 2025 09:21

gagantrivedi requested a review from a team as a code owner October 30, 2025 09:21

gagantrivedi requested review from khvn26 and removed request for a team October 30, 2025 09:21

khvn26 requested changes Nov 5, 2025

View reviewed changes

gagantrivedi added 3 commits November 6, 2025 11:59

fix multivariate segment override

b95fec7

chore: Fix clippy warnings in engine_eval module

0525956

- Fix collapsible_if warning: collapse nested if statements - Fix manual_strip warning: use strip_suffix instead of manual slicing - Fix unwrap_or_default warning: use or_default() instead of or_insert_with(Vec::new)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Add evaluation context #23

feat: Add evaluation context #23

Uh oh!

gagantrivedi commented Oct 28, 2025

Uh oh!

khvn26 left a comment

Uh oh!

Uh oh!

Uh oh!

khvn26 Nov 5, 2025

Uh oh!

khvn26 Nov 5, 2025

Uh oh!

khvn26 Nov 5, 2025

Uh oh!

khvn26 Nov 5, 2025

Uh oh!

khvn26 Nov 5, 2025

Uh oh!

khvn26 Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	value: identifiers.join(","),
	value: serde_json::to_string(identifiers.join),

	value: identifiers.join(","),
	value: ConditionValue::Values(identifiers),

feat: Add evaluation context #23

Are you sure you want to change the base?

feat: Add evaluation context #23

Uh oh!

Conversation

gagantrivedi commented Oct 28, 2025

Uh oh!

khvn26 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

khvn26 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

khvn26 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants