clarkmcc / cel-rust

Common Expression Language interpreter written in Rust
https://crates.io/crates/cel-interpreter
MIT License
375 stars 21 forks source link

Timestamp issue? #67

Closed alexsnaps closed 3 months ago

alexsnaps commented 3 months ago

I'm not sure whether I'm the one doing something here, but I find this slightly confusing:

let script = "ts == timestamp('2023-05-28T00:00:00+00:00')";
let program = Program::compile(script).unwrap();
let mut context = Context::default();
let ts: DateTime<FixedOffset> = DateTime::parse_from_rfc3339("2023-05-28T00:00:00+00:00").unwrap();
context.add_variable("ts", Value::Timestamp(ts)).unwrap();
assert_eq!(program.execute(&context), Ok(true.into()));

Interestingly, this yields comparing: Timestamp(2023-05-28T00:00:00+00:00) vs String("2023-05-28T00:00:00+00:00") Where the lhs is the timestamp('2023-05-28T00:00:00+00:00', but for some reason ts ends up being a ... String? Am I missing something here?

clarkmcc commented 3 months ago

This was a regression introduced in #40 (extension of changes in #33)

It is because of this trait https://github.com/clarkmcc/cel-rust/blob/2cb75cb3cc985583c199ea4891ddd5450d6b1beb/interpreter/src/context.rs#L50

Which ultimately calls this function, converting the timestamp to a string https://github.com/clarkmcc/cel-rust/blob/2cb75cb3cc985583c199ea4891ddd5450d6b1beb/interpreter/src/ser.rs#L577

Still thinking about exactly how to solve this. In the meantime, you can use this to add a Value type directly and skip serialization.

context.add_variable_from_value("foo", Value::Timestamp(DateTime::default()))
clarkmcc commented 3 months ago

I need to think through the repercussions of #68 but it does resolve the issue (test added). Because of this addition, I don't think there will be any breaking changes. We were effectively doing that before, just with an unnecessary intermediate serialization step.