ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): an array in object-form where() means IN on writes too

#1127
Merged glennmichael123 wants to merge fix/1114-array-in-write-where into main
2 files +189 -21
8 days ago

Closes #1114.

The select builder has read where({ id: [3, 2] }) as id IN (3, 2) since #1013/#1083. The write builders bound the whole array to a single placeholder, so the same input meant two different things depending on which statement it was attached to:

await db.selectFrom('t').select('id').where({ id: [3, 2] }).execute()  // -> rows 2 and 3
await db.deleteFrom('t').where({ id: [3, 2] }).execute()               // -> deletes nothing

This is the opposite direction to #1101 — the write is too narrow, so it destroys nothing. But it fails silently and reports success, and it does so in exactly the case people write it for: the bulk delete of a set of ids.

One renderer, three call sites

updateTable.where, deleteFrom.where and updateMany each had their own copy of the same loop. They now share renderColumnCondition(). The divergence, not any one branch, is what kept producing this family of bugs — #1101, #1112, #1113 and this one are all two implementations of the same idea disagreeing.

Two details the shared renderer settles:

An empty array renders as 1 = 0, matching the select builder, rather than vanishing. A filter the caller supplied must never widen to "every row" because the collection came back empty — the { id: [] } case is the one the issue flagged as needing a decision, and on a write there is only one safe answer.

Parameters are numbered as the array is built, not as baseIdx + i + 1. The old arithmetic assumed one parameter per key, so in where({ id: [2, 3], tenant: 1 }) every parameter after the list bound to the wrong placeholder. Now:

DELETE FROM "t" WHERE "id" IN (?, ?) AND "tenant" = ?

That case gets both a text assertion and an execution assertion, since it is the one the shared code most easily gets wrong.

Verification

Executed against sqlite, asserting on surviving rows. 6 of the 11 new tests fail without this change. Full suite 2088 pass / 0 fail, typecheck clean, pickier 0 errors.

One claim from the original sweep is still not reproduced and is not addressed here: that the array is consumed as the entire positional bind list on sqlite, dropping other predicates. I could not make that happen before the fix, and the mixed array+scalar test above pins the behaviour either way.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.