RReverser / serde-xml-rs

xml-rs based deserializer for Serde (compatible with 1.0+)
https://crates.io/crates/serde-xml-rs
MIT License
270 stars 90 forks source link

Unable to deserialize newtype structs #38

Open Michael-F-Bryan opened 7 years ago

Michael-F-Bryan commented 7 years ago

I just encountered an issue when deserializing a newtype struct. You usually use a newtype struct (e.g. struct Foo(Bar)) to put something inside a tag, however serde_xml_rs seems to want to treat it as a map.

Recycling bits from the tests/test.rs file:

#[derive(Debug, PartialEq, Deserialize)]
struct NewType(Item);

#[test]
fn newtypes_are_just_wrappers_around_the_inner_type() {
    let src = "<NewType>
                <Item>
                    <name>foo</name>
                    <source>/path/to/foo</source>
                </Item>
            </NewType>";

    let should_be = NewType(Item {
        name: String::from("foo"),
        source: String::from("/path/to/foo"),
    });

    let got: NewType = from_str(src).unwrap();
    assert_eq!(got, should_be);
}

When run (cargo test --tests), this errors with:

---- newtypes_are_just_wrappers_around_the_inner_type stdout ----
    DEBUG - Peeked StartElement(NewType, {"": "", "xml": "http://www.w3.org/XML/1998/namespace", "xmlns": "http://www.w3.org/2000/xmlns/"})
DEBUG - Fetched StartElement(NewType, {"": "", "xml": "http://www.w3.org/XML/1998/namespace", "xmlns": "http://www.w3.org/2000/xmlns/"})
thread 'newtypes_are_just_wrappers_around_the_inner_type' panicked at 'called `Result::unwrap()` on an `Err` value: Error(Custom("invalid type: map, expected tuple struct NewType"), State { next_error: None, backtrace: None })', /checkout/src/libcore/result.rs:906:4
note: Run with `RUST_BACKTRACE=1` for a backtrace.
farodin91 commented 7 years ago

Seems to be the same bug as #26

Michael-F-Bryan commented 7 years ago

Possibly, although in that case it's more to do with tagged enums, while this one is purely about structs.

It could also be because the Serializer::deserialize_newtype_struct() function forwards to deserialize_any() and that's not interpreting it correctly. Should I have a go at implementing the deserialize_newtype_struct() method manually and see if that fixes the problem?

farodin91 commented 7 years ago

The following code seems to solve only partial failures.

fn deserialize_newtype_struct<V: de::Visitor<'de>>(self, _name: &str, visitor: V) -> Result<V::Value>
    {
        visitor.visit_newtype_struct(self)
    }
Michael-F-Bryan commented 7 years ago

I don't think that would work. You don't read the opening tag and check it's correct, so it'll probably blindly let anything through.

How was the deserializer designed? I've got an initial implementation for deserialize_newtype_struct() which correctly passes my tests, but it's pretty hard to understand the rest of the code with all the macros, read_inner_value(), and the magical is_map_value bool...

farodin91 commented 7 years ago

It would be cool to refactor the code in more serde json like version

Michael-F-Bryan commented 7 years ago

It would be cool to refactor the code in more serde json like version

What did you have in mind? From memory the serde_json deserializer incorporates both a generic JSON parser and a serde::Deserializer, while we're handing the parsing bit off to xml-rs.

farodin91 commented 7 years ago

I through about looking how deserialize in JSON works.

Michael-F-Bryan commented 7 years ago

Mmm, I don't know if that'll work for us. Their code is doing a lot more than us (compare serde_json to us) and they deal in characters and are doing the parsing with all the difficulties that come along with that (unicode and string escaping). While we're just dealing in XML events and the parsing has already been done for us.

In theory it should just be a case of peeking at the next XML event and then either reading that node or recursively deserializing its components... Which I believe is pretty much what we're doing already.

kotx commented 1 year ago

I was unable to deserialize a #[serde(rename = "$value")] tuple struct here. Is it possible to have this implemented?