impl_deserialize_for_internally_tagged_enum panic on deserialize when inner element is itself an enum

[mattr555]

Hi there, pardon the extra context in this example, but please observe this assertion failure: https://www.rustexplorer.com/b/7hmqkv. It seems that parsing is mismatching on open and close tags:

thread 'main' (80) panicked at /home/explorer/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quick-xml-0.38.4/src/de/map.rs:982:21:
assertion `left == right` failed
  left: QName("root")
 right: QName("action")

See also how this example, which doesn't have the Create action, works okay: https://www.rustexplorer.com/b/6kiqvq

I don't know how best to account for this difference myself, but I'm guessing it's that the child field of the Create member is itself an enum Element instead of a struct like ElementHolder.

Here is a more complete backtrace if you find it useful:

   0: __rustc::rust_begin_unwind
             at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/std/src/panicking.rs:698:5
   1: core::panicking::panic_fmt
             at /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/core/src/panicking.rs:75:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /Users/matt/.rustup/toolchains/stable-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/panicking.rs:394:5
   4: <quick_xml::de::map::MapValueSeqAccess<R,E> as serde_core::de::SeqAccess>::next_element_seed
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quick-xml-0.38.4/src/de/map.rs:982:21
   5: serde_core::de::SeqAccess::next_element
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/serde_core-1.0.228/src/de/mod.rs:1773:14
   6: <serde_core::de::impls::<impl serde_core::de::Deserialize for alloc::vec::Vec<T>>::deserialize::VecVisitor<T> as serde_core::de::Visitor>::visit_seq
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/serde_core-1.0.228/src/de/impls.rs:1164:50
   7: <quick_xml::de::map::MapValueDeserializer<R,E> as serde_core::de::Deserializer>::deserialize_seq
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quick-xml-0.38.4/src/de/map.rs:623:17
   8: serde_core::de::impls::<impl serde_core::de::Deserialize for alloc::vec::Vec<T>>::deserialize
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/serde_core-1.0.228/src/de/impls.rs:1175:22
   9: <core::marker::PhantomData<T> as serde_core::de::DeserializeSeed>::deserialize
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/serde_core-1.0.228/src/de/mod.rs:825:9
  10: <quick_xml::de::map::ElementMapAccess<R,E> as serde_core::de::MapAccess>::next_value_seed
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quick-xml-0.38.4/src/de/map.rs:371:42
  11: serde_core::de::MapAccess::next_value
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/serde_core-1.0.228/src/de/mod.rs:1918:14
  12: <mapbattle::osm::aug_diff::_::<impl serde_core::de::Deserialize for mapbattle::osm::aug_diff::AugmentedDiff>::deserialize::__Visitor as serde_core::de::Visitor>::visit_map
             at ./src/osm.rs:215:30
  13: <&mut quick_xml::de::Deserializer<R,E> as serde_core::de::Deserializer>::deserialize_struct
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quick-xml-0.38.4/src/de/mod.rs:3203:42
  14: mapbattle::osm::aug_diff::_::<impl serde_core::de::Deserialize for mapbattle::osm::aug_diff::AugmentedDiff>::deserialize
             at ./src/osm.rs:215:30
  15: quick_xml::de::from_str
             at /Users/matt/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/quick-xml-0.38.4/src/de/mod.rs:2501:5

[Mingun]

Unfortunately rustexplorer links does not open for me. Could you post the code here?

[mattr555]

@Mingun apologies, here is the code that demonstrates the test assert:

/*
[dependencies]
quick-xml = { version = "0.38.4", features = ["serde-types", "serialize"] }
serde = { version = "1.0", features = ["derive"] }
pretty_assertions = "1.0"
*/
use pretty_assertions::assert_eq;
use quick_xml::impl_deserialize_for_internally_tagged_enum;
use serde::Deserialize;

#[derive(Default, Debug, Deserialize, PartialEq)]
struct Root {
    #[serde(rename = "$value")]
    actions: Vec<Action>,
}

#[derive(Debug, Deserialize, Clone, PartialEq)]
struct Tag {
    #[serde(rename = "@k")]
    k: String,
    #[serde(rename = "@v")]
    v: String,
}

#[derive(Debug, Deserialize, PartialEq)]
#[serde(rename_all = "kebab-case")]
enum Element {
    Node {
        #[serde(rename = "@id")]
        id: u64,
        #[serde(rename = "$value")]
        tags: Option<Vec<Tag>>,
    },
    Way,
    #[serde(other)]
    Other,
}

#[derive(Debug, Deserialize, PartialEq)]
struct ElementHolder {
    #[serde(rename = "$value")]
    e: Element,
}

#[derive(Debug, PartialEq)]
enum Action {
    Create(Element),
    Modify {
        old: ElementHolder,
        new: ElementHolder,
    },
    Delete {
        old: ElementHolder,
        new: ElementHolder,
    },
}

impl_deserialize_for_internally_tagged_enum! {
    Action, "@type",
    ("create" => Create(Element)),
    ("modify" => Modify {
        old: ElementHolder,
        new: ElementHolder
    }),
    ("delete" => Delete {
        old: ElementHolder,
        new: ElementHolder
    })
}

