ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): refuse a write whose predicate the builder cannot read

#1109
Merged glennmichael123 wants to merge fix/1101-silent-full-table-write into main
2 files +251 -5
8 days ago

Closes #1101. Closes #1095.

updateTable().where() and deleteFrom().where() accepted argument shapes they could not turn into a predicate, then returned this unchanged. The statement kept no WHERE at all, ran against every row, and reported the full affected count as success.

Measured on live Postgres 17, 4 rows seeded, before → after:

where(...)DELETE beforeUPDATE beforeafter (both)
undefinedall 4 deletedall 4 rewrittenthrows, table intact
nullall 4 deletedall 4 rewrittenthrows, table intact
{}SQL syntax errorSQL syntax errorthrows, naming the problem
raw('id = 1')invalid input syntax for type booleanall 4 rewrittenaffects exactly 1 row

Why raw() is the serious one

It is not a mistake the types catch. The declared signature is

// client.ts:1981
expr: WhereExpression<DB[TTable]['columns']> | K | SqlFragment

and raw is a public export. So where(raw('id = 1')) reads correctly, typechecks, passes review — and rewrote the table.

Two shapes, two different remedies

A fragment is honoured, on both builders, because the signature promises it. Refusing it would trade a silent catastrophe for a broken advertised API. Note the delete side was already broken here in a different way: routed through applyWhere() it was bound as a value, which is why Postgres complained about a boolean. That works now too.

Fragments are matched ahead of the object branch on purpose: { sql, parameters } is an object, so the old ordering would have rendered it as "sql" = ? AND "parameters" = ? and matched nothing. There is a test pinning that specific ordering trap.

Anything genuinely unreadable is refused, synchronously at the where() call rather than at execute() — it fails at the point of the mistake, before a statement exists. A write builder is the wrong place to read "I could not understand your filter" as "match every row".

This is the same fall-through the select builder dropped in #1083. The write builders kept it.

#1095 comes along with it

where({}) now throws naming the problem instead of emitting a dangling WHERE and dying on a syntax error. Worth recording that the symptom #1095 reports was always the safe one — it never wrote anything. The dangerous siblings were sitting in the same method unreported.

Deliberately still possible

await db.deleteFrom('t').execute()   // whole table, on purpose

Refusing an unreadable filter must not remove the ability to write a whole table intentionally. The point is that it has to be explicit. There is a test for this.

Tests

test/client.write-predicate-required.test.ts — 15 tests, 11 fail without the src change. The remaining 4 are the invariants that ordinary writes are untouched.

Every assertion checks surviving rows, not merely that a call threw. The bug was damage, so the table is the thing worth asserting on — a test that only checked for an exception would pass against a builder that threw after deleting everything.

Out of scope, found while measuring

where({ id: [1, 2] }) on both write builders renders "id" = $1 rather than id IN (...), so it matches nothing — silently deleting/updating zero rows where the same call on a select matches both. That is a narrowing failure rather than the widening one this PR is about, and it deserves its own issue rather than being folded in here.

Checks

  • typecheck and pickier clean
  • bun test — 1970 pass, 4 skip, 1 fail (the stale-bin/qbx CLI > version test, pre-existing and reproducing on clean main)

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.