-
Notifications
You must be signed in to change notification settings - Fork 737
add explicit message deser to all s3 errors #3237
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
Conversation
|
fixes #2921 |
|
|
||
| private static Shape withMessage(Shape shape) { | ||
| return ((StructureShape) shape).toBuilder() | ||
| .addMember("Message", ShapeId.from("com.amazonaws.s3#Message")) |
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.
If we're always adding a message to the struct, would it be an issue if the Message field is added to the model later 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.
same question, should we iterate members first to see if message already exists?
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.
Nothing happens -- our addMember overrides whatever was there. Message is a "protected" (how strictly I don't know) field in errors across all AWS protocols so if they did add it ever it would be a string type and nothing would change other than our customization becoming redundant.
If you both are really concerned I can guard it but it doesn't really change anything.
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 suspect this also means that the shape of the error is always of shape StructureShape? because I suspect we also lose all other information from the shape when casting and calling toBuilder.
You know more than I do and seem pretty sure about the shape change, so I trust it
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.
@error MUST target a structure, yes (that's something you can constrain) - https://smithy.io/2.0/spec/type-refinement-traits.html#error-trait
relates #3229
Unlike other API models, S3 does not explicitly model a
Messagefield in its error types. This means that error messages will only bubble up to the caller for generic (unmodeled) errors, since in that path, the shared error deserialization logic kicks in.We rely on the message member being present in other services such that the deser logic is generated to pull that field out of the body. This PR backfills a message field onto all modeled errors such that the correct deser is generated.
Output of
panic(err)on a CreateBucket call before:(above output is literal, there is no text after
BucketAlreadyExists:and after: