Optimistic locking for delete scenario with TransactWriteItemsEnhancedRequest

[roamariei]

Description

Optimistic locking while using DynamoDbEnhancedClient - DeleteItem with TransactWriteItemsEnhancedRequest

Motivation and Context

#2358

Modifications

• Modified addDeleteItem method in TransactWriteItemsEnhancedRequest to invoke a new method, generateTransactDeleteItem, which includes an additional parameter: Map<String, AttributeValue> itemMap.
• Redirected delete operations to an internal method that constructs the version check condition.
• Added an internal BeforeDelete interface, similar to BeforeWrite, to handle version validation before execution. The implementation of this method was placed in VersionedRecordExtension and called from DeleteItemOperation.

Testing

Added unit tests for ChainExtension, VersionRecordTest and integration tests for delete scenario with matching version and with exception if the version mismatch.

Screenshots (if appropriate)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Checklist

• I have read the CONTRIBUTING document
• Local run of mvn install succeeds
• My code follows the code style of this project
• My change requires a change to the Javadoc documentation
• I have updated the Javadoc documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed
• I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
• My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

• I confirm that this pull request can be released under the Apache 2 license

[joviegas]

This is a breaking change right ?

What if currently user was using this API to delete any API m and such cases we will start getting exceptions ?

[anasatirbasa]

This is a breaking change right ?

What if currently user was using this API to delete any API m and such cases we will start getting exceptions ?

Hello, @joviegas

This is not a breaking change, as described in the Solution Proposal. A validation step was added to ensure that the version in the delete request matches the version stored in the database.

If the versions do not match, the operation now throws a TransactionCanceledException, which is the expected behavior for conditional operations in DefaultDynamoDbClient in case a condition (in one of the condition expressions) is not met. This scenario is covered by the integration test deleteItemWithTransactWrite_shouldFailIfVersionMismatch in CrudWithResponseIntegrationTest.java.

Before the changes from the current PR, the item would have been deleted even if the version did not match, and no exception was thrown. The new behavior enforces optimistic locking to improve data integrity.

[joviegas]

This is a breaking change right ?
What if currently user was using this API to delete any API m and such cases we will start getting exceptions ?

Hello, @joviegas

This is not a breaking change, as described in the Solution Proposal. A validation step was added to ensure that the version in the delete request matches the version stored in the database.

If the versions do not match, the operation now throws a TransactionCanceledException, which is the expected behavior for conditional operations in DefaultDynamoDbClient in case a condition (in one of the condition expressions) is not met. This scenario is covered by the integration test deleteItemWithTransactWrite_shouldFailIfVersionMismatch in CrudWithResponseIntegrationTest.java.

Before the changes from the current PR, the item would have been deleted even if the version did not match, and no exception was thrown. The new behavior enforces optimistic locking to improve data integrity.

Right now, customers can delete items without worrying about versions, and it just works. But after our update, they're going to start getting TransactionCanceledException errors out of nowhere.

Think about it this way: let's say someone has code that tries to delete item X with version 1. They don't really care if the actual version is 2 or 3 - they just want the item gone. Today, that works fine. Tomorrow, after they upgrade, boom - exception thrown and their code breaks.

That's a classic breaking change. Their apps that work perfectly today will suddenly fail. They'll have to go in and modify their code to handle these new exceptions. And there's no way for them to get the old "just delete it" behavior back without changing their code.

Don't get me wrong - I totally get that this change makes things better from a data integrity standpoint with the optimistic locking. Some customers probably built their apps specifically counting on that "delete regardless of version" behavior.
We need to think of approach which is not breaking the existing customers. What do you think?

[anasatirbasa]

Hello @joviegas,
 
From this standpoint indeed it would be a breaking change. To address the potential breaking change and ensure full backward compatibility while introducing optimistic locking support, we propose the addition of a new overload for the addDeleteItem API:

• Current method signature (no Optimistic Locking support):
  public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem)

• Proposed overload (with Optimistic Locking support):
  public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem, Boolean useOptimisticLocking)
   

The current method will internally delegate to the new method using useOptimisticLocking = false, thereby maintaining the existing "delete regardless of version" behavior without introducing any breaking change.
Developers who opt in to optimistic locking can explicitly call the new overload with useOptimisticLocking = true, which will enforce version checks during deletion and improve data integrity.

We will refactor the tests to add coverage on both current and new behavior. This approach allows us to introduce the feature in a backward-compatible manner, giving users the choice to adopt the new behavior at their discretion.

We look forward to your feedback on this approach.

[joviegas]

Hello @joviegas, From this standpoint indeed it would be a breaking change. To address the potential breaking change and ensure full backward compatibility while introducing optimistic locking support, we propose the addition of a new overload for the addDeleteItem API:

* Current method signature (no Optimistic Locking support):
  `public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem)`

* Proposed overload (with Optimistic Locking support):
  `public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem, Boolean useOptimisticLocking)`

The current method will internally delegate to the new method using useOptimisticLocking = false, thereby maintaining the existing "delete regardless of version" behavior without introducing any breaking change. Developers who opt in to optimistic locking can explicitly call the new overload with useOptimisticLocking = true, which will enforce version checks during deletion and improve data integrity.

We will refactor the tests to add coverage on both current and new behavior. This approach allows us to introduce the feature in a backward-compatible manner, giving users the choice to adopt the new behavior at their discretion.

We look forward to your feedback on this approach.

Thanks anasatirbasa@ for the new version , will take a look at it in between this week

[joviegas]

Hello @joviegas, From this standpoint indeed it would be a breaking change. To address the potential breaking change and ensure full backward compatibility while introducing optimistic locking support, we propose the addition of a new overload for the addDeleteItem API:

* Current method signature (no Optimistic Locking support):
  `public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem)`

* Proposed overload (with Optimistic Locking support):
  `public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem, Boolean useOptimisticLocking)`

The current method will internally delegate to the new method using useOptimisticLocking = false, thereby maintaining the existing "delete regardless of version" behavior without introducing any breaking change. Developers who opt in to optimistic locking can explicitly call the new overload with useOptimisticLocking = true, which will enforce version checks during deletion and improve data integrity.

We will refactor the tests to add coverage on both current and new behavior. This approach allows us to introduce the feature in a backward-compatible manner, giving users the choice to adopt the new behavior at their discretion.

We look forward to your feedback on this approach.

Thanks anasatirbasa@ for the new version , will take a look at it in between this week

[joviegas]

Thanks @anasatirbasa for the PR!

I've reviewed the changes and tested some scenarios. I have a concern about the current implementation scope: optimistic locking shouldn't be limited to just TransactWriteItemsEnhancedRequest. Since this feature is tied to the "VersionIdExtension," its behavior should remain consistent across all APIs, same as putItem, as demonstrated below:

@Test
public void deleteExistingRecordVersionMatches() {
    mappedTable.putItem(r -> r.item(new Record().setId("id").setAttribute("one")));

    mappedTable.putItem(new Record().setId("id").setAttribute("one").setVersion(1));
    mappedTable.putItem(new Record().setId("id").setAttribute("one").setVersion(2));

    // This fails as expected
    // mappedTable.putItem(new Record().setId("id").setAttribute("one").setVersion(1));
    
    // This should also fail for consistency when user is expecting Optimistic operation [In our case they will provide settings]
    mappedTable.deleteItem(new Record().setId("id").setAttribute("one").setVersion(1));
}

Alternative Approach Suggestion:

Instead of overriding specific APIs, I recommend adding a feature flag to the VersionIdExtension (similar to the approach in PR #6019). We could introduce an "optimisticDelete" flag that:

• Defaults to false to maintain backward compatibility
• Can be enabled by users who want optimistic locking for delete operations across all APIs
• Ensures consistent behavior throughout the extension

Next Steps:

Could you please conduct a surface API review considering:

1. The recommended feature flag approach
2. Alternative implementation strategies
3. Consistency requirements across all VersionIdExtension operations

This would help ensure we're taking the most comprehensive and user-friendly approach.

[anasatirbasa]

Hello, @joviegas

We understand your concern and we've analyzed your proposal (defining a feature flag for optimistic locking on delete operations at extension level - VersionRecordExtension.java)
 
We have identified the following SDK APIs used for delete operations:

Class	Method Signature
DefaultDynamoDbTable<T>	- public void deleteItem(DeleteItemEnhancedRequest request) - public T deleteItem(Consumer<DeleteItemEnhancedRequest.Builder> requestConsumer) - public T deleteItem(Key key) - public T deleteItem(T keyItem)
TransactWriteItemsEnhancedRequest.Builder	- public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, TransactDeleteItemEnhancedRequest request) - @Deprecated public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, DeleteItemEnhancedRequest request) - public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, Key key) - public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem)
DefaultDynamoDbAsyncTable<T>	- public CompletableFuture<T> deleteItem(DeleteItemEnhancedRequest request) - public CompletableFuture<T> deleteItem(Consumer<DeleteItemEnhancedRequest.Builder> requestConsumer) - public CompletableFuture<T> deleteItem(Key key) - public CompletableFuture<T> deleteItem(T keyItem)
WriteBatch.Builder<T>	- public Builder<T> addDeleteItem(DeleteItemEnhancedRequest request) - public Builder<T> addDeleteItem(Consumer<DeleteItemEnhancedRequest.Builder> requestConsumer) - public Builder<T> addDeleteItem(Key key) - public Builder<T> addDeleteItem(T keyItem)

