-
Notifications
You must be signed in to change notification settings - Fork 906
DDB Enhanced: Allow custom versioning #6019
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: master
Are you sure you want to change the base?
DDB Enhanced: Allow custom versioning #6019
Conversation
abcd87a
to
4e82d7e
Compare
...tom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/Expression.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
Thank you for picking up this change! It will greatly simplify our version handling. Please let me know if I can do anything to help. |
9e39a83
to
3d505a6
Compare
.changes/next-release/feature-DynamoDBEnhancedClient-2a501d8.json
Outdated
Show resolved
Hide resolved
...tom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/Expression.java
Outdated
Show resolved
Hide resolved
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
if (!existingVersionValue.isPresent() || isNullAttributeValue(existingVersionValue.get()) || | ||
(existingVersionValue.get().n() != null && | ||
((versionStartAtFromAnnotation.isPresent() && | ||
Long.parseLong(existingVersionValue.get().n()) == versionStartAtFromAnnotation.get()) || | ||
Long.parseLong(existingVersionValue.get().n()) == this.startAt))) { |
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.
- This is a very complex expression. Let's move this to a separate function so it's easier to reason about and we can document it
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.
+1. Why do we need to check existingVersionValue.get().n() != null && ((versionStartAtFromAnnotation.isPresent() && Long.parseLong(existingVersionValue.get().n()) == versionStartAtFromAnnotation.get()) || Long.parseLong(existingVersionValue.get().n()) == this.startAt)))
?
Suggesting the following
if (existingValueNotAvailable(existingAttributeValue)) {
long startVersion = versionStartAtFromAnnotation.orElse(this.startAt);
long incrementVersion = versionIncrementByFromAnnotation.orElse(this.incrementBy);
...
} else {
....
}
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.
we need the expanded evaluation to see what constitutes an "initial version".
There are three ways a version can be considered "initial":
- The version doesn't exist in DynamoDB yet (existing behavior)
- The version is explicitly set to null (existing behavior)
- The version equals a configured start value (new behavior)
For case 3, the extension must check if the version matches either:
- The start value from the annotation (
@DynamoDbVersionAttribute(startAt = 3)
) - The start value from the builder (
builder().startAt(3).build()
)
newVersionValue = AttributeValue.builder().n(Integer.toString(existingVersion + 1)).build(); | ||
|
||
long increment = versionIncrementByFromAnnotation.orElse(this.incrementBy); | ||
newVersionValue = AttributeValue.builder().n(Long.toString(existingVersion + increment)).build(); |
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.
It doesn't look like the original version handled this, but how do we deal with integer overflow? Originally incrementBy
was always 1
so it's unlikely to happen, but with incrementBy
being configurable now, it's technically more likely now if the user sets a relatively large value.
Can we check how v1's mapper handles this (if at all)?
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.
Doesn't seem like v1 had any sort of integer overflow protection?
if (!existingVersionValue.isPresent() || isNullAttributeValue(existingVersionValue.get()) || | ||
(existingVersionValue.get().n() != null && | ||
((versionStartAtFromAnnotation.isPresent() && | ||
Long.parseLong(existingVersionValue.get().n()) == versionStartAtFromAnnotation.get()) || | ||
Long.parseLong(existingVersionValue.get().n()) == this.startAt))) { |
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.
+1. Why do we need to check existingVersionValue.get().n() != null && ((versionStartAtFromAnnotation.isPresent() && Long.parseLong(existingVersionValue.get().n()) == versionStartAtFromAnnotation.get()) || Long.parseLong(existingVersionValue.get().n()) == this.startAt)))
?
Suggesting the following
if (existingValueNotAvailable(existingAttributeValue)) {
long startVersion = versionStartAtFromAnnotation.orElse(this.startAt);
long incrementVersion = versionIncrementByFromAnnotation.orElse(this.incrementBy);
...
} else {
....
}
.../main/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtension.java
Outdated
Show resolved
Hide resolved
...t/java/software/amazon/awssdk/enhanced/dynamodb/extensions/VersionedRecordExtensionTest.java
Show resolved
Hide resolved
3d505a6
to
b7b79c2
Compare
|
Motivation and Context
This PR builds on the work started in issue #3894 where the
VersionedRecordExtension
didn't support starting version numbers at 0, requiring clients to use Integer objects (with null initial values) rather than long primitives. As mentioned by @akiesler in the original issue, developers might expect versions to start at 0 and increment from there, rather than having a special case where the value must be initialized to null.This implementation allows the extension to be more flexible by allowing:
Modifications
VersionedRecordExtension
to support explicitstartAt
andincrementBy
values via builder methods (making it opt-in).DynamoDbVersionAttribute
annotation to supportstartAt
andincrementBy
parametersTesting
startAt
andincrementBy
values through both builder and annotationsstartAt
, zero/negativeincrementBy
)