ironthree / dxr

Declarative XML-RPC in Rust
Apache License 2.0
17 stars 8 forks source link

deps: bump quick-xml dependency from 0.30 to 0.31 #21

Closed decathorpe closed 3 months ago

decathorpe commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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: https://github.com/ironthree/dxr/blob/e991bdfe8f7beb42798895a83d7e7cd49b0b02e2/dxr/src/values/ser_de.rs#L235-L236 Replace that arm with

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

Note, that #[serde(rename = "$value")] is useless in the Type definition here and there: https://github.com/ironthree/dxr/blob/65f629bc0abe0c64fbe3592ed32beebb72449d96/dxr/src/values/types.rs#L112-L113 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 commented 3 months ago
---- 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 https://github.com/ironthree/dxr/blob/65f629bc0abe0c64fbe3592ed32beebb72449d96/dxr/src/values/ser_de.rs#L190-L191

I will use test with shorter XML for explanation: https://github.com/ironthree/dxr/blob/65f629bc0abe0c64fbe3592ed32beebb72449d96/dxr/src/tests/xml/structs.rs#L12-L18

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:

dxr types:

serde types:

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 commented 3 months ago

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 commented 3 months ago

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

decathorpe commented 3 months ago

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