Fix DynamoDbv2 bug for incorrect DateTime epoch serialization when date falls out of epoch supported range #3672

ashishdhingra · 2025-02-25T22:38:15Z

Description

Taking over the PR #3442 to get it across finish line. @sander1095 Thanks for the initial PR.

NOTE (for reviewer):

Added AWSSSDKUtils.ConvertFromUnixEpochSeconds(long) to convert the stored epoch seconds string back to DateTime. Please review for it's correctness.
Please use commit df4d78d1d429daf6f37a1b9f9025da87d19290f0 to see the changes made on top of customer's changes.

IMPORTANT: Per @boblodgett, there were some UTC DateTime fix implemented in V4. So we need to make sure we don't overwrite these when merging to v4-development branch.

Motivation and Context

#3442

Testing

Dry run DRY_RUN-3c72ed89-be29-4254-9bd1-299cbc34e081 completed successfully.

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

peterrsongg · 2025-02-26T21:47:44Z

sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Document.cs

+
+                if (dateTime.HasValue)
+                {
+                    entry = (Primitive)(dateTime.Value);


if we want to modify the caller's reference of entry shouldn't we do something like:

entry = new Primitive(datetime.Value)

Aren't we just modifying the local copy of the reference here? I see that being done below, but maybe you forgot that here?

This was copied from the existing EpochSecondsToDateTime.

Yes, we are just modifying the local copy of the reference here and then returning it. entry = new Primitive(datetime.Value) would also the same, but in a more elegant way. 😄

The function DateTimeToEpochSecondsLong() was added by customer which uses new Primitive(epochSecondsAsString, saveAsNumeric: true), so didn't touched it.

peterrsongg · 2025-02-26T21:49:34Z

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

@@ -822,7 +835,7 @@ private static void PopulateConfigFromType(ItemStorageConfig config, Type type)
                        }
                    }
                }
-                
+


nit: added whitespace?

Removed it somehow.

normj · 2025-02-26T23:55:00Z

sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Document.cs

+
+                // To prevent the data from being (de)serialized incorrectly, we choose to throw the exception instead
+                // of swallowing it.
+                throw;


We should throw our own exception calling out our failure to convert the DateTime to a Epoch. Provide the value of the DateTime, attributeName, and the original exception as the inner exception. If we don't users will get a random numeric conversion exception and have no context why they are getting the error and how to fix it.

Done. Please review.

normj · 2025-02-26T23:56:58Z

sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Document.cs

+                    "Encountered error attempting to convert '{0}' with value '{1}' to epoch seconds: {1}",
+                    attributeName, entry, e);
+
+                // To prevent the data from being (de)serialized incorrectly, we choose to throw the exception instead


For historical purposes can you call out in the comment this was a purposeful change from the existing DateTimeToEpochSeconds to avoid issues silently fallback to string format DateTime.

normj · 2025-02-27T00:00:32Z

sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Document.cs

+                throw;
+            }
+
+            if (epochSecondsAsString != null)


This can never be false anymore since we are always throwing an exception if we fail to convert. To avoid the code getting refactored in weird ways in the future I would add line 336 right after the ConvertToUnixEpochSecondsString call and then return. That would leave no code after the catch block and we don't need the extra if check.

Done. Please review.

normj · 2025-02-27T00:05:32Z

sdk/test/Services/DynamoDBv2/IntegrationTests/DataModelTests.cs

@@ -346,6 +354,7 @@ public void TestContext_CustomDateTimeConverter(bool retrieveDateTimeInUtc)
            Context.ConverterCache.Add(typeof(DateTime), new DateTimeUtcConverter());

            var currTime = DateTime.Now;
+            var longEpochTime = new DateTime(2039, 2, 5, 17, 49, 55, DateTimeKind.Local); // DateTime.Now returns Local kind.


Can you add a test that uses a date before 1970 the start of the Unix epoch?

