fix: ignore __type when deserializing union for AWS Json protocols #964

0marperez · 2023-09-27T13:45:22Z

Issue #

Description of changes

-Added IgnoreKey trait and adapted JsonDeserializer to it

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

aajtodd · 2023-09-27T14:19:45Z

runtime/serde/common/src/aws/smithy/kotlin/runtime/serde/SdkFieldDescriptor.kt

- * in a tag called "boo", pass an instance of XmlList to the FieldDescriptor of `XmlList(elementName="boo")`.
- */
-@InternalApi
-public interface FieldTrait


Why are these moved?

Since the file was named SdkFieldDescriptor I thought it would make more sense to have field traits in a different place, I can change it to how it was though

aajtodd · 2023-09-27T14:20:55Z

runtime/serde/common/src/aws/smithy/kotlin/runtime/serde/SerdeTraits.kt

+ *
+ */
+@InternalApi
+public data class IgnoreKey(public val key: String, public val regardlessOfInModel: Boolean = true) : FieldTrait


This should be in serde-json as it only applies there right? (see JsonFieldTraits.kt)

Question: Why do we need regardlessOfInModel, I would assume if we set IgnoreKey as a trait on the object descriptor it's going to be ignored regardless of whether it's modeled. This seems unnecessary to me but I'd like to understand what the motivation was.

I was thinking of a case where we have a union with a member called __type. If that happens we would not deserialize that member, because we're ignoring that key regardless of what the model says. The default is true anyway so it behaves as expected unless set explicitly to false.

I didn't put it in a JSON specific file because I think this might be useful in the future for other deserializers like the XML one. I don't know if we have a use case already but that was my reasoning behind it.

What do you think? Should regardlessOfInModel stay? Is this something that could be useful elsewhere?

I was thinking of a case where we have a union with a member called __type. If that happens we would not deserialize that member, because we're ignoring that key regardless of what the model says

Shouldn't this just be a choice by codegen though? If we want it deserialized because it's in the model don't add the IgnoreKey trait.

I didn't put it in a JSON specific file because I think this might be useful in the future for other deserializers like the XML one. I don't know if we have a use case already but that was my reasoning behind it.

YAGNI probably. Also IgnoreKey was a suggestion for the name based on JSON objects are "keyed". The same isn't really true of XML.

That makes sense but the issue this is trying to solve: smithy-lang/smithy#1945
is that sometimes a unnecessary __type is serialized. What you're suggesting is to always ignore __type. But there could be a cases where __type could be in the model and we would be ignoring it because sometimes there's an extra one there unnecessarily

What you're suggesting is to always ignore __type. But there could be a cases where __type could be in the model and we would be ignoring it because sometimes there's an extra one there unnecessarily

I don't follow, if __type is in the model and we want to deserialize it then just don't emit the IgnoreKey trait during codegen. What am I missing here?

I guess that logic could live in codegen, now it's in the deserializer

lauzadis · 2023-09-27T17:14:12Z

...rde-json/common/test/aws/smithy/kotlin/runtime/serde/json/JsonDeserializerIgnoresKeysTest.kt

+class JsonDeserializerIgnoresKeysTest {
+    class IgnoresKeysTest {


this double-nested class seems unnecessary, I think you can just declare these at the top-level without using a companion object for simplicity

lauzadis · 2023-09-27T17:27:25Z

...rde-json/common/test/aws/smithy/kotlin/runtime/serde/json/JsonDeserializerIgnoresKeysTest.kt

+            val X_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("x"))
+            val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
+            val Z_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("z"))


duplicated, can re-use the others descriptors above

ianbotsf

Looks good overall!

ianbotsf · 2023-09-27T18:50:16Z

runtime/serde/serde-json/common/src/aws/smithy/kotlin/runtime/serde/json/JsonDeserializer.kt

