ReviewOS

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
8 days ago

Follow-up to #1150, which added Model.delete(id) and made the disagreement obvious.

Two ways to delete one row, opposite results

instance.delete() marks a soft-deletable row and runs beforeDelete/afterDelete. Model.delete(id) goes through it. Model.destroy(id) did not — it issued a bare

DELETE FROM posts WHERE id = ?

on every model. So on a model carrying useSoftDeletes, destroy removed the row that delete on the same model would have marked, no hook saw it, and nothing could restore it. The destructive one was the one that reads as emphatic.

destroy(id) now delegates to delete(id). Plain models are unaffected in what they do — the row still goes — and now run their delete hooks, which they never did before.

The purge half, named

  • Model.forceDelete(id) — new. Permanent, on any model, and it looks through the trashed scope, so it can purge a row destroy already marked.
  • Model.remove(id) — now an alias of forceDelete rather than of destroy, keeping the meaning it already has one page over: docs/delete.md documents db.remove(table, id) as "issues a real DELETE, so it removes the row whether or not it was already soft-deleted".
  • instance.forceDelete() — new, the same for one loaded row.

Hooks run on every path above.

ModelQueryBuilder.delete() is deliberately untouched: it is the bulk path, and onlyTrashed().delete() purging the trash is the behaviour #1111 fixed.

A primary key of 0 was read as no primary key

Both delete guards tested the key for truthiness:

if (!pkValue) throw new Error('Cannot delete a model without a primary key')

A row whose key is 0 or '' — imported data, a natural key, MySQL's classic id 0 — therefore threw, and survived, with a message that was wrong about the state of the world. Since destroy now routes through the instance, the old bare DELETE was the only thing that still worked on those rows; the guards now test pkValue == null, so an unsaved instance still throws and a real key of 0 deletes. An adversarial review pass found this one.

Breaking

Model.destroy(id) on a model with useSoftDeletes marks the row instead of removing it, and answers false for a row that is already trashed (it no longer sees one). Model.remove(id) keeps removing rows for good, so callers who wanted a purge and wrote remove are unaffected; callers who wrote destroy and meant purge want forceDelete. Delete hooks now fire on destroy/remove/forceDelete, which fired nothing before.

Nothing in stacksjs/stacks or trailbuddy calls either method — their generated destroy routes go through the client builder.

Tests

test/orm.destroy-soft-deletes.test.ts, 15 tests: destroy marking and restoring, its hooks, the missing and already-trashed cases, plain models still being removed and now firing hooks, forceDelete/remove purging (soft-deletable, already-trashed, and plain), instance.forceDelete() including on a trashed instance and the unsaved-instance throw, a key of 0 on both paths, and remove destructured off the model. 8 fail against main before the review additions.

Locally: full suite 2261 pass / 0 fail / 10 skip, tsc clean, pickier . at its 330-warning baseline, docs-lint passing.

Left for later

The review surfaced these; none are touched here:

  • docs/delete.md's pre-existing db.deleteFrom('users').onlyTrashed().forceDelete() example calls two methods the client's delete builder does not have.
  • ModelQueryBuilder.delete() runs no model hooks on any path, so bulk deletes bypass them entirely.
  • db.remove(table, id) hard-codes where({ id }), so it targets the wrong column when primaryKey is not id.
  • There is no Model.restore(id) to pair with Model.destroy(id).
  • ModelInstance.save() has the same falsy-primary-key assumption this PR fixed in the delete guards, which makes create() with a client-assigned key take the UPDATE branch.

🤖 Generated with Claude Code

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

Sign in to comment on this pull request.