-
Notifications
You must be signed in to change notification settings - Fork 199
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
Deserialize Extended S3 Errors #429
Conversation
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.
Looks good! Just some minor comments.
error: smithy_types::Error, | ||
response: &http::Response<B>, | ||
) -> smithy_types::Error { | ||
let mut builder = error.into_builder(); |
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.
Minor optimization: don't convert into a builder unless you know for certain you have a host_id to add.
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.
converting into a builder will probably just get optimized, it's just a newtype that exposes setters
fn add_error_fields() { | ||
let resp = http::Response::builder() | ||
.header( | ||
"x-amz-id-2", |
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.
Add a test case for when the header is missing.
serviceId == ShapeId.from("com.amazonaws.s3#AmazonS3") | ||
|
||
override fun protocols(serviceId: ShapeId, currentProtocols: ProtocolMap): ProtocolMap { | ||
val baseProtocols = super.protocols(serviceId, currentProtocols) |
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.
Since this is inheriting an interface, there isn't really a super. It has a default implementation, but that just returns currentProtocols
. This line doesn't seem necessary.
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.
derp, you are totally right
👍 |
Issue #, if available: Fixes #422
Description of changes: This adds support for deserializing host id from errors for S3.
Semi related changes:
meta()
on generated errorsExample generated code:
The advantage of this style of override is we actually only generating this change in a single place and just invoke this function when deserializing all s3 errors
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.