fix S3 response xml root tag names #8037
Conversation
There was a problem hiding this comment.
I think this is a much better solution than patching all this in the spec 👍, thanks for digging in to this. And also I like that we scraped this from the docs, so should be pretty confident now that it's correct.
maybe we could add a couple of regression unit tests to the serializer to make sure the XML is serialized as expected. unless we have some already :-)
|
I'll look for the unit tests for the serializer! We have one integration test testing previously patched operations: We could have a test using the specs + the dict from the serializer with the right node names and check if the serializer is returning the right name for the operation? I'm not sure how I'd construct this unit test, I'll have a look tomorrow! |
So following #8021, #7983 and #7545, I've investigated and made a list of all S3 operations with a different root node name than the specs. Basically, 41 of 44 operations returning an XML body do not follow the specs. Here's the list:
❌: wrong name, 🟠: fixed by patches, ✅: right from the specs
Note: I've removed
GetBucketLocationas it's a special case with no root node wrapper, andSelectObjectContentas it's a streaming response.This PR implements a fix by directly renaming the root node while still taking advantages of the specs. Previously, modifying the specs would also have effects on the parser where, for example, the shape would already exists (see
GetBucketTaggingandGetObjectTagging, both returns a root node withTaggingname but the shape is different). Now we can still differentiate for each operations.Now, the issue is systematic testing of those operations. I'm confident the new root node names are the right ones, but it would be nice to have replicable tests against AWS. We already have a test in place (
tests.integration.s3.test_s3.TestS3.test_response_structure) testing some operations (and we can see that removing the patches didn't make the test fail as the new serializer part is now taking care of it).However, as 41 of 44 operations needs to be tested for their XML response, this makes it a bit difficult as you would need to setup the resources to be able to query them afterwards (ex.
GetBucketCorsConfigurationwould need a configuration to be able to be queried).would fix #8030