Deferring to @suhailopensource who indicated in the issue template they'd like to submit the fix themselves after maintainer approval. Closing this PR to avoid duplicate work.

Reopening this. I closed it on 18 August because the reporter said they wanted to send their own PR for #18306. None has been opened in the 11 days since, so I am putting this back up.

Nothing on the branch changed. It still applies cleanly on main and CI was green. If the reporter or anyone else opens a PR for this, I will close mine again.

I have no issues with @lazerg raising a PR.

No actionable comments were generated in the recent review. 🎉

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

THis probably needs the one gaurd against user footguns. I would say you should update this to throw when that happens.

I chose the literal (0 = 1) over a throw on purpose. #18250 did the same for empty Op.notIn, and a maintainer approved that choice. #18286 proposes throwing on empty Op.or/Op.not, but that PR has been open three weeks with no maintainer decision yet. A throw here would break the current documented behavior, where {[Op.in]: []} matches no rows, and it would not match how Op.notIn works today. I think this needs a maintainer to decide the direction, since it affects more than this one PR. I can switch to a throw once someone confirms that's the way to go.

I chose the literal (0 = 1) over a throw on purpose. #18250 did the same for empty Op.notIn, and a maintainer approved that choice. #18286 proposes throwing on empty Op.or/Op.not, but that PR has been open three weeks with no maintainer decision yet. A throw here would break the current documented behavior, where {[Op.in]: []} matches no rows, and it would not match how Op.notIn works today. I think this needs a maintainer to decide the direction, since it affects more than this one PR. I can switch to a throw once someone confirms that's the way to go.

I am a maintainer, but I will defer to @WikiRik

This is still an alpha so it is the time to make breaking changes like this. I also cannot, ever, see a reason why someone would want to intentionally use that operation. But there is always an XKCD dude out there. If there is, I still don't think we should support him for the 99.9999999% of people who would have just nuked their DB from a broken call.

I looked at the write paths specifically, since that is the part you flagged.

updateQuery (query-generator.js:567) and bulkDeleteQuery (query-generator-typescript.ts:947) both call the same whereQuery that findAll calls. The WHERE text is identical in all three cases. FormatWhereOptions (query-generator-typescript.ts:133) carries only model, mainAlias and the bind options, so WhereSqlBuilder cannot tell which statement it is building. A throw limited to writes is not expressible at the Op.in layer without threading the statement type through every caller.

What this PR changes for writes:

• Model.destroy({ where: { id: { [Op.in]: [] } } }) was DELETE FROM "Ts" WHERE "id" IN (NULL), now DELETE FROM "Ts" WHERE 0 = 1. Deletes nothing, before and after.
• Model.update(v, { where: { [Op.not]: { id: { [Op.in]: [] } } } }) was UPDATE "Ts" SET ... WHERE NOT ("id" IN (NULL)), now ... WHERE NOT (0 = 1). Zero rows before, every row now.

The second one is your case and it is a real change. The old result was not safe either though. It silently did nothing while the caller had asked for every row.

A throw on empty Op.in also catches the common shape Model.destroy({ where: { id: ids } }) where ids came back empty. That deletes nothing today, which is correct, and people rely on it. It would also disagree with #18250, which you approved, where empty Op.notIn still emits 1 = 1.

There is already a write-only guard here: destroy at model.js:2731, and _optionsMustContainWhere at model.js:3405 which update and increment call. findAll has nothing equivalent. Both check that a where exists, and the error text points people at sql`1 = 1` when they do mean all rows. If you want a guard against a write whose WHERE matches everything, that is the layer that fits, and it would also cover where: {} and an empty Op.or, not just this one operator.

I am happy to write that guard, here or as a separate PR. I am also happy to switch this PR to a throw if @WikiRik wants that direction. In that case empty Op.notIn should throw too, so the pair stays symmetric.

A throw on empty Op.in also catches the common shape Model.destroy({ where: { id: ids } }) where ids came back empty. That deletes nothing today, which is correct, and people rely on it. It would also disagree with #18250, which you approved, where empty Op.notIn still emits 1 = 1.

Yeah, I didn't realize that until now, it's a hard problem to tackle and maybe out of scope for this. But it would have to thread through every call to not stop people updating everything when they do want to. its more guarding against people updating with a where that has an empty notIn, as it is expressing that they do want to filter something, but there is something broken in their implementation, otherwise they wouldn't have added the superfluous where statement.

it does get harder too, because it needs to be when it is the only filter. I'm not saying I know the answer, just what I noticed when thinking about it more.

I'm working on a broader PR to fix this and similar issues. Should be opened later today

I ran the empty notIn case you describe. It already wipes data on main today.

Built main at 704b36e, ran it against SQLite:

whereQuery notIn []  ->  "WHERE 1 = 1"
Model.update(v, { where: { id: { [Op.notIn]: [] } } })  ->  3 of 3 rows updated
Model.destroy({ where: { id: { [Op.notIn]: [] } } })    ->  3 of 3 rows deleted
Model.destroy({ where: { id: { [Op.in]: [] } } })       ->  0 rows deleted

It is older than #18250 too. On 7.0.0-alpha.48, which predates that PR, the empty notIn fragment was an empty string, so the statement ran with no WHERE at all. Same outcome, 3 of 3 rows deleted. So the case you found is pre-existing, and this PR does not touch that branch.

For an empty Op.in on a write, nothing is deleted or updated, before or after this PR. The one write that changes is the negated form { [Op.not]: { id: { [Op.in]: [] } } }, which goes from 0 rows to all rows. I checked both on the same build.

@WikiRik does your broader PR cover empty Op.in as well? If it does, I will close this one. If it does not, I can rebase on top of it once it lands.

Broader PR is #18324 (with #18323 as follow up as broader refactor later). Feel free to review, alongside the corresponding website PR

Thanks for the pointer. #18324 covers this more thoroughly (and folds in #18286 and #18306 too), closing this one in favor of it.

@lazerg

Sorry you missed the credit on it, you were the one that brought it up. Thanks for the contribution and discussion 🥇

@lazerg Nice work man .