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

Merged

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed edge case in the isInitialVersion function. Previously it was not evaluating the precedence that annotation values should take over builder values.

This caused inconsistent behavior when both annotation and builder values were both provided with different values, potentially treating records as initial versions incorrectly when their version matched the builder's startAt value but not the annotation's value.

This is now fixed. Added a test case that combines input from both the builder and the annotation.

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
Copy link

sonarqubecloud bot commented Jun 3, 2025

@RanVaknin RanVaknin force-pushed the rvaknin/ddb-enhanced-client-versioned-record-custom-startAt branch from 38e83d3 to 2ffe47c Compare July 8, 2025 20:46
@RanVaknin RanVaknin force-pushed the rvaknin/ddb-enhanced-client-versioned-record-custom-startAt branch from 2ffe47c to 18bb268 Compare July 17, 2025 22:22
@RanVaknin
Copy link
Contributor Author

Fixed edge case in the isInitialVersion function. Previously it was not evaluating the precedence that annotation values should take over builder values.

This caused inconsistent behavior when both annotation and builder values were both provided with different values, potentially treating records as initial versions incorrectly when their version matched the builder's startAt value but not the annotation's value.

This is now fixed. Added a test case that combines input from both the builder and the annotation.

Copy link

@zoewangg zoewangg requested a review from Copilot July 18, 2025 16:45
Copy link

@Copilot Copilot AI left a 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 enhances the DynamoDB Enhanced Client's VersionedRecordExtension to support custom versioning strategies. Instead of requiring versions to start at 1 and increment by 1, developers can now configure starting values (including 0) and custom increment amounts through both builder patterns and annotations.

Key Changes:

  • Added configurable startAt and incrementBy parameters to VersionedRecordExtension.Builder
  • Enhanced @DynamoDbVersionAttribute annotation with startAt and incrementBy parameters
  • Implemented validation to prevent negative start values and non-positive increment values
  • Added support for annotation-based configuration with precedence over builder configuration

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
VersionedRecordExtension.java Core implementation with builder methods, validation, and version calculation logic
DynamoDbVersionAttribute.java Added annotation parameters for startAt and incrementBy
VersionRecordAttributeTags.java Updated to pass annotation parameters to version attribute tags
VersionedRecordExtensionTest.java Comprehensive test coverage for new functionality
VersionedRecordTest.java Integration tests with various configuration scenarios
FakeVersionedImmutableItem.java Test model with annotation-based versioning
FakeVersionedStaticImmutableItem.java Test model for static schema versioning
Expression.java Added toString method for debugging
pom.xml Temporary exclusion of new annotation methods from API checks
.changes/next-release/feature-DynamoDBEnhancedClient-2a501d8.json Release notes entry
Comments suppressed due to low confidence (2)


long increment = versionIncrementByFromAnnotation;

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the convention for inline comment is // rather than /*

@RanVaknin RanVaknin added this pull request to the merge queue Jul 22, 2025
Merged via the queue into master with commit e648519 Jul 22, 2025
35 of 36 checks passed
Copy link

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants