-
Notifications
You must be signed in to change notification settings - Fork 16
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
forbid inFilter for table updateOne and deleteOne command #1693
forbid inFilter for table updateOne and deleteOne command #1693
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some changes for the error message, and I think the test can check the message. couple of other nit picks.
Approving for you to fix those and merge.
src/main/java/io/stargate/sgv2/jsonapi/service/operation/tables/WhereCQLClauseAnalyzer.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/UpdateAttemptBuilder.java
Show resolved
Hide resolved
...st/java/io/stargate/sgv2/jsonapi/service/operation/tables/DeleteUpdateWhereAnalyzerTest.java
Outdated
Show resolved
Hide resolved
…r-table-deleteone-updateone
…r-table-deleteone-updateone
body: |- | ||
The command used a $in or $nin filter to select the rows the modify, however these filters can select more than one row. | ||
|
||
Update One and Delete One commands can only modify a single rows, and so cannot use $in or $nin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo: should be "modify a single row" not "modify a single rows"
code: INVALID_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE | ||
title: In filter is not allowed for UpdateOne and DeleteOne commands | ||
body: |- | ||
The command used a $in or $nin filter to select the rows the modify, however these filters can select more than one row. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure there's an internal reason to disallow calling updateOne()
with $in
/$nin
, but this sentence doesn't seem to justify why you shouldn't use $in
or $nin
with updateOne()
.
For example, a common pattern I've seen with updateOne()
is updateOne({ _id: 42, status: { $in: ['pending', 'active'] } }, update)
: update the document with _id: 42
if it has status 'pending' or 'active'. This pattern can only update at most one row.
Similarly, updateOne({ isDeleted: false })
can select many rows.
Not critical, but it would be nice to make this error message more accurately summarize the reasoning for this behavior.
This is to ban $in and $nin from table updateOne and deleteOne commands.
Fixes: #1688
Checklist