Proposed Behavior with Optimistic Locking Flag

• Flag disabled
  – Delete operations proceed without version checks; the item is removed unconditionally.

• Flag enabled & version provided
  – The supplied version is compared to the stored version; deletion occurs only if they match.

• Flag enabled & no version provided
  – Version is not checked; the item is removed unconditionally.

---

Implementation Strategies

1. DynamoDbEnhancedClient level configuration

DynamoDbEnhancedClient client = DynamoDbEnhancedClient.builder()
    .dynamoDbClient(getDynamoDbClient())
    .extensions(VersionedRecordExtension.builder()
        .optimisticLocking(true)
        .build())
    .build();

We will have to pass the optimistic locking flag when we attach the custom VersionedRecordExtension when building DynamoDbEnhancedClient.

2. Annotation-based configuration

@DynamoDbVersionAttribute(optimisticLocking = true)
public Integer getVersion() {
    return version;
}

Allows per-entity control of optimistic locking via annotations.

---

The first approach offers a single, centralized configuration but lacks per-schema flexibility. The annotation-based approach provides control at the model level, allowing different entities to opt in or out independently.

Could you please advise which solution fits better from your perspective?
Thank you!

[joviegas]

Hello, @joviegas

We understand your concern and we've analyzed your proposal (defining a feature flag for optimistic locking on delete operations at extension level - VersionRecordExtension.java) We have identified the following SDK APIs used for delete operations:
Class Method Signature
DefaultDynamoDbTable<T> - public void deleteItem(DeleteItemEnhancedRequest request)

• public T deleteItem(Consumer<DeleteItemEnhancedRequest.Builder> requestConsumer)
• public T deleteItem(Key key)
• public T deleteItem(T keyItem)
TransactWriteItemsEnhancedRequest.Builder - public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, TransactDeleteItemEnhancedRequest request)
• @Deprecated public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, DeleteItemEnhancedRequest request)
• public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, Key key)
• public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, T keyItem)
DefaultDynamoDbAsyncTable<T> - public CompletableFuture<T> deleteItem(DeleteItemEnhancedRequest request)
• public CompletableFuture<T> deleteItem(Consumer<DeleteItemEnhancedRequest.Builder> requestConsumer)
• public CompletableFuture<T> deleteItem(Key key)
• public CompletableFuture<T> deleteItem(T keyItem)
WriteBatch.Builder<T> - public Builder<T> addDeleteItem(DeleteItemEnhancedRequest request)
• public Builder<T> addDeleteItem(Consumer<DeleteItemEnhancedRequest.Builder> requestConsumer)
• public Builder<T> addDeleteItem(Key key)
• public Builder<T> addDeleteItem(T keyItem)

Proposed Behavior with Optimistic Locking Flag

* **Flag disabled**
  – Delete operations proceed without version checks; the item is removed unconditionally.

* **Flag enabled & version provided**
  – The supplied version is compared to the stored version; deletion occurs only if they match.

* **Flag enabled & no version provided**
  – Version is not checked; the item is removed unconditionally.

Implementation Strategies

1. DynamoDbEnhancedClient level configuration

DynamoDbEnhancedClient client = DynamoDbEnhancedClient.builder()
    .dynamoDbClient(getDynamoDbClient())
    .extensions(VersionedRecordExtension.builder()
        .optimisticLocking(true)
        .build())
    .build();

