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

Testing

Dry run DRY_RUN-3c72ed89-be29-4254-9bd1-299cbc34e081 completed successfully.
2nd dry run DRY_RUN-dff1146e-110b-45ac-97ed-855f4250280f 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.

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

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.

IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 that this is changed to DateTimeKind.Utc. This test may still work in v4 but it could have an issue from changing the default: DynamoDB RetrieveDateTimeInUtc has been switched to true as the default.

Reminder what an epoch is: number of seconds that have passed since Thursday 1 January 1970 00:00:00 UTC

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. Used updateMinimum as false since only AWSSDK.DynamoDBv2 uses it.

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.

boblodgett · 2025-02-28T22:18:29Z

sdk/src/Core/Amazon.Util/AWSSDKUtils.cs

+        /// <returns>Converted DateTime structure</returns>
+        public static DateTime ConvertFromUnixLongEpochSeconds(long seconds)
+        {
+            return new DateTime(seconds * 10000000L + EPOCH_START.Ticks, DateTimeKind.Utc).ToLocalTime();


IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 removes the .ToLocalTime().

boblodgett · 2025-02-28T22:22:10Z

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

+            var longEpochTime = new DateTime(2039, 2, 5, 17, 49, 55, DateTimeKind.Local); // DateTime.Now returns Local kind.
+            var longEpochTimeBefore1970 = new DateTime(1969, 12, 30, 23, 59, 59, DateTimeKind.Local);


IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 that this is changed to DateTimeKind.Utc. This test may still work in v4 but it could have an issue from changing the default: DynamoDB RetrieveDateTimeInUtc has been switched to true as the default.

Reminder what an epoch is: number of seconds that have passed since Thursday 1 January 1970 00:00:00 UTC

boblodgett · 2025-02-28T22:22:57Z

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.


IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 that this is changed to DateTimeKind.Utc. This test may still work in v4 but it could have an issue from changing the default: DynamoDB RetrieveDateTimeInUtc has been switched to true as the default.

Reminder what an epoch is: number of seconds that have passed since Thursday 1 January 1970 00:00:00 UTC

boblodgett · 2025-02-28T22:23:31Z

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

+            var longEpochTime = new DateTime(2039, 7, 23, 2, 3, 4, DateTimeKind.Local);
+            var longEpochTimeBefore1970 = new DateTime(1955, 12, 30, 23, 59, 59, DateTimeKind.Local);


IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 that this is changed to DateTimeKind.Utc. This test may still work in v4 but it could have an issue from changing the default: DynamoDB RetrieveDateTimeInUtc has been switched to true as the default.

Reminder what an epoch is: number of seconds that have passed since Thursday 1 January 1970 00:00:00 UTC

boblodgett · 2025-02-28T22:28:33Z

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

@@ -25,6 +25,8 @@ public partial class DynamoDBTests : TestBase<AmazonDynamoDBClient>
        private static readonly DateTime EpochDate = DateTime.Now.AddDays(7);
        private static readonly TimeSpan Epsilon = TimeSpan.FromSeconds(1);
        private static readonly int EpochSeconds = AWSSDKUtils.ConvertToUnixEpochSeconds(EpochDate);
+        private static readonly DateTime LongEpochDate = new DateTime(2039, 1, 1, 2, 13, 23, DateTimeKind.Local);


IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 that this is changed to DateTimeKind.Utc. This test may still work in v4 but it could have an issue from changing the default: DynamoDB RetrieveDateTimeInUtc has been switched to true as the default.

Reminder what an epoch is: number of seconds that have passed since Thursday 1 January 1970 00:00:00 UTC

boblodgett · 2025-02-28T22:32:07Z

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

+            var longEpochTime = new DateTime(2039, 7, 23, 2, 3, 4, DateTimeKind.Local);
+            var longEpochTimeBefore1970 = new DateTime(1955, 12, 30, 23, 59, 59, DateTimeKind.Local);


IMPORTANT: We need to ensure if this PR is merged into v3 that any future merge into v4 that this is changed to DateTimeKind.Utc. This test may still work in v4 but it could have an issue from changing the default: DynamoDB RetrieveDateTimeInUtc has been switched to true as the default.

Reminder what an epoch is: number of seconds that have passed since Thursday 1 January 1970 00:00:00 UTC

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 8 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.

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

Refactored code and updated test case based on review comments.

c5b619b

ashishdhingra force-pushed the PR-3442-DynamoDB-DateTimeEpoch branch from 7bce707 to c5b619b Compare February 27, 2025 21:46

boblodgett reviewed Feb 28, 2025

View reviewed changes

normj approved these changes Feb 28, 2025

View reviewed changes

peterrsongg approved these changes Mar 3, 2025

View reviewed changes

ashishdhingra merged commit 43086a3 into main-staging Mar 3, 2025
1 check passed

ashishdhingra deleted the PR-3442-DynamoDB-DateTimeEpoch branch March 3, 2025 23:27

ashishdhingra mentioned this pull request Mar 4, 2025

Critical DateTime to Epoch serialization issue for DynamoDb when date is beyond epoch supported date #3443

Closed

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 27, 2025

ashishdhingra Feb 27, 2025

normj Feb 27, 2025

ashishdhingra Feb 27, 2025

ashishdhingra Feb 27, 2025

boblodgett Feb 28, 2025

dscpinheiro Feb 27, 2025

ashishdhingra Feb 27, 2025 •

edited

Loading

boblodgett Feb 28, 2025

boblodgett Feb 28, 2025

boblodgett Feb 28, 2025

boblodgett Feb 28, 2025

boblodgett Feb 28, 2025

boblodgett Feb 28, 2025

		var longEpochTime = new DateTime(2039, 2, 5, 17, 49, 55, DateTimeKind.Local); // DateTime.Now returns Local kind.
		var longEpochTimeBefore1970 = new DateTime(1969, 12, 30, 23, 59, 59, DateTimeKind.Local);

		var longEpochTime = new DateTime(2039, 7, 23, 2, 3, 4, DateTimeKind.Local);
		var longEpochTimeBefore1970 = new DateTime(1955, 12, 30, 23, 59, 59, DateTimeKind.Local);

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

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

ashishdhingra Feb 27, 2025 • edited Loading

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

ashishdhingra Feb 27, 2025 •

edited

Loading