ReviewOS

stacks/bun-query-builder

returning() discards every filter applied after it, so the UPDATE/DELETE runs against the whole table

#1110
Closed glennmichael123 opened this 22 days ago · 0 comments
22 days ago

Found on main @ a258137, verified by execution against live Postgres 17.

returning() snapshots the statement text at call time and hands back an object whose filter methods are no-ops:

// client.ts, update builder ~:7000 and delete builder ~:7192
{ where: () => obj, andWhere: () => obj, orWhere: () => obj,
  orderBy: () => obj, limit: () => obj, offset: () => obj }

Every filter expressed after .returning() is silently dropped and the unfiltered statement executes.

Repro

// table has 4 rows
await db.updateTable('t').set({ name: 'X' }).returning('id').where({ id: 1 }).execute()
callreturnedactual effect
updateTable.set(v).returning('id').where({id:1})[{id:1},{id:2},{id:3},{id:4}]all 4 rewritten
.returning('id').where('id','=',99).executeTakeFirst(){id:1}all 4 rewritten
deleteFrom('t').returning('id').where({id:1})[{id:1}…{id:4}]table emptied
.where({id:1}).returning('id') (control)[{id:1}]correct — 1 row

Only the returning-then-filter order is affected.

Why this is worse than a dropped filter

It reports success for work it did not scope. .returning('id').where('id','=',99).executeTakeFirst() returns {id:1} — a row that cannot match id = 99 — while rewriting the whole table. A caller reading that return value concludes one row was touched.

The types invite it. returning() is declared to return SelectQueryBuilder (client.ts:2021), which declares where (:737), andWhere (:1046), orWhere (:1062), orderBy (:1088), limit (:1162). So the broken order typechecks under --strict with no casts.

It bypasses hooks. The delete path through returning() skips beforeDelete/afterDelete, so an application-level delete guard never runs.

Also reachable through the Laravel-style surface: db.table('t').update({...}).returning('id').where({id:1}) and db.table('t').delete().returning('id').where({id:1}).

Suggested fix

Either defer building until execute() so a later where() genuinely applies, or make these methods throw and require filtering before returning(). The current shape — accepting the call, typing it as supported, and discarding it — is the one option that must not stay.

If deferral is impractical, note that the declared return type should stop being SelectQueryBuilder, since it promises a surface that is not implemented.

Found by an automated sweep for silent predicate loss; the numbers above are from my own reproduction, not the sweep's.

Sign in to comment on this issue.