ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(orm): scope destructive statements the way reads are scoped

#1116
Merged glennmichael123 wants to merge fix/1111-orm-soft-delete-scope into main
2 files +210 -7

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bun-query-builder/src/orm.tsmodified+48-7
@@ -2672,6 +2672,33 @@ class ModelQueryBuilder<
26722672 * the soft-delete predicate. User clauses are parenthesised when both are
26732673 * present so a top-level `OR` can't escape the soft-delete filter.
26742674 */
2675 /**
2676 * Refuse a destructive statement that carries clauses it will not emit.
2677 *
2678 * `delete()`, `update()` and `increment()` read none of `_limit`, `_offset`
2679 * or `_orderBy`, so `query().where(...).orderBy('id').limit(1).delete()`
2680 * quietly deleted every matching row rather than one — the caller's cap was
2681 * dropped on the floor. LIMIT on an UPDATE/DELETE is not portable (Postgres
2682 * has no such form), so these cannot simply be emitted; being loud is the
2683 * honest option. See #1111.
2684 */
2685 private assertNoUnappliedClauses(method: string): void {
2686 const ignored: string[] = []
2687 if (this._limit !== undefined)
2688 ignored.push('limit()')
2689 if (this._offset !== undefined)
2690 ignored.push('offset()')
2691 if (this._orderBy.length > 0)
2692 ignored.push('orderBy()')
2693
2694 if (ignored.length > 0) {
2695 throw new TypeError(
2696 `[orm] ${method}() cannot apply ${ignored.join(', ')} — an UPDATE/DELETE takes no ORDER BY or LIMIT on every supported dialect. `
2697 + `Previously these were ignored, so the statement affected every matching row. Narrow it with where() instead.`,
2698 )
2699 }
2700 }
2701
26752702 private composeWhere(params: unknown[]): string {
26762703 const userClause = this._wheres.length > 0 ? this.buildWhereClauses(params) : ''
26772704 const sd = this.softDeleteClause()
@@ -3020,6 +3047,7 @@ class ModelQueryBuilder<
30203047 */
30213048 async increment<K extends NumericColumns<TDef>>(column: K, amount = 1): Promise<number> {
30223049 assertValidIdentifier(column, 'increment(column)')
3050 this.assertNoUnappliedClauses('increment')
30233051 const exec = getExecutor()
30243052 const params: unknown[] = [amount]
30253053
@@ -3031,9 +3059,11 @@ class ModelQueryBuilder<
30313059 params.push(formatNow())
30323060 }
30333061
3034 if (this._wheres.length > 0) {
3035 const clauses = this.buildWhereClauses(params)
3036 sql += ` WHERE ${clauses}`
3062 // See delete(): composeWhere is the only path that adds the soft-delete
3063 // predicate, so a scoped counter bump has to go through it. #1111.
3064 const whereBody = this.composeWhere(params)
3065 if (whereBody) {
3066 sql += ` WHERE ${whereBody}`
30373067 }
30383068
30393069 return (await exec.run(sql, params)).changes
@@ -3185,18 +3215,26 @@ class ModelQueryBuilder<
31853215 }
31863216
31873217 async delete(): Promise<number> {
3218 this.assertNoUnappliedClauses('delete')
31883219 const exec = getExecutor()
31893220 const params: unknown[] = []
31903221 let sql = `DELETE FROM ${this._definition.table}`
31913222
3192 if (this._wheres.length > 0) {
3193 sql += ` WHERE ${this.buildWhereClauses(params)}`
3223 // composeWhere, not buildWhereClauses: the soft-delete predicate is added
3224 // only by composeWhere, so building the WHERE here meant a scope that
3225 // exists purely as that predicate contributed nothing to the statement.
3226 // `onlyTrashed().delete()` — purge the trash — emitted a bare DELETE and
3227 // removed every row, live ones included. See #1111.
3228 const whereBody = this.composeWhere(params)
3229 if (whereBody) {
3230 sql += ` WHERE ${whereBody}`
31943231 }
31953232
31963233 return (await exec.run(sql, params)).changes
31973234 }
31983235
31993236 async update(data: Partial<Pick<InferModelAttributes<TDef>, FillableKeys<TDef>>>): Promise<number> {
3237 this.assertNoUnappliedClauses('update')
32003238 const exec = getExecutor()
32013239 const entries = Object.entries(data)
32023240 const sets = entries.map(([k]) => `${sqlColumn(k)} = ?`).join(', ')
@@ -3208,8 +3246,11 @@ class ModelQueryBuilder<
32083246
32093247 let sql = `UPDATE ${this._definition.table} SET ${sets}${timestampsEnabled(this._definition) ? ', updated_at = ?' : ''}`
32103248
3211 if (this._wheres.length > 0) {
3212 sql += ` WHERE ${this.buildWhereClauses(params)}`
3249 // See delete(): composeWhere is the only path that adds the soft-delete
3250 // predicate, so a scoped update has to go through it. #1111.
3251 const whereBody = this.composeWhere(params)
3252 if (whereBody) {
3253 sql += ` WHERE ${whereBody}`
32133254 }
32143255
32153256 return (await exec.run(sql, params)).changes
packages/bun-query-builder/test/orm.destructive-scope.test.tsadded+162-0
@@ -0,0 +1,162 @@
1/**
2 * The ORM's destructive statements must be scoped the way its reads are.
3 * stacksjs/bun-query-builder#1111.
4 *
5 * `delete()`, `update()` and `increment()` built their WHERE with
6 * `buildWhereClauses()`, guarded by `this._wheres.length > 0`. Every read path
7 * uses `composeWhere()` — and `composeWhere` is the only place the soft-delete
8 * predicate is added.
9 *
10 * So a scope expressed purely as that predicate contributed nothing to a
11 * write, and when it was the ONLY scope the statement went out with no WHERE
12 * at all:
13 *
14 * Post.query().onlyTrashed().delete() -> DELETE FROM posts (every row)
15 *
16 * "Purge the trash" was the exact call that destroyed the live rows.
17 *
18 * These assert row counts rather than emitted SQL. The defect was the gap
19 * between two WHERE builders, so only the resulting table settles it.
20 */
21
22import { Database } from 'bun:sqlite'
23import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
24import { clearModelRegistry, configureOrm, createModel, createTableFromModel } from '../src'
25
26let db: Database
27let Post: any
28
29beforeEach(async () => {
30 clearModelRegistry()
31 db = new Database(':memory:', { create: true })
32 configureOrm({ database: db })
33
34 Post = createModel({
35 name: 'Sdpost',
36 table: 'sd_posts',
37 primaryKey: 'id',
38 autoIncrement: true,
39 traits: { useSoftDeletes: true },
40 attributes: {
41 title: { type: 'string', fillable: true },
42 views: { type: 'integer', fillable: true },
43 },
44 } as any)
45
46 await createTableFromModel(Post.getDefinition())
47 for (const title of ['a', 'b', 'c', 'd'])
48 await Post.create({ title, views: 0 })
49 // 'a' and 'b' are trashed; 'c' and 'd' are live.
50 db.run(`UPDATE sd_posts SET deleted_at = '2026-01-01' WHERE title IN ('a','b')`)
51})
52
53afterEach(() => {
54 clearModelRegistry()
55})
56
57const total = (): number => (db.query('SELECT count(*) c FROM sd_posts').get() as any).c
58const alive = (): number => (db.query('SELECT count(*) c FROM sd_posts WHERE deleted_at IS NULL').get() as any).c
59const titles = (): string[] => (db.query('SELECT title FROM sd_posts ORDER BY title').all() as any[]).map(r => r.title)
60
61describe('delete() honours the soft-delete scope (#1111)', () => {
62 it('onlyTrashed().delete() removes the trashed rows and spares the live ones', async () => {
63 expect([total(), alive()]).toEqual([4, 2])
64
65 await Post.query().onlyTrashed().delete()
66
67 // Previously: total 0 — the live rows went too.
68 expect(total()).toBe(2)
69 expect(alive()).toBe(2)
70 expect(titles()).toEqual(['c', 'd'])
71 })
72
73 it('an unscoped delete still excludes trashed rows, as reads do', async () => {
74 await Post.query().delete()
75
76 // The default scope hides trashed rows, so a hard delete must not reach
77 // them either — otherwise the same query means two different row sets
78 // depending on whether it reads or writes.
79 expect(titles()).toEqual(['a', 'b'])
80 })
81
82 it('a user filter still applies alongside the scope', async () => {
83 await Post.query().where('title', 'c').delete()
84
85 expect(titles()).toEqual(['a', 'b', 'd'])
86 })
87})
88
89describe('update() and increment() honour the scope (#1111)', () => {
90 it('onlyTrashed().update() rewrites only the trashed rows', async () => {
91 await Post.query().onlyTrashed().update({ title: 'PURGED' })
92
93 const purged = (db.query(`SELECT count(*) c FROM sd_posts WHERE title='PURGED'`).get() as any).c
94 expect(purged).toBe(2)
95 expect(total()).toBe(4)
96 })
97
98 it('onlyTrashed().increment() bumps only the trashed rows', async () => {
99 await Post.query().onlyTrashed().increment('views', 5)
100
101 const bumped = (db.query('SELECT count(*) c FROM sd_posts WHERE views = 5').get() as any).c
102 expect(bumped).toBe(2)
103 })
104
105 it('an unscoped update leaves trashed rows alone', async () => {
106 await Post.query().update({ views: 9 })
107
108 const bumped = (db.query('SELECT count(*) c FROM sd_posts WHERE views = 9').get() as any).c
109 expect(bumped).toBe(2)
110 })
111})
112
113describe('clauses a write cannot emit are refused (#1111)', () => {
114 // Silently ignoring limit() is what turned `limit(1).delete()` into a delete
115 // of every matching row. LIMIT on UPDATE/DELETE is not portable — Postgres
116 // has no such form — so being loud is the honest option.
117 it('limit() on a delete throws instead of being dropped', async () => {
118 await expect(Post.query().limit(1).delete()).rejects.toThrow(/cannot apply limit/)
119 expect(total()).toBe(4)
120 })
121
122 it('orderBy() on a delete throws instead of being dropped', async () => {
123 await expect(Post.query().orderBy('id').delete()).rejects.toThrow(/cannot apply .*orderBy/)
124 expect(total()).toBe(4)
125 })
126
127 it('limit() on an update throws instead of being dropped', async () => {
128 await expect(Post.query().limit(1).update({ views: 3 })).rejects.toThrow(/cannot apply limit/)
129 const bumped = (db.query('SELECT count(*) c FROM sd_posts WHERE views = 3').get() as any).c
130 expect(bumped).toBe(0)
131 })
132
133 it('a write with no ordering or limit is unaffected', async () => {
134 await expect(Post.query().where('title', 'c').update({ views: 7 })).resolves.toBeDefined()
135 const bumped = (db.query('SELECT count(*) c FROM sd_posts WHERE views = 7').get() as any).c
136 expect(bumped).toBe(1)
137 })
138})
139
140describe('a model without soft deletes is unchanged (#1111)', () => {
141 it('deletes exactly what the filter selects', async () => {
142 clearModelRegistry()
143 const plain = new Database(':memory:', { create: true })
144 configureOrm({ database: plain })
145
146 const Item = createModel({
147 name: 'Plainitem',
148 table: 'plain_items',
149 primaryKey: 'id',
150 autoIncrement: true,
151 attributes: { name: { type: 'string', fillable: true } },
152 } as any)
153 await createTableFromModel((Item as any).getDefinition())
154 for (const name of ['x', 'y', 'z'])
155 await (Item as any).create({ name })
156
157 await (Item as any).query().where('name', 'y').delete()
158
159 const left = (plain.query('SELECT name FROM plain_items ORDER BY name').all() as any[]).map(r => r.name)
160 expect(left).toEqual(['x', 'z'])
161 })
162})