-
Notifications
You must be signed in to change notification settings - Fork 868
Features/dynamo db update behavior attribute #4049
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?
Features/dynamo db update behavior attribute #4049
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 adds support for the [DynamoDbUpdateBehavior]
attribute to the .NET SDK for DynamoDB, enabling control over property update behavior during DynamoDB update operations. The feature aligns with the Java SDK's @DynamoDbUpdateBehavior
annotation and introduces an IfNotExists
mode that sets property values only when items are created (not on updates), alongside the default Always
mode.
Key Changes
- New
DynamoDbUpdateBehaviorAttribute
andUpdateBehavior
enum to specify update behavior (Always/IfNotExists) - Updates to the update expression generation logic to support conditional
if_not_exists
clauses for properties marked withIfNotExists
behavior - Integration with existing auto-generated timestamp functionality to enable create-only timestamp tracking
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
sdk/src/Services/DynamoDBv2/Custom/DataModel/Attributes.cs | Adds new DynamoDbUpdateBehaviorAttribute and UpdateBehavior enum |
sdk/src/Services/DynamoDBv2/Custom/DataModel/InternalModel.cs | Adds UpdateBehaviorMode property to PropertyStorage and validation logic |
sdk/src/Services/DynamoDBv2/Custom/DataModel/ContextInternal.cs | Implements GetUpdateIfNotExistsAttributeNames helper to identify properties with IfNotExists behavior |
sdk/src/Services/DynamoDBv2/Custom/DataModel/Context.cs | Updates save operations to pass IfNotExists attribute names to update helpers |
sdk/src/Services/DynamoDBv2/Custom/DataModel/TransactWrite.cs | Updates transact write to support IfNotExists attributes |
sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Table.cs | Modifies UpdateHelper methods to accept and process IfNotExists attribute names |
sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Util.cs | Updates expression generation to wrap IfNotExists attributes in if_not_exists() function |
sdk/src/Services/DynamoDBv2/Custom/DocumentModel/Expression.cs | Adds MergeUpdateExpressions method for combining multiple update expressions |
sdk/src/Services/DynamoDBv2/Custom/DocumentModel/DocumentTransactWrite.cs | Updates transact write document operations to support IfNotExists attributes |
sdk/src/Services/DynamoDBv2/Custom/DataModel/Utils.cs | Updates error message for validation |
sdk/test/Services/DynamoDBv2/UnitTests/Custom/DataModel/ContextInternalTests.cs | Adds unit tests for GetUpdateIfNotExistsAttributeNames functionality |
sdk/test/Services/DynamoDBv2/UnitTests/Custom/DocumentModel/TableTests.cs | New test file covering various update scenarios with and without IfNotExists attributes |
sdk/test/Services/DynamoDBv2/UnitTests/Custom/DocumentModel/ExpressionsTest.cs | New test file for expression merging functionality |
sdk/test/Services/DynamoDBv2/IntegrationTests/DataModelTests.cs | Updates integration tests to verify IfNotExists behavior with auto-generated timestamps |
generator/.DevConfigs/9490947f-209f-47e9-8c70-3698872df304.json | Adds dev config for minor version bump |
|
||
var update = kvp.Value; | ||
|
||
var createOnly = ifNotExistAttributeNames?.Contains(attribute) ?? false; |
Copilot
AI
Oct 15, 2025
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.
Linear search through ifNotExistAttributeNames
list occurs for every attribute update. Consider converting the list to a HashSet<string>
at the call site for O(1) lookup performance when processing multiple attribute updates.
Copilot uses AI. Check for mistakes.
if (update.Action == AttributeAction.DELETE && !createOnly) | ||
{ |
Copilot
AI
Oct 15, 2025
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.
DELETE actions are silently suppressed for createOnly attributes without any indication to the caller. This could lead to unexpected behavior where a delete operation is ignored. Consider throwing an exception or logging a warning when attempting to delete a createOnly attribute.
if (update.Action == AttributeAction.DELETE && !createOnly) | |
{ | |
if (update.Action == AttributeAction.DELETE) | |
{ | |
if (createOnly) | |
{ | |
throw new InvalidOperationException( | |
$"DELETE action is not allowed for createOnly attribute '{attribute}'."); | |
} |
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); |
Copilot
AI
Oct 15, 2025
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 Common.Combine
method is being called but may throw InvalidOperationException
for duplicate keys with different values (as shown in the test at line 96-113 of ExpressionsTest.cs). However, this exception would be thrown inside Common.Combine
, not in the calling code shown here. Consider documenting this behavior or handling the exception at this level to provide better error context.
var mergedNames = Common.Combine(left.ExpressionAttributeNames, right.ExpressionAttributeNames, StringComparer.Ordinal); | |
Dictionary<string, string> mergedNames; | |
try | |
{ | |
mergedNames = Common.Combine(left.ExpressionAttributeNames, right.ExpressionAttributeNames, StringComparer.Ordinal); | |
} | |
catch (InvalidOperationException ex) | |
{ | |
throw new InvalidOperationException("Failed to combine ExpressionAttributeNames due to duplicate keys with different values.", ex); | |
} |
Copilot uses AI. Check for mistakes.
if (positions.Count == 0) | ||
{ | ||
// No recognized keywords, treat as a single section | ||
result[string.Empty] = expr.Trim(); |
Copilot
AI
Oct 15, 2025
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.
Using an empty string as a dictionary key when no keywords are found is unclear and could lead to unexpected behavior. Consider using a more explicit sentinel value or throwing an exception if no valid update expression keywords are found.
Copilot uses AI. Check for mistakes.
var returnValues = counterConditionExpression == null && !updateIfNotExistsAttributeName.Any() | ||
? ReturnValues.None | ||
: ReturnValues.AllNewAttributes; |
Copilot
AI
Oct 15, 2025
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 return value logic now includes updateIfNotExistsAttributeName.Any()
which forces enumeration. Since this list is used multiple times (lines 381, 406, and later in UpdateHelperAsync), consider caching the result of .Any()
in a boolean variable to avoid multiple enumerations.
Copilot uses AI. Check for mistakes.
|
||
/// <summary> | ||
/// 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. |
Copilot
AI
Oct 15, 2025
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 documentation is incomplete and incorrect. It states 'only updated if not null' but the actual behavior is 'only set if the attribute does not exist in DynamoDB'. Update to: 'This attribute can be used to control whether a property is always updated or only set when the item is created (if the attribute does not exist).'
/// This attribute can be used to control whether a property is always updated, only updated if not null. | |
/// This attribute can be used to control whether a property is always updated or only set when the item is created (if the attribute does not exist). |
Copilot uses AI. Check for mistakes.
} | ||
else | ||
{ | ||
var setStatement = updateExpression != null ? updateExpression.ExpressionStatement : ""; |
Copilot
AI
Oct 15, 2025
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 there are no sets or removes and only an existing updateExpression, this code appends the expression statement directly without properly handling the case where it might be empty. If updateExpression.ExpressionStatement
is empty or whitespace, this will create an invalid update expression with just whitespace. Add validation to only append when the statement is not null or whitespace.
var setStatement = updateExpression != null ? updateExpression.ExpressionStatement : ""; | |
var setStatement = (updateExpression != null && !string.IsNullOrWhiteSpace(updateExpression.ExpressionStatement)) | |
? updateExpression.ExpressionStatement | |
: ""; |
Copilot uses AI. Check for mistakes.
ill give this a review later this week. thanks for the pr! |
Add support for [DynamoDbUpdateBehavior] attribute to the .NET SDK for DynamoDB, enabling update behavior for a property when performing DynamoDB update operations. This feature aligns with the Java SDK's @DynamoDbUpdateBehavior.
Description
New Attribute: [DynamoDbUpdateBehavior]
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
Screenshots (if appropriate)
Types of changes
Checklist
License