Optimistic Locking for Delete Operations#6747
Optimistic Locking for Delete Operations#6747anasatirbasa wants to merge 14 commits intoaws:masterfrom
Conversation
| */ | ||
| public static Expression createVersionCondition(AttributeValue versionValue, String versionAttributeName) { | ||
| return Expression.builder() | ||
| .expression(versionAttributeName + " = :version_value") |
There was a problem hiding this comment.
This uses the raw attribute name directly in the expression string. The existing VersionedRecordExtension uses keyRef() and expressionNames maps to safely handle DynamoDB reserved words. Should we do the same thing here ?
There was a problem hiding this comment.
I have updated the code in order to use keyRef(), in the same way it is used in VersionedRecordExtension.
| * @throws IllegalArgumentException if any parameter is null | ||
| */ | ||
| public Builder withOptimisticLocking(AttributeValue versionValue, String versionAttributeName) { | ||
| Expression optimisticLockingCondition = createVersionCondition(versionValue, versionAttributeName); |
There was a problem hiding this comment.
is it possible that withOptimisticLocking() silently overwrites any existing conditionExpression ?
should this use Use Expression.join() to merge with any existing condition instead ?
There was a problem hiding this comment.
I have updated the code in order to merge the optimistic locking condition with the other existing conditions, if available:
/**
* Adds optimistic locking to this delete request.
* <p>
* If a {@link #conditionExpression(Expression)} was already set, this will combine it with the optimistic locking
* condition using {@code AND}. If either expression has conflicting name/value tokens, {@link Expression#join} will throw
* {@link IllegalArgumentException}.
*
* @param versionValue the expected version value that must match for the deletion to succeed
* @param versionAttributeName the name of the version attribute in the DynamoDB table
* @return a builder of this type with optimistic locking condition applied (and merged if needed)
*/
public Builder withOptimisticLocking(AttributeValue versionValue, String versionAttributeName) {
Expression optimisticLockingCondition = createVersionCondition(versionValue, versionAttributeName);
this.conditionExpression = Expression.join(this.conditionExpression, optimisticLockingCondition, " AND ");
return this;
}| * @throws IllegalArgumentException if any parameter is null | ||
| */ | ||
| public Builder withOptimisticLocking(AttributeValue versionValue, String versionAttributeName) { | ||
| Expression optimisticLockingCondition = createVersionCondition(versionValue, versionAttributeName); |
There was a problem hiding this comment.
is it possible that withOptimisticLocking() silently overwrites any existing conditionExpression ?
should this use Use Expression.join() to merge with any existing condition instead ?
There was a problem hiding this comment.
I have updated the code in order to merge the optimistic locking condition with the other existing conditions, if available:
/**
* Adds optimistic locking to this delete request.
* <p>
* If a {@link #conditionExpression(Expression)} was already set, this will combine it with the optimistic locking
* condition using {@code AND}. If either expression has conflicting name/value tokens, {@link Expression#join} will throw
* {@link IllegalArgumentException}.
*
* @param versionValue the expected version value that must match for the deletion to succeed
* @param versionAttributeName the name of the version attribute in the DynamoDB table
* @return a builder of this type with optimistic locking condition applied (and merged if needed)
*/
public Builder withOptimisticLocking(AttributeValue versionValue, String versionAttributeName) {
Expression optimisticLockingCondition = createVersionCondition(versionValue, versionAttributeName);
this.conditionExpression = Expression.join(this.conditionExpression, optimisticLockingCondition, " AND ");
return this;
}| @Deprecated | ||
| public T deleteItem(T keyItem) { | ||
| return deleteItem(keyFrom(keyItem)); | ||
| return deleteItem(keyItem, false); |
There was a problem hiding this comment.
This is more of a question, the old deleteItem(T keyItem) used to call deleteItem(keyFrom(keyItem)) which goes through deleteItem(Key) → deleteItem(Consumer) → deleteItem(DeleteItemEnhancedRequest). The new code delegates to deleteItem(keyItem, false) which builds a DeleteItemEnhancedRequest directly and calls deleteItem(DeleteItemEnhancedRequest). It is called with the false flag means no optimistic locking is applied, the code path is different. The old path went through the Consumer overload, the new path constructs the request directly. If any subclass overrides the Consumer-based overload (which is a default method on the interface, so unlikely but possible), this changes behavior.
Is this a concern ? how do we think about backward compatibility ? (same applies to async table)
There was a problem hiding this comment.
Yes, you are right — this could introduce a potential backward compatibility issue.
Previously, deleteItem(T keyItem) delegated through the Consumer-based overload.
The new implementation calls deleteItem(keyItem, false) directly, which bypasses that path.
Although it is unlikely that users override the Consumer-based method (since it is a default interface method), it is technically possible. In such a case, behavior could change.
To avoid breaking existing behavior, I kept the old method unchanged and marked it as deprecated:
/**
* @deprecated Use {@link #deleteItem(Object, boolean)} instead to explicitly
* control optimistic locking behavior.
*/This preserves backward compatibility while guiding users toward the new API.
The same approach was applied to the async table implementation.
| // 2. deleteItem(T item) on Versioned record and versions match | ||
| // -> Optimistic Locking is applied -> deletes the record | ||
| @Test | ||
| public void deleteItem_onVersionedRecord_whenVersionsMatch_appliesOptimisticLockingAndDeletesTheRecord() { |
There was a problem hiding this comment.
The test suggests optimistic locking is applied. But deleteItem(savedItem) is the deprecated overload, which calls deleteItem(keyItem, false) optimistic locking is off. The delete goes to DynamoDB as an unconditional DeleteItem with no condition expression. No version check happens I guess ? (same applies to async version test)
@Test
public void deleteItem_onVersionedRecord_whenVersionsMatch_appliesOptimisticLockingAndDeletesTheRecord() {
VersionedRecord item = new VersionedRecord().setId("123").setSort(10).setStringAttribute("Test Item");
versionedRecordTable.putItem(item); // version becomes 1
VersionedRecord savedItem = versionedRecordTable.getItem(r -> r.key(recordKey)); // gets item with version=1
versionedRecordTable.deleteItem(savedItem); // calls deprecated overload
// asserts item is deleted
}There was a problem hiding this comment.
I have modified the tests and added 3 new scenarios for the deprecated method:
1. Non-versioned record:
When deleting a non-versioned record using the deprecated method, optimistic locking is not applied
and the record is deleted unconditionally.
@Test
public void deprecatedDeleteItem_onNonVersionedRecord_skipsOptimisticLockingAndUnconditionallyDeletes() {
Record item = new Record().setId("123").setSort(10).setStringAttribute("test");
Key recordKey = Key.builder().partitionValue(item.getId()).sortValue(item.getSort()).build();
mappedTable.putItem(item);
Record savedItem = mappedTable.getItem(r -> r.key(recordKey));
mappedTable.deleteItem(savedItem);
Record deletedItem = mappedTable.getItem(r -> r.key(recordKey));
assertThat(deletedItem).isNull();
}2. Versioned record with matching version:
When deleting a versioned record using the deprecated method, optimistic locking is not enforced,
even when the version matches. The record is deleted unconditionally.
@Test
public void deprecatedDeleteItem_onVersionedRecordAndMatchingVersions_skipsOptimisticLockingAndUnconditionallyDeletes() {
VersionedRecord item = new VersionedRecord().setId("123").setSort(10).setStringAttribute("test");
Key recordKey = Key.builder().partitionValue(item.getId()).sortValue(item.getSort()).build();
versionedRecordTable.putItem(item);
VersionedRecord savedItem = versionedRecordTable.getItem(r -> r.key(recordKey));
versionedRecordTable.deleteItem(savedItem);
VersionedRecord deletedItem = versionedRecordTable.getItem(r -> r.key(recordKey));
assertThat(deletedItem).isNull();
}3. Versioned record with stale version:
When deleting a versioned record with a stale version using the deprecated method, optimistic locking is still not applied. The record is deleted even though the version does not match.
@Test
public void deprecatedDeleteItem_onVersionedRecordAndStaleVersion_skipsOptimisticLockingAndUnconditionallyDeletes() {
VersionedRecord item = new VersionedRecord().setId("123").setSort(10).setStringAttribute("test");
Key recordKey = Key.builder().partitionValue(item.getId()).sortValue(item.getSort()).build();
versionedRecordTable.putItem(item);
VersionedRecord savedItem = versionedRecordTable.getItem(r -> r.key(recordKey));
// Simulate a stale version by changing the version number
savedItem.setVersion(2);
versionedRecordTable.deleteItem(savedItem);
VersionedRecord deletedItem = versionedRecordTable.getItem(r -> r.key(recordKey));
// The item is deleted even though the version was stale because
// the deprecated method does not apply optimistic locking.
assertThat(deletedItem).isNull();
}| } | ||
|
|
||
| @Test | ||
| public void buildDeleteItemEnhancedRequest_preservesExistingRequestProperties() { |
There was a problem hiding this comment.
you may want to also create another test around here, like builder_withOptimisticLocking_afterConditionExpression_shouldMergeBothConditions to verify the behavior
There was a problem hiding this comment.
I have added a test in that checks this:
@Test public void buildDeleteItemEnhancedRequest_withOptimisticLockingAndCustomCondition_mergesConditions() {
Key key = Key.builder().partitionValue("id").build();
AttributeValue versionValue = AttributeValue.builder().n("1").build();
String versionAttributeName = "version";
Map<String, String> expressionNames = new HashMap<>();
expressionNames.put("#key1", "key1");
expressionNames.put("#key2", "key2");
Map<String, AttributeValue> expressionValues = new HashMap<>();
expressionValues.put(":value1", numberValue(10));
expressionValues.put(":value2", numberValue(20));
Expression conditionExpression =
Expression.builder()
.expression("#key1 = :value1 OR #key2 = :value2")
.expressionNames(Collections.unmodifiableMap(expressionNames))
.expressionValues(Collections.unmodifiableMap(expressionValues))
.build();
DeleteItemEnhancedRequest result =
DeleteItemEnhancedRequest.builder()
.key(key)
.conditionExpression(conditionExpression)
.optimisticLocking(versionValue, versionAttributeName)
.build();
assertThat(result.key()).isEqualTo(key);
assertThat(result.conditionExpression()).isNotNull();
assertThat(result.conditionExpression().expression()).isEqualTo(
"(#key1 = :value1 OR #key2 = :value2) AND (#AMZN_MAPPED_version = :AMZN_MAPPED_version)");
Map<String, String> expectedExpressionNames = new HashMap<>();
expectedExpressionNames.put("#AMZN_MAPPED_version", "version");
expectedExpressionNames.put("#key1", "key1");
expectedExpressionNames.put("#key2", "key2");
assertThat(result.conditionExpression().expressionNames()).containsExactlyInAnyOrderEntriesOf(expectedExpressionNames);
Map<String, AttributeValue> expectedExpressionValues = new HashMap<>();
expectedExpressionValues.put(":AMZN_MAPPED_version", AttributeValue.builder().n("1").build());
expectedExpressionValues.put(":value1", AttributeValue.builder().n("10").build());
expectedExpressionValues.put(":value2", AttributeValue.builder().n("20").build());
assertThat(result.conditionExpression().expressionValues()).containsExactlyInAnyOrderEntriesOf(expectedExpressionValues);
}| import software.amazon.awssdk.services.dynamodb.model.DeleteTableRequest; | ||
| import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; | ||
|
|
||
| public class OptimisticLockingCrudTest extends LocalDynamoDbSyncTestBase { |
There was a problem hiding this comment.
Do we want to have a test that calls the deprecated deleteItem(savedItem) with a stale version and verify it still deletes (because no condition is applied (probably also in OptimisticLockingCrudTest)
There was a problem hiding this comment.
I have added a test for this scenario in both OptimisticLockingCrudTest / OptimisticLockingAsyncCrudTest:
// 3. deleteItem(T item) - deprecated - on Versioned record, with stale version
// -> Optimistic Locking is not applied -> unconditionally deletes the record
@Test public void deprecatedDeleteItem_onVersionedRecordAndStaleVersion_skipsOptimisticLockingAndUnconditionallyDeletes() {
VersionedRecord item = new VersionedRecord().setId("123").setSort(10).setStringAttribute("test");
Key recordKey = Key.builder().partitionValue(item.getId()).sortValue(item.getSort()).build();
versionedRecordTable.putItem(item);
VersionedRecord savedItem = versionedRecordTable.getItem(r -> r.key(recordKey));
// Simulate a stale version by changing the version number
savedItem.setVersion(2);
versionedRecordTable.deleteItem(savedItem);
VersionedRecord deletedItem = versionedRecordTable.getItem(r -> r.key(recordKey));
// the item is deleted even though the version was stale because the old method does not apply optimistic locking
assertThat(deletedItem).isNull();
}| import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; | ||
|
|
||
| public class OptimisticLockingCrudTest extends LocalDynamoDbSyncTestBase { | ||
|
|
There was a problem hiding this comment.
deleteItem(Consumer) path with optimistic locking via builder -> The deleteItem(Consumer) overload is never tested with optimistic locking. (also in OptimisticLockingCrudTest)
There was a problem hiding this comment.
I have added tests for The deleteItem(Consumer) overload for both sync / async implementations.
For each of them, 6 scenarios were defined:
-
deleteItemWithConsumer_onVersionedRecord_whenVersionsMatch_deletesTheRecord
Versioned record with optimistic locking enabled and matching version. The operation fails and the record is not deleted. -
deleteItemWithConsumer_onVersionedRecord_whenVersionsMismatch_doesNotDeleteTheRecord
Versioned record with optimistic locking enabled but mismatching version. The operation fails and the record is not deleted. -
deleteItemWithConsumer_whenOptimisticLockingAndCustomConditionAreRespected_deletesTheRecord
Version matches and the custom condition expression evaluates successfully. The record is deleted. -
deleteItemWithConsumer_whenOptimisticLockingConditionRespected_butCustomConditionFails_doesNotDeleteTheRecord
Version matches but the custom condition expression fails. The operation fails and the record is not deleted. -
deleteItemWithConsumer_whenCustomConditionRespected_butOptimisticConditionFails_doesNotDeleteTheRecord
Custom condition passes but the optimistic locking version check fails. The operation fails and the record is not deleted. -
deleteItemWithConsumer_whenOptimisticLockingAndCustomConditionNotRespected_doesNotDeleteTheRecord
Both the optimistic locking check and the custom conditions expression fail. The operation fails and the record is not deleted.
| import software.amazon.awssdk.services.dynamodb.model.ConditionalCheckFailedException; | ||
| import software.amazon.awssdk.services.dynamodb.model.DeleteTableRequest; | ||
| import software.amazon.awssdk.services.dynamodb.model.TransactionCanceledException; | ||
|
|
There was a problem hiding this comment.
can test deleteItem on an item that doesn't exist in the table, no test covers calling deleteItem(item, true) when the item doesn't exist in DynamoDB. With optimistic locking enabled and a non-null version on the item, the condition version = :version_value will fail because the item doesn't exist.
| } | ||
|
|
||
| @Test | ||
| public void conditionallyApplyOptimistic_onTransactDelete_whenFlagTrueAndVersionedRecord_addsConditionExpression() { |
There was a problem hiding this comment.
the test name says "adds condition expression" but the assertion proves the opposite
- optimisticLockingEnabled = true
- Uses RecordWithUpdateBehaviors — this IS a versioned record (has @DynamoDbVersionAttribute)
- But keyItem.setVersion(...) is never called — the version field is null
- conditionallyApplyOptimisticLocking finds the version attribute name in the schema, calls tableSchema.attributeValue(keyItem, versionAttributeName), gets null, and hits the version != null ? ... : request branch — returning the original request unchanged
5.The assertion is assertEquals(originalRequest, result) — confirming no condition was added
if this is the purpose, should the test name be something like conditionallyApplyOptimistic_onTransactDelete_whenFlagTrueAndVersionedRecordWithNullVersion_returnsOriginalRequest ?
There was a problem hiding this comment.
Thank you for the observation!
I have updated the test name in order to reflect the correct behavior: conditionallyApplyOptimistic_onTransactDelete_whenFlagTrueAndVersionedRecordWithNullVersion_returnsOriginalRequest.
| DeleteItemEnhancedRequest request = | ||
| DeleteItemEnhancedRequest.builder() | ||
| .key(Key.builder().partitionValue("id").build()) | ||
| .withOptimisticLocking(versionValue, "version") |
There was a problem hiding this comment.
.with is more of an SDKv1 approach, AFAIK, SDKv2 does not use this naming convention?
There was a problem hiding this comment.
Updated the builder method name to "optimisticLocking".
| * @return the deleted item, or null if the item was not found | ||
| */ | ||
| @Override | ||
| public T deleteItem(T keyItem, boolean useOptimisticLocking) { |
There was a problem hiding this comment.
Should optimistic locking not follow the same experience as UpdateItem/PutItem, where the user passes the entire item? I believe its useful to keep this version, but also align the dev experience with existing API's
There was a problem hiding this comment.
Since this is for backwards compatability, perhaps its cleaner to do this at the annotation level, rather than changing the API signature:
@DynamoDbVersionAttribute(useVersionOnDelete = true)
There was a problem hiding this comment.
It would also prevent the "boolean trap".
| @Override | ||
| public CompletableFuture<T> deleteItem(T keyItem, boolean useOptimisticLocking) { | ||
| DeleteItemEnhancedRequest request = DeleteItemEnhancedRequest.builder().key(keyFrom(keyItem)).build(); | ||
| request = conditionallyApplyOptimisticLocking(request, keyItem, tableSchema, useOptimisticLocking); |
There was a problem hiding this comment.
I think we should pass the builder here as the conditionallyApplyOptimisticLocking may need to add (overwrite actually) the condition expression with the OptLocking condition. Builder objects work well in these cases where the building logic is complex and scattered.
Here the withOptimisticLocking methods in OptimisticLockingHelper won't need to rebuild the request.
There was a problem hiding this comment.
I have updated the method in order to pass the builder instead of the actual request.
| .expression(String.format("%s = :version_value", attributeKeyRef)) | ||
| .putExpressionValue(":version_value", versionValue) |
There was a problem hiding this comment.
I think we should use keyRef and valueRef to prevent collisions.
There was a problem hiding this comment.
I have done the changes in order to use "keyRef" and "valueRef":
String attributeKeyRef = keyRef(versionAttributeName);
String attributeValueRef = valueRef(versionAttributeName);
return Expression.builder()
.expression(String.format("%s = %s", attributeKeyRef, attributeValueRef))
.expressionNames(Collections.singletonMap(attributeKeyRef, versionAttributeName))
.expressionValues(Collections.singletonMap(attributeValueRef, versionValue))
.build();| * @throws IllegalArgumentException if {@code versionAttributeName} is null or empty | ||
| */ | ||
| public static Expression createVersionCondition(AttributeValue versionValue, String versionAttributeName) { | ||
| if (versionAttributeName == null || versionAttributeName.trim().isEmpty()) { |
There was a problem hiding this comment.
Why are we checking the versionAttributeName but not versionValue?
There was a problem hiding this comment.
Added a check for "versionValue" also:
if (versionAttributeName == null || versionAttributeName.trim().isEmpty()) {
throw new IllegalArgumentException("Version attribute name must not be null or empty.");
}
if (versionValue == null || versionValue.n() == null || versionValue.n().trim().isEmpty()) {
throw new IllegalArgumentException("Version value must not be null or empty.");
}4 participants
Description
Adds explicit optimistic locking support for delete operations in the DynamoDB Enhanced Client, while preserving full backward compatibility.
Until now,
deleteItemoperations did not automatically apply version-based conditional checks, even when a record was annotated with@DynamoDbVersionAttribute. This created an inconsistency withputItemandupdateItem, which already integrate optimistic locking viaVersionedRecordExtension.Motivation and Context
This PR addresses community request #2358, where users expected delete operations to respect optimistic locking semantics similar to put/update operations.
An earlier attempt (PR #6043) proposed implicitly enabling version checks on deletes. However, this risked breaking existing applications that relied on unconditional deletes even when using
@DynamoDbVersionAttribute.To preserve backward compatibility, this PR introduces explicit opt-in optimistic locking.
What This PR Introduces
1. Boolean Overloads (Sync & Async)
New overloads allow explicit control of optimistic locking behavior:
T deleteItem(T keyItem, boolean useOptimisticLocking);CompletableFuture<T> deleteItem(T keyItem, boolean useOptimisticLocking);Usage:
The existing
deleteItem(T keyItem)method is now deprecated and delegates todeleteItem(keyItem, false).2. Fluent Builder API
Added discoverable builder methods:
DeleteItemEnhancedRequest.Builder#withOptimisticLocking(...)TransactDeleteItemEnhancedRequest.Builder#withOptimisticLocking(...)Example:
3. Centralized Helper: OptimisticLockingHelper
A new
@SdkPublicApiclass that:<versionAttributeName> = :version_valueANDuseOptimisticLocking == trueIf any of these conditions are not met, the delete proceeds unconditionally.
Modifications
deleteItem(T keyItem, boolean useOptimisticLocking)(Sync & Async)deleteItem(T keyItem)methods.withOptimisticLocking(...)to request buildersOptimisticLockingHelperVersionedRecordExtensionTesting
Includes:
All new and existing tests pass successfully.
Test Coverage on modified classes:
Test Coverage Checklist
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License