Support update expressions in single request update#6471
Support update expressions in single request update#6471anasatirbasa wants to merge 11 commits intoaws:masterfrom
Conversation
anasatirbasa
changed the title
anasatirbasa
force-pushed
the
feature/support-update-expressions-in-single-request-update
branch
2 times, most recently
from
bfa9f26 to
a7fe576
Compare
anasatirbasa
force-pushed
the
feature/support-update-expressions-in-single-request-update
branch
2 times, most recently
from
f26a4ec to
8791c1f
Compare
anasatirbasa
force-pushed
the
feature/support-update-expressions-in-single-request-update
branch
from
ef8f4c0 to
a0eeea7
Compare
anasatirbasa
force-pushed
the
feature/support-update-expressions-in-single-request-update
branch
from
716b7c8 to
dabb912
Compare
| .ignoreNulls(ignoreNulls) | ||
| .ignoreNullsMode(ignoreNullsMode) | ||
| .conditionExpression(conditionExpression) | ||
| .returnValuesOnConditionCheckFailure(returnValuesOnConditionCheckFailure); |
There was a problem hiding this comment.
Should we add updateExpression here?
There was a problem hiding this comment.
Yes, it should be added to the buillder. I've made the changes.
| * request expressions (highest). | ||
| * | ||
| * <p><b>Steps:</b> Identify attributes used by extensions/requests to prevent REMOVE conflicts → | ||
| * create item SET/REMOVE actions → merge extensions (override item) → merge request (override all). |
There was a problem hiding this comment.
This is not happening, this concatenates and DynamoDB will throw a conflict error, see updateExpressionInRequest_whenAttributeAlsoInPojo_shouldThrowConflictError test.
There was a problem hiding this comment.
The referenced test is no longer present because the implementation was changed to add an opt-in flag (updateExpressionMergeStrategy) in order to preserve backward compatibility.
Default behavior remains LEGACY: actions are concatenated and overlapping paths may still fail in DynamoDB (Two document paths overlap).
Conflict resolution is applied only when users opt into PRIORITIZE_HIGHER_SOURCE (request > extension > POJO).
The javadoc was updated and additional details are captured here.
| .collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| public static UpdateExpression generateItemSetExpression(Map<String, AttributeValue> itemMap, |
There was a problem hiding this comment.
Should this be package private as it is used only by unit tests?
Same for generateItemRemoveExpression.
There was a problem hiding this comment.
The two methods were moved to UpdateExpressionUtils to centralize shared update-expression construction logic and keep the resolver focused on merge behavior.
Thay were also made package-private, since they are just used by UpdateExpressionResolver and tests in the same package.
| .build(); | ||
| } | ||
|
|
||
| public static UpdateExpression generateItemRemoveExpression(Map<String, AttributeValue> itemMap, |
There was a problem hiding this comment.
The method was moved to UpdateExpressionUtils to centralize shared update-expression construction logic and keep the resolver focused on merge behavior.
This was also made package-private, since it's only used by UpdateExpressionResolver and tests in the same package.
| .collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| public static UpdateExpression generateItemSetExpression(Map<String, AttributeValue> itemMap, |
There was a problem hiding this comment.
The method was moved to UpdateExpressionUtils to centralize shared update-expression construction logic and keep the resolver focused on merge behavior.
This was also made package-private, since it's only used by UpdateExpressionResolver and tests in the same package.
| } | ||
|
|
||
| public UpdateExpressionResolver build() { | ||
| return new UpdateExpressionResolver(this); |
There was a problem hiding this comment.
If nonKeyAttributes() is never called on the builder, this.nonKeyAttributes remains null. The resolve() method calls nonKeyAttributes.isEmpty() which will throw NullPointerException.
There was a problem hiding this comment.
Null handling was added in Builder.nonKeyAttributes(...) to set Collections.emptyMap() when null is passed, so resolve() wil not throw NullPointerException:
public Builder nonKeyAttributes(Map<String, AttributeValue> nonKeyAttributes) {
if (nonKeyAttributes == null) {
this.nonKeyAttributes = Collections.emptyMap();
} else {
this.nonKeyAttributes = Collections.unmodifiableMap(new HashMap<>(nonKeyAttributes));
}
return this;
}| } | ||
|
|
||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
We don't have new line at the end
There was a problem hiding this comment.
Added new line.
| * if there are attributes to be updated (most likely). If both exist, they are merged and the code generates a final | ||
| * Expression that represent the result. | ||
| * Merges UpdateExpressions from three sources in priority order: POJO attributes (lowest), | ||
| * extensions (medium), request (highest). Higher priority sources override conflicting actions. |
There was a problem hiding this comment.
We may want to clarify the javadoc here, If I understand this right,
Given a POJO with: item.setCounter(100L)
And a request UpdateExpression: SET counter = counter + :inc
The resolver produces BOTH actions (concat, not override):
SET #AMZN_MAPPED_counter = :AMZN_MAPPED_counter, counter = counter + :inc
DynamoDB rejects this with: "Two document paths overlap with each other"
The only conflict that IS auto-resolved: if item.setCounter(null), the POJO would generate REMOVE counter, but the resolver suppresses that REMOVE because counter appears in the request expression.
There was a problem hiding this comment.
The implementation and javadoc were updated and details related to the new approach are captured here.
| * Tests transactWriteItems() operation Verifies that transactional write operations work correctly. | ||
| */ | ||
| @Test | ||
| public void transactWriteItems_withUpdateExpression() { |
There was a problem hiding this comment.
This test does NOT exercise TransactUpdateItemEnhancedRequest.updateExpression(), it only performs transactional delete + put operations. There is no test that verifies the request-level
UpdateExpression flows through the TransactUpdateItemEnhancedRequest → UpdateItemOperation →
UpdateExpressionResolver path (the right-side of the Either).
A test using:
.addUpdateItem(mappedTable, TransactUpdateItemEnhancedRequest.builder(...)
.item(keyRecord).updateExpression(expression).build())is needed to cover this code path.
There was a problem hiding this comment.
The referenced test is no longer present, it was replaced during the refactor.
This scenario is covered by a new test: transactWriteItems_givenTransactUpdateWithRequestExpression_whenExecute_thenListElementUpdated, which uses TransactUpdateItemEnhancedRequest.updateExpression(...) via addUpdateItem(...) and verifies the update is persisted end to end.
|
The available approaches were analyzed and the main goal was to avoid breaking backward compatibility. If this is not set, behavior stays LEGACY (same as today). The one existing exception is null-remove suppression: if a POJO field is null, it would normally produce a REMOVE, but that REMOVE is skipped when the same attribute is explicitly updated by an extension or request expression. This existing protective behavior is unchanged. If users want conflict handling, they can opt into PRIORITIZE_HIGHER_SOURCE. Examples:
Tha Javadoc was also updated to reflect all of this: why the flag exists, default behavior, available values, precedence, and path grouping. Tests were updated and expanded to cover these scenarios end-to-end, including:
|
| * | ||
| * @return A list of top level attribute names that have update actions associated. | ||
| */ | ||
| public static List<String> findAttributeNames(UpdateExpression updateExpression) { |
There was a problem hiding this comment.
findAttributeNames can return duplicate entries for the same attribute, which causes incorrect grouping in PRIORITIZE_HIGHER_SOURCE mode.
The method combines two lists:
- listPathsWithoutTokens — collects action paths that don't contain #
- listAttributeNamesFromTokens — collects values from the merged expressionNames map
An action with a raw path (no #) that also carries an expressionNames entry for that same attribute contributes to both lists. Simple example:
SetAction action = SetAction.builder()
.path("counter") // raw path → included by listPathsWithoutTokens
.value(":v")
.putExpressionName("#counter", "counter") // value "counter" → included by listAttributeNamesFromTokens
.putExpressionValue(":v", AttributeValue.builder().n("1").build())
.build();
UpdateExpression expr = UpdateExpression.builder().addAction(action).build();
UpdateExpressionConverter.findAttributeNames(expr);
// → ["counter", "counter"] ← same attribute counted twiceIn mergeBySourcePriority, the result is wrapped in new HashSet<>(...) which masks the duplicate, but any caller that iterates the list directly (or uses it for counting) will get wrong results. The existing test findAttributeNames_noComposedNames_duplicates already documents this behaviour, which means the contract is broken by design.
Suggested fix — deduplicate inside findAttributeNames before returning:
public static List<String> findAttributeNames(UpdateExpression updateExpression) {
if (updateExpression == null) {
return Collections.emptyList();
}
List<String> attributeNames = listPathsWithoutTokens(updateExpression);
List<String> attributeNamesFromTokens = listAttributeNamesFromTokens(updateExpression);
attributeNames.addAll(attributeNamesFromTokens);
// Deduplicate: a raw path and its expressionNames entry can both resolve to the same attribute name
return attributeNames.stream().distinct().collect(Collectors.toList());
}This makes the method's contract match its Javadoc ("find the list of attribute names that will be updated") and removes the silent dependency on callers wrapping the result in a Set. The test findAttributeNames_noComposedNames_duplicates should be updated to assert ["attribute1"] instead of ["attribute1", "attribute1"].
findAttributeNames already had this bug before this PR but was never triggered because it was only called with SDK-internal extension expressions (which always use # token paths). This PR is the first to call it with user-provided request expressions, where raw paths and expressionNames entries can coexist for the same attribute, making the bug reachable. The fix belongs in findAttributeNames itself, but the root cause was introduced in PR #3036.
There was a problem hiding this comment.
The suggested fix was applied - keeping only the distinct attribute names:
public static List<String> findAttributeNames(UpdateExpression updateExpression) {
...
return attributeNames.stream().distinct().collect(Collectors.toList());
}Updated javadoc and fixed tests:
@Test
void findAttributeNames_noComposedNames_duplicates() {
...
assertThat(attributes).containsExactly("attribute1");
}@Test
void findAttributeNames_composedNames_duplicates() {
...
assertThat(attributes).containsExactly("attribute1");
}| /** | ||
| * For {@link UpdateExpressionMergeStrategy#PRIORITIZE_HIGHER_SOURCE}: assigns each top-level attribute name to at most one | ||
| * source by priority (request, then extension, then POJO), then keeps only that source's actions for each assigned name. | ||
| */ | ||
| private static UpdateExpression mergeBySourcePriority(UpdateExpression itemExpression, |
There was a problem hiding this comment.
Item in DynamoDB: { "profile": { "name": "Alice", "city": "London" } }
Extension sets: profile.name → "Bob" (e.g. an audit/normalisation extension)
Request sets: profile.city → "Paris" (user wants to update only the city)
Both paths share top-level name profile. mergeBySourcePriority assigns profile to the request (highest priority), then does extensionOwned.removeAll(requestOwned) so profile is removed from extensionOwned. The extension's profile.name action is then filtered out by filterByAttributes. Only profile.city = "Paris" is sent to DynamoDB.
Result: profile.name silently stays "Alice" instead of being updated to "Bob". DynamoDB would have accepted both actions without any conflict profile.name and profile.city are completely independent paths.
Details:
PRIORITIZE_HIGHER_SOURCE groups actions by top-level attribute name and keeps only the highest-priority source's actions for each name. This is too coarse: it drops lower-priority actions even when their specific paths don't actually overlap with the higher-priority source's paths.
Example: extension sets profile.name, request sets profile.city. Both resolve to top-level name profile, so the extension's action is dropped but DynamoDB would accept both in the same request without any conflict, since profile.name and profile.city are independent paths.
True DynamoDB path overlap only occurs when one path is a prefix of another (e.g. profile vs profile.name, or list vs list[0]). Sibling paths like profile.name and profile.city do not overlap.
Suggested fix: replace the top-level-name grouping with a prefix check. For each lower-priority action, only drop it if a higher-priority action's path is a prefix of it (or vice versa):
private static boolean conflictsWith(String candidatePath, Set<String> higherPriorityPaths) {
for (String higher : higherPriorityPaths) {
// overlap: one path is a prefix of the other
if (candidatePath.equals(higher)
|| candidatePath.startsWith(higher + ".")
|| candidatePath.startsWith(higher + "[")
|| higher.startsWith(candidatePath + ".")
|| higher.startsWith(candidatePath + "[")) {
return true;
}
}
return false;
}Then in mergeBySourcePriority, instead of removing whole top-level names from extensionOwned/itemOwned, filter individual actions by whether their resolved full path conflicts with any higher-priority action's full path. This preserves non-conflicting sibling actions from lower-priority sources while still correctly resolving true overlaps.
Note: the current Javadoc on UpdateExpressionMergeStrategy.PRIORITIZE_HIGHER_SOURCE and the resolve() method should also be updated to accurately describe whichever behavior is chosen, so users can reason about what will and won't be sent to DynamoDB.
There was a problem hiding this comment.
The suggested changes were implemented:
- Real overlap: same path, or parent vs child (e.g.
parentvsparent.child,itemsvsitems[0]) → keep the higher-priority source (request → extension → item). - No overlap: different leaves under the same map (e.g.
profile.namevsprofile.city) → keep both. - Lists: different indices, including POJO + extension + request and paths with
#placeholders → keep each non-overlapping index.
New tests were added in:
- UpdateExpressionTest.java
- UpdateItemOperationTest.java
- UpdateExpressionResolverTest.java
Javadocs on UpdateExpressionMergeStrategy, UpdateExpressionResolver.resolve() and related request / operation types were updated to match the changes.
| public UpdateExpressionResolver build() { | ||
| return new UpdateExpressionResolver(this); | ||
| } |
There was a problem hiding this comment.
tableMetadata has no default value, so if the caller never calls .tableMetadata(...), the field is null. resolve() passes tableMetadata to generateItemSetExpression, which passes it to UpdateBehaviorTag.resolveForAttribute, which will NPE. The requireNonNull guard is only on the setter, it doesn't protect against the setter never being called.
So, it may cause a NPE at runtime with a confusing stack trace instead of a clear IllegalStateException at build time.
You can add validation in build():
public UpdateExpressionResolver build() {
Validate.paramNotNull(tableMetadata, "tableMetadata");
return new UpdateExpressionResolver(this);
}There was a problem hiding this comment.
Added validation in UpdateExpressionResolver:
public UpdateExpressionResolver build() {
if (!nonKeyAttributes.isEmpty()) {
Validate.paramNotNull(tableMetadata, "tableMetadata");
}
return new UpdateExpressionResolver(this);
}and tests in UpdateExpressionResolverTest that cover this scenario:
- build_nonKeyAttributesWithoutTableMetadata_throwsNullPointerException
- build_emptyNonKeyAttributesWithoutTableMetadata_succeeds
|
|
||
| UpdateExpression result = resolver.resolve(); | ||
|
|
||
| assertNull(result); |
There was a problem hiding this comment.
Minor: I think existing enhanced client test suite uses AssertJ exclusively. You may consider replacing all assertNull(result) with assertThat(result).isNull().
There was a problem hiding this comment.
The tests were updated in order to use Assertj.
| } | ||
| } | ||
| return UpdateExpressionConverter.toExpression(updateExpression); | ||
| private Expression generateUpdateExpressionIfExist( |
There was a problem hiding this comment.
UpdateItemOperationTest can be updated as part of this PR to add tests for the new updateExpression and updateExpressionMergeStrategy fields flowing through generateRequest()
There was a problem hiding this comment.
UpdateItemOperationTest and UpdateItemOperationTransactTest were updated with the following scenarios:
| Scenario | Result |
|---|---|
| Request only | Only the request update expression. |
| Extension + request, different paths (LEGACY or PRIORITIZE with no overlap) | The full extension expression and the full request expression—every action from each, on its own paths. |
| Extension + request, same path (PRIORITIZE_HIGHER_SOURCE) | The request side for that path; extension actions that hit the same path are dropped. |
| Extension + request, same path (LEGACY) | Both extension and request actions on that path, all chained together. |
| POJO + request (LEGACY) | Everything from the item map plus everything from the request expression. |
| POJO + extension (LEGACY) | Everything from the item map plus everything from the extension expression. |
| POJO + extension + request (LEGACY) | Item, extension, and request contributions, all merged. |
| POJO + request (PRIORITIZE_HIGHER_SOURCE, same attribute in both) | Request wins on the shared attribute; the item still supplies updates for attributes the request does not take over. |
| POJO + extension (PRIORITIZE_HIGHER_SOURCE, same attribute in both) | Extension wins on the shared attribute; the item still supplies updates for attributes the extension does not take over. |
| POJO + extension + request (PRIORITIZE_HIGHER_SOURCE) | Request actions first; then extension actions that do not collide with the request; then item actions that do not collide with either. |
| Transact (same setups) | Same rules; the merged result is what appears on both the plain UpdateItemRequest and the transact Update payload. |
| } | ||
|
|
||
| @Test | ||
| public void resolve_prioritizeHigherSource_ownedAttributesButNoResolvedPathMatches_returnsNull() { |
There was a problem hiding this comment.
The test creates an expression with path #missing but expressionNames = {"#other": "logicalAttr"} — the token #missing has no mapping. This is a malformed expression that would fail at DynamoDB anyway. The test asserts null is returned, but the real-world scenario this is meant to cover is unclear. It's testing an edge case that shouldn't occur in practice and could give false confidence.
Consider either documenting clearly what real scenario this covers, or removing it in favor of a test that covers a realistic edge case.
There was a problem hiding this comment.
Agreed, that test used an invalid expression, so it was removed.
| } | ||
|
|
||
| @Test | ||
| public void updateItem_givenPrioritizeHigherSourceMerge_whenPojoSetsDocumentRootAndRequestSetsNestedField_thenNestedPathPersists() { |
There was a problem hiding this comment.
The test asserts persistedRecord.getObjectAttribute().getCity() equals "originCity", but originCity is the value set during putItem. Since the request expression only sets objectAttribute.name (not objectAttribute.city), and the POJO's objectAttribute is dropped by PRIORITIZE_HIGHER_SOURCE, the city field should remain from the original item. This assertion is correct, but it's only valid because the initial putItem set city = "originCity". If the initial put didn't set city, the assertion would still pass (null == null). The test should explicitly set and verify city to make the assertion meaningful.
There was a problem hiding this comment.
Added a read after the initial put and assert city is "originCity" before the update, so we’re not accidentally passing on null == null:
@Test
public void updateItem_givenPrioritizeHigherSourceMerge_whenPojoSetsDocumentRootAndRequestSetsNestedField_thenNestedPathPersists() {
initClientWithExtensions();
RecordForUpdateExpressions record = createFullRecord();
mappedTable.putItem(record);
RecordForUpdateExpressions afterPut = mappedTable.getItem(record);
assertThat(afterPut.getObjectAttribute().getCity()).isEqualTo("originCity");
...
assertThat(persistedRecord.getObjectAttribute().getCity()).isEqualTo("originCity");
}| return this; | ||
| } | ||
|
|
||
| public UpdateExpressionResolver build() { |
There was a problem hiding this comment.
Javadoc was added.
5 participants
Motivation and Context
#5554
Enhanced
UpdateItemand transact-update requests can now carry a request-levelUpdateExpression. That expression is merged with updates coming from the mapped item (POJO) and frombeforeWriteextensions into the single update expression DynamoDB sees.Merge strategies
LEGACY(default, backward-compatible)Two document paths overlap).REMOVEfor an attribute but that attribute also appears in an extension or request expression, theREMOVEis skipped so remove and set do not fight on the same name.PRIORITIZE_HIGHER_SOURCE(opt-in).) or list index ([).profile.namevsprofile.city) or between different list indices (e.g.items[0]vsitems[1]).profilevsprofile.name, oritemsvsitems[0]).request > extension > POJO.Extension actions dropped in step 2 are not considered when filtering POJO actions in step 3.
Path overlap examples (for
PRIORITIZE_HIGHER_SOURCE)profile.nameandprofile.city— no overlap (both may appear).profileandprofile.name— overlap (lower-priority action on that path is dropped).items[0]anditems[1]— no overlap (both may appear).items[0]— overlap (higher-priority source wins for that path).If
updateExpressionMergeStrategyis not set, LEGACY is used.What Changed
API
UpdateItemEnhancedRequest/TransactUpdateItemEnhancedRequest:updateExpression,updateExpressionMergeStrategy.Implementation
UpdateItemOperationmerges sources throughUpdateExpressionResolver, thenUpdateExpressionConverterbuilds the low-levelUpdateItemRequest.generateRequest(), soTransactWriteItem.update()uses the sameupdateExpression,expressionAttributeNames, andexpressionAttributeValuesas the non-transact path.Validation
SET/REMOVEgeneration),UpdateExpressionResolver.BuilderrequirestableMetadataat build time.Testing
Resolver and merge logic
UpdateExpressionResolverTest(and related update-expression utilities) exercises LEGACY concatenation, PRIORITIZE_HIGHER_SOURCE overlap and precedence, path resolution (including expression name placeholders), and build validation when POJO-derived updates need table metadata.Unit tests
UpdateItemOperationTest— Asserts the merged result ofgenerateRequest()forUpdateItemEnhancedRequest:DELETEvs requestSET).FakeItemWithSortwith non-key attributes) + request and/or extension under LEGACY (all sources merged).UpdateItemOperationTransactTest— Same scenarios usingTransactUpdateItemEnhancedRequest, with two checks per case:generateRequest()output matches expectations.generateTransactWriteItem()output matches that same request onupdateExpressionand expression attribute maps (parity with the transactUpdatepayload).Functional tests
UpdateExpressionTestcovers end-to-end enhanced client flows for representative path and merge cases, consistent with the overlap rules documented onUpdateExpressionMergeStrategy.Test Results
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