instant-labs / instant-xml

11 stars 3 forks source link

Allow empty strings, instead of treating them as missing #62

Closed wez closed 1 month ago

wez commented 1 month ago

Note that this PR is based on https://github.com/instant-labs/instant-xml/pull/61, but doesn't depend upon it; I didn't want to just push this straight into that PR, and I didn't want to create too many more branches to reconcile the version of this crate that is pulled into my downstream project!

wez commented 1 month ago

once both this and #61 are merged, I think it's reasonable to cut a new release of instant-xml; I've been sitting on both of these changes for the past week or so and haven't run into other weird cases around them in that time.

wez commented 1 month ago

Hmm, hold on merging this: I tweaked the implementation and tests to make the PR look nicer, but running the tests in my downstream project now fail. I'm investigating!

wez commented 1 month ago

OK, the problem case was with an empty nested direct field being treated as missing:

#[derive(Debug, PartialEq, FromXml)]
struct ArtUri {
    #[xml(direct)]
    uri: String,
}

#[derive(Debug, PartialEq, FromXml)]
struct Container {
    art: Option<ArtUri>,
}

#[test]
fn container_empty_string() {
    assert_eq!(
        from_str("<Container><ArtUri></ArtUri></Container>"),
        Ok(Container {
            art: Some(ArtUri {
                uri: "".to_string()
            })
        })
    );
}

I've adjusted the codegen for that case; it now looks like this; I've annotated the new sections with HERE:

    fn deserialize<'cx>(
        into: &mut Self::Accumulator,
        field: &'static str,
        deserializer: &mut ::instant_xml::Deserializer<'cx, 'xml>,
    ) -> ::std::result::Result<(), ::instant_xml::Error> {
        use ::instant_xml::de::Node;
        use ::instant_xml::{Accumulate, Error, FromXml, Id, Kind};
        enum __Elements {
            __Ignore,
        }
        enum __Attributes {
            __Ignore,
        }
        let mut __value0 = <String as FromXml>::Accumulator::default();
        let mut seen_direct = false; // HERE: control flag to remember if we saw the text node
        loop {
            let node = match deserializer.next() {
                Some(result) => result?,
                None => break,
            };
            match node {
                Node::Attribute(attr) => {
                    let id = deserializer.attribute_id(&attr)?;
                    let field = __Attributes::__Ignore;
                    match field {
                        __Attributes::__Ignore => {}
                    }
                }
                Node::Open(data) => {
                    let id = deserializer.element_id(&data)?;
                    let element = __Elements::__Ignore;
                    match element {
                        __Elements::__Ignore => {
                            let mut nested = deserializer.nested(data);
                            nested.ignore()?;
                        }
                    }
                }
                Node::Text(text) => {
                    seen_direct = true; // HERE: set control flag
                    let mut nested = deserializer.for_node(Node::Text(text));
                    <String>::deserialize(&mut __value0, "ArtUri::uri", &mut nested)?;
                }
                node => {
                    return Err(
                        Error::UnexpectedNode({
                            let res = ::alloc::fmt::format(
                                format_args!("{0:?} in {1}", node, "ArtUri"),
                            );
                            res
                        }),
                    );
                }
            }
        }
        // HERE: this is the #after_loop code, same as the Node::Text code but with an
        // implied empty string value
        if !seen_direct {
            let mut nested = deserializer.for_node(Node::Text("".into()));
            <String>::deserialize(&mut __value0, "ArtUri::uri", &mut nested)?;
        }
        *into = Some(Self {
            uri: __value0.try_done("ArtUri::uri")?,
        });
        Ok(())
    }
djc commented 1 month ago

This makes sense to me! I rebased and made some minor style changes in the impls module.

djc commented 1 month ago