-
Notifications
You must be signed in to change notification settings - Fork 195
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
Add "invalid xml body root" check exemption for S3's GetObjectAttributes
#1665
Conversation
…hecking add: XML body root checking exemption for com.amazonaws.s3#GetObjectAttributesOutput fix: minor IDE lints
A new generated diff is ready to view.
A new doc preview is ready to view. |
There was a problem hiding this 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.
aws/sdk/integration-tests/s3/tests/ignore-invalid-xml-body-root.rs
Outdated
Show resolved
Hide resolved
codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/RestXml.kt
Outdated
Show resolved
Hide resolved
.../kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/RestXmlParserGenerator.kt
Outdated
Show resolved
Hide resolved
.../kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/RestXmlParserGenerator.kt
Outdated
Show resolved
Hide resolved
.../kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/RestXmlParserGenerator.kt
Outdated
Show resolved
Hide resolved
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"), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
…ithy/protocols/RestXml.kt Co-authored-by: John DiSanti <[email protected]>
…ithy/protocols/parse/RestXmlParserGenerator.kt Co-authored-by: John DiSanti <[email protected]>
remove: leftover comments
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
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 namedGetObjectAttributesResponse
instead ofGetObjectAttributesOutput
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
CHANGELOG.next.toml
if I made changes to the smithy-rs codegen or runtime cratesCHANGELOG.next.toml
if I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.