tafia / quick-xml

Rust high performance xml reader and writer
MIT License
1.21k stars 236 forks source link

`Unexpected Event::End(...)` or `missing field '...'` #567

Closed jimy-byerley closed 1 year ago

jimy-byerley commented 1 year ago

Hello dear maintainers

I'm trying to alter ethercat-esi to use quick_xml instead of serde-xml-rs. This is pretty straight forward as ethercat-esi crate is only using the serde macros and calling the xml parser at one place.

// ethercat-esi//src/lib.rs
let raw_info: parser::EtherCATInfo = quick_xml::de::from_str(xml)
            .map_err(|e| Error::new(ErrorKind::Other, e.to_string()))?;

Running this code (in lib or in a test file of that crate) gives me the following output on both xml files

$ target/debug/test/some ~/erob-custom.xml
Error: Custom { kind: Other, error: "Unexpected `Event::End(\"Groups\")`" }

// then I comment the Groups section in the xml
$ target/debug/test/some ~/erob-custom.xml
Error: Custom { kind: Other, error: "missing field `ProductCode`" }

// then I change quick-xml version from v0.27.1 to v0.26.0
$ target/debug/test/some ~/erob-custom.xml
Error: Custom { kind: Other, error: "missing field `Sm`" }

These fields and the Groups markups are good in the xml files. And it doesn't seems to come from the serde declarations of the xml data (in ethercat-esi/src/mod.rs), so I suggest there is an issue with the way the quick_xml functions are calling or checking results of the serde deserialization functions. (especially since the error is not the same depending on the version of quick_xml) How do you think I could solve this problem ?

jimy-byerley commented 1 year ago

I forgot to add the xml files I was using for tests: xml.zip

Mingun commented 1 year ago

This is duplicate of #510 and the error is generated by this (and the others Option<Vec<T>>) piece of code: https://github.com/ethercat-rs/ethercat-esi/blob/3d295ca4c1f8c86f1046a85ce93d14b5d85dcc04/src/parser/mod.rs#L43-L46

This is already fixed in master and I plan to release soon (I think in week or two). You can also change the definition to Vec<T> with the default value as suggested here.

jimy-byerley commented 1 year ago

Thank you very much for your quick answer I tried both the solutions you proposed, but I still get the missing field issue

Here are the branches where I put the modifications you suggested

Is there something I missed ? :thinking:

Mingun commented 1 year ago

Yeah, forgot to mention that you should also tweak your mappings, because quick-xml expect that attributes are named starting from @. Check out the reference.

jimy-byerley commented 1 year ago

Indeed, I didn't paid pattention to the fact that quick-xml wasn't implicitely using attributes instead of fields when they were missing. Sorry for that. I tweaked it as you said. It does find more fields, but now it it gets an even stranger error: missing field '@StartAddress'

Mingun commented 1 year ago

Probably that attribute is really missed? Check the all Sm elements in your XML to see if that true.

jimy-byerley commented 1 year ago

Unfortunately it is not missing :thinking: The same error happens with the unit tests of ethercat-esi, those fields are not missing either but what is strange is that only @StartAddress is not found whereas before a lot of fields where. I wonder if there is something special about it. I double checked for mistypes but it appears to be fine ...

jimy-byerley commented 1 year ago

Maybe it will be easier to reproduce the issue using the unit tests rather than using my xml files. those are the failing unit tests in ethercat-esi:

Mingun commented 1 year ago

Ok, there is a bug when you have a list in the enum variant:

#[test]
fn issue567() {
    #[derive(Debug, Deserialize, PartialEq)]
    struct Device {
        #[serde(rename = "$value")]
        items: Vec<DeviceProperty>,
    }

    #[derive(Debug, Deserialize, PartialEq)]
    enum DeviceProperty {
        Sm(Vec<()>),
    }

    assert_eq!(// currently failed with Unexpected::End
        Device {
            items: vec![
                DeviceProperty::Sm(vec![]),
            ]
        },
        // Currently failed at unwrap() with UnexpectedEnd(b"Device")
        from_str("<Device><Sm/></Device>").unwrap()
    );
}
jimy-byerley commented 1 year ago

Hum, like for Option<Vec> I guess :thinking:

Mingun commented 1 year ago

The error probably in the line 2723: https://github.com/tafia/quick-xml/blob/642de0a28721447822867ec966c79437c1dc1436/src/de/mod.rs#L2715-L2725 We should pass the deserializer with another implementation of deserialize_seq method here. The implementation should be similar to https://github.com/tafia/quick-xml/blob/642de0a28721447822867ec966c79437c1dc1436/src/de/map.rs#L708-L715 but with de: &Deserializer instead of map: &MapAccess.