implement polymorphism support for DynamoDB entries #3643

irina-herciu · 2025-02-07T12:47:00Z

implement DynamoDBPolymorphicTypeAttribute for polymorphism support for table entries and properties polymorphism support that should solve Saving nested items with class hierarchies Saving nested items with class hierarchies #2985

Description

New DynamoDB Attribute added as DynamoDBPolymorphicTypeAttribute in order to add support for polymorphic types serialization and deserialization
[DynamoDBPolymorphicType("A2", typeof(ModelA2))]

Type Attribute name can be configurable from DerivedTypeAttributeName on DynamoDBContextConfig and by default uses "$type".

      //class level use
      [DynamoDBPolymorphicType("dog", typeof(Dog))]
      [DynamoDBPolymorphicType("cat", typeof(Cat))]
      public class Animal { }

       //property level
       public class Zoo
       {
        [DynamoDBPolymorphicType("dog", typeof(Dog))]
        [DynamoDBPolymorphicType("cat", typeof(Cat))]
        public Animal Resident { get; set; }
       }

Motivation and Context

#2985

Testing

Integration tests added

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

implement DynamoDBDerivedTypeAttribute for polymorphism support update nested items for V4

dscpinheiro · 2025-02-07T13:04:30Z

Can the other PR (that was targeting V3) - #3633 - be closed?

irina-herciu · 2025-02-07T13:17:26Z

@dscpinheiro the issue that the PR has in scope to fix was affecting AWSSDK.DynamoDBv2 3.7.104, but as @normj instructed in #3633 the fix is raised for V4 also.

dscpinheiro · 2025-02-07T13:32:13Z

Ok, my read from Norm's comment in the other PR was that we should only make this change for V4.

But I can double-check with him offline.

normj · 2025-02-07T22:43:30Z

@irina-herciu Can you add a change file like you did in your previous PR. https://github.com/aws/aws-sdk-net/pull/3614/files

normj · 2025-02-07T22:54:32Z

@irina-herciu A concern I have is a user saves the item with .NET but another parts reads the item with a different language have a mechanism that only stores the .NET type won't be portable. The AWS Kotlin SDK has similar support for polymorphism in their high level. It supports a mechanism customers can use to control the identifier for the type and they could potentially use a language neutral identifier. Can you rework this to have some sort of system that allows users to register a type conversation mapping. By default it can rely on .NET type but if a user can opt-in their own mechanism that would unblock polyglot customers.

Validation should be a customer can save a item using polymorphism in our Kotlin SDK and have a way to read it back out using your changes. My understanding in the Kotlin SDK you use the HeterogenerousItemConverter https://sdk.amazonaws.com/kotlin/api/latest/dynamodb-mapper/aws.sdk.kotlin.hll.dynamodbmapper.items/-heterogeneous-item-converter/index.html

irina-herciu · 2025-02-10T08:38:01Z

@normj The functionality implemented of this feature is different from the 'HeterogenerousItemConverter', as the Kotlin SDK feature is designed to convert top-level items in a DynamoDB table, meaning it applies polymorphism only when retrieving or saving items at the root level and requires a custom 'AttributeConverter' to handle polymorphic nested items.

irina-herciu · 2025-02-10T08:38:24Z

@irina-herciu Can you add a change file like you did in your previous PR. https://github.com/aws/aws-sdk-net/pull/3614/files

Change file added

normj · 2025-02-17T08:13:14Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Attributes.cs

@@ -71,6 +71,38 @@ public DynamoDBTableAttribute(string tableName, bool lowerCamelCaseProperties)
        }
    }

+    /// <summary>
+    /// DynamoDB attribute that marks a class for polymorphism support.


Can you add more depth to the docs. Explaining how you add multiple declaration of the attribute for each possible subtype and how the TypeDiscriminator is used.

added extra code comments for docs

normj · 2025-02-17T08:15:25Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Attributes.cs

+
+        /// <summary>
+        /// Derived type of the Property type.
+        /// Type must be a subclass of the Property type.


What happens if the property type is an interface? Instead of a subclass can it be a type that implements the interface?

both interfaces and and subclasses are supported. Integration tests added cover this scenario.

normj · 2025-02-17T08:27:11Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Attributes.cs

@@ -243,6 +275,36 @@ public DynamoDBPropertyAttribute(string attributeName, bool storeAsEpoch)
        public bool StoreAsEpoch { get; set; }
    }

+    /// <summary>
+    /// DynamoDB Polymorphic Property Attribute that marks up current member for polymorphism support.