We will have to pass the optimistic locking flag when we attach the custom VersionedRecordExtension when building DynamoDbEnhancedClient.

2. Annotation-based configuration

@DynamoDbVersionAttribute(optimisticLocking = true)
public Integer getVersion() {
    return version;
}

Allows per-entity control of optimistic locking via annotations.

The first approach offers a single, centralized configuration but lacks per-schema flexibility. The annotation-based approach provides control at the model level, allowing different entities to opt in or out independently.

Could you please advise which solution fits better from your perspective? Thank you!

Thank you so much for the detailed documentation and thorough analysis of the APIs. I really appreciate the comprehensive breakdown of the implementation approach.

I apologize for the delay - I had posted in our internal Slack asking for a quick API design review and was waiting for feedback. However, I see you've already provided excellent documentation directly in this PR, which is very helpful.

Important clarification needed: Since we're using the generic name optimisticLocking, could you please confirm that when this flag is enabled, it will apply to all write operations (create, update, delete) consistently? Per the DynamoDB documentation:

"Optimistic locking is a strategy to ensure that the client-side item that you are updating (or deleting) is the same as the item in Amazon DynamoDB."

This would mean (please help me confirm):

Create operations: Would fail if an item with the same key already exists
Update operations: Would require version matching (existing behavior)
Delete operations: Would require version matching (your proposed new behavior)

Regarding your implementation approach:

I see you're proposing both client-level configuration via the builder AND annotation support. A few questions that would help me better understand the design:

Precedence: When both are specified, which takes precedence - the client-level setting or the annotation?
Batch operations: How would batch/transactional operations handle mixed scenarios where some items are not in sync with version number? Is it possible to have a batch with some items having optmisticlocking enabled and some disabled

• Why are we deprecating addDeleteItem with this approach ?

- @Deprecated public <T> Builder addDeleteItem(MappedTableResource<T> mappedTableResource, 

If you could help clarify these points, I'll complete a quick internal review with the team and provide feedback.

[anasatirbasa]

Hello @joviegas,

Thanks for your feedback and the request for consistency across all operations.

Just to clarify, we have provided two alternatives for how the optimistic locking flag could be introduced, either at Version extension level or at annotation level, but not both together. For the moment let’s consider only the flag at extension level option.

Regarding the consistency across all operations, below is a clarification of our design options and the trade‑offs we see:

1. Option A: Flag at Version Extension level across all operations

• Introduce a single global flag (e.g., optimisticLocking) on the Version extension.
• All operations (create, update, delete) will behave based on this flag consistently.
• Major drawback: this changes the default behavior. Today, create/update implicitly enforce optimistic version checks, while delete does not. Under this model, users must explicitly enable the Version extension with optimisticLocking = true when building their enhanced client to retain existing create/update behavior. If they omit it, those checks go away.
• This breaks backward compatibility unless we make the flag default to true, but in that case it will break the compatibility for delete operations, as currently the delete doesn’t consider optimisticLocking checks.

2. Option B: Flag at Version Extension level only for Delete Operations

• Introduce a flag (e.g. optimisticDelete) scoped only to delete behavior.
• Create/update continue working exactly as today (optimistic by default), so existing clients remain totally unaffected. Delete becomes version‑checked only if extension is explicitly defined with optimisticDelete = true.
• Drawback: create/update/delete won't be symmetric, but that's already the case today.

Also: the @Deprecated addDeleteItem(...) method was deprecated previously; we are not deprecating anything new as part of this change.

Question for you
Given these constraints, which path would you prefer?

• Option A: Introduce a globally scoped optimistic‑locking flag to enforce consistent versioning behavior across create, update, and delete, but requiring explicit opt‑in to maintain existing behavior.
• Option B: Introduce a delete‑specific flag (e.g. optimisticDelete)—preserving all current behavior unchanged for create/update, at the cost of not having full consistency.

Happy to refine either path further given your input.

Thanks again and looking forward to your feedback!

[joviegas]

Introduce a single global flag (e.g., optimisticLocking) on the Version extension.

@anasatirbasa
Thanks for the response
Option B: Flag at Version Extension level only for Delete Operations makes more sense.
Something like #6327 , I just did some quick Proof of concept PR , here we can even avoid the new api for beforeWrite, what do you think of this approach.