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

Don't demote range key condition to filter condition #800

Merged
merged 7 commits into from
Jun 25, 2020

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Jun 18, 2020

No description provided.

@ikonst ikonst marked this pull request as ready for review June 18, 2020 13:22
@ikonst
Copy link
Contributor Author

ikonst commented Jun 18, 2020

@jpinner-lyft better?

@ikonst ikonst changed the title Remove range key condition demotion to filter condition Don't demote range key condition to filter condition Jun 19, 2020
@jpinner-lyft
Copy link
Contributor

I think this is better -- my only other thought would be now that we're not at tempting to modify the query we could just pass the filter_condition to dynamo un-inspected and let it return any errors. This has the drawback that you don't detect early but would be less code maintenance on our end.

@ikonst
Copy link
Contributor Author

ikonst commented Jun 23, 2020

I had this thought as well (the error checking seemed redundant and we risk being needlessly restrictive) but I didn't venture to check what errors DynamoDB returns in those cases -- that is, maybe this was done because DynamoDB's messaging is confusing? Your suggestion that this checking was introduced only for the demotion's sake seems plausible, though.

@ikonst
Copy link
Contributor Author

ikonst commented Jun 24, 2020

This removes a bunch of code, which is nice.

The server-side errors are:

pynamodb.exceptions.QueryError: Failed to query items: An error occurred (ValidationException) on request (<REQUEST ID>) on table (<TABLE>) when calling the Query operation: ...

  1. Using hash key in range key condition:

    KeyConditionExpressions must only contain one condition per key

  2. Using non-key in condition:

    Query key condition not supported

  3. Using invalid operator (e.g. exists():

    Invalid operator used in KeyConditionExpression: attribute_exists

Would that be clear enough?

@ikonst ikonst merged commit e3f801a into master Jun 25, 2020
@ikonst ikonst deleted the invalid_range_key_condition2 branch June 25, 2020 03:33
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.

2 participants