deps: bump quick-xml dependency from 0.30 to 0.31

[decathorpe]

This compiles but fails tests. There are assertion failures internal to quick-xml code that I don't understand (see CI output once it runs).

Maybe @Mingun has an idea what's going on? I suppose our custom Deserializer for Value is the cause, but I can't tell why:

https://github.com/ironthree/dxr/blob/quick-xml-0.31/dxr/src/values/ser_de.rs#L94

[decathorpe]

There are two kinds of failures, these are examples:

 ---- tests::xml::arrays::from_value_array stdout ----
thread 'tests::xml::arrays::from_value_array' panicked at dxr/src/tests/xml/arrays.rs:71:41:
called `Result::unwrap()` on an `Err` value: UnexpectedEnd([118, 97, 108, 117, 101])

---- tests::xml::call::from_method_call_one_arg stdout ----
thread 'tests::xml::call::from_method_call_one_arg' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/quick-xml-0.31.0/src/de/map.rs:296:21:
assertion `left == right` failed
  left: QName("param")
 right: QName("value")

[Mingun]

That is not the reason of the mentioned errors, but you at least has one error in your ValueVisitor implementation. You should always call .next_value() if you call .next_key() which is not the case here:

dxr/dxr/src/values/ser_de.rs

Lines 235 to 236 in e991bdf

	#[cfg(feature = "nil")]
	Field::Nil => Ok(Value::nil()),

Replace that arm with

#[cfg(feature = "nil")]
Field::Nil => {
    let _: IgnoredAny = map.next_value()?;
    Ok(Value::nil())
}

[Mingun]

Note, that #[serde(rename = "$value")] is useless in the Type definition here and there:

dxr/dxr/src/values/types.rs

Lines 112 to 113 in 65f629b

	#[serde(rename = "i4", alias = "int")]
	Integer(#[serde(rename = "$value")] i32),

In serde model you cannot rename field of a tuple or a newtype (tuple with 1 element). All variants of Type are newtypes in serde model. Actually, that is serde bug that it do not report error about not applicable attribute here.

In terms of quick-xml $value conversion is applied anyway here.

[Mingun]

---- tests::xml::call::from_method_call_one_arg stdout ----
thread 'tests::xml::call::from_method_call_one_arg' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/quick-xml-0.31.0/src/de/map.rs:296:21:
assertion `left == right` failed
  left: QName("param")
 right: QName("value")

This error because you also does not implement ValueVisitor::visit_map correctly. You should consume all keys until next_key() will return None or return an error if more that one key are present. You read only one key here

dxr/dxr/src/values/ser_de.rs

Lines 190 to 191 in 65f629b

	if let Some(key) = map.next_key()? {
	match key {

I will use test with shorter XML for explanation:

dxr/dxr/src/tests/xml/structs.rs

Lines 12 to 18 in 65f629b

	#[test]
	fn from_member() {
	let value = "<member><name>answer</name><value><i4>42</i4></value></member>";
	let expected = Member::new(String::from("answer"), Value::i4(42));
	assert_eq!(from_str::<Member>(value).unwrap(), expected);
	}

When Value starts parsing, the unparsed input is <value><i4>42</i4></value></member>. Then the events as follow:

// <value><i4>42</i4></value></member>
let value: Value = MapValueDeserializer::deserialize_any(ValueVisitor);
    // ....actually, because the first event at start is not Text
    let value: Value = MapValueDeserializer::deserialize_map(ValueVisitor);
    // ...actually, because deserialize_map implemented via deserialize_struct
    let value: Value = MapValueDeserializer::deserialize_struct("", &[] ValueVisitor);
    // ...actually, MapValueDeserializer::deserialize_struct delegates to Deserializer::deserialize_struct
    let value: Value = Deserializer::deserialize_struct("", &[] ValueVisitor);
    // consumes <value>
    let value: Value = ValueVisitor.visit_map(ElementMapAccess)
        // key
        // <i4>42</i4></value></member>
        let field: Field = ElementMapAccess::next_key()
        let field: Field = ElementMapAccess::next_key_seed(PhantomData)
        let field: Field = PhantomData::deserialize(QNameDeserializer("i4"))
        let field: Field = Field::deserialize(QNameDeserializer("i4"))
        let field: Field = QNameDeserializer("i4").deserialize_identifier(FieldVisitor)
        let field: Field = FieldVisitor.visit_str("i4"); // returns Field::i4
        // value
        // <i4>42</i4></value></member>
        let value: i32 = ElementMapAccess::next_value()
        let value: i32 = ElementMapAccess::next_value_seed(PhantomData)
        let value: i32 = PhantomData::deserialize(MapValueDeserializer)
        let value: i32 = i32::deserialize(MapValueDeserializer)
        let value: i32 = MapValueDeserializer::deserialize_i32(PrimitiveVisitor)
            let s: String = MapValueDeserializer::read_string()
            // Consumes <i4>
            let s: String = Deserializer::read_string_impl(true)
            // 42</i4></value></member>
            // Consumes 42</i4>
            let s: String = Deserializer::read_text()
            // </value></member>
        let value: i32 = PrimitiveVisitor::visit_i32("42".parse()?); // Returns 42
    // Because you read only one key, </value> is not consumed

Here
quick_xml types:

• MapValueDeserializer
• Deserializer
• ElementMapAccess
• QNameDeserializer

dxr types:

• Value
• ValueVisitor
• Field
• FieldVisitor

serde types:

• PrimitiveVisitor

ValueVisitor::visit_map implementation should consume everything between <value>...</value>, i.e. <i4>42</i4>. quick-xml consumes key (<i4> in that case) only when it consumes value to support case when key is a enumeration designator. quick-xml returns None from the MapAccess::next_key() when it see that next event is a close tag equal to the tag that started sub-map and consumes that closing tag then.

So because of bug in your implementation, </value> was not consumed and later, when sub-map for <param> closed it detects that situation.

Both errors are not quick-xml specific, you will get the same results with JSON, for example.

[Mingun]

Error

 ---- tests::xml::arrays::from_value_array stdout ----
thread 'tests::xml::arrays::from_value_array' panicked at dxr/src/tests/xml/arrays.rs:71:41:
called `Result::unwrap()` on an `Err` value: UnexpectedEnd([118, 97, 108, 117, 101])

due to the same reason, it's just the execution path is different, so you get a different error.

[decathorpe]

Oof 😬 thank you for the detailed response! I suspected that our custom deserialization logic was to blame.

[decathorpe]

Looks like applying your fixes made all the difference. All the tests pass now. Thank you so much! 🚀

[Mingun]

Probably changing Field to have Unknown(Cow<str>) variant will be better to handle error more gracefully here. Then the error could be more clear that you have unexpected key even if that is not a key that Field can recognize. Currently for the field abcdef you will get "unknown field" error instead of "Unexpected key" error, but I think, that this key in the first place is unexpected and only after that is unknown.

With that variant emit unknown field error in match at line 191.

Also, it maybe worth to add some formal tests for the deserializer implementation using serde_test.

[decathorpe]

I'll look into this, thank you!