DynamoDB: Path overlap check for all types of updates #8648
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8648 +/- ##
==========================================
- Coverage 92.78% 92.78% -0.01%
==========================================
Files 1252 1252
Lines 109074 109109 +35
==========================================
+ Hits 101201 101233 +32
- Misses 7873 7876 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @BlizzardOf! First of all apologies for the late review - I just haven't had time to look into this.
The changes look great! Adding new validators is a very good approach, and it's nice to have all validation in one place now.
All the new tests pass against AWS, so that's a great step towards parity in this area.
Would you mind resolving the merge conflicts? I'd be happy to merge after that.
| @@ -704,7 +811,6 @@ def test_update_item_non_existent_table(): | |||
| "expression", | |||
| [ | |||
| "set example_column = :example_column, example_column = :example_column", | |||
| "set example_column = :example_column ADD x :y set example_column = :example_column", | |||
There was a problem hiding this comment.
:suggestion: This whole test can probably be removed now? From what I can see, the new (AWS-validated) tests cover the same scenarios.
| @@ -94,7 +94,7 @@ def test_transact_write_items__update_with_multiple_set_clauses(table_name=None) | |||
| { | |||
| "Update": { | |||
| "TableName": table_name, | |||
| "Key": {"pk": {"S": "s"}}, | |||
| "Key": {"pk": {"S": "s"}, "sk": {"S": "t"}}, | |||
There was a problem hiding this comment.
Just a note to explain this change to future us: This change is necessary because the validation order has now changed. Before, we would first validate the UpdateExpression, and afterwards validate the supplied Key.
With the changes in this PR, we first check whether the correct Key is supplied. Because this test didn't do that, it would fail with a different error message.
Fixing the Key ensures that we only fail when validating the UpdateExpression.
This is technically a reversal in parity with AWS, as the original test did pass against AWS - so AWS seems to validate the UpdateExpression first. This is not a big deal though IMO. These validations will all be caught during development to tell the user that something is wrong - as long as the validation happens, and saves the user the headache of finding this out in production, the exact order should not matter.
BlizzardOf
force-pushed
the
path-overlap-check-for-all-types-of-updates
branch
from
906b282 to
f7e42ba
Compare
bblommers
changed the title
|
Oh yeah meant to comment it's ready, thanks! |
This addresses issue 8582.
I discovered a lot about the AWS api's handling of overlapping paths while making test cases for this one:
Based on this, I found the following issues with the current validation:
I realized that by limiting a validation to an UpdateExpression, I could add top-level checks to the already existing set of validations for update statements. I felt this was more natural, and the expression names were already available, so I moved the validations that previously belongs to the AST object into the update expression validator.
However, this made it more difficult to do the update validation at the response level for transact_write_items. Rather than duplicate the whole thing, I had the transact_write_items backend pipe validation errors through the transaction failure error handling, since in AWS the validation occurs before any transaction execution.
As part of this I discovered that both updates and transactions can only have one "set" statement, so I added it to the validations as well.