Expand Counter/TTL functionality to UpdateItem, BatchWriteItem, and TransactWriteItems#1923
Conversation
ianbotsf
added
acknowledge-api-break
|
A new generated diff is ready to view. |
| fun AttributePath.update(value: Expression) { | ||
| _updates += UpdateClauseExpr(action, this, value) | ||
| } |
There was a problem hiding this comment.
Question: I don't understand why for update operation, you append the value to the list. For example in CounterInterceptor.augmentUpdateExpr it try to set the counter + 1, but if the conter already exists, you would store 2 counters.
E.g. User calls:
table.updateItem {
key = Key("myId")
update {
set { attr["counter1"] = 99 } // user explicitly touches a counter field
}
}
The interceptor's modifyBeforeSerialization calls augmentUpdateExpron the user's update expression, appending another SET counter1 = if_not_exists(counter1, 0) + 1.
There was a problem hiding this comment.
That's a good point. Attribute paths cannot be repeated in an update expression, even in separate clauses, so a user attempting to manually update a counter/TTL field would cause a runtime error during high-low conversion. I'll rework the updates field into a map that replaces updates rather than appending them.
There was a problem hiding this comment.
Ok, I'll withdraw the approval and once you're done, I'll review it.
There was a problem hiding this comment.
Fixed in latest revision. This turned out to be a good change since it simplified/reduced a lot of code! 😀
luigi617
left a comment
luigi617
left a comment
There was a problem hiding this comment.
The code looks incredibly good to me. Just left a question that I did not understant.
…n _appending_ them
|
A new generated diff is ready to view. |
| val clauseExp = UpdateClauseExpr(action, this, value) | ||
| val previousUpdate = updates.put(clauseExp.target, clauseExp) | ||
| if (previousUpdate != null) { | ||
| println("Warning: Replacing previous update of ${previousUpdate.target}") // FIXME log a proper warning |
There was a problem hiding this comment.
Nit: are you going to fix this in this PR?
There was a problem hiding this comment.
No, threading the logger through DDB Mapper will be its own separate PR.
2 participants
Issue #
Related to #472
Description of changes
This change expands the utilization of
CounterInterceptorandTtlInterceptorto cover the new mutating operationsUpdateItem,BatchWriteItem, andTransactWriteItems. As part of this work a few other related changes were made:update { }DSL's previous behavior of replacing any prior updates was flawed. This PR updatesupdate(heh!) to append to any prior updates rather than replace.TtlFieldsfromSet<Pair<String, Long>>>toMap<String, Long>. Maps are more natural & idiomatic than sets of pairs and the single-value-per-key constraint matches the intended use case better (sets of pairs allow duplicated keys).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.