-
Notifications
You must be signed in to change notification settings - Fork 867
implement DynamoDBAutoGeneratedTimestampAttribute #3998
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
base: development
Are you sure you want to change the base?
implement DynamoDBAutoGeneratedTimestampAttribute #3998
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the [DynamoDBAutoGeneratedTimestamp]
attribute for automatic timestamp generation during DynamoDB save operations, aligning with the Java SDK's functionality. The implementation automatically sets properties to the current UTC timestamp when they are null.
Key changes:
- Added the
DynamoDBAutoGeneratedTimestampAttribute
class with support for custom attribute names and converters - Integrated auto-generated timestamp logic into the serialization pipeline
- Extended validation to ensure proper type checking and incompatible attribute combinations
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
Attributes.cs | Adds the new DynamoDBAutoGeneratedTimestampAttribute class with constructors for various configurations |
InternalModel.cs | Adds IsAutoGeneratedTimestamp property storage flag and validation logic for timestamp type checking |
ContextInternal.cs | Implements core timestamp generation logic during object serialization |
Context.cs | Updates save operation flow to handle auto-generated properties |
Utils.cs | Adds ValidateTimestampType method for ensuring proper DateTime/DateTimeOffset nullable types |
PropertyStorageTests.cs | Unit tests for property storage validation and timestamp-related functionality |
ContextInternalTests.cs | Unit tests for timestamp generation behavior in various scenarios |
DataModelTests.cs | Integration tests covering timestamp generation with other DynamoDB features |
c952ab1e-3056-4598-9d0e-f7f02187e982.json | Dev config file marking this as a patch-level change |
if (memberType.IsGenericType && memberType.GetGenericTypeDefinition() == typeof(Nullable<>) && | ||
(memberType.IsAssignableFrom(typeof(DateTime)) || | ||
memberType.IsAssignableFrom(typeof(DateTimeOffset)))) | ||
{ | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IsAssignableFrom
check is incorrect for generic nullable types. Should use memberType.GetGenericArguments()[0]
to get the underlying type, then check if it equals typeof(DateTime)
or typeof(DateTimeOffset)
.
if (memberType.IsGenericType && memberType.GetGenericTypeDefinition() == typeof(Nullable<>) && | |
(memberType.IsAssignableFrom(typeof(DateTime)) || | |
memberType.IsAssignableFrom(typeof(DateTimeOffset)))) | |
{ | |
return; | |
if (memberType.IsGenericType && memberType.GetGenericTypeDefinition() == typeof(Nullable<>)) | |
{ | |
var underlyingType = memberType.GetGenericArguments()[0]; | |
if (underlyingType == typeof(DateTime) || underlyingType == typeof(DateTimeOffset)) | |
{ | |
return; | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the intent of this function to only allow DateTime/DateTimeOffsets to be used? if so can you check this suggestion to see if its accurate or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit tests added for the method and the suggestion does not look to be accurate
sdk/test/Services/DynamoDBv2/IntegrationTests/DataModelTests.cs
Outdated
Show resolved
Hide resolved
if (TryGetValue(toStore, propertyStorage.Member, out value)) | ||
{ | ||
DynamoDBEntry dbe = ToDynamoDBEntry(propertyStorage, value, flatConfig, propertyStorage.ShouldFlattenChildProperties); | ||
DynamoDBEntry dbe = ToDynamoDBEntry(propertyStorage, value, flatConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method signature change removes the propertyStorage.ShouldFlattenChildProperties
parameter but there's no clear indication of how this affects the flattening behavior. This could be a breaking change that needs verification.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the propertyStorage.ShouldFlattenChildProperties was wrongly sent as canReturnScalarInsteadOfList params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normj should we mention this as well in the change log that we fixed this incorrect logic? Also wondering since we changed this, how is that that no unit tests broke (were there not any existing ones that cover this case?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DynamoDbFlatten annotation was introduced in this pr: #3833 and this specific case was not covered (list properties in flatten objects)
sdk/test/Services/DynamoDBv2/UnitTests/Custom/DataModel/ContextInternalTests.cs
Outdated
Show resolved
Hide resolved
generator/.DevConfigs/c952ab1e-3056-4598-9d0e-f7f02187e982.json
Outdated
Show resolved
Hide resolved
if (TryGetValue(toStore, propertyStorage.Member, out value)) | ||
{ | ||
DynamoDBEntry dbe = ToDynamoDBEntry(propertyStorage, value, flatConfig, propertyStorage.ShouldFlattenChildProperties); | ||
DynamoDBEntry dbe = ToDynamoDBEntry(propertyStorage, value, flatConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the reason for this change?
if (memberType.IsGenericType && memberType.GetGenericTypeDefinition() == typeof(Nullable<>) && | ||
(memberType.IsAssignableFrom(typeof(DateTime)) || | ||
memberType.IsAssignableFrom(typeof(DateTimeOffset)))) | ||
{ | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the intent of this function to only allow DateTime/DateTimeOffsets to be used? if so can you check this suggestion to see if its accurate or not?
Co-authored-by: Copilot <[email protected]>
I know I'm late reviewing this PR. Giving a status update that this week I'm slammed but hoping to review this next week. |
Add
Description
New Attribute:
[DynamoDBAutoGeneratedTimestamp]
[DynamoDBAutoGeneratedTimestamp]
Automatically sets the property to the current UTC timestamp (DateTime.UtcNow
) on item save.Usage:
Motivation and Context
Testing
unit and integration tests added
Screenshots (if appropriate)
Types of changes
Checklist
License