From 06f36c4d7c363652878ba9bb8cc2c794be9a67ed Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 30 Mar 2026 17:20:20 +0200 Subject: [PATCH 1/3] fix(fractional): support nested JSON Logic, boolean names, negative weight clamping, null key error Three related fixes to the fractional operator: 1. Evaluate inner bucket elements as JSON Logic Previously bucket name/weight inner elements were taken as literal JSON values after the outer array was evaluated. Now each inner element is evaluated through the JSON Logic evaluator, enabling nested expressions: - [{'if': [{'==': [{'var': 'tier'}, 'premium']}, 'premium', 'standard']}, 50] - [{'var': 'color'}, 50] and boolean literals used when fractional is a condition: - [false, 0], [true, 100] 2. Clamp negative weights to zero Previously a negative weight triggered a hard error. Negative values are now clamped to 0 so the bucket participates in the name list but receives no traffic, matching the expected spec behavior. 3. Return error on null bucket key expression When the first argument is a JSON Logic expression that evaluates to null or a non-string value, return an error so the flag engine falls back to the defaultVariant. The existing implicit fallback (flagKey+targetingKey) is preserved only for array literals as the first argument (no-seed form). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner --- src/operators/fractional.rs | 214 ++++++++++++++++++++++-------------- 1 file changed, 132 insertions(+), 82 deletions(-) diff --git a/src/operators/fractional.rs b/src/operators/fractional.rs index 24d0286..af5447e 100644 --- a/src/operators/fractional.rs +++ b/src/operators/fractional.rs @@ -27,27 +27,39 @@ impl Operator for FractionalOperator { )); } - // Evaluate the first argument to determine bucketing key logic + // Evaluate the first argument to determine bucketing key logic. + // If the first arg is an Array literal, treat all args as buckets (no explicit seed). + // If the first arg is an expression that evaluates to a String, use it as seed. + // If the first arg is an expression that evaluates to null/non-string, return an error + // so the flag engine falls back to the defaultVariant. let evaluated_first = evaluator.evaluate(&args[0], context)?; - let (bucket_key, start_index) = if let Value::String(s) = &evaluated_first { - // Explicit bucketing key provided - (s.clone(), 1) - } else { - // Fallback: use flagKey + targetingKey from context data - let data = context.root().data().clone(); - let targeting_key = data - .get("targetingKey") - .and_then(|v| v.as_str()) - .unwrap_or(""); - let flag_key = data - .get("$flagd") - .and_then(|v| v.get("flagKey")) - .and_then(|v| v.as_str()) - .unwrap_or(""); - (format!("{}{}", flag_key, targeting_key), 0) + let (bucket_key, start_index) = match (&args[0], &evaluated_first) { + (_, Value::String(s)) => (s.clone(), 1), + (Value::Array(_), _) => { + // First arg is explicitly an array — no seed provided, use flagKey+targetingKey + let data = context.root().data().clone(); + let targeting_key = data + .get("targetingKey") + .and_then(|v| v.as_str()) + .unwrap_or(""); + let flag_key = data + .get("$flagd") + .and_then(|v| v.get("flagKey")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + (format!("{}{}", flag_key, targeting_key), 0) + } + _ => { + // Expression resolved to null or non-string — no valid seed; signal fallback + return Err(DataLogicError::Custom( + "fractional: bucket key expression resolved to a non-string value".into(), + )); + } }; - // Parse bucket definitions from remaining arguments + // Parse bucket definitions from remaining arguments. + // Each inner element (name and weight) is evaluated through JSON Logic so that + // nested expressions like {"if": [...]} and {"var": "..."} are resolved first. let mut bucket_values: Vec = Vec::new(); if start_index == 1 && args.len() == 2 { @@ -66,13 +78,14 @@ impl Operator for FractionalOperator { for arg in &args[start_index..] { let evaluated = evaluator.evaluate(arg, context)?; if let Some(bucket_def) = evaluated.as_array() { - // Each bucket is [name, weight] or [name] (weight=1) + // Each bucket is [name, weight] or [name] (weight=1). + // Evaluate each inner element so nested JSON Logic is resolved. if bucket_def.len() >= 2 { - bucket_values.push(bucket_def[0].clone()); - bucket_values.push(bucket_def[1].clone()); + bucket_values.push(evaluator.evaluate(&bucket_def[0], context)?); + bucket_values.push(evaluator.evaluate(&bucket_def[1], context)?); } else if bucket_def.len() == 1 { // Shorthand: [name] implies weight of 1 - bucket_values.push(bucket_def[0].clone()); + bucket_values.push(evaluator.evaluate(&bucket_def[0], context)?); bucket_values.push(Value::Number(1.into())); } } else { @@ -85,7 +98,7 @@ impl Operator for FractionalOperator { } match fractional(&bucket_key, &bucket_values) { - Ok(bucket_name) => Ok(Value::String(bucket_name)), + Ok(value) => Ok(value), Err(e) => Err(DataLogicError::Custom(e)), } } @@ -97,6 +110,13 @@ impl Operator for FractionalOperator { /// a list of bucket definitions with integer weights. It uses consistent hashing /// to always assign the same bucket key to the same bucket. /// +/// Bucket names may be any JSON scalar (string, boolean, number). The name is +/// serialised to its JSON representation for display/return, while the hash is +/// computed on the bucket key string only. +/// +/// Negative weights are clamped to zero (the bucket still participates in the +/// name list but never receives any traffic). +/// /// # Algorithm /// /// Uses high-resolution integer arithmetic instead of float-based percentage @@ -107,58 +127,67 @@ impl Operator for FractionalOperator { /// bucket = (u64(hash) * u64(totalWeight)) >> 32 /// ``` /// -/// Weights must be non-negative integers summing to at most `i32::MAX` -/// (2,147,483,647). +/// Weights must sum to at most `i32::MAX` (2,147,483,647). /// /// # Arguments /// * `bucket_key` - The key to use for bucket assignment (e.g., user ID) /// * `buckets` - Array of [name, weight, name, weight, ...] values /// /// # Returns -/// The name of the selected bucket, or an error if the input is invalid -/// -/// # Example -/// ```json -/// {"fractional": ["user123", ["control", 50, "treatment", 50]]} -/// ``` -/// This will consistently assign "user123" to either "control" or "treatment" -/// based on its hash value. -pub fn fractional(bucket_key: &str, buckets: &[Value]) -> Result { +/// The value of the selected bucket as a `serde_json::Value`, or an error if +/// the input is invalid. +pub fn fractional(bucket_key: &str, buckets: &[Value]) -> Result { if buckets.is_empty() { return Err("Fractional operator requires at least one bucket".to_string()); } // Parse bucket definitions: [name1, weight1, name2, weight2, ...] - let mut bucket_defs: Vec<(String, u64)> = Vec::new(); + let mut bucket_defs: Vec<(Value, u64)> = Vec::new(); let mut total_weight: u64 = 0; let mut i = 0; while i < buckets.len() { - // Get bucket name - let name = match &buckets[i] { - Value::String(s) => s.clone(), - _ => return Err(format!("Bucket name at index {} must be a string", i)), - }; + // Accept any scalar JSON value as a bucket name. + let name_value = buckets[i].clone(); + if matches!(name_value, Value::Object(_) | Value::Array(_)) { + return Err(format!( + "Bucket name at index {} must be a scalar value (string, boolean, or number), got a complex type", + i + )); + } i += 1; - // Get bucket weight + // Get bucket weight — negative weights are clamped to zero. if i >= buckets.len() { - return Err(format!("Missing weight for bucket '{}'", name)); + return Err(format!("Missing weight for bucket at index {}", i - 1)); } - let weight = match &buckets[i] { - Value::Number(n) => n.as_u64().ok_or_else(|| { - format!("Weight for bucket '{}' must be a positive integer", name) - })?, - _ => return Err(format!("Weight for bucket '{}' must be a number", name)), + let weight: u64 = match &buckets[i] { + Value::Number(n) => { + if let Some(u) = n.as_u64() { + u + } else if let Some(signed) = n.as_i64() { + // Negative integer — clamp to zero + signed.max(0) as u64 + } else { + // Float — round down, clamp to zero + n.as_f64().unwrap_or(0.0).max(0.0) as u64 + } + } + _ => { + return Err(format!( + "Weight for bucket at index {} must be a number", + i + )) + } }; total_weight = total_weight .checked_add(weight) .ok_or_else(|| "Total weight overflow".to_string())?; - bucket_defs.push((name, weight)); + bucket_defs.push((name_value, weight)); i += 1; } @@ -183,22 +212,25 @@ pub fn fractional(bucket_key: &str, buckets: &[Value]) -> Result let hash: u32 = murmurhash3_x86_32(bucket_key.as_bytes(), 0); // Map the 32-bit hash uniformly into [0, totalWeight) using integer arithmetic. - // This replaces the previous float-based approach (abs(hash)/i32::MAX * 100) - // with higher resolution and no floating-point imprecision. let bucket_value: u64 = (hash as u64 * total_weight) >> 32; - // Find which bucket this value falls into by accumulating weights + // Find which bucket this value falls into by accumulating weights. + // Buckets with zero weight are skipped (their cumulative weight never advances). let mut cumulative_weight: u64 = 0; - for (name, weight) in &bucket_defs { + for (value, weight) in &bucket_defs { cumulative_weight += weight; if bucket_value < cumulative_weight { - return Ok(name.clone()); + return Ok(value.clone()); } } - // Unreachable for valid inputs: bucket_value < total_weight is always true - // since (hash * total_weight) >> 32 < total_weight. Fall back defensively. - Ok(bucket_defs.last().unwrap().0.clone()) + // Unreachable for valid inputs, but fall back to last non-zero bucket defensively. + Ok(bucket_defs + .iter() + .rev() + .find(|(_, w)| *w > 0) + .map(|(v, _)| v.clone()) + .unwrap_or(Value::Null)) } #[cfg(test)] @@ -223,13 +255,10 @@ mod tests { let mut seen_control = false; let mut seen_treatment = false; for i in 0..100 { - match fractional(&format!("user-{}", i), &buckets) - .unwrap() - .as_str() - { - "control" => seen_control = true, - "treatment" => seen_treatment = true, - other => panic!("Unexpected bucket: {}", other), + match fractional(&format!("user-{}", i), &buckets).unwrap() { + Value::String(s) if s == "control" => seen_control = true, + Value::String(s) if s == "treatment" => seen_treatment = true, + other => panic!("Unexpected bucket: {:?}", other), } } assert!(seen_control, "control bucket must be reachable"); @@ -243,13 +272,10 @@ mod tests { let mut small_count = 0u32; let mut large_count = 0u32; for i in 0..1000 { - match fractional(&format!("user-{}", i), &buckets) - .unwrap() - .as_str() - { - "small" => small_count += 1, - "large" => large_count += 1, - other => panic!("Unexpected bucket: {}", other), + match fractional(&format!("user-{}", i), &buckets).unwrap() { + Value::String(s) if s == "small" => small_count += 1, + Value::String(s) if s == "large" => large_count += 1, + other => panic!("Unexpected bucket: {:?}", other), } } // 90/10 split — large should dominate @@ -275,9 +301,9 @@ mod tests { let mut counts = std::collections::HashMap::new(); for i in 0..3000 { let r = fractional(&format!("u-{}", i), &buckets).unwrap(); - *counts.entry(r).or_insert(0u32) += 1; + *counts.entry(r.to_string()).or_insert(0u32) += 1; } - for bucket in ["a", "b", "c"] { + for bucket in ["\"a\"", "\"b\"", "\"c\""] { let c = counts.get(bucket).copied().unwrap_or(0); // Each should get roughly 1/3; allow generous tolerance assert!( @@ -329,7 +355,8 @@ mod tests { #[test] fn test_fractional_invalid_name_type() { - let buckets = vec![json!(123), json!(50)]; + // Complex types (objects/arrays) are rejected as bucket names + let buckets = vec![json!({"key": "value"}), json!(50)]; assert!(fractional("user-123", &buckets).is_err()); } @@ -342,14 +369,13 @@ mod tests { #[test] fn test_fractional_single_bucket() { let buckets = vec![json!("only"), json!(100)]; - assert_eq!(fractional("any-key", &buckets).unwrap(), "only"); - assert_eq!(fractional("another-key", &buckets).unwrap(), "only"); + assert_eq!(fractional("any-key", &buckets).unwrap(), json!("only")); + assert_eq!(fractional("another-key", &buckets).unwrap(), json!("only")); } #[test] fn test_fractional_integer_arithmetic_matches_spec() { // Verify the formula: bucket_value = (hash as u64 * totalWeight) >> 32 - // for a known hash, so the algorithm is pinned against regression. let key = "test-key"; let hash = murmurhash3_x86_32(key.as_bytes(), 0); let total_weight: u64 = 100; @@ -359,13 +385,37 @@ mod tests { let result = fractional(key, &buckets).unwrap(); let expected = if expected_bucket_value < 50 { - "low" + json!("low") } else { - "high" + json!("high") }; - assert_eq!( - result, expected, - "bucket assignment must match the spec formula" - ); + assert_eq!(result, expected, "bucket assignment must match the spec formula"); + } + + #[test] + fn test_fractional_boolean_bucket_names() { + // Boolean values are valid bucket names — used when fractional is a condition + let buckets = vec![json!(false), json!(0u64), json!(true), json!(100u64)]; + let result = fractional("any-key", &buckets).unwrap(); + assert_eq!(result, json!(true), "100% weight on true must always select true"); + } + + #[test] + fn test_fractional_negative_weight_clamped_to_zero() { + // Negative weight is clamped to 0; the other bucket gets 100% of traffic + let buckets = vec![ + json!("one"), + Value::Number(serde_json::Number::from(-50i64)), + json!("two"), + json!(100), + ]; + let result = fractional("any-key", &buckets).unwrap(); + assert_eq!(result, json!("two"), "negative weight must be clamped to 0"); + } + + #[test] + fn test_fractional_all_zero_weights_error() { + let buckets = vec![json!("one"), json!(0), json!("two"), json!(0)]; + assert!(fractional("any-key", &buckets).is_err()); } } From bc065836b3c6416586cd4a96d8e72e37ac8cd0bf Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 30 Mar 2026 17:27:41 +0200 Subject: [PATCH 2/3] refactor(fractional): address Gemini review feedback - Avoid unnecessary .clone() on context data by holding root reference - Replace manual while-loop with idiomatic chunks(2) iterator Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner --- src/operators/fractional.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/operators/fractional.rs b/src/operators/fractional.rs index af5447e..02e9f35 100644 --- a/src/operators/fractional.rs +++ b/src/operators/fractional.rs @@ -37,7 +37,8 @@ impl Operator for FractionalOperator { (_, Value::String(s)) => (s.clone(), 1), (Value::Array(_), _) => { // First arg is explicitly an array — no seed provided, use flagKey+targetingKey - let data = context.root().data().clone(); + let root = context.root(); + let data = root.data(); let targeting_key = data .get("targetingKey") .and_then(|v| v.as_str()) @@ -145,25 +146,22 @@ pub fn fractional(bucket_key: &str, buckets: &[Value]) -> Result let mut bucket_defs: Vec<(Value, u64)> = Vec::new(); let mut total_weight: u64 = 0; - let mut i = 0; - while i < buckets.len() { + for (i, chunk) in buckets.chunks(2).enumerate() { // Accept any scalar JSON value as a bucket name. - let name_value = buckets[i].clone(); + let name_value = chunk[0].clone(); if matches!(name_value, Value::Object(_) | Value::Array(_)) { return Err(format!( "Bucket name at index {} must be a scalar value (string, boolean, or number), got a complex type", - i + i * 2 )); } - i += 1; - - // Get bucket weight — negative weights are clamped to zero. - if i >= buckets.len() { - return Err(format!("Missing weight for bucket at index {}", i - 1)); + if chunk.len() < 2 { + return Err(format!("Missing weight for bucket at index {}", i * 2)); } - let weight: u64 = match &buckets[i] { + // Get bucket weight — negative weights are clamped to zero. + let weight: u64 = match &chunk[1] { Value::Number(n) => { if let Some(u) = n.as_u64() { u @@ -178,7 +176,7 @@ pub fn fractional(bucket_key: &str, buckets: &[Value]) -> Result _ => { return Err(format!( "Weight for bucket at index {} must be a number", - i + i * 2 + 1 )) } }; @@ -188,7 +186,6 @@ pub fn fractional(bucket_key: &str, buckets: &[Value]) -> Result .ok_or_else(|| "Total weight overflow".to_string())?; bucket_defs.push((name_value, weight)); - i += 1; } if bucket_defs.is_empty() { From 8ae755d1774dfdefd266bb2ad29486910d48c678 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Mon, 30 Mar 2026 17:44:56 +0200 Subject: [PATCH 3/3] fix(fractional): fix integration tests after return type change to Value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update two callers in integration_tests.rs that assumed fractional() returned String — now it returns serde_json::Value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner --- .gitignore | 1 + src/operators/fractional.rs | 11 +++++++++-- testbed | 2 +- tests/integration_tests.rs | 6 +++--- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index 2e7a432..8c94ae8 100644 --- a/.gitignore +++ b/.gitignore @@ -47,3 +47,4 @@ temp/ Thumbs.db ehthumbs.db Desktop.ini +worktrees/ diff --git a/src/operators/fractional.rs b/src/operators/fractional.rs index 02e9f35..b2dd14e 100644 --- a/src/operators/fractional.rs +++ b/src/operators/fractional.rs @@ -386,7 +386,10 @@ mod tests { } else { json!("high") }; - assert_eq!(result, expected, "bucket assignment must match the spec formula"); + assert_eq!( + result, expected, + "bucket assignment must match the spec formula" + ); } #[test] @@ -394,7 +397,11 @@ mod tests { // Boolean values are valid bucket names — used when fractional is a condition let buckets = vec![json!(false), json!(0u64), json!(true), json!(100u64)]; let result = fractional("any-key", &buckets).unwrap(); - assert_eq!(result, json!(true), "100% weight on true must always select true"); + assert_eq!( + result, + json!(true), + "100% weight on true must always select true" + ); } #[test] diff --git a/testbed b/testbed index b51d723..6f0d50a 160000 --- a/testbed +++ b/testbed @@ -1 +1 @@ -Subproject commit b51d723f9a1e09b44ebeca73ff02c98d5ef9fb51 +Subproject commit 6f0d50a7af8e658f0d5762307fb8f10258fa752c diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index ece505b..e3d1b99 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -1616,8 +1616,8 @@ fn test_fractional_distribution_uniformity() { // 100k sequential keys — dense sample, fast, and tight enough for 10% tolerance. for i in 0u64..100_000 { let key = format!("{}", i); - let bucket_str = fractional(&key, &bucket_defs).unwrap(); - let bucket_idx: usize = bucket_str.parse().unwrap(); + let bucket_val = fractional(&key, &bucket_defs).unwrap(); + let bucket_idx: usize = bucket_val.as_str().unwrap().parse().unwrap(); hits[bucket_idx] += 1; } @@ -1656,7 +1656,7 @@ fn test_fractional_boundary_hashes_do_not_panic() { let result = fractional(key, &buckets); assert!(result.is_ok(), "key={key:?} should not error: {:?}", result); assert!( - ["a", "b", "c", "d"].contains(&result.unwrap().as_str()), + ["a", "b", "c", "d"].contains(&result.unwrap().as_str().unwrap()), "key={key:?} must map to a valid bucket" ); }