I'm confused on the purpose of separating the attribute that goes on the type vs the property. Seems like they solve same purpose. You are giving them the option of defining the polymorphism on either the type or the property. What happens if the declaration is on both the type and the property that uses the type. Should that be allowed?

I reworked the implementation in order to have one single attribute for this scope. If the attribute is used on both the type and property, the property one allows more granular control, overriding or supplementing the class-level behavior.

normj · 2025-02-17T08:28:47Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

@@ -140,6 +140,11 @@ public DynamoDBContextConfig()
        /// Service calls made via <see cref="AmazonDynamoDBClient"/> will always return 
        /// <see cref="DateTime"/> attributes in UTC.</remarks>
        public bool? RetrieveDateTimeInUtc { get; set; }
+
+        /// <summary>
+        /// Property indicating the name of the attribute used for derived types conversion.


Expand on this documentation. I assume this is the attribute that stores the metadata used by this library to figure out the type to instantiate.

This attribute is missing listing the default attribute.

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

normj · 2025-02-17T08:32:11Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/InternalModel.cs

        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2072",
            Justification = "The user's type has been annotated with DynamicallyAccessedMemberTypes.All with the public API into the library. At this point the type will not be trimmed.")]
-        internal SimplePropertyStorage(MemberInfo member)
+        public SimplePropertyStorage(MemberInfo member)


Why is this being changed to public?

reverted to internal

normj · 2025-02-17T08:34:41Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/InternalModel.cs

@@ -341,10 +373,17 @@ internal StorageConfig([DynamicallyAccessedMembers(InternalConstants.DataModelMo
    /// <summary>
    /// Storage information for a specific class that is associated with a table
    /// </summary>
-    internal class ItemStorageConfig : StorageConfig
+    internal class ItemStorageConfig 


Can you explain why the removal of the base class? When you propose this implementation changes please give background in the PR description to help speed up PR reviews.

StorageConfig was removed as the base class of ItemStorageConfig and composition used for Storage configuration in order to support multiple Storage configurations for supported derived types.

normj · 2025-02-26T08:35:13Z

Running the branch through the internal build system for validation.

normj · 2025-02-26T23:02:40Z

Internal Dry run DRY_RUN-2b085cb8-c005-4606-9a0f-aafb76c2ad6e was successful

implement DynamoDBDerivedTypeAttribute for polymorphism support

39f6def

implement DynamoDBDerivedTypeAttribute for polymorphism support update nested items for V4

normj self-requested a review February 7, 2025 23:49

add change file

a3510c0

add PolymorphicType support for entries

b8ac44c

irina-herciu changed the title ~~implement DynamoDBDerivedTypeAttribute for polymorphism support~~ implement polymorphism support for DynamoDB entries Feb 11, 2025

ashishdhingra requested a review from philasmar February 13, 2025 17:35

normj requested changes Feb 17, 2025

View reviewed changes

Refactoring and additional integration testing

e1ee970

normj approved these changes Feb 26, 2025

View reviewed changes

dscpinheiro added the v4 label Feb 28, 2025

philasmar approved these changes Feb 28, 2025

View reviewed changes

dscpinheiro merged commit 49f166f into aws:v4-development Feb 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement polymorphism support for DynamoDB entries #3643

implement polymorphism support for DynamoDB entries #3643

irina-herciu commented Feb 7, 2025 •

edited

Loading

dscpinheiro commented Feb 7, 2025

irina-herciu commented Feb 7, 2025

dscpinheiro commented Feb 7, 2025

normj commented Feb 7, 2025

normj commented Feb 7, 2025

irina-herciu commented Feb 10, 2025

irina-herciu commented Feb 10, 2025

normj Feb 17, 2025

irina-herciu Feb 25, 2025

normj Feb 17, 2025

irina-herciu Feb 25, 2025

normj Feb 17, 2025

irina-herciu Feb 25, 2025

normj Feb 17, 2025

normj Feb 17, 2025

irina-herciu Feb 25, 2025

normj Feb 17, 2025

irina-herciu Feb 17, 2025

normj commented Feb 26, 2025

normj commented Feb 26, 2025

implement polymorphism support for DynamoDB entries #3643

implement polymorphism support for DynamoDB entries #3643

Conversation

irina-herciu commented Feb 7, 2025 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

dscpinheiro commented Feb 7, 2025

irina-herciu commented Feb 7, 2025

dscpinheiro commented Feb 7, 2025

normj commented Feb 7, 2025

normj commented Feb 7, 2025

irina-herciu commented Feb 10, 2025

irina-herciu commented Feb 10, 2025

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

normj commented Feb 26, 2025

normj commented Feb 26, 2025

irina-herciu commented Feb 7, 2025 •

edited

Loading