fn main() {
    assert_eq!(
        quick_xml::de::from_str::<Root>(
            r#"
<root>
  <action type="create">
    <node id="123">
      <tag k="key" v="value"/>
    </node>
  </action>
  <action type="modify">
    <old>
      <node id="456">
        <tag k="key" v="old_value"/>
      </node>
    </old>
    <new>
      <node id="456">
        <tag k="key" v="new_value"/>
      </node>
    </new>
  </action>
  <action type="modify">
    <old>
      <way />
    </old>
    <new>
      <way />
    </new>
  </action>
</root>
     "#
        )
        .unwrap(),
        Root {
            actions: vec![
                Action::Create(Element::Node {
                    id: 123,
                    tags: Some(vec![Tag {
                        k: "key".to_string(),
                        v: "value".to_string()
                    }])
                }),
                Action::Modify {
                    old: ElementHolder {
                        e: Element::Node {
                            id: 456,
                            tags: Some(vec![Tag {
                                k: "key".to_string(),
                                v: "old_value".to_string()
                            }])
                        }
                    },
                    new: ElementHolder {
                        e: Element::Node {
                            id: 456,
                            tags: Some(vec![Tag {
                                k: "key".to_string(),
                                v: "new_value".to_string()
                            }])
                        }
                    }
                },
                Action::Modify {
                    old: ElementHolder { e: Element::Way },
                    new: ElementHolder { e: Element::Way }
                }
            ]
        },
    );
}

This assertion passes as expected:

fn main() {
    assert_eq!(
        quick_xml::de::from_str::<Root>(
            r#"
<root>
  <action type="modify">
    <old>
      <node id="456">
        <tag k="key" v="old_value"/>
      </node>
    </old>
    <new>
      <node id="456">
        <tag k="key" v="new_value"/>
      </node>
    </new>
  </action>
  <action type="modify">
    <old>
      <way />
    </old>
    <new>
      <way />
    </new>
  </action>
</root>
     "#
        )
        .unwrap(),
        Root {
            actions: vec![
                Action::Modify {
                    old: ElementHolder {
                        e: Element::Node {
                            id: 456,
                            tags: Some(vec![Tag {
                                k: "key".to_string(),
                                v: "old_value".to_string()
                            }])
                        }
                    },
                    new: ElementHolder {
                        e: Element::Node {
                            id: 456,
                            tags: Some(vec![Tag {
                                k: "key".to_string(),
                                v: "new_value".to_string()
                            }])
                        }
                    }
                },
                Action::Modify {
                    old: ElementHolder { e: Element::Way },
                    new: ElementHolder { e: Element::Way }
                }
            ]
        },
    );
}

[Mingun]

While some bug exists, you mapping not entirely correct. Note, that every Action variant is deserialized from the <action> tag (look carefully, you assume this in Modify and Delete variants), so your Action::Create should contain ElementHolder as well. See:

XML	Rust value
<tag k="key" v="old_value"/>	Tag { ... }
<node id="123"><tag k="key" v="old_value"/></node>	Element::Node { ... }
<action><node id="123"><tag k="key" v="old_value"/></node></action>	ElementHolder { ... }
<old ><node id="123"><tag k="key" v="old_value"/></node></old>	ElementHolder { ... }
<new ><node id="123"><tag k="key" v="old_value"/></node></new>	ElementHolder { ... }

Also, it seems, that actions and tags fields are unnecessarily accept any XML tag (because they are renamed to $value). It seems that it is enough for them to be renamed to action and tag correspondingly (the tag names in XML).

Considering all above, the correct mapping should be:

#[derive(Default, Debug, Deserialize, PartialEq)]
struct Root {
    #[serde(rename = "action")] // <<<<< not $value. Also probably "default"/Option required for empty list of actions
    actions: Vec<Action>,
}

#[derive(Debug, Deserialize, Clone, PartialEq)]
struct Tag {
    #[serde(rename = "@k")]
    k: String,
    #[serde(rename = "@v")]
    v: String,
}

#[derive(Debug, Deserialize, PartialEq)]
#[serde(rename_all = "kebab-case")]
enum Element {
    Node {
        #[serde(rename = "@id")]
        id: u64,
        #[serde(rename = "tag")] // <<<<< not $value
        tags: Option<Vec<Tag>>,
    },
    Way,
    #[serde(other)]
    Other,
}

#[derive(Debug, Deserialize, PartialEq)]
struct ElementHolder {
    #[serde(rename = "$value")]
    e: Element,
}

#[derive(Debug, PartialEq)]
enum Action {
    Create(ElementHolder),
    Modify {
        old: ElementHolder,
        new: ElementHolder,
    },
    Delete {
        old: ElementHolder,
        new: ElementHolder,
    },
}

Unfortunately, this is also not work. Under the hood, content of Action variants deserialized from the MapAccessDeserializer. So, ElementHolder will be deserialized from the MapAccessDeserializer<quick_xml::de::map::ElementMapAccess>.

ElementMapAccess is created by the impl_deserialize_for_internally_tagged_enum when it calls deserialize_map:

quick-xml/src/serde_helpers.rs

Lines 286 to 288 in d7838c3

	// Tell the deserializer to deserialize the data as a map,
	// using the TheVisitor as the decoder
	deserializer.deserialize_map(TheVisitor)

Because of using deserialize_map instead of deserialize_struct the ElementMapAccess does not known that specially named field $value exists in the ElementHolder and cannot provide the corresponding key from MapAccess::next_key_seed.

I doesn't see easy generic solution here. It seems to me, that you should implement impl_deserialize_for_internally_tagged_enum manually for your use case and call deserialize_struct("", &["$value"], TheVisitor) instead of deserialize_map.

[Mingun]

Ok, actually, extend the impl_deserialize_for_internally_tagged_enum! macro was not so hard, so in the next version you could use it for your case. That's not fixes the original bug (we shouldn't trigger debug asserts), so I keep this open until the reason of the bug would be investigated & fixed.

[mattr555]

Thanks very much for your support! Your suggestions make sense to me - indeed I had tried that but was led astray by the missing $value field.