ironthree / dxr

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

Feature request: Handle requests without typed value #7

Closed bahlo closed 1 year ago

bahlo commented 1 year ago

I'm working with an XML-RPC client which makes (I think) invalid requests, like this:

<?xml version="1.0" encoding="iso-8859-1"?>
<methodCall>
  <methodName>system.listMethods</methodName>
  <params>
    <param>
      <value>whatever</value>
    </param>
  </params>
</methodCall>

As you can see the string whatever should be wrapped in <string></string>.

Is this something that you would see in this library? I 100 % understand if not as I think it's not standard-compliant, I'd have to roll my own in that case.

Edit: According to Wikipedia it's actually valid XML-RPC.

decathorpe commented 1 year ago

Looks like I missed this sentence when I implemented the spec in dxr:

If no type is indicated, the type is string.

c.f. http://xmlrpc.com/spec.md#scalar-values

So having no type is indeed valid (though unusual) and should be interpreted as a string.

Sadly the code for (de)serializing XML (with quick-xml / serde) is already quite complicated and brittle - I'll try to make type-less strings work, but I can't make promises. :)

decathorpe commented 1 year ago

I tried implementing something like this but I couldn't get it to work ... I'll need to think about this more. Any suggestions for how to solve this are welcome - the code that needs changes lives in dxr/src/values/types.rs.

bahlo commented 1 year ago

I‘ve played around with custom deserializers but didn’t really get anywhere. A custom deserializer for Type would work, but introduce a lot of complexity 👀

bahlo commented 1 year ago

I think the rename = "$value" is breaking the whole thing, I have a commit here which probably looks much like yours (but doesn't work) https://github.com/bahlo/dxr/commit/8f549306b6932e337de8f2745cdd97a14338c361

The tests all fail with

unknown variant `$value`, expected one of `i4`, `i8`, `boolean`, `string`, `double`, `dateTime.iso8601`, `base64`, `struct`, `array`, `nil`
decathorpe commented 1 year ago

Yep, that's the same error message that I was getting ... I assume that this is a limitation of how deserialization of XML works with quick-xml, and I haven't yet found a workaround for it.

bahlo commented 1 year ago

I got somewhere with https://github.com/ironthree/dxr/commit/4a08c719a6f8502907fcb3438b8609ec1472b804, this test passes:

#[test]
fn test_deserialize_value() {
    let value = r#"<value><i4>42</i4></value>"#;
    let value: super::Value = quick_xml::de::from_str(value).unwrap();
    assert_eq!(value, super::Value::i4(42));

    let value = r#"<value><string>hi</string></value>"#;
    let value: super::Value = quick_xml::de::from_str(value).unwrap();
    assert_eq!(value, super::Value::string("hi"));

    let value = r#"<value>hi</value>"#;
    let value: super::Value = quick_xml::de::from_str(value).unwrap();
    assert_eq!(value, super::Value::string("hi"));
}

What do you think of that approach? Still needs some work (i.e. cleanup, call custom deserialisers, etc.).

decathorpe commented 1 year ago

Oof, that looks quite hairy ... but only a bit more ugly than the custom (de)serializers that are already there for datetime and base64 types. If this indeed works as expected, I'd be fine with adding a custom Deserialize implementation for this case. Can you move the serde specific code into dxr/src/values/ser_de.rs where the other custom serde implementations are before you submit a PR? :)