Failing to deserialize from `serde_json::Value`

[morph-dev]

The NodeId can't deserialize from serde_json::Value object.

Here is a simple test (unwrap crashes because deserialization returns None)

#[cfg(feature = "serde")]
#[test]
fn test_serde_value() {
    let node = NodeId::random();
    let value = serde_json::to_value(&node).unwrap();
    assert_eq!(node, serde_json::from_value::<NodeId>(value).unwrap());
}

One way to fix it is to change deserialize function to:

let raw: String = serde::Deserialize::deserialize(deserializer)?;
let src = raw.strip_prefix("0x").unwrap_or(&raw);
hex::FromHex::from_hex(src).map_err(serde::de::Error::custom)

but this is not optimal in other cases.

If you are fine with this fix, I can create a PR.

[morph-dev]

Might be worth noting that maybe the best way is to try the current implementation (deserializing to byte slice) and only try my fix if that one fails.

[divagant-martian]

thanks for the issue @morph-dev. I don't fully get what's going on, could you explain from your understanding why this serialization is failing? A bit surprising.

Regardless, we definitely care first about correctness than performance so I would be on board with this change. But being honest, I first need to understand the root of the issue for a proper review

[morph-dev]

My understanding is that deserializing into byte slice works fine for serde_json::from_slice and serde_json::from_str because they accept borrowed parameter.

However, serde_json::from_value moves the Value object, so we can't deserialize it into byte slice, but deserializing it into String works fine.

I believe that previous implementation, that used serde-hex crate had the same issue. There is forked version, the stremio-serde-hex, which implemented the special serde::de::Visitor (link) to deal with it.

I will create a PR with suggested changes.

[morph-dev]

Also, we (Trin) would like to remove our own implementation of NodeId and use this one instead (which we already do in many places).

So we would appreciate if new release could be published soon after this is fixed.

[weipin]

( This is something rushed; please double-check and verify. )

The problem is that the types don't match. The current NodeId deserializing code requires a str, but what Value can provide is a String.

I am afraid we cannot find a way to force serde_json::from_value to give us a value with a lifetime such as str, because the trait involves is DeserializeOwned , not Deserialize.

The good news is that we don't need to return the "string" itself, but to find a way to handle the two possible input types (borrowed and owned).

I figure that Cow<str> could be used here, and the code below seems to work. This way, we might be able to preserve the performance (avoiding string copying) and at the same time support from_value.

"import" Cow:

/// Serialize with the 0x prefix.
#[cfg(feature = "serde")]
mod serde_hex_prfx {
    use std::borrow::Cow;
    ...

Main code:

    {
        let raw: Cow<str> = serde::Deserialize::deserialize(deserializer)?;
        let src = raw.strip_prefix("0x").unwrap_or(&raw);
        hex::FromHex::from_hex(src).map_err(serde::de::Error::custom)
    } 

[divagant-martian]

This was a good lead. It needs some work because since there is no explicit lifetime serde produces an owned string everytime. I've fixed the pr and will merge soon. A release is unlikely for the next week though

[morph-dev]

Thanks for fixing it.

Can you maybe ping this issue once new released is pushed. Thanks