@@ -198,7 +198,13 @@ private class JsonFieldIterator(
                val token = reader.nextTokenOf<JsonToken.Name>()
                val propertyName = token.value
                val field = descriptor.fields.find { it.serialName == propertyName }
-                field?.index ?: Deserializer.FieldIterator.UNKNOWN_FIELD
+
+                if (descriptor.traits.contains(IgnoreKey(propertyName))) {


Style: Consider if (IgnoreKey(propertyName) in descriptor.traits)

ianbotsf · 2023-09-27T18:51:14Z

...rde-json/common/test/aws/smithy/kotlin/runtime/serde/json/JsonDeserializerIgnoresKeysTest.kt

+    private val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
+    private val Z_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("z"))
+    private val OBJ_DESCRIPTOR = SdkObjectDescriptor.build {
+        trait(IgnoreKey("z")) // <----


Question: What does the comment // <---- mean? Same question on L120-121.

ianbotsf · 2023-09-27T18:52:21Z

...src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/serde/JsonParserGenerator.kt

+        shape: Shape,
+        members: List<MemberShape>,
+        writer: KotlinWriter,
+    ): JsonSerdeDescriptorGenerator = JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members, supportsJsonNameTrait)


Style: Line length > 120 chars

aajtodd

Looks good, one suggestion but otherwise approved.

aajtodd · 2023-09-27T20:37:28Z

...rde-json/common/test/aws/smithy/kotlin/runtime/serde/json/JsonDeserializerIgnoresKeysTest.kt

+import kotlin.test.Test
+import kotlin.test.assertEquals
+
+class JsonDeserializerIgnoresKeysTest {


Suggestion: Every test has explicitly modeled field descriptors. You're missing coverage for unmodeled fields that we want to skip rather than enumerate as unknown.

sonarqubecloud · 2023-09-28T14:27:58Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
39.9% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

IgnoreKey added to Json deserializer

3c7febc

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Sep 27, 2023

0marperez marked this pull request as ready for review September 27, 2023 14:11

0marperez requested a review from a team as a code owner September 27, 2023 14:11

aajtodd reviewed Sep 27, 2023

View reviewed changes

Removed regardlessOfModel option from IgnoreKeys trait

febc8da

lauzadis approved these changes Sep 27, 2023

View reviewed changes

Simplified IgnoreKeys tests

c2cfdbb

ianbotsf approved these changes Sep 27, 2023

View reviewed changes

Style changes

c207a32

aajtodd approved these changes Sep 27, 2023

View reviewed changes

0marperez added 2 commits September 28, 2023 10:22

Added test for key not in model

5769b44

Simplified test

5258f30

0marperez merged commit a9aa47c into main Sep 28, 2023
8 of 9 checks passed

0marperez deleted the ignore-type-unions-aws-json-protocols branch September 28, 2023 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ignore __type when deserializing union for AWS Json protocols #964

fix: ignore __type when deserializing union for AWS Json protocols #964

0marperez commented Sep 27, 2023

aajtodd Sep 27, 2023

0marperez Sep 27, 2023

aajtodd Sep 27, 2023

aajtodd Sep 27, 2023

0marperez Sep 27, 2023

0marperez Sep 27, 2023

0marperez Sep 27, 2023

aajtodd Sep 27, 2023

aajtodd Sep 27, 2023

0marperez Sep 27, 2023

aajtodd Sep 27, 2023

0marperez Sep 27, 2023

lauzadis Sep 27, 2023

lauzadis Sep 27, 2023

ianbotsf left a comment

ianbotsf Sep 27, 2023

ianbotsf Sep 27, 2023

ianbotsf Sep 27, 2023

aajtodd left a comment

aajtodd Sep 27, 2023

sonarqubecloud bot commented Sep 28, 2023

		class JsonDeserializerIgnoresKeysTest {
		class IgnoresKeysTest {

fix: ignore __type when deserializing union for AWS Json protocols #964

fix: ignore __type when deserializing union for AWS Json protocols #964

Conversation

0marperez commented Sep 27, 2023

Issue #

Description of changes

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

Choose a reason for hiding this comment

ianbotsf 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

aajtodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 28, 2023