Feat/serializer

[lfbrehm]

Introduces missing macros mentioned in #32 and implements Serializer to address issue #15.

[clarkmcc]

First of all, excellent work. This is a huge improvement to the developer experience of this library and was much needed, I just didn't have the time to support it since there was no apparent demand at the time. I am thrilled that you've added the support.

A couple thoughts that I'd like to see implemented before merging.

1. The implementation as it stands today requires that you call to_value on anything that implements Serialize when you want to pass it to ctx.add_variable(...), but thanks to Rust's type system, we shouldn't even need to require that, just introduce a new trait. See the patch at the bottom of this comment for an example of how to get that to work. The patch is not complete, nor does it have proper error handling, it just illustrates the idea that I'd like to see implemented. And it allows us to go from this

#[derive(Serialize)]
struct MyStruct {
    a: i64,
    b: i64,
}

context.add_variable("foo", to_value(MyStruct { a: 1, b: 2 }));

to this

#[derive(Serialize)]
struct MyStruct {
    a: i64,
    b: i64,
}

context.add_variable("foo", MyStruct { a: 1, b: 2 });

which in my opinion is a great win for developer experience.

2. I see that there is a SerializationError which is excellent, but I see that we're using the InvalidKey variant (the only variant) in cases where the problem is an unsupported type conversion. What's the idea behind calling these type conversion errors "invalid key" errors? Wondering if this error should be a struct rather than an enum if there's only ever one way that it could fail.
3. By implementing item 1, ctx.add_variable(...), is going to return an error, let's make sure to plumb your new SerializationError type all the way through.

I'll go through and do a more in-depth review once we're closer to the merge, but at a high level, this is really excellent. Thank you for your contribution!

Index: interpreter/src/objects.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/interpreter/src/objects.rs b/interpreter/src/objects.rs
--- a/interpreter/src/objects.rs	(revision 565da99351b327f5c512606c4a589cd7f8a02533)
+++ b/interpreter/src/objects.rs	(date 1707945458178)
@@ -1,10 +1,11 @@
 use crate::context::Context;
 use crate::functions::FunctionContext;
-use crate::ExecutionError;
 use crate::ExecutionError::NoSuchKey;
+use crate::{to_value, ExecutionError};
 use cel_parser::{ArithmeticOp, Atom, Expression, Member, RelationOp, UnaryOp};
 use chrono::{DateTime, Duration, FixedOffset};
 use core::ops;
+use serde::Serialize;
 use std::cmp::Ordering;
 use std::collections::HashMap;
 use std::convert::TryInto;
@@ -118,6 +119,34 @@
     Null,
 }
 
