From 106601fe846d3b8a5c6ce462db5167aabba5ec80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yusuf=20To=CC=88r?= <3296904+yusuftor@users.noreply.github.com> Date: Wed, 21 Jan 2026 16:01:05 +0100 Subject: [PATCH 1/4] Simplify string normalization - don't convert numeric strings to numbers This is a simpler fix for the string comparison issues. Instead of the complex `extract_string_compared_variables` mechanism that tried to selectively skip normalization, we now simply don't convert numeric strings to numbers at all. Changes: - Modified `normalize_variables` to only convert "true"/"false" strings to booleans, leaving all other strings as strings - Modified `normalize_ast_variables` similarly for AST atoms - Removed `extract_string_compared_variables` and related functions - Removed `is_number` and `is_atom_number` functions - Removed `skip_normalization` parameter threading through the code This fixes: - `device.appBuildString == "5690201"` now works correctly - `device.appVersionPadded > "007.003.001"` still works for version comparisons - Version strings like "009.000" stay as strings for lexicographic comparison The fix is simpler and more predictable: quoted strings stay as strings. If users want a number, they should write it without quotes. Co-Authored-By: Claude Opus 4.5 --- CHANGELOG.md | 38 +++- Cargo.toml | 2 +- src/lib.rs | 343 ++++++++----------------------------- tests/coverage_tests.rs | 28 +-- tests/integration_tests.rs | 18 +- 5 files changed, 137 insertions(+), 292 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aeeb2a1..4c7476d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,53 +1,82 @@ # CHANGELOG +## 1.0.13 + +### Fixes + +- Simplify string normalization: numeric strings now stay as strings instead of being converted to numbers. This fixes issues where `device.appBuildString == "5690201"` would fail because the string was incorrectly converted to an integer. Only "true" and "false" strings are converted to booleans; all other strings remain as strings. Removed the complex `extract_string_compared_variables` mechanism in favor of this simpler approach. + +## 1.0.12 + +### Fixes + +- Fix string literal normalization in expressions where quoted strings like `"5690201"` were incorrectly converted to integers + +## 1.0.11 + +### Fixes + +- Fix string-to-number normalization for variables compared against string literals (e.g., version comparisons like `"009.000" > "007.003.001"`) + ## 1.0.10 -## Fixes +### Fixes + - Removes modulemap from outputs ## 1.0.9 ## Fixes + - Ensures XC projects also work with module SPM setup + ## 1.0.8 ## Fixes + - Fix generic module name ## 1.0.7 ## Fixes + - Fix iOS build script ## 1.0.6 ## Fixes + - Ensure new module headers point to right places ## 1.0.5 ## Fixes + - Fixes namespaces for SPM module headers ## 1.0.4 ## Enhancements + - Disable Android Cleaner in UniFFI builds ## 1.0.3 ## Enhancements + - Ensure that previously set compilation flags do not affect Android compilation - Add flags to ensure common page size is being passed to Cross ## 1.0.2. ## Enhancements + - Removes log print, reduces binary size ## 1.0.1 ## Enhancements + - Adds `hasFn` function that checks for the existance of a function or returns `false` - Enhance `hasFn` and `has` checks to do the following: - If a `device.` or `computed.` function is used, or a variable is accessed in an expression @@ -60,16 +89,19 @@ - Removes `string.toBool()`,`string.toInt()`, `string.toFloat()` functions as every possible valid atom conversion is done in the AST ## General + - Adds more tests, improves test coverage, adds displaying coverage badge - Improves `README.MD` and adds an `interpretation-flow.md` to serve as a guide for how things are interpreted ## 1.0.0 ## Enhancements + - Adds truthiness and string normalization so value such as "true", "false", "1.1" etc are treated as true, false, 1.1. This occurs on both left and right side of an expression. - Adds conversion methods `bool.toString()`, `float.toString()`, `int.toString()`, `bool.toString()` and `string.toBool()`,`string.toInt()`, `string.toFloat()` to enable typecasting in CEL ## Truthiness + - Fixes issues with undeclared references for properties and functions by wrapping them in a has(x)? x : Null tertiary expression ## 0.2.8 @@ -91,17 +123,21 @@ ## 0.2.5 ### Enhancements + - Moves the HostContext to a Sync version with callback - Updates Android NDK to support 16kb page sizes - Updates Uniffi version ## 0.2.4 + - Fix aarch64 build for Android ## 0.2.3 + - Fix aarch64 build for Android ## 0.2.2 + - Version bump for deployment purposes ## 0.2.1 diff --git a/Cargo.toml b/Cargo.toml index 0c4d0b6..af10e3e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "cel-eval" -version = "1.0.10" +version = "1.0.13" edition = "2021" # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.htmlž diff --git a/src/lib.rs b/src/lib.rs index ad7320c..88fc47f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,7 +13,7 @@ use cel_interpreter::extractors::This; use cel_interpreter::objects::{Key, Map, TryIntoValue}; use cel_interpreter::{Context, ExecutionError, Expression, FunctionContext, Program, Value}; use cel_parser::parse; -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::error::Error; use std::fmt; use std::fmt::Debug; @@ -75,13 +75,8 @@ pub fn evaluate_ast_with_context(definition: String, host: Arc) }; let host = host.clone(); - // Convert to Expression first to extract skip set + // Convert to Expression and transform for null-safe property access let expr: Expression = data.expression.into(); - - // Extract variables that are compared against string literals - let skip_normalization = extract_string_compared_variables(&expr); - - // Transform the expression for null-safe property access let transformed_expr = transform_expression_for_null_safety( expr, SUPPORTED_FUNCTIONS, @@ -94,7 +89,6 @@ pub fn evaluate_ast_with_context(definition: String, host: Arc) data.computed, data.device, host, - skip_normalization, ) .map(|val| val.to_passable()) .map_err(|err| err.to_string()); @@ -150,10 +144,6 @@ pub fn evaluate_with_context(definition: String, host: Arc) -> let parsed_expr = parse(data.expression.as_str()); let result = match parsed_expr { Ok(expr) => { - // Extract variables that are compared against string literals - // These should not have string-to-number normalization applied - let skip_normalization = extract_string_compared_variables(&expr); - let transformed_expr = transform_expression_for_null_safety( expr, SUPPORTED_FUNCTIONS, @@ -166,7 +156,6 @@ pub fn evaluate_with_context(definition: String, host: Arc) -> data.computed, data.device, host, - skip_normalization, ) .map(|val| val.to_passable()) .map_err(|err| err.to_string()) @@ -208,7 +197,6 @@ fn execute_with( computed: Option>>, device: Option>>, host: Arc, - skip_normalization: HashSet, ) -> Result { let supported_fn = SUPPORTED_FUNCTIONS; let host = host.clone(); @@ -224,12 +212,12 @@ fn execute_with( .clone(); // Add predefined variables locally to the context - // Skip string-to-number normalization for variables compared against string literals + // Normalize "true"/"false" strings to booleans, but leave other strings as-is let standardized_variables = variables .map .iter() .map(|it| { - let next = normalize_variables_with_skip(it.1.clone(), &skip_normalization, it.0); + let next = normalize_variables(it.1.clone()); (it.0.clone(), next) }) .collect(); @@ -441,9 +429,7 @@ fn execute_with( ) }) .chain(total_device_properties.iter().map(|(k, v)| { - // Use the skip set when normalizing device properties - let path = format!("device.{}", k); - let mapped_val = normalize_variables_with_skip(v.clone(), &skip_normalization, &path); + let mapped_val = normalize_variables(v.clone()); ( Key::String(Arc::new(k.clone())), mapped_val.to_cel().clone(), @@ -592,49 +578,28 @@ fn execute_with( * This is used for variables that are compared against string literals in the expression. */ pub fn normalize_variables(passable_value: PassableValue) -> PassableValue { - normalize_variables_with_skip(passable_value, &HashSet::new(), "") -} - -fn normalize_variables_with_skip( - passable_value: PassableValue, - skip_normalization: &HashSet, - current_path: &str, -) -> PassableValue { match passable_value.clone() { PassableValue::String(data) => { - let res = match data.as_str() { + // Only convert "true"/"false" strings to booleans + // Don't convert numeric strings to numbers - they should stay as strings + // This ensures string comparisons like device.appBuildString == "5690201" work correctly + match data.as_str() { "true" => PassableValue::Bool(true), "false" => PassableValue::Bool(false), - _ => { - // Skip number conversion if this variable is compared against a string - if skip_normalization.contains(current_path) { - passable_value - } else { - is_number(passable_value) - } - } - }; - res + _ => passable_value, + } } PassableValue::PMap(map) => { let mut new_map = HashMap::new(); for (key, value) in map { - let child_path = if current_path.is_empty() { - key.clone() - } else { - format!("{}.{}", current_path, key) - }; - new_map.insert( - key, - normalize_variables_with_skip(value, skip_normalization, &child_path), - ); + new_map.insert(key, normalize_variables(value)); } PassableValue::PMap(new_map) } PassableValue::List(list) => { let new_list = list .into_iter() - .map(|v| normalize_variables_with_skip(v, skip_normalization, current_path)) + .map(|v| normalize_variables(v)) .collect(); PassableValue::List(new_list) } @@ -643,201 +608,24 @@ fn normalize_variables_with_skip( } /** - * Recursively standardizes `cel_parser::Atom::String` structures by normalizing - * string representations of booleans and numbers into their appropriate types. + * Normalizes `cel_parser::Atom::String` structures by converting + * string representations of booleans into their appropriate types. * - * If the string is a: - * - "true"/"false" => `cel_parser::Atom::Bool(true/false)` - * - `i64` => `cel_parser::Atom::Int` - * - `u64` => `cel_parser::Atom::UInt` - * - `f64` => `cel_parser::Atom::Float` - * - All other variants are returned unchanged + * Only "true" and "false" strings are converted to booleans. + * Numeric strings are NOT converted to numbers - quoted strings stay as strings. + * If the user wanted a number, they would write it without quotes. */ pub fn normalize_ast_variables(atom: cel_parser::Atom) -> cel_parser::Atom { match atom.clone() { cel_parser::Atom::String(data) => match data.as_str() { "true" => cel_parser::Atom::Bool(true), "false" => cel_parser::Atom::Bool(false), - _ => is_atom_number(atom), + _ => atom, }, _ => atom, } } -/** -* Tries parsing a string atom as a number, and if it is a number, converts it. -*/ -fn is_atom_number(atom: cel_parser::Atom) -> cel_parser::Atom { - match atom.clone() { - cel_parser::Atom::String(data) => { - match data.parse::() { - Ok(i) => return cel_parser::Atom::Int(i), - Err(_) => {} - } - match data.parse::() { - Ok(i) => return cel_parser::Atom::UInt(i), - _ => {} - } - match data.parse::() { - Ok(f) => { - if f.fract() == 0.0 { - let as_i64 = f as i64; - if as_i64 as f64 == f { - return cel_parser::Atom::Int(as_i64); - } - let as_u64 = f as u64; - if as_u64 as f64 == f { - return cel_parser::Atom::UInt(as_u64); - } - } - return cel_parser::Atom::Float(f); - } - _ => {} - } - atom - } - _ => atom, - } -} - -/** -* Tries parsing a string value as a number, and if it is a number, converts it. -*/ -fn is_number(passable: PassableValue) -> PassableValue { - match passable.clone() { - PassableValue::String(data) => { - match data.parse::() { - Ok(i) => return PassableValue::Int(i), - _ => {} - } - match data.parse::() { - Ok(i) => return PassableValue::UInt(i), - _ => {} - } - match data.parse::() { - Ok(f) => { - if f.fract() == 0.0 { - let as_i64 = f as i64; - if as_i64 as f64 == f { - return PassableValue::Int(as_i64); - } - let as_u64 = f as u64; - if as_u64 as f64 == f { - return PassableValue::UInt(as_u64); - } - } - return PassableValue::Float(f); - } - _ => {} - } - passable - } - _ => passable, - } -} - -/** - * Extracts variable paths that are compared against string literals in the expression. - * These variables should NOT have their string values converted to numbers during normalization, - * as they are meant to be compared as strings (e.g., version comparisons like "009.000" > "007.003.001"). - */ -fn extract_string_compared_variables(expr: &Expression) -> HashSet { - let mut result = HashSet::new(); - extract_string_compared_variables_internal(expr, &mut result); - result -} - -fn extract_string_compared_variables_internal(expr: &Expression, result: &mut HashSet) { - match expr { - Expression::Relation(lhs, _op, rhs) => { - // Check if RHS is a string literal - let rhs_is_string = matches!(rhs.as_ref(), Expression::Atom(cel_parser::Atom::String(_))); - // Check if LHS is a string literal - let lhs_is_string = matches!(lhs.as_ref(), Expression::Atom(cel_parser::Atom::String(_))); - - if rhs_is_string { - // Extract variable path from LHS - if let Some(path) = extract_variable_path(lhs) { - result.insert(path); - } - } - if lhs_is_string { - // Extract variable path from RHS - if let Some(path) = extract_variable_path(rhs) { - result.insert(path); - } - } - - // Continue recursing into both sides - extract_string_compared_variables_internal(lhs, result); - extract_string_compared_variables_internal(rhs, result); - } - Expression::And(lhs, rhs) | Expression::Or(lhs, rhs) => { - extract_string_compared_variables_internal(lhs, result); - extract_string_compared_variables_internal(rhs, result); - } - Expression::Ternary(cond, if_true, if_false) => { - extract_string_compared_variables_internal(cond, result); - extract_string_compared_variables_internal(if_true, result); - extract_string_compared_variables_internal(if_false, result); - } - Expression::Unary(_, operand) => { - extract_string_compared_variables_internal(operand, result); - } - Expression::List(elements) => { - for elem in elements { - extract_string_compared_variables_internal(elem, result); - } - } - Expression::Map(entries) => { - for (k, v) in entries { - extract_string_compared_variables_internal(k, result); - extract_string_compared_variables_internal(v, result); - } - } - Expression::FunctionCall(func, args, _) => { - extract_string_compared_variables_internal(func, result); - for arg in args { - extract_string_compared_variables_internal(arg, result); - } - } - Expression::Member(operand, _) => { - extract_string_compared_variables_internal(operand, result); - } - Expression::Arithmetic(lhs, _, rhs) => { - extract_string_compared_variables_internal(lhs, result); - extract_string_compared_variables_internal(rhs, result); - } - _ => {} - } -} - -/** - * Extracts the variable path from a member access expression (e.g., "device.appVersionPadded"). - * Returns None if the expression is not a simple variable path. - */ -fn extract_variable_path(expr: &Expression) -> Option { - match expr { - Expression::Ident(name) => Some(name.to_string()), - Expression::Member(operand, member) => { - let parent_path = extract_variable_path(operand)?; - match member.as_ref() { - cel_parser::Member::Attribute(attr) => Some(format!("{}.{}", parent_path, attr)), - cel_parser::Member::Index(idx_expr) => { - // For index access like device["key"], try to extract the key - if let Expression::Atom(cel_parser::Atom::String(s)) = idx_expr.as_ref() { - Some(format!("{}.{}", parent_path, s)) - } else { - None - } - } - _ => None, - } - } - _ => None, - } -} - /** * Check if an expression is an atomic value (string, int, float, bool, etc.) */ @@ -2320,7 +2108,8 @@ mod tests { ); assert_eq!(res4, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); - // Test device.some_key == 1 - left side is device property, right side is integer 1 + // Test device.some_key == "1" - int compared to string should be false (different types) + // In standard CEL, 1 != "1" because they are different types let res5 = evaluate_with_context( r#"{ "variables": { @@ -2341,9 +2130,10 @@ mod tests { .to_string(), ctx.clone(), ); - assert_eq!(res5, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + // Int != String in CEL, so this should be false + assert_eq!(res5, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); - // Test device.some_key == 1.23 - left side is device property, right side is float 1.23 + // Test device.some_key == "1.23" - float compared to string should be false (different types) let res6 = evaluate_with_context( r#"{ "variables": { @@ -2364,7 +2154,8 @@ mod tests { .to_string(), ctx.clone(), ); - assert_eq!(res6, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + // Float != String in CEL, so this should be false + assert_eq!(res6, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); } #[test] fn test_hasfn_wrapping_for_device_functions() { @@ -2416,22 +2207,21 @@ mod tests { }); // Test that utility functions are registered and working - // We mainly test that the functions exist and can convert basic string types - - // Test toInt extension method - this works because string normalization happens first + // Numeric strings stay as strings (not converted to numbers) let res = evaluate_with_context( r#"{"variables": {"map": {"str_num": {"type": "string", "value": "42"}}}, "expression": "str_num"}"#.to_string(), ctx.clone(), ); - assert_eq!(res, "{\"Ok\":{\"type\":\"int\",\"value\":42}}"); + // String "42" should stay as string, not be converted to int + assert_eq!(res, "{\"Ok\":{\"type\":\"string\",\"value\":\"42\"}}"); - // Test that invalid string conversion returns null (toInt function was removed) + // Test that non-numeric strings also stay as strings let res = evaluate_with_context( r#"{"variables": {"map": {"str_invalid": {"type": "string", "value": "not_a_number"}}}, "expression": "str_invalid"}"#.to_string(), ctx.clone(), ); - // Should just return the string as-is since automatic conversion doesn't happen for invalid strings - assert!(res.contains("not_a_number") || res.contains("Null")); + // Should just return the string as-is + assert!(res.contains("not_a_number")); // Test utility functions that still exist - string conversion functions let res = evaluate_with_context( @@ -2499,7 +2289,7 @@ mod tests { map: HashMap::new(), }); - // Test simple boolean variable + // Test that numeric strings stay as strings (not converted to numbers) let simple_test = r#" { "variables": { @@ -2516,7 +2306,8 @@ mod tests { .to_string(); let res_simple = evaluate_with_context(simple_test, ctx.clone()); - assert_eq!(res_simple, "{\"Ok\":{\"type\":\"int\",\"value\":1}}"); + // String "1" should stay as string, not be converted to int + assert_eq!(res_simple, "{\"Ok\":{\"type\":\"string\",\"value\":\"1\"}}"); let data = r#" { @@ -2589,7 +2380,8 @@ mod tests { .to_string(), ctx.clone(), ); - assert_eq!(res2, "{\"Ok\":{\"type\":\"int\",\"value\":8}}"); + // String "8.00000" should stay as string, not be converted to int + assert_eq!(res2, "{\"Ok\":{\"type\":\"string\",\"value\":\"8.00000\"}}"); } #[test] @@ -2639,7 +2431,7 @@ mod tests { #[test] fn test_normalization_edge_cases() { - // Test is_number with edge cases + // Test that numeric strings stay as strings (not converted to numbers) let edge_cases = vec![ PassableValue::String("18446744073709551615".to_string()), // u64 max PassableValue::String("-9223372036854775808".to_string()), // i64 min @@ -2655,17 +2447,11 @@ mod tests { for case in edge_cases { let normalized = normalize_variables(case.clone()); - // Should not panic and should return some valid value - match normalized { - PassableValue::String(_) - | PassableValue::Int(_) - | PassableValue::UInt(_) - | PassableValue::Float(_) => {} - _ => panic!("Unexpected normalization result for {:?}", case), - } + // Numeric strings should stay as strings (not converted to numbers) + assert_eq!(normalized, case, "Numeric string should stay as string"); } - // Test nested normalization + // Test nested normalization - only "true"/"false" are converted let mut nested_map = std::collections::HashMap::new(); nested_map.insert( "nested_bool".to_string(), @@ -2680,8 +2466,10 @@ mod tests { let normalized = normalize_variables(complex_value); if let PassableValue::PMap(map) = normalized { + // "true" string should become Bool(true) assert_eq!(map.get("nested_bool"), Some(&PassableValue::Bool(true))); - assert_eq!(map.get("nested_num"), Some(&PassableValue::Int(42))); + // "42" string should stay as String("42") - not converted to Int + assert_eq!(map.get("nested_num"), Some(&PassableValue::String("42".to_string()))); } else { panic!("Expected normalized map"); } @@ -2891,18 +2679,6 @@ mod tests { assert!(!result2.is_empty()); } - #[test] - fn test_extract_string_compared_variables() { - // Test that we correctly extract variable paths from expressions - use cel_parser::parse; - - let expr = parse(r#"device.appVersionPadded > "007.003.001""#).unwrap(); - let skip_set = super::extract_string_compared_variables(&expr); - - println!("Skip set: {:?}", skip_set); - assert!(skip_set.contains("device.appVersionPadded"), "Should contain device.appVersionPadded, got: {:?}", skip_set); - } - #[test] fn test_version_string_comparison_not_normalized_to_number() { // Test that version strings like "009.000" are NOT converted to numbers @@ -2994,6 +2770,37 @@ mod tests { assert!(res4.contains("true"), "Expected true but got: {}", res4); } + #[test] + fn test_app_build_string_equality() { + // Test for device.appBuildString == "5690201" + // This was a bug where the string literal "5690201" was normalized to Int(5690201), + // causing the comparison to fail when the variable was a string. + // The fix: don't convert numeric strings to numbers - quoted strings stay as strings. + let ctx = Arc::new(TestContext { + map: HashMap::new(), + }); + + // Test case: device.appBuildString == "5690201" where appBuildString = "5690201" + let res = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "appBuildString": {"type": "string", "value": "5690201"} + } + } + } + }, + "expression": "device.appBuildString == \"5690201\"" + }"#.to_string(), + ctx.clone(), + ); + + assert!(res.contains("true"), "Expected true but got: {}", res); + } + #[test] fn test_error_handling_in_property_resolution() { // Test error path in property resolution (lines 500-507) diff --git a/tests/coverage_tests.rs b/tests/coverage_tests.rs index 0326ae6..de61466 100644 --- a/tests/coverage_tests.rs +++ b/tests/coverage_tests.rs @@ -411,13 +411,17 @@ fn test_complex_device_computed_resolution() { #[test] fn test_all_atom_types_normalization() { // Test normalize_ast_variables with all atom types + // Note: Only "true" and "false" strings are converted to booleans. + // Numeric strings stay as strings because quoted literals should preserve their type. use cel_parser::Atom; - // Test string atom normalization + // Test that numeric string atoms stay as strings (not converted to numbers) + // This is correct behavior: "42" in an expression should stay as String("42") let string_atom = Atom::String(Arc::new("42".to_string())); - let result = normalize_ast_variables(string_atom); - assert_eq!(result, Atom::Int(42)); + let result = normalize_ast_variables(string_atom.clone()); + assert_eq!(result, string_atom); // Should stay as String + // Test "true" and "false" strings are converted to booleans let bool_string = Atom::String(Arc::new("true".to_string())); let result = normalize_ast_variables(bool_string); assert_eq!(result, Atom::Bool(true)); @@ -426,21 +430,23 @@ fn test_all_atom_types_normalization() { let result = normalize_ast_variables(false_string); assert_eq!(result, Atom::Bool(false)); + // Large numeric strings stay as strings let uint_string = Atom::String(Arc::new("18446744073709551615".to_string())); - let result = normalize_ast_variables(uint_string); - assert_eq!(result, Atom::UInt(18446744073709551615)); + let result = normalize_ast_variables(uint_string.clone()); + assert_eq!(result, uint_string); // Should stay as String + // Float-like strings stay as strings let float_string = Atom::String(Arc::new("3.14159".to_string())); - let result = normalize_ast_variables(float_string); - assert_eq!(result, Atom::Float(3.14159)); + let result = normalize_ast_variables(float_string.clone()); + assert_eq!(result, float_string); // Should stay as String let fractional_zero = Atom::String(Arc::new("42.0".to_string())); - let result = normalize_ast_variables(fractional_zero); - assert_eq!(result, Atom::Int(42)); // Should convert to int + let result = normalize_ast_variables(fractional_zero.clone()); + assert_eq!(result, fractional_zero); // Should stay as String let non_numeric = Atom::String(Arc::new("not_a_number".to_string())); - let result = normalize_ast_variables(non_numeric); - assert_eq!(result, Atom::String(Arc::new("not_a_number".to_string()))); + let result = normalize_ast_variables(non_numeric.clone()); + assert_eq!(result, non_numeric); // Should stay as String // Test non-string atoms (should return unchanged) let int_atom = Atom::Int(42); diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 4b4cc92..bee2949 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -137,7 +137,7 @@ fn test_normalize_variables_function() { #[test] fn test_normalization_edge_cases() { - // Test is_number with edge cases + // Test that numeric strings stay as strings (not converted to numbers) let edge_cases = vec![ PassableValue::String("18446744073709551615".to_string()), // u64 max PassableValue::String("-9223372036854775808".to_string()), // i64 min @@ -152,16 +152,10 @@ fn test_normalization_edge_cases() { ]; for case in edge_cases { let normalized = normalize_variables(case.clone()); - // Should not panic and should return some valid value - match normalized { - PassableValue::String(_) - | PassableValue::Int(_) - | PassableValue::UInt(_) - | PassableValue::Float(_) => {} - _ => panic!("Unexpected normalization result for {:?}", case), - } + // Numeric strings should stay as strings (not converted to numbers) + assert_eq!(normalized, case, "Numeric string should stay as string"); } - // Test nested normalization + // Test nested normalization - only "true"/"false" are converted to booleans let mut nested_map = std::collections::HashMap::new(); nested_map.insert( "nested_bool".to_string(), @@ -176,8 +170,10 @@ fn test_normalization_edge_cases() { let normalized = normalize_variables(complex_value); if let PassableValue::PMap(map) = normalized { + // "true" string should become Bool(true) assert_eq!(map.get("nested_bool"), Some(&PassableValue::Bool(true))); - assert_eq!(map.get("nested_num"), Some(&PassableValue::Int(42))); + // "42" string should stay as String("42") - not converted to Int + assert_eq!(map.get("nested_num"), Some(&PassableValue::String("42".to_string()))); } else { panic!("Expected normalized map"); } From ee63d5f31994606e8008950b8c4134a7735f3c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yusuf=20To=CC=88r?= <3296904+yusuftor@users.noreply.github.com> Date: Fri, 23 Jan 2026 15:25:42 +0100 Subject: [PATCH 2/4] Add type coercion for equality comparisons with numeric strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When comparing a value to a numeric string (e.g., "1" or "1.23"), the comparison now matches both the string and numeric representations. For example: - `x == "1"` becomes `(x == "1") || (x == 1)` - `x != "1"` becomes `(x != "1") && (x != 1)` This allows: - `1 == "1"` → true (int matches numeric string) - `1.23 == "1.23"` → true (float matches numeric string) - `"5690201" == "5690201"` → true (string matches string) Co-Authored-By: Claude Opus 4.5 --- src/lib.rs | 105 ++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 17 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 88fc47f..c77d787 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,7 +12,7 @@ use crate::ExecutableType::{CompiledProgram, AST}; use cel_interpreter::extractors::This; use cel_interpreter::objects::{Key, Map, TryIntoValue}; use cel_interpreter::{Context, ExecutionError, Expression, FunctionContext, Program, Value}; -use cel_parser::parse; +use cel_parser::{parse, RelationOp}; use std::collections::HashMap; use std::error::Error; use std::fmt; @@ -626,6 +626,78 @@ pub fn normalize_ast_variables(atom: cel_parser::Atom) -> cel_parser::Atom { } } +/** + * Checks if a string atom represents a numeric value and returns the numeric atom if so. + * Returns None if the string is not a valid numeric representation. + */ +fn try_parse_string_to_number(s: &str) -> Option { + // Try to parse as int first + if let Ok(i) = s.parse::() { + return Some(cel_parser::Atom::Int(i)); + } + // Then try to parse as float + if let Ok(f) = s.parse::() { + return Some(cel_parser::Atom::Float(f)); + } + None +} + +/** + * Creates a type-coerced comparison expression for equality/inequality operators. + * When comparing with a numeric string, this creates an OR expression that matches + * both the string and numeric representations. + * + * For example: `x == "1"` becomes `(x == "1") || (x == 1)` + * This allows both `"1" == "1"` and `1 == "1"` to return true. + * + * For NotEquals: `x != "1"` becomes `(x != "1") && (x != 1)` + * This ensures that neither representation matches. + */ +fn create_type_coerced_comparison( + lhs: Box, + op: RelationOp, + rhs: Box, +) -> Expression { + // Only apply coercion for Equals and NotEquals + let is_equality = matches!(op, RelationOp::Equals | RelationOp::NotEquals); + if !is_equality { + return Expression::Relation(lhs, op, rhs); + } + + // Check if RHS is a string atom that can be parsed as a number + if let Expression::Atom(cel_parser::Atom::String(s)) = rhs.as_ref() { + if let Some(numeric_atom) = try_parse_string_to_number(s.as_str()) { + let string_comparison = Expression::Relation( + lhs.clone(), + op.clone(), + rhs, + ); + let numeric_comparison = Expression::Relation( + lhs, + op.clone(), + Box::new(Expression::Atom(numeric_atom)), + ); + + // For Equals: (string_cmp || numeric_cmp) - either match is success + // For NotEquals: (string_cmp && numeric_cmp) - both must not match + return match op { + RelationOp::Equals => Expression::Or( + Box::new(string_comparison), + Box::new(numeric_comparison), + ), + RelationOp::NotEquals => Expression::And( + Box::new(string_comparison), + Box::new(numeric_comparison), + ), + _ => unreachable!(), + }; + } + } + + // No coercion needed, return original comparison + Expression::Relation(lhs, op, rhs) +} + /** * Check if an expression is an atomic value (string, int, float, bool, etc.) */ @@ -988,14 +1060,14 @@ fn transform_expression_for_null_safety_internal( Box::new(default_value), ); - Expression::Relation( + create_type_coerced_comparison( Box::new(safe_lhs), op, Box::new(transformed_rhs), ) } else { // Right side is not atom - wrap whole expression - let original_relation = Expression::Relation( + let original_relation = create_type_coerced_comparison( lhs.clone(), op, Box::new(transform_expression_for_null_safety_internal( @@ -1053,14 +1125,14 @@ fn transform_expression_for_null_safety_internal( Box::new(default_value), ); - Expression::Relation( + create_type_coerced_comparison( Box::new(hasfn_ternary_lhs), op, Box::new(transformed_rhs), ) } else { // Right side is not atom - wrap whole relation with hasFn condition - let original_relation = Expression::Relation( + let original_relation = create_type_coerced_comparison( lhs.clone(), op, Box::new(transformed_rhs), @@ -1074,7 +1146,7 @@ fn transform_expression_for_null_safety_internal( } } else { // Fallback - transform normally - Expression::Relation( + create_type_coerced_comparison( Box::new(transform_expression_for_null_safety_internal( *lhs, inside_has, @@ -1088,7 +1160,7 @@ fn transform_expression_for_null_safety_internal( } } else { // Fallback - transform normally - Expression::Relation( + create_type_coerced_comparison( Box::new(transform_expression_for_null_safety_internal( *lhs, inside_has, @@ -1101,8 +1173,8 @@ fn transform_expression_for_null_safety_internal( ) } } else { - // Fallback - transform normally - Expression::Relation( + // Fallback - transform normally + create_type_coerced_comparison( Box::new(transform_expression_for_null_safety_internal( *lhs, inside_has, @@ -1116,7 +1188,7 @@ fn transform_expression_for_null_safety_internal( } } else { // Left side is not simple member access or hasFn ternary, transform normally - Expression::Relation( + create_type_coerced_comparison( Box::new(transform_expression_for_null_safety_internal( *lhs, inside_has, @@ -2108,8 +2180,7 @@ mod tests { ); assert_eq!(res4, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); - // Test device.some_key == "1" - int compared to string should be false (different types) - // In standard CEL, 1 != "1" because they are different types + // Test device.some_key == "1" - int compared to numeric string should be true let res5 = evaluate_with_context( r#"{ "variables": { @@ -2130,10 +2201,10 @@ mod tests { .to_string(), ctx.clone(), ); - // Int != String in CEL, so this should be false - assert_eq!(res5, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + // Int compared to numeric string should be true (1 == "1") + assert_eq!(res5, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); - // Test device.some_key == "1.23" - float compared to string should be false (different types) + // Test device.some_key == "1.23" - float compared to numeric string should be true let res6 = evaluate_with_context( r#"{ "variables": { @@ -2154,8 +2225,8 @@ mod tests { .to_string(), ctx.clone(), ); - // Float != String in CEL, so this should be false - assert_eq!(res6, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + // Float compared to numeric string should be true (1.23 == "1.23") + assert_eq!(res6, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); } #[test] fn test_hasfn_wrapping_for_device_functions() { From b7cdf13aabb790d339ac492f805f463d0f21bf50 Mon Sep 17 00:00:00 2001 From: Ian Rumac Date: Fri, 23 Jan 2026 16:05:37 +0100 Subject: [PATCH 3/4] Add more test cases --- src/lib.rs | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/src/lib.rs b/src/lib.rs index c77d787..53dca55 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2227,6 +2227,81 @@ mod tests { ); // Float compared to numeric string should be true (1.23 == "1.23") assert_eq!(res6, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test 3.0 == "003.000" - float compared to string with leading zeros + // The string "003.000" should be coerced to 3.0, so 3.0 == 3.0 is true + let res7 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "some_key": { + "type": "float", + "value": 3.0 + } + } + } + } + }, + "expression": "device.some_key == \"003.000\"" + }"# + .to_string(), + ctx.clone(), + ); + // Float 3.0 compared to string "003.000" should be true (3.0 == 3.0 after coercion) + assert_eq!(res7, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test that type coercion works correctly with logical AND + // "true && 1 == '1'" should become "true && ((1 == '1') || (1 == 1))" + // which evaluates to true && (false || true) = true + let res8 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "some_key": { + "type": "int", + "value": 1 + } + } + } + } + }, + "expression": "true && device.some_key == \"1\"" + }"# + .to_string(), + ctx.clone(), + ); + // true && (1 == "1") should be true after coercion + assert_eq!(res8, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Also test with false && to make sure AND logic is preserved + let res9 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "some_key": { + "type": "int", + "value": 1 + } + } + } + } + }, + "expression": "false && device.some_key == \"1\"" + }"# + .to_string(), + ctx.clone(), + ); + // false && anything should be false + assert_eq!(res9, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); } #[test] fn test_hasfn_wrapping_for_device_functions() { From 6ef83b159391de1dee8b048108dddd6b0a75e0ff Mon Sep 17 00:00:00 2001 From: Ian Rumac Date: Fri, 23 Jan 2026 16:23:17 +0100 Subject: [PATCH 4/4] Update RHS coercion --- src/lib.rs | 332 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 327 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 53dca55..d6aba85 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -629,13 +629,21 @@ pub fn normalize_ast_variables(atom: cel_parser::Atom) -> cel_parser::Atom { /** * Checks if a string atom represents a numeric value and returns the numeric atom if so. * Returns None if the string is not a valid numeric representation. + * + * Parse order: i64 -> u64 -> f64 + * This ensures large unsigned integers (e.g., "18446744073709551615") are parsed + * as UInt rather than losing precision with float conversion. */ fn try_parse_string_to_number(s: &str) -> Option { - // Try to parse as int first + // Try to parse as signed int first if let Ok(i) = s.parse::() { return Some(cel_parser::Atom::Int(i)); } - // Then try to parse as float + // Then try unsigned int (for large values that overflow i64) + if let Ok(u) = s.parse::() { + return Some(cel_parser::Atom::UInt(u)); + } + // Finally try float if let Ok(f) = s.parse::() { return Some(cel_parser::Atom::Float(f)); } @@ -644,11 +652,12 @@ fn try_parse_string_to_number(s: &str) -> Option { /** * Creates a type-coerced comparison expression for equality/inequality operators. - * When comparing with a numeric string, this creates an OR expression that matches - * both the string and numeric representations. + * When comparing with a numeric string (on either side), this creates an OR expression + * that matches both the string and numeric representations. * * For example: `x == "1"` becomes `(x == "1") || (x == 1)` - * This allows both `"1" == "1"` and `1 == "1"` to return true. + * And symmetrically: `"1" == x` becomes `("1" == x) || (1 == x)` + * This allows both `"1" == 1` and `1 == "1"` to return true. * * For NotEquals: `x != "1"` becomes `(x != "1") && (x != 1)` * This ensures that neither representation matches. @@ -694,6 +703,36 @@ fn create_type_coerced_comparison( } } + // Check if LHS is a string atom that can be parsed as a number (symmetric coercion) + if let Expression::Atom(cel_parser::Atom::String(s)) = lhs.as_ref() { + if let Some(numeric_atom) = try_parse_string_to_number(s.as_str()) { + let string_comparison = Expression::Relation( + lhs, + op.clone(), + rhs.clone(), + ); + let numeric_comparison = Expression::Relation( + Box::new(Expression::Atom(numeric_atom)), + op.clone(), + rhs, + ); + + // For Equals: (string_cmp || numeric_cmp) - either match is success + // For NotEquals: (string_cmp && numeric_cmp) - both must not match + return match op { + RelationOp::Equals => Expression::Or( + Box::new(string_comparison), + Box::new(numeric_comparison), + ), + RelationOp::NotEquals => Expression::And( + Box::new(string_comparison), + Box::new(numeric_comparison), + ), + _ => unreachable!(), + }; + } + } + // No coercion needed, return original comparison Expression::Relation(lhs, op, rhs) } @@ -2302,6 +2341,289 @@ mod tests { ); // false && anything should be false assert_eq!(res9, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + + // Test symmetric case: "1" == device.some_key (string literal on LHS, int on RHS) + // Coercion now works symmetrically - both LHS and RHS strings are coerced + let res10 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "some_key": { + "type": "int", + "value": 1 + } + } + } + } + }, + "expression": "\"1\" == device.some_key" + }"# + .to_string(), + ctx.clone(), + ); + // "1" == 1 should be true with symmetric coercion + assert_eq!(res10, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + } + + #[test] + fn test_type_coercion_edge_cases() { + let ctx = Arc::new(TestContext { + map: HashMap::new(), + }); + + // Test 1.0 == 1 (float == int) - should be true (CEL handles this natively) + let res1 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1.0 == 1"}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res1, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test 1 == 1.0 (int == float) - should be true + let res2 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1 == 1.0"}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res2, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test 1.0 == "1" (float == numeric string) - should be true after coercion + let res3 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1.0 == \"1\""}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res3, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test "1.0" == 1 (numeric string == int) - should be true after coercion + let res4 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "\"1.0\" == 1"}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res4, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test NotEquals: 1 != "1" - should be false (they are equal after coercion) + let res5 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1 != \"1\""}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res5, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + + // Test NotEquals: 1 != "2" - should be true (they are different) + let res6 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1 != \"2\""}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res6, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test NotEquals symmetric: "1" != 2 - should be true + let res7 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "\"1\" != 2"}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res7, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test negative numbers: -1 == "-1" - should be true + let res8 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "neg_val": {"type": "int", "value": -1} + } + } + } + }, + "expression": "device.neg_val == \"-1\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res8, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test negative numbers symmetric: "-1" == device.neg_val + let res9 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "neg_val": {"type": "int", "value": -1} + } + } + } + }, + "expression": "\"-1\" == device.neg_val" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res9, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test invalid numeric string: 1 == "abc" - should be false (no coercion for non-numeric) + let res10 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1 == \"abc\""}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res10, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + + // Test whitespace: 1 == " 1" - should be false (no trimming) + let res11 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "1 == \" 1\""}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res11, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + + // Test boolean NotEquals: true != "false" - should be true + let res12 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "flag": {"type": "bool", "value": true} + } + } + } + }, + "expression": "device.flag != \"false\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res12, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test float with trailing zeros: 1.0 == "1.00" - should be true + let res13 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "val": {"type": "float", "value": 1.0} + } + } + } + }, + "expression": "device.val == \"1.00\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res13, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test uint: uint == "42" + let res14 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "uint_val": {"type": "uint", "value": 42} + } + } + } + }, + "expression": "device.uint_val == \"42\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res14, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test empty string: 0 == "" - should be false (empty string is not numeric) + let res15 = evaluate_with_context( + r#"{"variables": {"map": {}}, "expression": "0 == \"\""}"#.to_string(), + ctx.clone(), + ); + assert_eq!(res15, "{\"Ok\":{\"type\":\"bool\",\"value\":false}}"); + + // Test negative float: -1.5 == "-1.5" + let res16 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "neg_float": {"type": "float", "value": -1.5} + } + } + } + }, + "expression": "device.neg_float == \"-1.5\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res16, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test large uint: max u64 value (18446744073709551615) == string + // This tests that u64 parsing is used for values that overflow i64 + let res17 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "large_uint": {"type": "uint", "value": 18446744073709551615} + } + } + } + }, + "expression": "device.large_uint == \"18446744073709551615\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res17, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test large uint symmetric: string on LHS + let res18 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "large_uint": {"type": "uint", "value": 18446744073709551615} + } + } + } + }, + "expression": "\"18446744073709551615\" == device.large_uint" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res18, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); + + // Test value just above i64 max (9223372036854775808) - should parse as u64 + let res19 = evaluate_with_context( + r#"{ + "variables": { + "map": { + "device": { + "type": "map", + "value": { + "above_i64_max": {"type": "uint", "value": 9223372036854775808} + } + } + } + }, + "expression": "device.above_i64_max == \"9223372036854775808\"" + }"# + .to_string(), + ctx.clone(), + ); + assert_eq!(res19, "{\"Ok\":{\"type\":\"bool\",\"value\":true}}"); } #[test] fn test_hasfn_wrapping_for_device_functions() {