-
Notifications
You must be signed in to change notification settings - Fork 869
Add support for DynamoDBAutoGeneratedTimestampAttribute and DynamoDbUpdateBehaviorAttribute #3892
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?
Add support for DynamoDBAutoGeneratedTimestampAttribute and DynamoDbUpdateBehaviorAttribute #3892
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.
I don't see the code paths updated for the batch and transact write operations.
sdk/src/Services/DynamoDBv2/Custom/DataModel/ContextInternal.cs
Outdated
Show resolved
Hide resolved
Transact write operation is updated and there is an integration test added - Test_AutoGeneratedTimestampAttribute_TransactWrite_Simple Batch write operation does not offer support for autogenerated values as the operation does not support update expressions and this behavior is consistent with the Java SDK as well as DynamoDBVersionAttribute. |
after testing, looks like Batch write operation on Java SDK it offers support for autogenerated timestamps and ignores the DynamoDbUpdateBehavior. I will update the implementation to cover this. |
autogenerated timestamp implementation updated to cover also batch write operation |
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 adds two new attributes to the .NET SDK for DynamoDB to align with Java SDK functionality: [DynamoDBAutoGeneratedTimestamp]
and [DynamoDbUpdateBehavior]
. The auto-generated timestamp attribute automatically sets DateTime properties to the current UTC timestamp during save operations, while the update behavior attribute controls when properties are updated (Always or IfNotExists).
Key Changes:
- Added
DynamoDBAutoGeneratedTimestampAttribute
andDynamoDbUpdateBehaviorAttribute
classes with validation logic - Extended PropertyStorage to support timestamp auto-generation and update behavior modes
- Modified save operations to handle timestamp generation and conditional updates using
if_not_exists
DynamoDB expressions
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
Attributes.cs | Adds new attribute classes with proper constructors and enum definitions |
InternalModel.cs | Extends PropertyStorage with timestamp and update behavior properties plus validation |
ContextInternal.cs | Implements timestamp generation logic during object serialization |
Context.cs | Updates save operations to handle conditional update behaviors |
Utils.cs | Adds timestamp type validation helper method |
Table.cs | Modifies update operations to support if_not_exists conditions |
Util.cs | Updates expression building to handle conditional attribute updates |
Expression.cs | Adds utility method to merge update expressions |
DocumentTransactWrite.cs | Extends transactional writes to support conditional updates |
TransactWrite.cs | Updates transactional operations to use new conditional logic |
Multiple test files | Comprehensive unit and integration tests for new functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
(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 validation logic for timestamp types is incorrect. IsAssignableFrom
checks if the current type can be assigned from the parameter type, but here you want to check if the nullable's underlying type is DateTime or DateTimeOffset. Use memberType.GetGenericArguments()[0]
to get the underlying type and check if it equals typeof(DateTime)
or 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.
} | ||
else | ||
{ | ||
var setStatement = updateExpression != null ? updateExpression.ExpressionStatement : ""; |
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.
When sets.Length == 0
but there's an existing updateExpression
, the code appends just the existing expression statement without proper SET keyword handling. This could result in malformed update expressions. The logic should handle merging existing expressions more carefully.
Copilot uses AI. Check for mistakes.
|
||
if (dbe == null && propertyStorage.IsAutoGeneratedTimestamp) | ||
{ | ||
document[attributeName] = new Primitive(now.ToString("o")); |
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 timestamp is always stored as an ISO string regardless of the property's storage configuration (StoreAsEpoch, StoreAsEpochLong). This will cause inconsistent storage formats. The code should respect the property's storage settings and convert to epoch time when appropriate.
document[attributeName] = new Primitive(now.ToString("o")); | |
if (propertyStorage.StoreAsEpoch) | |
{ | |
// Store as seconds since Unix epoch (int) | |
var epochSeconds = (int)(now - DateTime.UnixEpoch).TotalSeconds; | |
document[attributeName] = new Primitive(epochSeconds); | |
} | |
else if (propertyStorage.StoreAsEpochLong) | |
{ | |
// Store as milliseconds since Unix epoch (long) | |
var epochMilliseconds = (long)(now - DateTime.UnixEpoch).TotalMilliseconds; | |
document[attributeName] = new Primitive(epochMilliseconds); | |
} | |
else | |
{ | |
// Store as ISO string | |
document[attributeName] = new Primitive(now.ToString("o")); | |
} |
Copilot uses AI. Check for mistakes.
keywordsOrder.Where(k => mergedSections.ContainsKey(k)) | ||
.Select(k => $"{k} {mergedSections[k]}")); | ||
|
||
var mergedNames = Common.Combine(left.ExpressionAttributeNames, right.ExpressionAttributeNames, StringComparer.Ordinal); ; |
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.
There's an extra semicolon at the end of the line, creating a redundant empty statement.
var mergedNames = Common.Combine(left.ExpressionAttributeNames, right.ExpressionAttributeNames, StringComparer.Ordinal); ; | |
var mergedNames = Common.Combine(left.ExpressionAttributeNames, right.ExpressionAttributeNames, StringComparer.Ordinal); |
Copilot uses AI. Check for mistakes.
@@ -231,6 +237,11 @@ public void Validate(DynamoDBContext context) | |||
if (IsHashKey && IsRangeKey) | |||
throw new InvalidOperationException("Property " + PropertyName + " cannot be both hash and range key"); | |||
|
|||
if (UpdateBehaviorMode == UpdateBehavior.IfNotExists && (IsKey || IsVersion || IsCounter)) | |||
{ | |||
throw new InvalidOperationException("Property " + PropertyName + " cannot be a key and have UpdateBehavior set to IfNotExists at the same time."); |
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.
[nitpick] The validation logic could be more descriptive by specifying which specific property type (key, version, or counter) is incompatible with IfNotExists behavior in the exception message.
throw new InvalidOperationException("Property " + PropertyName + " cannot be a key and have UpdateBehavior set to IfNotExists at the same time."); | |
string propertyType = IsKey ? "key" : IsVersion ? "version" : IsCounter ? "counter" : "unknown"; | |
throw new InvalidOperationException($"Property {PropertyName} cannot be a {propertyType} and have UpdateBehavior set to IfNotExists at the same time."); |
Copilot uses AI. Check for mistakes.
@irina-herciu is it possible to split this into two PRs? one for the DynamoDBAutoGeneratedTimestampAttribute and one for the other attribute? it will make it easier for me to review |
Add
Description
New Attributes:
[DynamoDBAutoGeneratedTimestamp]
and[DynamoDbUpdateBehavior]
[DynamoDBAutoGeneratedTimestamp]
Automatically sets the property to the current UTC timestamp (DateTime.UtcNow
) on item save.[DynamoDbUpdateBehavior]
Specifies the update behavior for a property when performing DynamoDB update operations. This attribute can be used to control whether a property is always updated, only updated if not null.Update Behavior Modes:
IfNotExists
: Set the value only when the item is created.Always
: Set the value on both create and update.Usage:
Motivation and Context
Testing
unit and integration tests added
Screenshots (if appropriate)
Types of changes
Checklist
License