+pub trait TryIntoValue {
+    fn try_into_value(self) -> Result<Value, ()>;
+}
+
+impl TryIntoValue for Value {
+    fn try_into_value(self) -> Result<Value, ()> {
+        Ok(self)
+    }
+}
+
+impl<T: Serialize> TryIntoValue for T {
+    fn try_into_value(self) -> Result<Value, ()> {
+        to_value(self).map_err(|_| ())
+    }
+}
+
+impl TryIntoValue for &Value {
+    fn try_into_value(self) -> Result<Value, ()> {
+        Ok(self.clone())
+    }
+}
+
+impl TryIntoValue for &Key {
+    fn try_into_value(self) -> Result<Value, ()> {
+        Ok(self.clone().into())
+    }
+}
+
 pub enum ValueType {
     List,
     Map,
Index: interpreter/src/context.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/interpreter/src/context.rs b/interpreter/src/context.rs
--- a/interpreter/src/context.rs	(revision 565da99351b327f5c512606c4a589cd7f8a02533)
+++ b/interpreter/src/context.rs	(date 1707945406173)
@@ -1,5 +1,5 @@
 use crate::magic::{Function, FunctionRegistry, Handler};
-use crate::objects::Value;
+use crate::objects::{TryIntoValue, Value};
 use crate::{functions, ExecutionError};
 use cel_parser::Expression;
 use std::collections::HashMap;
@@ -43,14 +43,14 @@
     pub fn add_variable<S, V>(&mut self, name: S, value: V)
     where
         S: Into<String>,
-        V: Into<Value>,
+        V: TryIntoValue,
     {
         match self {
             Context::Root { variables, .. } => {
-                variables.insert(name.into(), value.into());
+                variables.insert(name.into(), value.try_into_value().unwrap());
             }
             Context::Child { variables, .. } => {
-                variables.insert(name.into(), value.into());
+                variables.insert(name.into(), value.try_into_value().unwrap());
             }
         }
     }
Index: example/Cargo.toml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/example/Cargo.toml b/example/Cargo.toml
--- a/example/Cargo.toml	(revision 565da99351b327f5c512606c4a589cd7f8a02533)
+++ b/example/Cargo.toml	(date 1707944942550)
@@ -7,6 +7,7 @@
 
 [dependencies]
 cel-interpreter = { path = "../interpreter" }
+serde = { versin = "1.0.196", features = ["derive"] }
 chrono = "0.4.26"
 
 [[bin]]
@@ -23,4 +24,8 @@
 
 [[bin]]
 name = "threads"
-path = "src/threads.rs"
\ No newline at end of file
+path = "src/threads.rs"
+
+[[bin]]
+name = "serde"
+path = "src/serde.rs"
\ No newline at end of file
Index: example/src/serde.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/example/src/serde.rs b/example/src/serde.rs
new file mode 100644
--- /dev/null	(date 1707945742729)
+++ b/example/src/serde.rs	(date 1707945742729)
@@ -0,0 +1,18 @@
+use cel_interpreter::{to_value, Context, Program};
+use serde::Serialize;
+
+#[derive(Serialize)]
+struct MyStruct {
+    a: i64,
+    b: i64,
+}
+
+context.add_variable("foo", MyStruct { a: 1, b: 2 });
+
+fn main() {
+    let program = Program::compile("foo.a == 1 && foo.b == 2").unwrap();
+    let mut context = Context::default();
+    context.add_variable("foo", MyStruct { a: 1, b: 2 });
+    let value = program.execute(&context).unwrap();
+    assert_eq!(value, true.into());
+}

[lfbrehm]

Thanks for the feedback!

I should probably mention that the implementation of Serializer is more or less copied from serde_json as mentioned in the serde documentation. They use a pretty similar Value enum, so this was straightforward. I also added a disclaimer to interpreter/src/ser.rs.

1. I implemented the TryToValue trait in the new commit. It's probably just a preference, but I'd rather call to_value and get a separate SerializationError than serialize everything implicitly in add_variable, also because this error is specific to serialization and not adding variables.
2. I forgot to add the serde-specific variant to this error, should be fixed now.
3. I also added a new variant to ExecutionError to forward the error, but I am not happy with how I have approached error handling in general here.

I am open to any improvements and suggestions you may have from your side!

[clarkmcc]

@lfbrehm Thanks! First of all, I don't consider myself a Rust expert, so if there's an idiom that I'm not considering, I'm definitely happy to consider it. I found several cases[1] of functions accepting T: Serialize and returning errors so there's at least some precedence for this sort of thing, though our use-case here may not be entirely comparable.

My personal thought process was: if you still have to serialize and error handle, is it simpler on the user to do that all in one step rather than requiring them to import a conversion function.

let value = to_value(MyStruct{})?;
ctx.add_variable("value", value);

// vs

ctx.add_variable("value", MyStruct{})?;

In summary, if it's only a personal preference thing, then I would prefer the approach that requires less cognitive overhead for users that aren't familiar with the library (one of the big reasons I implemented #11). If there's an idiom that we'd be ignoring by implementing it this then I think I would prefer the idiom over my preference.

[1] https://sourcegraph.com/search?q=context:global+/T:+Serialize%3E%5C(/&patternType=keyword&sm=0

[clarkmcc]

I'll poke around a bit on the error handling and see if I can come up with something that makes sense. I agree that ctx.add_variable(...) should not return an ExecutionError, but I don't have a problem with it returning a SerializationError since that is the only way that function can fail.

[St4NNi]

Hey, first of all, thanks @lfbrehm for this PR. Since I was a bit involved in the creation of this PR behind the scenes, I figured I might be able to add some of my thoughts here as well:

I think there should definitely be an infallible function to add an existing Value to a context, otherwise it introduces unnecessary verbosity via execution errors that will most likely never occur (e.g. in all, exists and friends).

IMHO the best solution would be to add an additional add_variable_from_serialize function to the context, which would take any T: Serialize and convert it to a value, and then call the infallible add_variable. This would avoid the problem that users have to guess which function to import, and would only add slightly more verbosity. It would also preserve the current function signatures and keep the add_variable function backwards compatible. Additionally, it would make the function signatures easier to understand, for Into<Value> and T: Serialize most people have a clear understanding of what to expect.

[clarkmcc]

Yeah I'm comfortable with that.

I'd also be fine with doing the inverse of what you suggested, and introduce a new function name add_variable_from_value or something that's more geared towards internal usage. The idea here is the shorter/cleaner function (add_variable) is the one that should be used my users in most cases. It wouldn't be backwards compatible as you say, but we're pre-1.0 so backwards compatibility isn't a guarantee yet. If one API is better than the other, now would be the time for a breaking change in favor of a better API.

But as I said, if you guys would prefer to leave add_variable unchanged and introduce a new function, I would be good with that as well.

[lfbrehm]

I think your suggestion to add add_variable_from_value is the best step forward. This introduces an internal function to add already converted variables to the context without any error handling and could serve as a public function for users dealing only with values. Exposing an add_variable function, that takes serializable values is then the 'default' that introduces convenience for users. Error handling then is also intuitive, since ExecutionErrors are not mixed with SerializationErrors.

Feel free to add any suggestions you have for this new commit!

[clarkmcc]

lgtm!