OwnedMemberName etc. can not be deserialized from a reader, likely not from a string either due to the 'static lifetime #297

Closed zeenix closed 1 year ago

In GitLab by @tyrylu on Nov 7, 2022, 09:48

When working at #285, I came across something which likely is a lifetime issue. The &'static str for a, lets say, OwnedMemberName, can not be created from a dynamic string or a file, so the deserialization fails at runtime. I honestly don't know what solution you'd prefer. The MemberName is elegant in a way, and addition of the owned variants as well. Of course, i can continue the mentioned issue without this particular change, or we can change the internals of the zbus_names types as well.

The &'static str for a, lets say, OwnedMemberName, can not be created from a dynamic string or a file, so the deserialization fails at runtime.

What problem exactly? Can you share a sample code to demonstrate.

For the record, the 'static lifetime inside the Owned* types is the whole point of these types and to avoid users having to specify the 'static lifetime everywhere. If you want to borrow, you use the underlying borrowed types with lifetimes, which can deserialize with borrowing lifetime of the deserializer.

If you need owned types but can't directly dserialize for some reason, you can deserialize to the underlying reference type first and then convert to the owned equivalent wrapper type.

If you need owned types but can't directly dserialize for some reason, you can deserialize to the underlying reference type first and then convert to the owned equivalent wrapper type.

You can use to_owned or into_owned methods to convert to owned types.

In GitLab by @tyrylu on Nov 8, 2022, 09:44

Yes, but if you try to do something like:

use serde::Deserialize;
use quick_xml::de::from_reader;
use zbus_names::OwnedMemberName;

#[derive(Deserialize, Debug)]
struct Test {
    member: OwnedMemberName
}

fn main() {
    let xml_str = "<Test><member>some_member</member></Test>";
    match from_reader::<_, Test>(xml_str.as_bytes()) {
        Ok(val) => println!("Success: {:?}", val),
        Err(e) => println!("Failure: {}", e)
    }
}

You'll get: Failure: invalid type: string "some_member", expected a borrowed string So, deserializing from a reader is basically impossible, and the non-owned variant fails as well, but with a lifetime issue.

You'll get: Failure: invalid type: string "some_member", expected a borrowed string So, deserializing from a reader is basically impossible,

Seems like quick_xml::de::from_reader creates a deserializer that can only work with borrowed string underneath? In any case this looks very much like a quick_xml issue to me.

and the non-owned variant fails as well, but with a lifetime issue.

Kindly be specific. What's the code look like? Did you use quick_xml::de::from_str as the quick_xml::de::from_reader docs suggest for borrowed types?

A workaround would be to deserialize to &str using from_str and convert that. If we have to go this route, I' prefer a comment in the code with link to the relevant quick_xml issue.

In GitLab by @tyrylu on Nov 8, 2022, 13:35

Of course, using from_str works, as you have a clear owner of the string, which you of course lack when deserializing from a reader. The inability to do the deserialization from a reader is not quick_xml specific, it happens for serde_json, serde_xml_rs, and likely all others. So it appears that you can have an OwnedMemberName in a struct, or a from_reader constructor, but not both. And using a MemberName, like this:

use serde::Deserialize;
use serde_json::from_reader;
use zbus_names::MemberName;

#[derive(Deserialize, Debug)]
struct Test<'a> {
    #[serde(borrow)]
    member: MemberName<'a>
}

fn main() {
    let xml_str = "{\"member\": \"some_member\"}";
    match from_reader::<_, Test>(xml_str.as_bytes()) {
        Ok(val) => println!("Success: {:?}", val),
        Err(e) => println!("Failure: {}", e)
    }
}

does not help either, because then the implementation of Deserialize is not general enough (also likely with any deserialization library possible).

Of course, using from_str works, as you have a clear owner of the string, which you of course lack when deserializing from a reader.

Right, so the all good with deserialization of reference type then. You can call .to_owned and you've a way out so you're at least not blocked for #285.

The inability to do the deserialization from a reader is not quick_xml specific, it happens for serde_json, serde_xml_rs, and likely all others.

Well if you look at the fdo mod, you'll see a few instances of Owned* being returned from methods and they've to also satisfy DeserializeOwned and it all works fine. Although it's our own Deserializer being used so we've made sure this all works together.

does not help either, because then the implementation of Deserialize is not general enough (also likely with any deserialization library possible).

Sure, from_reader deserializes to owned types so of course this won't work. This is not a problem.

This code compiles just fine:

use serde::Deserialize;
use serde_json::from_reader;
use zbus_names::OwnedMemberName;

#[derive(Deserialize, Debug)]
struct Test {
    member: OwnedMemberName,
}

fn main() {
    let xml_str = "{\"member\": \"some_member\"}";
    let bytes = xml_str.as_bytes();
    match from_reader::<_, Test>(bytes) {
        Ok(val) => println!("Success: {:?}", val),
        Err(e) => println!("Failure: {}", e),
    }
}

but has the original problem you bumped into. So you're right that this is not specific to quick_xml but I'm not sure what the solution here is. :thinking: There is a small chance still that this is an issue in the quick_xml deserializer and have just gotten passed around from other impls. I sure did try to copy as much as possible from other deserializers when I was implementing the zvariant impls. :)

Oh seems serde_json::from_reader::<_, String>(bytes) hits the same issue. I wonder what from_reader expects? :thinking: This certainly doesn't look like problem with our types.

Oh seems serde_json::from_reader::<_, String>(bytes) hits the same issue. I wonder what from_reader expects? :thinking: This certainly doesn't look like problem with our types.

Ah nm. That's just wrong. :) This works:

#[derive(Deserialize, Debug)]
struct Test {
    member: String,
}

fn main() {
    let xml_str = "{\"member\": \"some_member\"}";
    let bytes = xml_str.as_bytes();
    match from_reader::<_, Test>(bytes) {
        Ok(val) => println!("Success: {:?}", val),
        Err(e) => println!("Failure: {}", e),
    }
}

Oh, I think I've figured out the issue. The Deserialize impl for Owned* is relying on Deserialize impl of the underlying ref type, which assumes a borrowed str. The fix should be simple: The Deserialize impl of owned types should deserialize to a String and convert that to the underlying borrowed type and then just wrap it.

Here is the fix:

impl<'de> Deserialize<'de> for OwnedMemberName {
    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
    where
        D: de::Deserializer<'de>,
    {
        String::deserialize(deserializer)
            .and_then(|n| MemberName::try_from(n).map_err(|e| de::Error::custom(e.to_string())))
            .map(Self)
    }
}

If you could submit an MR with this fix for all the Owned* (also in other subcrates, if needed), that'd be great help.

In GitLab by @tyrylu on Nov 8, 2022, 15:45

Thank you for figuring it out, I could likely do that myself, so, yes, expect a MR shortly.

Great, thanks so much.

mentioned in commit 595157022e2886b447da7ae01d46485827fa1f3b

dbus2 / zbus-old

OwnedMemberName etc. can not be deserialized from a reader, likely not from a string either due to the 'static lifetime #297