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

Verify XML APIs are returning XML with root elements that match the models #1668

Closed
Velfi opened this issue Aug 25, 2022 · 4 comments
Closed
Labels
bug Something isn't working sdk
Milestone

Comments

@Velfi
Copy link
Contributor

Velfi commented Aug 25, 2022

@jdisanti did some extra investigative work while reviewing my PR Add "invalid xml body root" check exemption for S3's GetObjectAttributes and found several other API that may require the same customization:

  • s3#GetBucketLocation
  • s3#ListObjects
  • s3#ListObjectVersions
  • ec2#GetConsoleOutput

Test these APIs to see if they return XML errors. If they do, apply the same customization from my PR.

Alternatively,

Implement John's suggestion to use xmlName instead of allowInvalidXmlRoot and update any buggy operations (don't forget s3#GetObjectAttributes)

@Velfi Velfi added good first issue Good for newcomers bug Something isn't working labels Aug 25, 2022
@jdisanti jdisanti added this to the SDK GA milestone Sep 30, 2022
@jdisanti jdisanti added the sdk label Sep 30, 2022
@jdisanti
Copy link
Collaborator

This gets especially muddy since some of the S3 operations overlap. For example, making a call to ListObjectsV2 without a bucket set results in the XML tag name being ListAllMyBucketsResult and a list of buckets in the response.

@jdisanti
Copy link
Collaborator

Errors are also complicated: awslabs/aws-sdk-rust#873

@rcoh
Copy link
Collaborator

rcoh commented Aug 30, 2023

For ListObjectsV2 (and other S3 operations with bucket) I think the right answer is actually making bucket required—Bucket is bound as a httpLabel effectively so it it's missing we're really not sending the right request

github-merge-queue bot pushed a commit that referenced this issue Aug 31, 2023
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
When a `@contextParam` is marked as required, we will enforce it on
inputs. Since these fields may influence endpoint, omitting them can
result in a different target being hit.

- #1668 
- aws-sdk-rust#873

## Description
<!--- Describe your changes in detail -->

## Testing
- [x] S3 Integration test

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates
- [x] 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._
@jdisanti jdisanti removed the sdk-ga label Nov 30, 2023
@jdisanti jdisanti removed the good first issue Good for newcomers label Apr 5, 2024
@jdisanti
Copy link
Collaborator

jdisanti commented Apr 5, 2024

I don't see value in keeping this issue open anymore. I think most of the XML root issues have been discovered and fixed at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working sdk
Projects
None yet
Development

No branches or pull requests

3 participants