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 field when deserializing unions in json protocols #956

Closed
wants to merge 5 commits into from

Conversation

0marperez
Copy link
Contributor

Issue #

closes aws-sdk-kotlin#1044

Description of changes

-Added a flag for JSON protocols
-Added a __type to union object descriptors
-Ignoring __type unless it's member explicitly in union shape

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

@0marperez 0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Sep 19, 2023
@0marperez 0marperez marked this pull request as ready for review September 19, 2023 16:00
@0marperez 0marperez requested a review from a team as a code owner September 19, 2023 16:00
Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

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

I think we should move more of this custom logic into the the JSON-specific classes. Burying this in abstract or base implementations defeats the purpose of polymorphism and makes the code harder to follow.

There's a bunch of ways this could be done. One way, I think, is the descriptor logic could be moved into JsonSerdeDescriptorGenerator, which could manually add a __type item to the memberShapes list. The logic for rendering the field deserializer could be moved into a JSON-specific union deserializer generator which inherits DeserializeUnionGenerator and overrides renderShapeDeserializer.

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

No Coverage information No Coverage information
71.8% 71.8% Duplication

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

Copy link
Contributor

@aajtodd aajtodd left a comment

Choose a reason for hiding this comment

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

These changes are to account for AWS specific protocol behaviors and as such mostly should be implemented in aws-sdk-kotlin codegen.

I also think we can simplify this and not need to special case __type as a pseudo field. Instead we can just define a new deserialization trait to ignore it all together and support that in the runtime deserializer.

e.g.

  val X_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("x"))
  val Y_DESCRIPTOR = SdkFieldDescriptor(SerialKind.Integer, JsonSerialName("y"))
  val OBJ_DESCRIPTOR = SdkObjectDescriptor.build {
      field(X_DESCRIPTOR)
      field(Y_DESCRIPTOR)
      trait(IgnoreKeys("__type"))   // new object trait to signal that `__type` should just be ignored and not even enumerated in the `findNextFieldIndex()` logic.
  }


deserializer.deserializeStruct(OBJ_DESCRIPTOR) {
    loop@ while (true) {
        when (findNextFieldIndex()) {
            X_DESCRIPTOR.index -> x = value = MyUnion.X(deserializeInt())
            Y_DESCRIPTOR.index -> y = MyUnion.Y(deserializeInt())
            null -> break@loop
            else -> value MyUnion.SdkUnknown.also { skipValue() }
        }
    }
}

This should require way less changes in codegen since the AWS protocols just need to override JsonSerdeDescriptorGenerator.getObjectDescriptorTraits() and be relatively easy to add to to the JsonDeserializer in the runtime.

@@ -24,6 +25,73 @@ open class JsonSerdeDescriptorGenerator(
private val supportsJsonNameTrait: Boolean = true,
) : AbstractSerdeDescriptorGenerator(ctx, memberShapes) {

override fun render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

fix: This logic is specific to AWS protocols and doesn't belong in smithy-kotlin

@@ -362,6 +362,110 @@ class JsonDeserializerTest {
assertTrue(found, "unknown field not enumerated")
}

private class BasicUnionTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests aren't really providing any additional value (especially since this behavior is covered by protocol tests anyway).

@0marperez 0marperez closed this Sep 28, 2023
@0marperez 0marperez deleted the ignore-type-for-unions branch September 28, 2023 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants