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

Add attribute name use checks to validation layer #8612

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

Conversation

BlizzardOf
Copy link

This addresses issue #8474

In the responses file, I added validation checks that every attribute name is used in the various expressions that each request takes. Without major changes, these validations have to be done at the responses level instead of the backend level, because some response functions call the same backend functions, and which expressions need to be included in the check varies between request types.

I had to adapt a couple of the parsers to track which attribute names they used during parsing.

I skipped batch_write_items because boto doesn't support any expressions in it, for some reason. I also skipped transact_get_items because it looks like moto doesn't support expressions for that call, even though moto does.

I used the aws.verified tag to get the tests working, but I actually haven't verified it against AWS. I was wondering if you had some tips for setting up a proper environment? I have no idea where to get started even.

Copy link

codecov bot commented Feb 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.73%. Comparing base (0238abd) to head (b7b1cde).
Report is 87 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8612      +/-   ##
==========================================
+ Coverage   92.62%   92.73%   +0.10%     
==========================================
  Files        1229     1246      +17     
  Lines      106421   107985    +1564     
==========================================
+ Hits        98576   100137    +1561     
- Misses       7845     7848       +3     
Flag Coverage Δ
servertests 28.17% <13.91%> (+0.24%) ⬆️
unittests 92.70% <100.00%> (+0.10%) ⬆️

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.

@@ -1142,6 +1258,25 @@ def transact_write_items(self) -> str:
item_attrs = item["Put"]["Item"]
table = self.dynamodb_backend.get_table(item["Put"]["TableName"])
validate_put_has_empty_keys(item_attrs, table)

condition_expression = item["Put"].get("ConditionExpression")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part seems to be the same for every action. Can we extract that somehow, to keep it DRY?

I don't want to prescribe exactly how you should do it, but I was thinking something along the lines of this:

for action in ["Put", ..]:
    if details := item.get(action):
        condition_expression = details.get("ConditionExpression")
        ...

@bblommers
Copy link
Collaborator

I used the aws.verified tag to get the tests working, but I actually haven't verified it against AWS. I was wondering if you had some tips for setting up a proper environment? I have no idea where to get started even.

Hi @BlizzardOf! You can use the environment variable MOTO_TEST_ALLOW_AWS_REQUEST=true to run tests against AWS.

All tests marked with aws_verified are setup in such a way that all resources are automatically deleted afterwards, even if the test fails - so you don't have to worry about dangling resources.

To run all tests in a single file/folder against AWS:

MOTO_TEST_ALLOW_AWS_REQUEST=true pytest -sv tests/test_dynamodb/exceptions/test_dynamodb_exceptions.py -m aws_verified -n 4

Note the -n 4 - not required, but it will run all tests in parallel to speed things up.

Or if you want to validate a single test:

MOTO_TEST_ALLOW_AWS_REQUEST=true pytest -sv tests/test_dynamodb/exceptions/test_dynamodb_exceptions.py::test_query_unused_attribute_name -vv

Note that this particular test (test_query_unused_attribute_name) actually fails against AWS, so that needs to be fixed first:

>       assert (
            err["Message"]
            == "Value provided in ExpressionAttributeNames unused in expressions: keys: {#count}"
        )
E       assert 'Invalid KeyConditionExpression: Syntax error; token: "1", near: "= 1)"' == 'Value provided in ExpressionAttributeNames unused in expressions: keys: {#count}'
E         - Value provided in ExpressionAttributeNames unused in expressions: keys: {#count}
E         + Invalid KeyConditionExpression: Syntax error; token: "1", near: "= 1)"

@BlizzardOf
Copy link
Author

Hm, it's bizarre, I can test against AWS when I run each test individually, but when I run a bunch of them at once, they all fail with
An error occurred (UnrecognizedClientException) when calling the CreateTable operation: The security token included in the request is invalid.
Seems to happen whether I run sequentially or not. You ever see something like that?

Whatever the issue is I can replicate the issue you mention, I should have updates up in a day or so.

@BlizzardOf
Copy link
Author

Alright, I think it's ready

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.

3 participants