ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(orm)!: destroy() honours soft deletes, and forceDelete() purges

#1159
Merged glennmichael123 wants to merge fix/destroy-honours-soft-deletes into main
4 files +254 -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.

docs/delete.mdmodified+23-0
@@ -126,6 +126,29 @@ await db
126126
127127```
128128
129## Deleting by id on a Model
130
131`Model.delete(id)` and `Model.destroy(id)` both delete the row the way an
132instance does: a model with `useSoftDeletes` is marked rather than removed, and
133`beforeDelete`/`afterDelete` run. Both answer `false` when there is no such row — including one that is already
134trashed, which they no longer see. Use `Model.forceDelete(id)` to purge that.
135
136```typescript
137await Post.delete(1) // or Post.destroy(1) — the same call
138await Post.onlyTrashed().first() // the row is still there, marked
139```
140
141`Model.forceDelete(id)` removes it for good, soft deletes or not, and finds a
142row that is already trashed. `Model.remove(id)` is the same call, matching what
143`db.remove(table, id)` means above. Delete hooks run either way.
144
145```typescript
146await Post.forceDelete(1) // or Post.remove(1)
147```
148
149An instance has the same pair: `post.delete()` marks a soft-deletable row,
150`post.forceDelete()` removes it.
151
129152## Delete with Returning
130153
131154```typescript
packages/bun-query-builder/src/__tests__/type-usage-compile.tsmodified+7-1
@@ -1268,7 +1268,13 @@ async function _defineModelUsage() {
12681268 Trail.query().latest().oldest('distance')
12691269 await Trail.update(1, { name: 'Ridge' })
12701270 const deleted: boolean = await Trail.delete(1)
1271 void deleted
1271 const destroyed: boolean = await Trail.destroy(1)
1272 void [deleted, destroyed]
1273
1274 // Purge statics, server-only — the browser model has no counterpart
1275 const purged: boolean = await Trail.forceDelete(1)
1276 const removed: boolean = await Trail.remove(1)
1277 void [purged, removed]
12721278
12731279 // Column names stay narrow
12741280 // @ts-expect-error — unknown column
packages/bun-query-builder/src/orm.tsmodified+47-6
@@ -1455,7 +1455,9 @@ class ModelInstance<
14551455 const pkValue = this._attributes[pk]
14561456 const hooks = this._definition.hooks
14571457
1458 if (!pkValue) throw new Error('Cannot delete a model without a primary key')
1458 // `== null`, not falsy: 0 and '' are primary keys a row can legitimately
1459 // carry, and rejecting them told the caller the instance had no key at all.
1460 if (pkValue == null) throw new Error('Cannot delete a model without a primary key')
14591461
14601462 await hooks?.beforeDelete?.(this as unknown as ModelHookInstance)
14611463
@@ -1478,6 +1480,28 @@ class ModelInstance<
14781480 return true
14791481 }
14801482
1483 /**
1484 * Delete this row for good, even on a soft-deletable model.
1485 *
1486 * `delete()` marks a soft-deletable row instead of removing it, which leaves
1487 * no way to purge one row through an instance. Delete hooks run either way.
1488 */
1489 async forceDelete(): Promise<boolean> {
1490 const pk = this._definition.primaryKey || 'id'
1491 const pkValue = this._attributes[pk]
1492 const hooks = this._definition.hooks
1493
1494 // `== null`, not falsy: 0 and '' are primary keys a row can legitimately
1495 // carry, and rejecting them told the caller the instance had no key at all.
1496 if (pkValue == null) throw new Error('Cannot delete a model without a primary key')
1497
1498 await hooks?.beforeDelete?.(this as unknown as ModelHookInstance)
1499 await getExecutor().run(`DELETE FROM ${this._definition.table} WHERE ${pk} = ?`, [pkValue])
1500 await hooks?.afterDelete?.(this as unknown as ModelHookInstance)
1501
1502 return true
1503 }
1504
14811505 /**
14821506 * Restore a soft-deleted row by clearing `deleted_at`. Throws if the model
14831507 * doesn't use soft deletes. See stacksjs/bun-query-builder#1024.
@@ -3589,6 +3613,7 @@ export type ModelStatic<TDef extends ModelDefinition> = StaticWhereOverloads<TDe
35893613 update: (id: number | string, data: FillableAttributes<TDef>) => Promise<ModelRecord<TDef>>
35903614 delete: (id: number | string) => Promise<boolean>
35913615 destroy: (id: number | string) => Promise<boolean>
3616 forceDelete: (id: number | string) => Promise<boolean>
35923617 remove: (id: number | string) => Promise<boolean>
35933618 truncate: () => Promise<void>
35943619 getDefinition: () => TDef
@@ -3784,14 +3809,30 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
37843809 return record ? record.delete() : false
37853810 },
37863811
3787 async destroy(id: number | string): Promise<boolean> {
3788 const exec = getExecutor()
3789 const pk = definition.primaryKey || 'id'
3790 return (await exec.run(`DELETE FROM ${definition.table} WHERE ${pk} = ?`, [id])).changes > 0
3812 /**
3813 * Delete the row with this primary key, the way `instance.delete()` does:
3814 * a soft-deletable model is soft-deleted, and delete hooks run.
3815 *
3816 * This used to issue a bare `DELETE` on every model, so `destroy()` removed
3817 * rows that `delete()` on the same model would have kept, and no hook saw
3818 * it. `forceDelete()` is the spelling for a permanent one.
3819 */
3820 destroy(id: number | string): Promise<boolean> {
3821 return model.delete(id)
3822 },
3823
3824 /**
3825 * Delete the row with this primary key for good, soft deletes or not.
3826 * Delete hooks run. `remove()` is the same call — the name the client
3827 * builder's `db.remove(table, id)` already uses for a real DELETE.
3828 */
3829 async forceDelete(id: number | string): Promise<boolean> {
3830 const record = await model.withTrashed().find(id)
3831 return record ? record.forceDelete() : false
37913832 },
37923833
37933834 remove(id: number | string): Promise<boolean> {
3794 return this.destroy(id)
3835 return model.forceDelete(id)
37953836 },
37963837
37973838 async truncate(): Promise<void> {
packages/bun-query-builder/test/orm.destroy-soft-deletes.test.tsadded+177-0
@@ -0,0 +1,177 @@
1/**
2 * `Model.destroy(id)` honours soft deletes and hooks; `forceDelete(id)` is the
3 * permanent one.
4 *
5 * `destroy` issued a bare `DELETE FROM … WHERE id = ?` on every model. So on a
6 * soft-deletable model it removed the row that `instance.delete()` — and
7 * `Model.delete(id)` — would have marked, and no `beforeDelete`/`afterDelete`
8 * hook ever saw it. Two spellings of "delete this row by id", opposite results,
9 * and the destructive one was the one that looked emphatic.
10 *
11 * `destroy` now goes through `instance.delete()`. `forceDelete` is the way to
12 * purge one row, and `remove` is an alias of it — the meaning `db.remove(table,
13 * id)` already has in docs/delete.md.
14 */
15
16import { Database } from 'bun:sqlite'
17import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
18import { clearModelRegistry, configureOrm, createModel, createTableFromModel } from '../src'
19
20let db: Database
21let Post: any
22let Tag: any
23const hooks: string[] = []
24
25beforeEach(async () => {
26 clearModelRegistry()
27 hooks.length = 0
28 db = new Database(':memory:', { create: true })
29 configureOrm({ database: db })
30
31 Post = createModel({
32 name: 'DsPost',
33 table: 'ds_posts',
34 primaryKey: 'id',
35 autoIncrement: true,
36 traits: { useSoftDeletes: true },
37 attributes: { title: { type: 'string', fillable: true } },
38 hooks: {
39 beforeDelete: (post: any) => { hooks.push(`before:${post.get('title')}`) },
40 afterDelete: (post: any) => { hooks.push(`after:${post.get('title')}`) },
41 },
42 } as any)
43 Tag = createModel({
44 name: 'DsTag',
45 table: 'ds_tags',
46 primaryKey: 'id',
47 autoIncrement: true,
48 attributes: { label: { type: 'string', fillable: true } },
49 hooks: {
50 beforeDelete: (tag: any) => { hooks.push(`before:${tag.get('label')}`) },
51 afterDelete: (tag: any) => { hooks.push(`after:${tag.get('label')}`) },
52 },
53 } as any)
54
55 await createTableFromModel(Post.getDefinition())
56 await createTableFromModel(Tag.getDefinition())
57 await Post.create({ title: 'A' })
58 await Post.create({ title: 'B' })
59 await Tag.create({ label: 'x' })
60})
61
62afterEach(() => {
63 clearModelRegistry()
64 db.close()
65})
66
67const ids = (table: string): number[] =>
68 (db.query(`SELECT id FROM ${table} ORDER BY id`).all() as { id: number }[]).map(r => r.id)
69const rows = (table: string): any[] => db.query(`SELECT id, deleted_at FROM ${table} ORDER BY id`).all() as any[]
70const live = (table: string): number[] => rows(table).filter(r => r.deleted_at == null).map(r => r.id)
71
72describe('Model.destroy(id) on a soft-deletable model (#1150 follow-up)', () => {
73 it('marks the row instead of removing it', async () => {
74 // Was a bare DELETE: the row was gone and could not be restored.
75 expect(await Post.destroy(1)).toBe(true)
76 expect(rows('ds_posts').length).toBe(2)
77 expect(live('ds_posts')).toEqual([2])
78
79 const trashed = await Post.onlyTrashed().first()
80 expect(trashed.get('title')).toBe('A')
81 await trashed.restore()
82 expect(live('ds_posts')).toEqual([1, 2])
83 })
84
85 it('runs the delete hooks', async () => {
86 await Post.destroy(1)
87 expect(hooks).toEqual(['before:A', 'after:A'])
88 })
89
90 it('is false for a missing row, and for one already trashed', async () => {
91 expect(await Post.destroy(99)).toBe(false)
92 await Post.destroy(1)
93 hooks.length = 0
94 expect(await Post.destroy(1)).toBe(false)
95 expect(hooks).toEqual([])
96 })
97
98 it('still removes the row on a model without soft deletes, and now runs its hooks', async () => {
99 // The bare DELETE ran no hooks on any model; that half is new for plain ones.
100 expect(await Tag.destroy(1)).toBe(true)
101 expect(ids('ds_tags')).toEqual([])
102 expect(hooks).toEqual(['before:x', 'after:x'])
103 })
104
105 it('deletes a row whose primary key is 0', async () => {
106 // The instance guard read a falsy key as "no primary key" and threw, so a
107 // row with id 0 survived a destroy that reported nothing wrong.
108 db.run(`INSERT INTO ds_tags (id, label) VALUES (0, 'zero')`)
109 expect(await Tag.destroy(0)).toBe(true)
110 expect(ids('ds_tags')).toEqual([1])
111 })
112})
113
114describe('Model.forceDelete(id) and remove(id)', () => {
115 it('remove permanently deletes, soft deletes or not', async () => {
116 expect(await Post.remove(1)).toBe(true)
117 expect(ids('ds_posts')).toEqual([2])
118 })
119
120 it('forceDelete purges a row that was already soft-deleted', async () => {
121 await Post.destroy(1)
122 expect(rows('ds_posts').length).toBe(2)
123
124 expect(await Post.forceDelete(1)).toBe(true)
125 expect(ids('ds_posts')).toEqual([2])
126 })
127
128 it('runs the delete hooks', async () => {
129 await Post.forceDelete(2)
130 expect(hooks).toEqual(['before:B', 'after:B'])
131 })
132
133 it('purges a plain model too, with its hooks', async () => {
134 expect(await Tag.forceDelete(1)).toBe(true)
135 expect(ids('ds_tags')).toEqual([])
136 expect(hooks).toEqual(['before:x', 'after:x'])
137 })
138
139 it('is false for a missing row', async () => {
140 expect(await Post.forceDelete(99)).toBe(false)
141 expect(await Tag.remove(99)).toBe(false)
142 })
143
144 it('purges a row whose primary key is 0', async () => {
145 db.run(`INSERT INTO ds_tags (id, label) VALUES (0, 'zero')`)
146 expect(await Tag.forceDelete(0)).toBe(true)
147 expect(ids('ds_tags')).toEqual([1])
148 })
149
150 it('survives being destructured off the model', async () => {
151 const { remove } = Post
152 expect(await remove(1)).toBe(true)
153 expect(ids('ds_posts')).toEqual([2])
154 })
155})
156
157describe('instance.forceDelete()', () => {
158 it('removes a soft-deletable row the instance would otherwise mark', async () => {
159 const post = await Post.find(1)
160 expect(await post.forceDelete()).toBe(true)
161 expect(ids('ds_posts')).toEqual([2])
162 expect(hooks).toEqual(['before:A', 'after:A'])
163 })
164
165 it('purges an already-trashed instance', async () => {
166 const post = await Post.find(1)
167 await post.delete()
168 expect(post.trashed()).toBe(true)
169
170 await post.forceDelete()
171 expect(ids('ds_posts')).toEqual([2])
172 })
173
174 it('refuses an instance with no primary key', async () => {
175 await expect(Post.make({ title: 'unsaved' }).forceDelete()).rejects.toThrow('Cannot delete a model without a primary key')
176 })
177})