-
Notifications
You must be signed in to change notification settings - Fork 868
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"services": [ | ||
{ | ||
"serviceName": "DynamoDBv2", | ||
"type": "minor", | ||
"changeLogMessages": [ | ||
"Add support for DynamoDBAutoGeneratedTimestampAttribute that sets current timestamp during persistence operations." | ||
] | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -158,6 +158,21 @@ internal static void ValidateNumericType(Type memberType) | |||||||||||||||||||||||||
throw new InvalidOperationException("Version property must be of primitive, numeric, integer, nullable type (e.g. int?, long?, byte?)"); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
internal static void ValidateTimestampType(Type memberType) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
if (memberType.IsGenericType && memberType.GetGenericTypeDefinition() == typeof(Nullable<>) && | ||||||||||||||||||||||||||
(memberType.IsAssignableFrom(typeof(DateTime)) || | ||||||||||||||||||||||||||
memberType.IsAssignableFrom(typeof(DateTimeOffset)))) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
return; | ||||||||||||||||||||||||||
Comment on lines
+163
to
+167
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
throw new InvalidOperationException( | ||||||||||||||||||||||||||
$"Timestamp properties must be of type Nullable<DateTime> (DateTime?) or Nullable<DateTimeOffset> (DateTimeOffset?). " + | ||||||||||||||||||||||||||
$"Invalid type: {memberType.FullName}. " + | ||||||||||||||||||||||||||
"Please ensure your property is declared as 'DateTime?' or 'DateTimeOffset?'." | ||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicConstructors)] | ||||||||||||||||||||||||||
internal static Type GetPrimitiveElementType(Type collectionType) | ||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||
|
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)