Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework our XML parsing #362

Closed
matthewkmayer opened this issue Aug 26, 2016 · 5 comments
Closed

Rework our XML parsing #362

matthewkmayer opened this issue Aug 26, 2016 · 5 comments

Comments

@matthewkmayer
Copy link
Member

@matthewkmayer matthewkmayer commented Aug 26, 2016

As seen in #313 we're out of date for xml-rs. My attempts at sliding it in with our existing abstractions didn't work well. I think we should switch our consumption of xml to pulling and handling what comes out. This is different than our current looping around the expected fields until we find the one we need.

Hopefully this will also help our performance issues as seen in #298 .

@matthewkmayer
Copy link
Member Author

@matthewkmayer matthewkmayer commented Sep 9, 2016

Extra incentive: while compiling with -vv xml-rs causes rustc to spit out a bunch of

warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

@matthewkmayer
Copy link
Member Author

@matthewkmayer matthewkmayer commented Jun 6, 2017

This issue needs more details!

Here's an existing snippet from S3 generated code:

try!(start_element(tag_name, stack));

let mut obj = AbortIncompleteMultipartUpload::default();

loop {
    let next_event = match stack.peek() {
        Some(&Ok(XmlEvent::EndElement { ref name, .. })) => DeserializerNext::Close,
        Some(&Ok(XmlEvent::StartElement { ref name, .. })) => {
            DeserializerNext::Element(name.local_name.to_owned())
        }
        _ => DeserializerNext::Skip,
    };

    match next_event {
        DeserializerNext::Element(name) => {
            match &name[..] {
                "DaysAfterInitiation" => {
                    obj.days_after_initiation =
                        Some(try!(DaysAfterInitiationDeserializer::deserialize("DaysAfterInitiation",
                                                                                stack)));
                }
                _ => skip_tree(stack),
            }
        }
        DeserializerNext::Close => break,
        DeserializerNext::Skip => {
            stack.next();
        }
    }
}

try!(end_element(tag_name, stack));

We're manually looping through the stack and doing a lot of comparisons (for data types with many fields, not this example with a single field). It's probably slow and it's definitely fragile. I worry if AWS sends us malformed data we may get into an infinite loop.

Compare this to a service that responds with JSON, like ECS:

Ok(serde_json::from_str::<ListClustersResponse>(String::from_utf8_lossy(&response.body).as_ref()).unwrap())

I'd much rather push the heavy lifting off to a library. 😉

@mattwoodyard
Copy link
Contributor

@mattwoodyard mattwoodyard commented Nov 16, 2018

Is there still interest in this? I'm working on generating servers from the service definitions and using something 'serde' like would make much of that code cleaner. If there is still interest in this I can put together a prototype using serde for the xml parts.

@matthewkmayer
Copy link
Member Author

@matthewkmayer matthewkmayer commented Nov 16, 2018

I'd love to see progress made on this. A prototype would be 💯 .

@matthewkmayer
Copy link
Member Author

@matthewkmayer matthewkmayer commented Mar 9, 2019

This issue is getting stale and I'm inclined to close it. Perhaps new XML parsing options will crop up later and we can reassess.

Any reason not to close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants