ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): choose WHERE vs AND from builder state, not a text search

#1118
Merged glennmichael123 wants to merge fix/1113-where-keyword-state into main
2 files +235 -27
8 days ago

Closes #1113.

Both write builders picked their keyword by testing the statement built so far for /\bWHERE\b/:

const getWhereKeyword = () => SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'

Plenty of text that is not this builder's predicate satisfies that test. The first real predicate then comes out as AND and fuses onto whatever preceded it:

UPDATE "t" SET "flag" = (SELECT count(*) FROM t x WHERE x.id > 3) AND "id" = ?
UPDATE "where" SET "name" = ? AND "id" = ?
UPDATE "t" SET "where" = ? AND "id" = ?
DELETE FROM "where" AND "id" = ?

The issue reported the first one. The other three came out of checking how far the defect reached: where is a legal quoted identifier, so any table or column with that name breaks both builders outright.

Why the UPDATE is the serious half

The DELETE is a syntax error — loud, caught immediately. The UPDATE parses. The predicate becomes part of the value being written, the statement carries no WHERE, and it rewrites every row. Postgres rejects it only when the SET target's type happens not to line up (argument of AND must be type boolean, not type bigint); where that target is boolean, or on SQLite, it runs exactly as written.

That is also why the test fixture seeds flag = 7 rather than 0. The repro in the issue writes 0 to rows that already hold 0, so the whole-table write leaves no trace — the damage is invisible unless the fixture makes it visible.

The change

A boolean on each builder, set when a predicate is actually appended. Nothing the caller can put in the statement can fool it.

Both builders now append through one appendPredicate() helper rather than repeating the keyword expression at nine call sites each, so there is one place left that can get this wrong.

Verification

Executed against sqlite, not asserted on SQL text alone:

  • the set() subquery case emits WHERE and updates only the selected row — [1, 7, 7, 7], previously [1, 0, 0, 0]
  • a table named where works on both UPDATE and DELETE
  • a column named where in set() no longer absorbs the predicate
  • chained predicates still join with AND — #1015 stays fixed
  • a raw fragment carrying its own WHERE is still treated as the first predicate

5 of the 10 new tests fail without the fix. Full suite 2037 pass / 0 fail, typecheck clean, pickier 0 errors.

Not in this PR

The select builder has a third copy of the same inference (client.ts:3350), reached after a set operator. selectFrom('a').where(...).union(selectFrom('b')).where(...) emits ... UNION SELECT * FROM b AND x = $2. Filed separately — different builder, different fix, and it is a syntax error rather than silent damage.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.