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

Add "invalid xml body root" check exemption for S3's GetObjectAttributes #1665

Merged

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Aug 24, 2022

Motivation and Context

aws-sdk-rust#609

Description

It is now possible to exempt specific operations from XML body root checking. To do this, add the AllowInvalidXmlRoot
trait to the output struct of the operation you want to exempt. Previously, S3's GetObjectAttributes would always fail because they return an XML object with a root named GetObjectAttributesResponse instead of GetObjectAttributesOutput as specified by their model.

They'll still be returning the incorrect XML, we just won't hold them to their model once this is merged.

Testing

I have included an integration test

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…hecking

add: XML body root checking exemption for com.amazonaws.s3#GetObjectAttributesOutput
fix: minor IDE lints
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on this! Most of my comments are minor, and feel free to punt on the ones that aren't minor to a follow up so that we can get a release out with this fixed.

private val logger: Logger = Logger.getLogger(javaClass.name)
private val invalidXmlRootAllowList = setOf(
// API returns GetObjectAttributes_Response_ instead of Output
ShapeId.from("com.amazonaws.s3#GetObjectAttributesOutput"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a brief stroll through botocore to see if there were any other obvious operations that have the wrong shape, and we may want to check s3#GetBucketLocation, s3#ListObjects, s3#ListObjectVersions, and ec2#GetConsoleOutput (this last one is using the ec2Query protocol, which still has XML responses). Not saying these ones meet the criteria, but that I see special code in botocore parsing these XML responses in botocore. If it's easy to check these, then we should, but probably not worth spending a ton of time on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've broken this work out into a separate issue: #1668

/**
* Indicates that a service is expected to send XML where the root element name does not match the modeled member name.
*/
class AllowInvalidXmlRoot : AnnotationTrait(ID, Node.objectNode()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was cross checking the S3 documentation with the S3 model to try and find other operations that may be broken (which unfortunately turned out to be a pointless endeavor—they're both wrong), I noticed that some S3 operations use the @xmlName trait to correct the wrong root. For example, GetBucketLocationOutput has @xmlName("LocationConstraint") to fix its root. Should the S3 customization just add that trait to the outputs for these cases rather than the codegen needing to handle a separate synthetic trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've broken this work out into a separate issue: #1668

Velfi and others added 4 commits August 25, 2022 12:17
…ithy/protocols/RestXml.kt

Co-authored-by: John DiSanti <jdisanti@amazon.com>
…ithy/protocols/parse/RestXmlParserGenerator.kt

Co-authored-by: John DiSanti <jdisanti@amazon.com>
remove: leftover comments
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti enabled auto-merge (squash) August 27, 2022 01:37
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti merged commit 63afb41 into main Aug 27, 2022
@jdisanti jdisanti deleted the add/invalid-xml-root-exemption-for-s3-get-object-attributes branch August 27, 2022 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants