Add @s3UnwrappedXmlOutput trait to support S3's GetBucketLocation
#839
Conversation
jdisanti
requested review from
JordonPhillips,
kstich and
mtdowling
as code owners
|
imho It seems a little weird to create a trait to fix a specific problem on a specific operation, as one would expect traits to be more general purpose. I'm guessing that this is the approach because, due to the structure of the response, Smithy is unable to describe it. Would a more general-purpose approach be a trait which would collapse the wrapper of any XML structure/shape, similar to how maps and lists can be flattened? |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
minor: Should have a newline at EOF.
| import software.amazon.smithy.model.traits.AnnotationTrait; | ||
|
|
||
| public final class S3UnwrappedXmlOutputTrait extends AnnotationTrait { | ||
| public static final ShapeId ID = ShapeId.from("smithy.api#s3UnwrappedXmlOutput"); |
There was a problem hiding this comment.
The namespace should be aws.customizations and not smithy.api here.
There was a problem hiding this comment.
Good catch! Fixed.
| @@ -130,3 +130,22 @@ resolved to "virtual" to enable this setting. | |||
| .. _path-style requests: https://docs.aws.amazon.com/AmazonS3/latest/dev/VirtualHosting.html#path-style-access | |||
| .. _dual-stack endpoints: https://docs.aws.amazon.com/AmazonS3/latest/dev/dual-stack-endpoints.html | |||
| .. _transfer acceleration: https://docs.aws.amazon.com/AmazonS3/latest/dev/transfer-acceleration.html | |||
|
|
|||
| S3 Unwrapped XML Responses | |||
| -------------------------- | |||
There was a problem hiding this comment.
This isn't a component of the bucket addressing customizations, so should probably be at a higher header level using === to declare it.
There was a problem hiding this comment.
Ah, I didn't notice that. Revised the documentation to match the other trait docs and gave it its own top-level section. Thanks!
| S3 Unwrapped XML Responses | ||
| -------------------------- | ||
|
|
||
| Clients code generated from Smithy for Amazon S3 MUST understand the `@s3UnwrappedXmlOutput` trait. |
There was a problem hiding this comment.
Since this is a trait, can the documentation instead look like our traditional XML trait documentation?
You can see the .rst layout here (sadly raw link because GH auto-renders).
|
@kggilmer - The intent is to be able to express S3 in Smithy while avoiding a more generic mechanism that would allow future services to have unwrapped responses like this. |
| Indicates the response output is not wrapped in an operation-level XML tag. | ||
|
|
||
| Trait selector | ||
| .. code-block:: none |
There was a problem hiding this comment.
Since the selector is so simple, this can be reduced to
Trait selector
``operation``
without the follow-up text.
| S3 Traits | ||
| ========= | ||
|
|
||
| ``s3UnwrappedXmlOutput`` trait |
There was a problem hiding this comment.
This should include the namespace.
| ------------------------------ | ||
|
|
||
| Summary | ||
| Indicates the response output is not wrapped in an operation-level XML tag. |
There was a problem hiding this comment.
Given the S3 context, this should probably be more specific:
Indicates that the response body from S3 is not wrapped in the :ref:`aws-restxml-protocol` operation-level XML node.
This should replace the opening sentence in the trait's documentation in aws.customizations.json as well.
| Annotation trait | ||
|
|
||
| This trait indicates the response output is not wrapped in an operation-level tag. | ||
| For example, rather than having some data wrapped in `SomeOperationResult` as shown below, |
There was a problem hiding this comment.
Simple code components in RST syntax are wrapped in double back-ticks, so:
``SomeOperationResult``
This should also be done in a couple places below.
You can make sure the rendered RST is correct by following the steps in the docs readme.
|
|
||
| .. code-block:: xml | ||
|
|
||
| <SomeOperationResult> |
There was a problem hiding this comment.
I think it would be better to have this example be the direct S3 one only, with the expected behavior listed first and the counter-example after it. This will be more straightforward for those reading the spec to implement.
|
|
||
| <LocationConstraint>us-west-2</LocationConstraint> | ||
|
|
||
| Client code generated from Smithy for Amazon S3 MUST understand the `@s3UnwrappedXmlOutput` trait |
There was a problem hiding this comment.
Can the opening of this be made to match the other customization fields?
A client for Amazon S3 MUST...
| params: { | ||
| "LocationConstraint": "us-west-2" | ||
| }, | ||
| protocol: "aws.protocols#restXml" |
There was a problem hiding this comment.
This can just be an unquoted restXml as it's a reference to the protocol trait.
| } | ||
|
|
||
| @httpResponseTests([{ | ||
| id: "GetBucketLocation", |
There was a problem hiding this comment.
Can this have "UnwrappedOutput" appended to the id for clarity of what this is testing? Can you also add a description?
| }]) | ||
| @http(uri: "/GetBucketLocation", method: "GET") | ||
| @s3UnwrappedXmlOutput | ||
| operation GetBucketLocation { |
There was a problem hiding this comment.
For ease of finding the tests themselves, can you move this operation and its traits up to where the others are?
This PR adds a new
@s3UnwrappedXmlOutputtrait so that the response shape for S3's GetBucketLocation call can be expressed in the Smithy model, and also adds an S3 protocol test to exercise the new trait.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.