Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jimy-byerley opened this issue Mar 1, 2023 · 11 comments · Fixed by #663
Closed

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

jimy-byerley opened this issue Mar 1, 2023 · 11 comments · Fixed by #663
Labels
arrays Issues related to mapping XML content onto arrays using serde bug serde Issues related to mapping from Rust types to XML

Comments

@jimy-byerley
Copy link

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
Copy link
Author

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

@Mingun
Copy link
Collaborator

Mingun commented Mar 1, 2023

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.

@Mingun Mingun closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2023
@Mingun Mingun added duplicate arrays Issues related to mapping XML content onto arrays using serde labels Mar 1, 2023
@jimy-byerley
Copy link
Author

jimy-byerley commented Mar 2, 2023

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

  • quick-xml-fix modifies ethercat-esi to use quick-xml master branch (commit 40b1448)
  • no-options modifies ethercat-esi to use quick-xml = 1.27.1 and remove mentions of Option<Vec>

Is there something I missed ? 🤔

@Mingun
Copy link
Collaborator

Mingun commented Mar 2, 2023

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
Copy link
Author

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
Copy link
Collaborator

Mingun commented Mar 2, 2023

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

@jimy-byerley
Copy link
Author

Unfortunately it is not missing 🤔
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
Copy link
Author

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
Copy link
Collaborator

Mingun commented Mar 3, 2023

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()
    );
}

@Mingun Mingun reopened this Mar 3, 2023
@Mingun Mingun removed the duplicate label Mar 3, 2023
@jimy-byerley
Copy link
Author

Hum, like for Option<Vec> I guess 🤔

@Mingun
Copy link
Collaborator

Mingun commented Mar 16, 2023

The error probably in the line 2723:

quick-xml/src/de/mod.rs

Lines 2715 to 2725 in 642de0a

fn next_element_seed<T>(&mut self, seed: T) -> Result<Option<T::Value>, Self::Error>
where
T: DeserializeSeed<'de>,
{
match self.peek()? {
DeEvent::Eof => Ok(None),
// Start(tag), End(tag), Text
_ => seed.deserialize(&mut **self).map(Some),
}
}

We should pass the deserializer with another implementation of deserialize_seq method here. The implementation should be similar to

quick-xml/src/de/map.rs

Lines 708 to 715 in 642de0a

struct SeqItemDeserializer<'de, 'a, 'm, R>
where
R: XmlRead<'de>,
{
/// Access to the map that created this deserializer. Gives access to the
/// context, such as list of fields, that current map known about.
map: &'m mut MapAccess<'de, 'a, R>,
}

but with de: &Deserializer instead of map: &MapAccess.

Mingun added a commit to Mingun/quick-xml that referenced this issue Oct 9, 2023
failures (2):
  serde-de-seq (1):
    seq::variable_name::fixed_size::list_of_enum
  serde-issues (1):
    issue567
Mingun added a commit to Mingun/quick-xml that referenced this issue Oct 9, 2023
…type variant

The implementation of `VariantAccess::newtype_variant` should be the same as
`Deserializer::visit_newtype_struct`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays Issues related to mapping XML content onto arrays using serde bug serde Issues related to mapping from Rust types to XML
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants