fix: Fixes S3 and Ec2 error parsing #787
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbelkins
reviewed
Dec 28, 2022
.../main/kotlin/software/amazon/smithy/aws/swift/codegen/customization/s3/S3ErrorIntegration.kt
Show resolved
Hide resolved
|
LGTM, could you add documentation for public APIs? |
ganeshnj
reviewed
Dec 30, 2022
Comment on lines
14
to
20
| var requestId: String? { | ||
| return headers.value(for: "x-amz-request-id") | ||
| } | ||
|
|
||
| var requestId2: String? { | ||
| return headers.value(for: "x-amz-id-2") | ||
| } |
There was a problem hiding this comment.
I wonder if this extension should be headers collection.
There was a problem hiding this comment.
Good question, and can go either way here. IMO the fact that these values come from headers are implementation details. That being said, it could be cool to live in a world where all the headers are defined on Headers.
jbelkins
approved these changes
Jan 5, 2023
ganeshnj
approved these changes
Jan 5, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes issues with parsing S3 and Ec2 errors.
S3 errors were failing to parse correctly when the error was a 404 with an empty body. The cause of this issue is that our local logic to check if the body is empty was incorrect. It previously, defined
isEmptyas having a httpBody instance that equaled.data(nil). Now, it definesisEmptyas having a httpBody instance that is either.none, or the data is nil or is empty.Ec2 errors were failing to parse correctly because the request id key in the response is
RequestIDwhile we were expectingRequestId. According to the smithy spec for Ec2Query protocol,RequestIdis correct so this appears to a special case for the Ec2 service. Now the Ec2 error models will attempt to parse using bothRequestIDandRequestId.After this issue is done, we can come back here and fix the Ec2 errors in a different way. We can define a custom key decoding strategy, and always replace the key
RequestIdwithRequestIDjust for the Ec2 client.Issue #
#761
#759
Description of changes
New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.