Skip to content

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

RanVaknin
Copy link
Contributor

@RanVaknin RanVaknin commented Apr 9, 2025

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:

  • Starting versions at 0 (or any non-negative long)
  • Configuring custom increment values (only positive numbers)
  • Supporting these configs both at the extension level and via annotations

Modifications

  • Refactored the VersionedRecordExtension to support explicit startAt and incrementBy values via builder methods (making it opt-in).
  • Expanded the DynamoDbVersionAttribute annotation to support startAt and incrementBy parameters
  • Added validation to prevent negative startAt values and non-positive incrementBy values

Testing

  • Custom startAt and incrementBy values through both builder and annotations
  • Validation of illegal values (negative startAt, zero/negative incrementBy)
  • Precedence rules between annotation and builder configurations
  • Version incrementation for both initial and existing records
  • Edge cases with different configuration combinations

@RanVaknin RanVaknin requested a review from a team as a code owner April 9, 2025 22:19
@RanVaknin RanVaknin force-pushed the rvaknin/ddb-enhanced-client-versioned-record-custom-startAt branch from abcd87a to 4e82d7e Compare April 10, 2025 22:54
@akiesler
Copy link
Contributor

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.

@RanVaknin RanVaknin force-pushed the rvaknin/ddb-enhanced-client-versioned-record-custom-startAt branch from 9e39a83 to 3d505a6 Compare April 21, 2025 23:42
Comment on lines 160 to 164
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))) {
Copy link
Contributor

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

Copy link
Contributor

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 {
  ....
}

Copy link
Contributor Author

@RanVaknin RanVaknin May 5, 2025

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":

  1. The version doesn't exist in DynamoDB yet (existing behavior)
  2. The version is explicitly set to null (existing behavior)
  3. 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();
Copy link
Contributor

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 160 to 164
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))) {
Copy link
Contributor

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 {
  ....
}

@RanVaknin RanVaknin force-pushed the rvaknin/ddb-enhanced-client-versioned-record-custom-startAt branch from 3d505a6 to b7b79c2 Compare May 6, 2025 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants