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

Improve error handling for invalid range key conditions #798

Closed
wants to merge 1 commit into from

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented Jun 11, 2020

For a condition to be a valid range key condition, it must:

  1. operate on the range key
  2. use one of the supported operators

Filter conditions cannot use range key attributes, so a condition whose only problem is an unsupported operator cannot be "demoted" to be a filter condition: we'd simply fail a few statements later with:

raise ValueError("'filter_condition' cannot contain key attributes")

Moreover, that message would've caught the user by suprirse, since the user passed it as a range_key_condition.

Changes:

  1. The error messages now indicates the reason for the condition being invalid.
  2. We only demote to a filter condition when permissible.
  3. We now issue a warning in this case. Key conditions have different cost and this demotion could be harmful. We should consider deprecating it.
  4. Removing the Condition.is_valid_range_key_condition method. It probably never served any purpose except to be used in this code, and I think it's a good candidate for deprecation in pynamodb 5.x.

@ikonst ikonst force-pushed the invalid_range_key_condition branch from f7830e6 to db9ae84 Compare June 11, 2020 05:34
@ikonst ikonst marked this pull request as ready for review June 11, 2020 05:36
@ikonst ikonst requested a review from jpinner-lyft June 11, 2020 05:37
@@ -14,10 +14,6 @@ def __init__(self, operator, *values):
self.operator = operator
self.values = values

# http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_Query.html#DDB-Query-request-KeyConditionExpression
def is_valid_range_key_condition(self, path):
Copy link
Contributor

Choose a reason for hiding this comment

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

TIOLI: i think this change makes the code less readable

Copy link
Contributor Author

@ikonst ikonst Jun 11, 2020

Choose a reason for hiding this comment

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

I don't think Condition needs a is_valid_range_key_condition method: I don't think it's a very likely scenario for a user to take a condition and, say, try to determine dynamically if it can be used as a range condition.

I agree it can be refactored into a method in TableConnection.query which is the only place where it's used now.

@jpinner-lyft
Copy link
Contributor

Wondering if it would be easier to just warn on the demotion and then remove it outright instead of trying to fix the error handling. Thoughts?

@ikonst
Copy link
Contributor Author

ikonst commented Jun 11, 2020

Wondering if it would be easier to just warn on the demotion and then remove it outright instead of trying to fix the error handling.

Can we just remove it in 5.x?

@ikonst
Copy link
Contributor Author

ikonst commented Jun 11, 2020

Also, isn't it valuable to have two error messages instead of one?

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.

None yet

3 participants