rusoto / rusoto

AWS SDK for Rust
MIT License
2.73k stars 450 forks source link

The function for generating XML deserializer should skip element inner tree #1377

Open operutka opened 5 years ago

operutka commented 5 years ago

The code defined here does not skip inner XML elements after matching an element start. This could be a problem in some cases.

For example, I'm using rusoto against localstack to perform an S3 object put. Localstack responds with the following document:

<PutObjectResponse xmlns="http://s3.amazonaws.com/doc/2006-03-01">
  <PutObjectResponse>
    <ETag>&#34;a83a03cd834b396b2e59882ea4c06085&#34;</ETag>
    <LastModified>2019-05-02T13:16:27.858Z</LastModified>
  </PutObjectResponse>
</PutObjectResponse>

The parser expects the end element immediately after the start element. Which is clearly not the case here.

It should be modified from:

start_element(tag_name, stack)?;
let obj = {name}::default();
end_element(tag_name, stack)?;
Ok(obj)

into something like:

start_element(tag_name, stack)?;
let obj = {name}::default();
loop {
    let end_element = match stack.peek() {
        Some(Ok(XmlEvent::EndElement { .. })) => true,
        _ => false,
    };
    if end_element {
        break;
    }
    skip_tree(stack);
}
end_element(tag_name, stack)?;
Ok(obj)
operutka commented 5 years ago

Any news here?

Steffey commented 5 years ago

I ran into the same problem. I opened this issue with localstack.

dholroyd commented 4 years ago

Maybe related https://github.com/spulec/moto/pull/2866

dholroyd commented 4 years ago

So it looks like the moto (therefore localstack) output is wrong in this case - there should be no body XML. So it's not really rusoto at fault here.

However, there is maybe a separate opportunity to skip XML deserialisation entirely (don't even generate the code) when no XML response is expected. That could mitigate then mitigate the problem in this ticket as a side-effect.

I the PR I linked above makes the resulting moto XML response 'better', but doesn't remove it, so a follow-up moto change is still required.

dholroyd commented 4 years ago

Anyone who was having this issue see an improvement since #1728 was merged? (Not yet released.)