-
Notifications
You must be signed in to change notification settings - Fork 870
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
|
||||||||||||||||||||||||||
| 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; | |
| } |
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
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.ShouldFlattenChildPropertiesparameter but there's no clear indication of how this affects the flattening behavior. This could be a breaking change that needs verification.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)