fix: s3 custom treatment getbucketlocation response #989

0marperez · 2023-07-19T14:37:20Z

Issue #

194 - Custom treatment of GetBucketLocation response

Description of changes

This was previously fixed using a customization but it left out error deserialization and deserialization of both wrapped and unwrapped non-error responses. Both possibilities are accounted for now

Also look at: smithy-lang/smithy-kotlin#897

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… s3-custom-treatment-getbucketlocation-response

aajtodd

I think we should fix this by implementing support generically for @s3UnwrappedXmlOutput trait.

This could be done by:

Add a new FieldTrait for XmlUnwrappedStruct or similar and implement support in the XmlDeserializer for it.
Either override the descriptor generator for the S3 generator OR define our own generic trait for this and map the s3UnwrappedXmlOutput to our own generic trait. Then smithy-kotlin just operates on our own trait and a customization can preprocess the model to map s3UnwrappedXmlOutput to whatever we are looking for/define.

… s3-custom-treatment-getbucketlocation-response

ianbotsf · 2023-07-26T16:39:03Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXMLOutputIntegration.kt

+ * Applies the [UnwrappedXMLOutput] custom-made [annotation trait](https://smithy.io/2.0/spec/model.html?highlight=annotation#annotation-traits) to a manually-curated list of structures
+ * to mark when special unwrapped xml output deserialization is required.
+ */
+class UnwrappedXMLOutputIntegration : KotlinIntegration {


Style: Generally our names should follow PascalCase, including treating abbreviations like "XML" as regular words for the purposes of capitalization (e.g., UnwrappedXmlOutputIntegration).

ianbotsf · 2023-07-26T16:44:17Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXMLOutputIntegration.kt

+            when {
+                shape.id.toString() in SHAPE_IDS -> {
+                    check(shape is StructureShape) { "Cannot apply UnwrappedXMLOutput to non-member shape" }
+                    shape.toBuilder().addTrait(UnwrappedXMLOutput()).build()
+                }
+                else -> shape
+            }


Question: Why do we need to check shape IDs from an allowlist? Why can't we just detect the aws.customizations#s3UnwrappedXmlOutput trait and then apply the new UnwrappedXMLOutput trait?

when { shape is StructureShape && shape.hasTrait<S3UnwrappedXmlOutputTrait>() -> shape.toBuilder().addTrait(UnwrappedXMLOutput()).build() else -> shape }

Yeah that seems cleaner.

aajtodd · 2023-07-26T19:00:38Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXMLOutputIntegration.kt

+            when {
+                shape.id.toString() in SHAPE_IDS -> {
+                    check(shape is StructureShape) { "Cannot apply UnwrappedXMLOutput to non-member shape" }
+                    shape.toBuilder().addTrait(UnwrappedXMLOutput()).build()
+                }
+                else -> shape
+            }


Yeah that seems cleaner.

aajtodd · 2023-07-26T19:01:24Z

.../s3/common/src/aws/sdk/kotlin/services/s3/internal/GetBucketLocationUnwrappedDeserializer.kt

+import aws.smithy.kotlin.runtime.serde.xml.XmlNamespace
+import aws.smithy.kotlin.runtime.serde.xml.XmlSerialName
+
+internal fun deserializeWrapped(builder: GetBucketLocationResponse.Builder, payload: ByteArray) {


fix: We shouldn't need this now right?

aajtodd · 2023-07-26T19:01:41Z

...n/kotlin/aws/sdk/kotlin/codegen/customization/s3/GetBucketLocationDeserializerIntegration.kt

- * Registers an integration that replaces the codegened GetBucketLocationDeserializer with
- * custom deserialization logic.
+ * Registers an integration that tries deserializing the `GetBucketLocationOperation`
+ * response as wrapped XML response when doing so as [unwrapped](https://smithy.io/2.0/aws/customizations/s3-customizations.html#aws-customizations-s3unwrappedxmloutput-trait) is unsuccessful.
 */
 class GetBucketLocationDeserializerIntegration : KotlinIntegration {


fix: Shouldn't need this now either right?

ianbotsf · 2023-07-28T16:37:29Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXmlOutputIntegration.kt

+    override fun preprocessModel(model: Model, settings: KotlinSettings): Model {
+        val unwrappedStructures = mutableSetOf<String>()
+        ModelTransformer
+            .create()
+            .mapShapes(model) { shape ->
+                when {
+                    shape is OperationShape && shape.hasTrait<S3UnwrappedXmlOutputTrait>() -> {
+                        if (shape.outputShape.toString() != "smithy.api#Unit") unwrappedStructures.add(shape.outputShape.toString())
+                        shape
+                    }
+                    else -> shape
+                }
+            }
+
+        return ModelTransformer
+            .create()
+            .mapShapes(model) { shape ->
+                when {
+                    shape.id.toString() in unwrappedStructures -> {
+                        check(shape is StructureShape) { "Cannot apply UnwrappedXMLOutput to non-structure shape" }
+                        shape.toBuilder().addTrait(UnwrappedXmlOutput()).build()
+                    }
+                    else -> shape
+                }
+            }
+    }


Comment: We don't need a full mapShapes call to find the initial set of structs. We can just iterate through the model directly:

override fun preprocessModel(model: Model, settings: KotlinSettings) { val unwrappedStructures = model .operationShapes .filter { it.hasTrait<S3UnwrappedXmlOutputTrait>() } .map { it.outputShape } .toSet() return ModelTransformer .create() .mapShapes(model) { shape -> when { shape.id in unwrappedStructures -> shape.toBuilder().addTrait(UnwrappedXmlOutput()).build() else -> shape } } }

ianbotsf · 2023-07-28T16:38:50Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXmlOutputIntegration.kt

+                        if (shape.outputShape.toString() != "smithy.api#Unit") unwrappedStructures.add(shape.outputShape.toString())
+                        shape


Question: Is checking for outputShape != smithy.api#Unit important here? It seems like that would be a malformed model since there's no output to unwrap.

ianbotsf · 2023-07-28T16:39:40Z

...gen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXmlOutputIntegration.kt

+                        check(shape is StructureShape) { "Cannot apply UnwrappedXMLOutput to non-structure shape" }
+                        shape.toBuilder().addTrait(UnwrappedXmlOutput()).build()


Question: Is checking for shape is StructureShape important here? It seems like the only things we add to the list are output shapes from operations marked with S3UnwrappedXmlOutputTrait, which must be structures right?

ianbotsf · 2023-07-28T16:41:09Z

...ommon/test/aws/sdk/kotlin/services/s3/internal/GetBucketLocationOperationDeserializerTest.kt

+    fun deserializeUnwrappedResponse() {
+        val responseXML = """
+            <?xml version="1.0" encoding="UTF-8"?>
+            <LocationConstraint xmlns="http://s3.amazonaws.com/doc/2006-03-01/">us-west-2</LocationConstraint>
+        """.trimIndent()
+
+        val response: HttpResponse = HttpResponse(
+            HttpStatusCode(200, "Success"),
+            Headers.invoke { },
+            HttpBody.fromBytes(responseXML.encodeToByteArray()),
+        )
+
+        val actual = runBlocking {
+            GetBucketLocationOperationDeserializer().deserialize(ExecutionContext(), response)
+        }
+
+        assertEquals("UsWest2", actual.locationConstraint.toString())
+    }


Question: I'm confused by this test. Why is the expected output UsWest2 when response XML stated us-west-2?

lauzadis · 2023-08-07T15:32:29Z

...src/test/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXmlOutputIntegrationTest.kt

+    }
+
+    @Test
+    fun test2() {


Can these test and test2 be given more precise names?

lauzadis · 2023-08-07T15:33:17Z

...src/test/kotlin/aws/sdk/kotlin/codegen/customization/s3/UnwrappedXmlOutputIntegrationTest.kt

+// Test s3 model where only the service name matters
+private fun model(serviceName: String): Model =
+    """
+        @http(method: "PUT", uri: "/foo")
+        operation Foo { }
+
+        @http(method: "POST", uri: "/bar")
+        operation Bar { }
+    """
+        .prependNamespaceAndService(operations = listOf("Foo", "Bar"), serviceName = serviceName)
+        .toSmithyModel()


You can grab this function from https://github.com/awslabs/aws-sdk-kotlin/blob/s3-custom-treatment-getbucketlocation-response/codegen/smithy-aws-kotlin-codegen/src/test/kotlin/aws/sdk/kotlin/codegen/testutil/ModelUtil.kt#L7 instead of duplicating it

Note this wasn't available when you first created this PR, I merged it in sometime after :)

sonarcloud · 2023-08-07T16:16:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

0marperez added 11 commits July 17, 2023 17:47

Added deserialization for both wrapped and unwrapped responses

b29814d

ktlint

8a84545

Added error deserialization

6b08de3

ktlint

c650c20

Added error deserialization test

32660c2

Made customization only for GetBucketLocation

5b73ec6

Clean up and documentation

c9f89b1

Added changelog

f25c14a

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

2494f09

… s3-custom-treatment-getbucketlocation-response

Added copyright header

b9db973

Documentation update

f5e02fd

0marperez requested a review from a team as a code owner July 19, 2023 14:37

0marperez mentioned this pull request Jul 19, 2023

fix: s3 custom treatment getbucketlocation response smithy-lang/smithy-kotlin#897

Merged

aajtodd requested changes Jul 19, 2023

View reviewed changes

0marperez added 3 commits July 24, 2023 10:49

Added support in XML deserializer for unwrapped payloads

e6bfe6d

Added generic support of unwrapped XML responses

770ecd1

Merge branch 'main' of https://github.com/awslabs/aws-sdk-kotlin into…

3a984c9

… s3-custom-treatment-getbucketlocation-response

ianbotsf reviewed Jul 26, 2023

View reviewed changes

aajtodd reviewed Jul 26, 2023

View reviewed changes

0marperez added 4 commits July 27, 2023 09:36

File name change

024d43b

Automatically detect s3unwrappedXmlOutput trait

12d8d61

Removed unnecessary customization

0dc2a2a

ktlint

6c10ccd

ianbotsf reviewed Jul 28, 2023

View reviewed changes

Clarity and correctness changes

e6508c9

aajtodd approved these changes Jul 31, 2023

View reviewed changes

lauzadis approved these changes Aug 7, 2023

View reviewed changes

0marperez and others added 2 commits August 7, 2023 12:03

gave tests proper names

8872e86

Imported util function instead of duplicating

c6d319e

0marperez merged commit f8c9121 into main Aug 8, 2023
11 of 12 checks passed

0marperez deleted the s3-custom-treatment-getbucketlocation-response branch August 8, 2023 15:36

aajtodd mentioned this pull request Sep 5, 2023

Support S3 specific traits #850

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: s3 custom treatment getbucketlocation response #989

fix: s3 custom treatment getbucketlocation response #989

0marperez commented Jul 19, 2023 •

edited

Loading

aajtodd left a comment

ianbotsf Jul 26, 2023

ianbotsf Jul 26, 2023

aajtodd Jul 26, 2023

aajtodd Jul 26, 2023

aajtodd Jul 26, 2023

aajtodd Jul 26, 2023

ianbotsf Jul 28, 2023

ianbotsf Jul 28, 2023

ianbotsf Jul 28, 2023

ianbotsf Jul 28, 2023

lauzadis Aug 7, 2023

lauzadis Aug 7, 2023

sonarcloud bot commented Aug 7, 2023

		if (shape.outputShape.toString() != "smithy.api#Unit") unwrappedStructures.add(shape.outputShape.toString())
		shape

		check(shape is StructureShape) { "Cannot apply UnwrappedXMLOutput to non-structure shape" }
		shape.toBuilder().addTrait(UnwrappedXmlOutput()).build()

fix: s3 custom treatment getbucketlocation response #989

fix: s3 custom treatment getbucketlocation response #989

Conversation

0marperez commented Jul 19, 2023 • edited Loading

Issue #

Description of changes

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 7, 2023

0marperez commented Jul 19, 2023 •

edited

Loading