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

Slight incompatibility of error path of size() function in condition expression #14592

Closed
nyh opened this issue Jul 9, 2023 · 1 comment
Closed
Labels
area/alternator Alternator related Issues
Milestone

Comments

@nyh
Copy link
Contributor

nyh commented Jul 9, 2023

The DynamoDB documentation for the size() function in ConditionException claims that it requires a path parameter - size(x) and size(#xyz) work. If the data read from that path has a type that size() can't work on (e.g., an integer), the condition fails with ConditionalCheckFailedException. So far so good, and Alternator behaves like DynamoDB in all these cases.

But it turns out that DynamoDB also allows size's parameter to be a value instead of a path - i.e., size(:xyz) works even thought it's not documented. If the value :xyz defined in ExpressionAttributeValues has a suitable type (e.g., a string) its size is defined and the condition using the size may pass. But if the value of :xyz has the wrong type (e.g., an integer), DynamoDB knows the error is in the query itself - not the data - and generates a ValidationException, with the error message: Invalid ConditionExpression: Incorrect operand type for operator or function; operator or function: size, operand type: N.

Trying to use size(size(x)) also generates the same ValidationException - it's also a wrong type (size(x) is an integer, and size() can't be called on that) and known from the query - not from the data.

The importance of this issue is very minor - it's only about the appropriate error type in a case that is in any case erroneous. Alternator also allows size(:xyz) in cases where it's properly defined.

@nyh nyh added the area/alternator Alternator related Issues label Jul 9, 2023
nyh added a commit to nyh/scylla that referenced this issue Jul 9, 2023
The DynamoDB documentation for the size() function claims that it only
works on paths (attribute names or references), but it actually works on
constants from the query (e.g., ":val") as well.

It turns out that Alternator supports this undocumented case already, but
gets the error path wrong: Usually, when size() is calculated on the data,
if the data has the wrong type of size() (e.g., an integer), the condition
simply doesn't match. But if the value comes from the query - it should
generate an error that the query is wrong - ValidationException.

This patch fixes this case, and also adds tests for it that pass on both
DynamoDB and Alternator (after this patch).

Fixes scylladb#14592

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@DoronArazii DoronArazii added this to the 5.4 milestone Aug 29, 2023
@denesb
Copy link
Contributor

denesb commented Dec 18, 2023

A minor, non-correctness improvement on the error path. Not backporting.

MeepoYang pushed a commit to storage-scsg/scylladb that referenced this issue Apr 28, 2024
The DynamoDB documentation for the size() function claims that it only
works on paths (attribute names or references), but it actually works on
constants from the query (e.g., ":val") as well.

It turns out that Alternator supports this undocumented case already, but
gets the error path wrong: Usually, when size() is calculated on the data,
if the data has the wrong type of size() (e.g., an integer), the condition
simply doesn't match. But if the value comes from the query - it should
generate an error that the query is wrong - ValidationException.

This patch fixes this case, and also adds tests for it that pass on both
DynamoDB and Alternator (after this patch).

Fixes scylladb#14592

Signed-off-by: Nadav Har'El <nyh@scylladb.com>

Closes scylladb#14593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alternator Alternator related Issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants