Skip to content
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

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

Merged
merged 6 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ object RuntimeTypes {
val asSdkSerializable = symbol("asSdkSerializable")
val field = symbol("field")

val IgnoreKey = symbol("IgnoreKey")

object SerdeJson : RuntimeTypePackage(KotlinDependency.SERDE_JSON) {
val JsonSerialName = symbol("JsonSerialName")
val JsonSerializer = symbol("JsonSerializer")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ open class JsonParserGenerator(

open val defaultTimestampFormat: TimestampFormatTrait.Format = TimestampFormatTrait.Format.EPOCH_SECONDS

open fun descriptorGenerator(
ctx: ProtocolGenerator.GenerationContext,
shape: Shape,
members: List<MemberShape>,
writer: KotlinWriter,
): JsonSerdeDescriptorGenerator = JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members, supportsJsonNameTrait)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Line length > 120 chars


override fun operationDeserializer(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, members: List<MemberShape>): Symbol {
val outputSymbol = op.output.get().let { ctx.symbolProvider.toSymbol(ctx.model.expectShape(it)) }
return op.bodyDeserializer(ctx.settings) { writer ->
Expand Down Expand Up @@ -127,7 +134,7 @@ open class JsonParserGenerator(
members: List<MemberShape>,
writer: KotlinWriter,
) {
JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members, supportsJsonNameTrait).render()
descriptorGenerator(ctx, shape, members, writer).render()
if (shape.isUnionShape) {
val name = ctx.symbolProvider.toSymbol(shape).name
DeserializeUnionGenerator(ctx, name, members, writer, defaultTimestampFormat).render()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,6 @@ package aws.smithy.kotlin.runtime.serde

import aws.smithy.kotlin.runtime.InternalApi

/**
* This tag interface provides a mechanism to attach type-specific metadata to any field.
* See [aws.smithy.kotlin.runtime.serde.xml.XmlList] for an example implementation.
*
* For example, to specify that a list should be serialized in XML such that values are wrapped
* in a tag called "boo", pass an instance of XmlList to the FieldDescriptor of `XmlList(elementName="boo")`.
*/
@InternalApi
public interface FieldTrait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Denotes that a Map or List may contain null values
* Details at https://awslabs.github.io/smithy/1.0/spec/core/type-refinement-traits.html#sparse-trait
*/
@InternalApi
public object SparseValues : FieldTrait

/**
* A protocol-agnostic type description of a field.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package aws.smithy.kotlin.runtime.serde

import aws.smithy.kotlin.runtime.InternalApi

/**
* This tag interface provides a mechanism to attach type-specific metadata to any field.
* See [aws.smithy.kotlin.runtime.serde.xml.XmlList] for an example implementation.
*
* For example, to specify that a list should be serialized in XML such that values are wrapped
* in a tag called "boo", pass an instance of XmlList to the FieldDescriptor of `XmlList(elementName="boo")`.
*/
@InternalApi
public interface FieldTrait

/**
* Indicates to deserializers to ignore field/key
*
* @param key The key to ignore in the payload
* @param regardlessOfInModel If true will ignore key even though the model indicates key should be there
*
*/
@InternalApi
public data class IgnoreKey(public val key: String, public val regardlessOfInModel: Boolean = true) : FieldTrait
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* Denotes that a Map or List may contain null values
* Details at https://awslabs.github.io/smithy/1.0/spec/core/type-refinement-traits.html#sparse-trait
*/
@InternalApi
public object SparseValues : FieldTrait
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,20 @@ 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, false))) {
if (field == null) { // not in the model
reader.skipNext()
return findNextFieldIndex()
} else {
field.index
}
} else if (descriptor.traits.contains(IgnoreKey(propertyName, true))) {
reader.skipNext() // the value of the ignored key
return findNextFieldIndex()
} else {
field?.index ?: Deserializer.FieldIterator.UNKNOWN_FIELD
}
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,245 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package aws.smithy.kotlin.runtime.serde.json

import aws.smithy.kotlin.runtime.serde.*
import kotlin.test.Test
import kotlin.test.assertEquals

class JsonDeserializerIgnoresKeysTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

class IgnoresKeysTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

companion object {
val X_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("x"))
val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
val Z_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("z"))
val OBJ_DESCRIPTOR = SdkObjectDescriptor.build {
trait(IgnoreKey("z")) // <----
field(X_DESCRIPTOR)
field(Y_DESCRIPTOR)
field(Z_DESCRIPTOR)
}
}
}

@Test
fun itIgnoresKeys() {
val payload = """
{
"x": 1,
"y": 2,
"z": 3
}
""".trimIndent().encodeToByteArray()

val deserializer = JsonDeserializer(payload)
var x: Int? = null
var y: Int? = null
var z: Int? = null
deserializer.deserializeStruct(IgnoresKeysTest.OBJ_DESCRIPTOR) {
loop@ while (true) {
when (findNextFieldIndex()) {
IgnoresKeysTest.X_DESCRIPTOR.index -> x = deserializeInt()
IgnoresKeysTest.Y_DESCRIPTOR.index -> y = deserializeInt()
IgnoresKeysTest.Z_DESCRIPTOR.index -> z = deserializeInt()
null -> break@loop
}
}
}

assertEquals(1, x)
assertEquals(2, y)
assertEquals(null, z)
}

