Fixed variable type changing due to serialization

[clarkmcc]

No description provided.

[alexsnaps]

Given Context::add_variable's signature I think this makes sense. I probably should have looked at other methods on context or read the code, but it looked the natural fit for the job… Tho tbh, I got thrown off by the Result returned… but happily ignored it for now and moved on.

Not a real proposal, as I may real be off wrt how people use this, but wondering if ::add_variable_from_value shouldn't be ::add_variable and have something along the ::try_add_variable for fallible TryIntoValue values…
On a more generic note, I wonder if folding the .try_into() within the method might be blurring the lines here. As it can fail, the user it left with dealing with the Err case, which is the only error case possible. Which now splits the contract to some extent. So wouldn't it be clearer to leave it to the user to call try_into_value()? Which would also reduce the code footprint of the resulting binary (tho the user could take that upon themselves if they cared)… Anyways, wondering 🤔

[alexsnaps]

Another question, and I haven't tried to answer it myself, sorry... Why trait TryIntoValue and not core's Try(From|Into)?

[clarkmcc]

Not a real proposal, as I may real be off wrt how people use this, but wondering if ::add_variable_from_value shouldn't be ::add_variable and have something along the ::try_add_variable for fallible TryIntoValue values…

I'd be comfortable with this. I think the original intent was to not make a breaking API change (except to introduce the result). Making add_variable accept really anything wouldn't require any code migrations from users. That being said, as a pre-1.0 project, I think idiom and expectation is probably more important than backwards compatibility.

On a more generic note, I wonder if folding the .try_into() within the method might be blurring the lines here.

This was a point of discussion in the initial PR. My argument at the time was that doing implicit serialization is consistent with the principle of least surprise, in other words, if the user was going to have to have their value serialized at some point, and if that serialization error needs to be handled, why not do as much as possible for the user so they don't need to know to do it on their own. I cited these examples1 of cases in the wild where T: Serialize is accepted instead of the already-serialized result.

But clearly both you and the original PR author at least initially expected a different API, so I'd be happy to hear your counter-arguments for this rationale.

Another question, and I haven't tried to answer it myself, sorry... Why trait TryIntoValue and not core's Try(From|Into)?

For one, we would need to do something like this impl<T: Serialize> TryFrom<T> for Value and that is not allowed because it conflicts with the blanket implementation impl<T, U> TryFrom<U> for T where U: std::convert::Into<T>;

• The blanket implementation covers any type T and any type U where U can be converted into T.
• The proposed TryFrom implementation also covers any type T that implements Serialize.
• Since Serialize is a widely implemented trait, many types will satisfy both conditions: they can be converted into other types (via Into) and they implement Serialize.

[alexsnaps]

I cited these examples[1](https://sourcegraph.com/search?q=context:global+/T:+Serialize%3E%5C(/&patternType=keyword&sm=0) of cases in the wild where T: Serialize is accepted instead of the already-serialized result.

Quickly looking through some of these tho, I found 2 patterns I think: either a utility functions, which aims at encapsulating the serialization logic; or method, that depending on the self's state, can avoid the serialization altogether in some cases. But I'm sure there are other cases.

I think this here differs in the sense that the fallible code path is always invoked (or at least today is), and the primary use of the method is to mutate the context. Again, I don't have a huge preference here. I probably would have done it differently, i.e. leave both the handing the serialization and dealing with the error to the user, more so that add_variable, rightly so, takes ownership of the value and the error case leaving the user with a Box<dyn Error> if I read this right.

as a pre-1.0 project, I think idiom and expectation is probably more important than backwards compatibility.

I strongly agree with that statement tho... So at this stage I think it might be better to "flag" this as something worth investigating a little more, as to what the best, yet flexible API for a user would be. And I know I don't have enough experience with CEL and its usages to be able to form a half valid opinion at this stage. But willing to keep on "playing" with the API to possibly come up with a proposal and matching usages to showcase pros & cons of any given approach...

So tl;dr I wouldn't necessarily change anything beyond what you have done at this stage.

[clarkmcc]

Understood. Yeah regardless, I think this feedback should be addressed as a separate PR. Feel free to open an issue when you're ready and we can continue the discussion on a better API design. I wasn't really thrilled with the proposed alternative at the time

cel-rust/example/src/serde.rs

Line 20 in 2cb75cb

let _cel_value = to_value(MyStruct { a: 2, b: 2 }).unwrap();

Maybe it's the right solution, maybe we instead implement a SerializeToValue trait to attach methods to any T: Serialize and perhaps that's cleaner. Not sure.

[alexsnaps]

Yeah... SerializeToValue is an interesting idea. There is some talk about protobuf & some JSON too in the cel spec. The former is actually close that what I'm dealing with, which is injecting Value in the Context that come from envoy, which is the host to my wasm module, integrated using proxy-wasm. So my values come as Vec<u8>, which could be quite a few things, but also some protobuf message. Anyways don't want to pollute this thread to much, but based on what I've seen, pulling values in some form into the context of a cel expression's evaluation seems to be a common thing. Some forms seem to be better supported/spec'ed than others... e.g. protobuf & json. If that's indeed the case, then I think these would be the first usecase I'd look into and see how they'd play along with the Rust ecosystem around these serialized forms.