Used var currTimeBefore1970 = new DateTime(1969, 6, 12, 4, 56, 43, DateTimeKind.Local); for attribute EpochDate2 (it was somehow not decorated with StoreAsEpoch. Decorated it with StoreAsEpoc.

Please advise if we need to also test it with StoreAsEpochLong.

dscpinheiro · 2025-02-27T13:38:35Z

generator/.DevConfigs/3436963d-3bfd-4d4b-a929-bc652e5c1b4d.json

@@ -0,0 +1,11 @@
+{
+    "services": [
+        {


You also changed Core (sdk/src/Core/Amazon.Util/AWSSDKUtils.cs) but didn't include it here.

Thanks for catching it. Fixed.

values outside of 2038 will silently be converted to its original value, in this case, a datetime.

This prevents big API changes

…rrect DateTime epoch serialization issue.

ashishdhingra mentioned this pull request Feb 25, 2025

Fix DynamoDbv2 bug for incorrect DateTime epoch serialization when date falls out of epoch supported range #3442

Closed

10 tasks

ashishdhingra requested a review from normj February 25, 2025 22:42

ashishdhingra force-pushed the PR-3442-DynamoDB-DateTimeEpoch branch 2 times, most recently from f64cf2c to b7fb176 Compare February 26, 2025 00:13

boblodgett requested a review from peterrsongg February 26, 2025 18:28

ashishdhingra requested a review from boblodgett February 26, 2025 18:35

peterrsongg requested changes Feb 26, 2025

View reviewed changes

normj requested changes Feb 27, 2025

View reviewed changes

ashishdhingra requested review from normj and peterrsongg February 27, 2025 07:24

dscpinheiro reviewed Feb 27, 2025

View reviewed changes

ashishdhingra force-pushed the PR-3442-DynamoDB-DateTimeEpoch branch from 9ea348c to 5eba8fd Compare February 27, 2025 15:26

ashishdhingra requested a review from dscpinheiro February 27, 2025 15:26

ashishdhingra force-pushed the PR-3442-DynamoDB-DateTimeEpoch branch from 5eba8fd to 037f1b2 Compare February 27, 2025 18:19

sander1095 and others added 9 commits February 27, 2025 10:35

Use Long instead of int for epoch

44ffd82

values outside of 2038 will silently be converted to its original value, in this case, a datetime.

Update AWSSDKUtils to return a long instead of int

914559e

Use int64 -> string method and reset the code for the int method

f4f5015

This prevents big API changes

Started adding the EpochLong attribute

43ce403

Suppress obsolete warning for succesful build

490b7b5

Fix formatting and throwing when data can't be (de)serialized

2a12dd4

Remove unnecessary suppression

f7c5e33

Fixed some unhandled scenarios and updated integration tests for inco…

9cea78b

…rrect DateTime epoch serialization issue.

Refactored code and updated test case based on review comments.

7bce707

ashishdhingra force-pushed the PR-3442-DynamoDB-DateTimeEpoch branch from 037f1b2 to 7bce707 Compare February 27, 2025 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DynamoDbv2 bug for incorrect DateTime epoch serialization when date falls out of epoch supported range #3672

Fix DynamoDbv2 bug for incorrect DateTime epoch serialization when date falls out of epoch supported range #3672

ashishdhingra commented Feb 25, 2025 •

edited

Loading

peterrsongg Feb 26, 2025

ashishdhingra Feb 26, 2025

peterrsongg Feb 26, 2025

ashishdhingra Feb 27, 2025

normj Feb 26, 2025

ashishdhingra Feb 27, 2025

normj Feb 26, 2025

ashishdhingra Feb 27, 2025

normj Feb 27, 2025

ashishdhingra Feb 27, 2025

normj Feb 27, 2025

ashishdhingra Feb 27, 2025

ashishdhingra Feb 27, 2025

dscpinheiro Feb 27, 2025

ashishdhingra Feb 27, 2025

Fix DynamoDbv2 bug for incorrect DateTime epoch serialization when date falls out of epoch supported range #3672

Are you sure you want to change the base?

Fix DynamoDbv2 bug for incorrect DateTime epoch serialization when date falls out of epoch supported range #3672

Conversation

ashishdhingra commented Feb 25, 2025 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

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

Choose a reason for hiding this comment

ashishdhingra commented Feb 25, 2025 •

edited

Loading