@Test
fun itIgnoresKeysOutOfOrder() {
val payload = """
{
"z": 3,
"x": 1,
"y": 2
}
""".trimIndent().encodeToByteArray()

val deserializer = JsonDeserializer(payload)
var x: Int? = null
var y: Int? = null
var z: Int? = null
deserializer.deserializeStruct(IgnoresKeysTest.OBJ_DESCRIPTOR) {
loop@ while (true) {
when (findNextFieldIndex()) {
IgnoresKeysTest.X_DESCRIPTOR.index -> x = deserializeInt()
IgnoresKeysTest.Y_DESCRIPTOR.index -> y = deserializeInt()
IgnoresKeysTest.Z_DESCRIPTOR.index -> z = deserializeInt()
null -> break@loop
}
}
}

assertEquals(1, x)
assertEquals(2, y)
assertEquals(null, z)
}

@Test
fun itIgnoresKeysManyTimes() {
val payload = """
{
"x": 1,
"y": 2,
"z": 3,
"z": 3,
"z": 3
}
""".trimIndent().encodeToByteArray()

val deserializer = JsonDeserializer(payload)
var x: Int? = null
var y: Int? = null
var z: Int? = null
deserializer.deserializeStruct(IgnoresKeysTest.OBJ_DESCRIPTOR) {
loop@ while (true) {
when (findNextFieldIndex()) {
IgnoresKeysTest.X_DESCRIPTOR.index -> x = deserializeInt()
IgnoresKeysTest.Y_DESCRIPTOR.index -> y = deserializeInt()
IgnoresKeysTest.Z_DESCRIPTOR.index -> z = deserializeInt()
null -> break@loop
}
}
}

assertEquals(1, x)
assertEquals(2, y)
assertEquals(null, z)
}

class IgnoresMultipleKeysTest {
companion object {
val W_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("w"))
val X_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("x"))
val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
val Z_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("z"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicated, can re-use the others descriptors above

val OBJ_DESCRIPTOR = SdkObjectDescriptor.build {
trait(IgnoreKey("w")) // <----
trait(IgnoreKey("z")) // <----
field(W_DESCRIPTOR)
field(X_DESCRIPTOR)
field(Y_DESCRIPTOR)
field(Z_DESCRIPTOR)
}
}
}

@Test
fun itIgnoresMultipleKeys() {
val payload = """
{
"w": 0,
"x": 1,
"y": 2,
"z": 3
}
""".trimIndent().encodeToByteArray()

val deserializer = JsonDeserializer(payload)
var w: Int? = null
var x: Int? = null
var y: Int? = null
var z: Int? = null
deserializer.deserializeStruct(IgnoresMultipleKeysTest.OBJ_DESCRIPTOR) {
loop@ while (true) {
when (findNextFieldIndex()) {
IgnoresMultipleKeysTest.W_DESCRIPTOR.index -> w = deserializeInt()
IgnoresMultipleKeysTest.X_DESCRIPTOR.index -> x = deserializeInt()
IgnoresMultipleKeysTest.Y_DESCRIPTOR.index -> y = deserializeInt()
IgnoresMultipleKeysTest.Z_DESCRIPTOR.index -> z = deserializeInt()
null -> break@loop
}
}
}

assertEquals(null, w)
assertEquals(1, x)
assertEquals(2, y)
assertEquals(null, z)
}

// Now testing `regardlessOfInModel = false` option
class IgnoresKeysConsideringModelTest {
companion object {
val X_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("x"))
val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
val Z_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("z"))
val OBJ_DESCRIPTOR = SdkObjectDescriptor.build {
trait(IgnoreKey("x", false)) // <----
field(X_DESCRIPTOR)
field(Y_DESCRIPTOR)
field(Z_DESCRIPTOR)
}
}
}

@Test
fun itDoesNotIgnoreKeyBecauseItWasInTheModel() {
val payload = """
{
"x": 0
}
""".trimIndent().encodeToByteArray()

val deserializer = JsonDeserializer(payload)
var unionValue: Int? = null
deserializer.deserializeStruct(IgnoresKeysConsideringModelTest.OBJ_DESCRIPTOR) {
loop@ while (true) {
when (findNextFieldIndex()) {
IgnoresKeysConsideringModelTest.X_DESCRIPTOR.index -> unionValue = deserializeInt()
IgnoresKeysConsideringModelTest.Y_DESCRIPTOR.index -> unionValue = deserializeInt()
IgnoresKeysConsideringModelTest.Z_DESCRIPTOR.index -> unionValue = deserializeInt()
null -> break@loop
else -> unionValue = deserializeInt()
}
}
}

assertEquals(0, unionValue)
}

class IgnoresKeysBecauseNotInModelTest {
companion object {
val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
val Z_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("z"))
val OBJ_DESCRIPTOR = SdkObjectDescriptor.build {
trait(IgnoreKey("x", false)) // <----
field(Y_DESCRIPTOR)
field(Z_DESCRIPTOR)
}
}
}

@Test
fun itIgnoresKeyBecauseWasNotInTheModel() {
val payload = """
{
"x": 0
}
""".trimIndent().encodeToByteArray()

val deserializer = JsonDeserializer(payload)
var unionValue: Int? = null
deserializer.deserializeStruct(IgnoresKeysBecauseNotInModelTest.OBJ_DESCRIPTOR) {
loop@ while (true) {
when (findNextFieldIndex()) {
IgnoresKeysBecauseNotInModelTest.Y_DESCRIPTOR.index -> unionValue = deserializeInt()
IgnoresKeysBecauseNotInModelTest.Z_DESCRIPTOR.index -> unionValue = deserializeInt()
null -> break@loop
else -> unionValue = deserializeInt()
}
}
}

assertEquals(null, unionValue)
}
}
Loading