ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): an empty list is a predicate, not the absence of one

#1087
Merged glennmichael123 wants to merge fix/empty-collection-predicates into main
2 files +131 -5
10 days ago

Refs #1083 — the two smaller defects reported at the end of that issue. The orWhere grouping bug, which is its headline, is not in here (see below).

whereIn(col, []) emitted IN ()

SQLite parses that and matches nothing — the sane reading. Postgres and MySQL reject it. Verified against live Postgres:

IN ()      (old emit)  ->  REJECTED: syntax error at or near ")"
1 = 0      (new emit)  ->  0 rows
NOT IN ()  (old emit)  ->  REJECTED: syntax error at or near ")"
1 = 1      (new emit)  ->  3 rows

So the same call site behaved differently depending on the engine. It now emits 1 = 0, which takes no parameters, parses identically everywhere, and preserves SQLite's answer.

The mirror needed care, and is why the four IN sites now go through one shared renderer rather than four inline copies: an empty NOT IN is TRUE, not FALSE. Nothing is a member of the empty set, so everything is a non-member. Writing the false constant there would silently drop every row from any query whose exclusion list happened to come back empty — the same shape of quiet, direction-flipping failure being fixed. There's a test named as the trap.

whereAny([], op, value) silently vanished

It returned the builder untouched, so the predicate disappeared and the query widened to include everything the filter existed to exclude:

before:  .where('s','=','p').whereAny([], 'like', '%x%')  ->  WHERE s = ?
after:   .where('s','=','p').whereAny([], 'like', '%x%')  ->  WHERE s = ? AND (1 = 0)

An empty column list is nearly always a .filter() that removed everything, so the failure lands exactly where someone is least likely to look — and it fails toward more rows.

whereAll([]) and whereNone([]) keep their no-op, because TRUE genuinely is their identity. That asymmetry is deliberate and asserted, so the next reader doesn't tidy it into a bug.

Verification

5 new tests; 3 of 5 fail against the unfixed client.ts. Non-empty IN is asserted unchanged.

Typecheck 0, lint 0, suite 1724 pass / 4 skip / 0 fail.

Why the orWhere fix isn't here

It's a much larger change than it looks. The select builder has no WHERE term list — just a string that 43 call sites append to (whereConditions exists but is dead: 24 pushes, 0 reads). Fixing precedence means introducing a real term list across all of them, plus the parallel _wheres implementation in orm.ts. It also changes emitted SQL for existing users, so it needs its own PR, its own changelog entry as a behaviour change, and a grouping API (whereGroup) shipped alongside so the old reading stays expressible.

It's fully investigated and specced — I just didn't want to land a half-finished refactor of SQL generation on the end of this one.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.