Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path overlap check for all types of updates #8648

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

BlizzardOf
Copy link

This addresses issue 8582.

I discovered a lot about the AWS api's handling of overlapping paths while making test cases for this one:

  • AWS does substitute attribute names in the error messages
  • AWS only catches overlapping paths if neither path is an attribute name
  • AWS does detect duplicate paths if one or more of them is an attribute name
  • AWS splits the path into a list in the error message

Based on this, I found the following issues with the current validation:

  • it only checked for conflicts between adds and deletes, or between set actions
  • it only checked for duplicate paths
  • it didn't handle attribute name substitution

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.

Copy link

codecov bot commented Mar 2, 2025

Codecov Report

Attention: Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.74%. Comparing base (0224977) to head (2350f36).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
moto/dynamodb/utils.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8648   +/-   ##
=======================================
  Coverage   92.74%   92.74%           
=======================================
  Files        1251     1251           
  Lines      108447   108476   +29     
=======================================
+ Hits       100578   100606   +28     
- Misses       7869     7870    +1     
Flag Coverage Δ
servertests 28.12% <77.61%> (+0.03%) ⬆️
unittests 92.71% <98.50%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant