search

tokio-rs / valuable

MIT License
185 stars 19 forks source link

Strange happenings with manual implementation of `Valuable` #88

Closed jaskij closed 2 years ago

jaskij commented 2 years ago

For the short term, I wanted to make myself a wrapper for anyhow::Error, to use the unstable support in tracing and log the values as structured.

What happened is that my manual implementation - although seemingly correct, fails to serialize. A different implementation, which stringifies early and uses#[derive(Valuable)] works just fine.

Because the code is a bit bigger than fits into an issue, I have created a reproduction in a separate repository, with both versions.

cc @hawkw

hawkw commented 2 years ago

I believe I've figured out what's going wrong here. Your not_working implementation records the error's cause by calling Valuable::as_value on it: https://github.com/jaskij/valuable-serde-bug-reproduction/blob/017613b07dfdf692d869573bbfbd099c73d58398/not_working/src/valuable_anyhow.rs#L31

Since root_cause is an Error, its as_value implementation returns Value::Error. This is correct. However, it's also why this code doesn't work. Note that the difference between the working code and the not_working code is that in working, error.root_cause() is formatted as a String first: https://github.com/jaskij/valuable-serde-bug-reproduction/blob/017613b07dfdf692d869573bbfbd099c73d58398/working/src/main.rs#L26

The problem is due to a bug in valuable-serde when recording the Value::Error variant. When valuable-serde encounters a Value::Error, it returns the error immediately as a serialization error: https://github.com/tokio-rs/valuable/blob/1fc2ad50fcae7fc0da81dc40a385c235636ba525/valuable-serde/src/lib.rs#L267

This is not correct. If a serde serializer returns an error, this means something has gone wrong while serializing a value. Returning an error makes the serialization fail. However, this is not what valuable's Value::Error means. Value::Error represents that we are recording a value which happens to be an Error, not that something went wrong while recording the value. This means that valuable-serde will currently return an Error (indicating that serialization failed) any time it attempts to record an Error value.

Instead, it should probably record the string representation of the error using serialize_str or something.