ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(orm): save() inserts or updates by whether the row exists

#1161
Merged glennmichael123 wants to merge fix/save-primary-key-detection into main
3 files +312 -16
packages/bun-query-builder/src/orm.tsmodified+44-16
@@ -1062,10 +1062,21 @@ class ModelInstance<
10621062 private _forced: Set<string> = new Set()
10631063 private _definition: TDef
10641064 private _hasSaved = false
1065 /**
1066 * Whether this instance is a row in the table: true when it was read from
1067 * the database or has been inserted, false for one built by `create()`,
1068 * `make()` or `replicate()` that has not been saved yet. save() branches on
1069 * this. It used to branch on whether the primary key was truthy, which sent
1070 * a caller-supplied key (`create({ id: 5 })`, or any string key) down the
1071 * UPDATE path, so nothing was inserted, and sent a loaded row whose key was
1072 * `0` down the INSERT path, so it was duplicated.
1073 */
1074 private _exists: boolean
10651075 private _relations: Record<string, ModelInstance<any, any>[] | ModelInstance<any, any> | null> = {}
10661076
1067 constructor(definition: TDef, attributes: Partial<ModelAttributes<TDef>> = {}) {
1077 constructor(definition: TDef, attributes: Partial<ModelAttributes<TDef>>, exists: boolean) {
10681078 this._definition = definition
1079 this._exists = exists
10691080 // The spread already gives us a private copy, so the normalizer may reuse
10701081 // it instead of building a second object per hydrated row.
10711082 this._attributes = normalizeAttributeKeysInPlace({ ...attributes })
@@ -1306,13 +1317,17 @@ class ModelInstance<
13061317 }
13071318 }
13081319
1309 if (this._attributes[pk]) {
1320 if (this._exists) {
13101321 // Update
13111322 await hooks?.beforeUpdate?.(this as unknown as ModelHookInstance, this.getChanges())
13121323
13131324 const changes = this.getChanges()
13141325 const changeKeys = Object.keys(changes)
13151326 if (changeKeys.length > 0) {
1327 // A row read without its key (`select('title')`) cannot be addressed.
1328 // This used to take the INSERT branch and write a copy of the row.
1329 if (this._attributes[pk] == null)
1330 throw new Error(`Cannot save a model without a primary key: '${pk}' was not selected`)
13161331 const sets = changeKeys.map(k => `${k} = ?`).join(', ')
13171332 const values = [...Object.values(changes), this._attributes[pk]]
13181333
@@ -1375,6 +1390,15 @@ class ModelInstance<
13751390 }
13761391 }
13771392
1393 // A key the caller supplied is written. Neither loop above picks it up
1394 // unless the model declares it as an attribute, and a string key has no
1395 // database default to fall back on.
1396 const suppliedKey = this._attributes[pk]
1397 const keyAttr = attrs[pk]
1398 const keyGuarded = keyAttr?.guarded && !this._forced.has(pk)
1399 if (suppliedKey != null && !(pk in data) && !keyGuarded)
1400 data[pk] = suppliedKey
1401
13781402 if (timestampsEnabled(this._definition)) {
13791403 const now = formatNow()
13801404 // An explicitly supplied `created_at` wins.
@@ -1416,8 +1440,12 @@ class ModelInstance<
14161440 for (const [key, value] of Object.entries(data)) {
14171441 this._attributes[key] = value
14181442 }
1419 if (result.lastInsertId != null)
1443 // Only a generated key is read back. For a supplied one the driver's
1444 // insert id is the rowid (SQLite) or 0 (MySQL on a non-AUTO_INCREMENT
1445 // key), not the key.
1446 if (!(pk in data) && result.lastInsertId != null)
14201447 this._attributes[pk] = result.lastInsertId
1448 this._exists = true
14211449
14221450 await hooks?.afterCreate?.(this as unknown as ModelHookInstance)
14231451 }
@@ -1446,7 +1474,7 @@ class ModelInstance<
14461474 if (id == null) return null
14471475 const row = await exec.get(`SELECT * FROM ${this._definition.table} WHERE ${pk} = ?`, [id])
14481476 if (!row) return null
1449 return new ModelInstance(this._definition, row as Partial<ModelAttributes<TDef>>)
1477 return new ModelInstance(this._definition, row as Partial<ModelAttributes<TDef>>, true)
14501478 }
14511479
14521480 async delete(): Promise<boolean> {
@@ -1569,7 +1597,7 @@ class ModelInstance<
15691597 delete attrs.uuid
15701598 delete attrs.created_at
15711599 delete attrs.updated_at
1572 return new ModelInstance<TDef, TSelected>(this._definition, attrs as any)
1600 return new ModelInstance<TDef, TSelected>(this._definition, attrs as any, false)
15731601 }
15741602
15751603 toArray(): Record<string, unknown> {
@@ -2280,7 +2308,7 @@ export class BelongsToManyRelationBuilder<TRel extends ModelDefinition> {
22802308 else if (k !== fkParent && k !== fkRelated)
22812309 pivotExtras[k] = v
22822310 }
2283 const inst = new ModelInstance(this._relatedDef, relatedRow as any)
2311 const inst = new ModelInstance(this._relatedDef, relatedRow as any, true)
22842312 ;(inst as any).pivot = pivotExtras
22852313 return inst
22862314 })
@@ -3036,7 +3064,7 @@ class ModelQueryBuilder<
30363064 }
30373065 for (const instance of instances) {
30383066 const related = grouped.get(instance.get(pk as any)) || []
3039 instance.setRelation(relationName, related.map(r => new ModelInstance(relDef as any, r as any)))
3067 instance.setRelation(relationName, related.map(r => new ModelInstance(relDef as any, r as any, true)))
30403068 }
30413069 }
30423070 else {
@@ -3051,7 +3079,7 @@ class ModelQueryBuilder<
30513079 }
30523080 for (const instance of instances) {
30533081 const row = byFk.get(instance.get(pk as any))
3054 instance.setRelation(relationName, row ? new ModelInstance(relDef as any, row as any) : null)
3082 instance.setRelation(relationName, row ? new ModelInstance(relDef as any, row as any, true) : null)
30553083 }
30563084 }
30573085 }
@@ -3083,7 +3111,7 @@ class ModelQueryBuilder<
30833111 for (const instance of instances) {
30843112 const fkVal = (instance as any)._attributes[rel.foreignKey]
30853113 const row = byPk.get(fkVal)
3086 instance.setRelation(relationName, row ? new ModelInstance(relDef as any, row as any) : null)
3114 instance.setRelation(relationName, row ? new ModelInstance(relDef as any, row as any, true) : null)
30873115 }
30883116 }
30893117
@@ -3144,7 +3172,7 @@ class ModelQueryBuilder<
31443172 .map((p) => {
31453173 const relRow = relatedByPk.get(p[rel.pivotFkRelated!])
31463174 if (!relRow) return null
3147 const inst = new ModelInstance(relDef as any, relRow as any)
3175 const inst = new ModelInstance(relDef as any, relRow as any, true)
31483176 // Attach pivot extras under instance.pivot
31493177 const extras: Record<string, unknown> = {}
31503178 for (const [k, v] of Object.entries(p)) {
@@ -3197,7 +3225,7 @@ class ModelQueryBuilder<
31973225 const parentVal = throughToParent.get(row[rel.targetForeignKey!])
31983226 if (parentVal == null) continue
31993227 if (!byParent.has(parentVal)) byParent.set(parentVal, [])
3200 byParent.get(parentVal)!.push(new ModelInstance(relDef as any, row as any))
3228 byParent.get(parentVal)!.push(new ModelInstance(relDef as any, row as any, true))
32013229 }
32023230
32033231 for (const instance of instances) {
@@ -3211,7 +3239,7 @@ class ModelQueryBuilder<
32113239 const exec = getExecutor()
32123240 const { sql, params } = this.buildQuery()
32133241 const rows = await exec.all(sql, params)
3214 const instances = rows.map(row => new ModelInstance<TDef, TSelected>(this._definition, row as any))
3242 const instances = rows.map(row => new ModelInstance<TDef, TSelected>(this._definition, row as any, true))
32153243
32163244 // Eager load relations
32173245 if (this._withRelations.length > 0) {
@@ -3710,7 +3738,7 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
37103738 // Exclude soft-deleted rows by default (use withTrashed()/query() to override).
37113739 const sd = softDeletesEnabled(definition) ? ` AND ${SOFT_DELETE_COLUMN} IS NULL` : ''
37123740 const row = await exec.get(`SELECT * FROM ${definition.table} WHERE ${pk} = ?${sd}`, [id])
3713 return row ? new ModelInstance<TDef>(definition, row as any) as ModelRecord<TDef> : undefined
3741 return row ? new ModelInstance<TDef>(definition, row as any, true) as ModelRecord<TDef> : undefined
37143742 },
37153743
37163744 async findOrFail(id: number | string): Promise<ModelRecord<TDef>> {
@@ -3724,7 +3752,7 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
37243752 const pk = definition.primaryKey || 'id'
37253753 const sd = softDeletesEnabled(definition) ? ` AND ${SOFT_DELETE_COLUMN} IS NULL` : ''
37263754 const rows = await exec.all(`SELECT * FROM ${definition.table} WHERE ${pk} IN (${ids.map(() => '?').join(', ')})${sd}`, ids)
3727 return rows.map(row => new ModelInstance<TDef>(definition, row as any) as ModelRecord<TDef>)
3755 return rows.map(row => new ModelInstance<TDef>(definition, row as any, true) as ModelRecord<TDef>)
37283756 },
37293757
37303758 all: () => new ModelQueryBuilder<TDef>(definition).get(),
@@ -3745,7 +3773,7 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
37453773 },
37463774
37473775 async create(data: FillableAttributes<TDef>): Promise<ModelRecord<TDef>> {
3748 const instance = new ModelInstance<TDef>(definition, data as any)
3776 const instance = new ModelInstance<TDef>(definition, data as any, false)
37493777 await instance.save()
37503778 return instance as ModelRecord<TDef>
37513779 },
@@ -3843,7 +3871,7 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
38433871 getTable: () => definition.table,
38443872
38453873 make(data: Partial<Attrs> = {}): ModelInstance<TDef> {
3846 return new ModelInstance<TDef>(definition, data as any)
3874 return new ModelInstance<TDef>(definition, data as any, false)
38473875 },
38483876
38493877 latest: (column: Cols = 'created_at' as Cols) => new ModelQueryBuilder<TDef>(definition).orderByDesc(column),
packages/bun-query-builder/test/orm.save-primary-key.pg.test.tsadded+103-0
@@ -0,0 +1,103 @@
1/**
2 * `save()` INSERT vs UPDATE against a live Postgres — see
3 * orm.save-primary-key.test.ts for the bug. Postgres inserts through
4 * `RETURNING <pk>`, a different path from SQLite's rowid, so the supplied-key
5 * and string-key cases are executed here too.
6 *
7 * Runs in a subprocess: an earlier configureOrm() in the same process pins the
8 * model layer to sqlite and would mask the network-driver path.
9 */
10
11import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
12import { tmpdir } from 'node:os'
13import { join, resolve } from 'node:path'
14import process from 'node:process'
15import { describe, expect, it } from 'bun:test'
16import { PG_URL, probePostgres } from './pg'
17
18const pgAvailable = await probePostgres()
19
20describe.skipIf(!pgAvailable)('save() primary key handling against live Postgres', () => {
21 it('inserts supplied and string keys, and updates a row keyed 0', () => {
22 const srcEntry = resolve(import.meta.dir, '../src/index.ts')
23 const dir = mkdtempSync(join(tmpdir(), 'qb-savepk-'))
24 const scriptPath = join(dir, 'probe.ts')
25
26 writeFileSync(scriptPath, `
27import { SQL } from 'bun'
28import { setConfig, resetConnection, defineModel, clearModelRegistry } from ${JSON.stringify(srcEntry)}
29
30const URL = ${JSON.stringify(PG_URL)}
31const failures = []
32const check = (label, got, want) => {
33 const g = JSON.stringify(got), w = JSON.stringify(want)
34 if (g !== w) failures.push(label + ': got ' + g + ', want ' + w)
35}
36const attempt = async (label, fn) => {
37 try { return await fn() }
38 catch (e) { failures.push(label + ': threw ' + (e && e.message)); return undefined }
39}
40
41const raw = new SQL(URL)
42await raw.unsafe('DROP TABLE IF EXISTS _qb_savepk_posts')
43await raw.unsafe('DROP TABLE IF EXISTS _qb_savepk_countries')
44await raw.unsafe('CREATE TABLE _qb_savepk_posts (id serial primary key, title text, created_at timestamptz, updated_at timestamptz)')
45await raw.unsafe('CREATE TABLE _qb_savepk_countries (code text primary key, name text)')
46const posts = async () => (await raw.unsafe('SELECT id, title FROM _qb_savepk_posts ORDER BY id')).map(r => [Number(r.id), r.title])
47const countries = async () => (await raw.unsafe('SELECT code, name FROM _qb_savepk_countries ORDER BY code')).map(r => [r.code, r.name])
48
49setConfig({ dialect: 'postgres', database: { url: URL } })
50resetConnection()
51clearModelRegistry()
52const Post = defineModel({ name: 'SavepkPost', table: '_qb_savepk_posts', primaryKey: 'id',
53 attributes: { title: { type: 'string', fillable: true } } })
54const Country = defineModel({ name: 'SavepkCountry', table: '_qb_savepk_countries', primaryKey: 'code',
55 traits: { useTimestamps: false }, attributes: { name: { type: 'string', fillable: true } } })
56
57const five = await attempt('create with id', () => Post.create({ id: 5, title: 'five' }))
58check('create with id: key', five && Number(five.get('id')), 5)
59check('create with id: rows', await posts(), [[5, 'five']])
60
61const auto = await attempt('create generated', () => Post.create({ title: 'auto' }))
62check('create generated: key read back', auto && Number(auto.get('id')) > 0, true)
63
64const ph = await attempt('string key create', () => Country.create({ code: 'PH', name: 'Philippines' }))
65check('string key: key', ph && ph.get('code'), 'PH')
66check('string key: rows', await countries(), [['PH', 'Philippines']])
67if (ph) {
68 ph.set('name', 'Republic of the Philippines')
69 await attempt('string key update', () => ph.save())
70 check('string key: updated in place', await countries(), [['PH', 'Republic of the Philippines']])
71}
72
73await raw.unsafe("INSERT INTO _qb_savepk_posts (id, title) VALUES (0, 'zero')")
74const zero = await attempt('find 0', () => Post.find(0))
75if (zero) {
76 zero.set('title', 'zero, edited')
77 await attempt('save 0', () => zero.save())
78}
79const after = await posts()
80check('key 0: updated, not copied', after.filter(([, t]) => String(t).startsWith('zero')), [[0, 'zero, edited']])
81
82await raw.unsafe('DROP TABLE IF EXISTS _qb_savepk_posts')
83await raw.unsafe('DROP TABLE IF EXISTS _qb_savepk_countries')
84await raw.end()
85
86if (failures.length) {
87 console.error('FAILURES:\\n' + failures.join('\\n'))
88 process.exit(1)
89}
90console.log('OK')
91process.exit(0)
92`)
93
94 const proc = Bun.spawnSync({ cmd: ['bun', scriptPath], stdout: 'pipe', stderr: 'pipe', cwd: process.cwd(), env: { ...process.env } })
95 const dec = new TextDecoder()
96 const out = dec.decode(proc.stdout).trim()
97 const err = dec.decode(proc.stderr).trim()
98 rmSync(dir, { recursive: true, force: true })
99
100 expect(proc.exitCode, `save() primary key pg probe failed.\nstdout: ${out}\nstderr: ${err}`).toBe(0)
101 expect(out).toContain('OK')
102 }, 30_000)
103})
packages/bun-query-builder/test/orm.save-primary-key.test.tsadded+165-0
@@ -0,0 +1,165 @@
1/**
2 * `save()` decides INSERT vs UPDATE by whether the row exists, not by whether
3 * its primary key is truthy.
4 *
5 * It used to test `if (this._attributes[pk])`. That sent three cases the wrong
6 * way, all without an error:
7 *
8 * - `create({ id: 5, … })` took the UPDATE branch, matched no row, and returned
9 * an instance with id 5 for a row that was never written.
10 * - A model keyed by a string (`code`, a slug, a UUID) could not create rows at
11 * all, for the same reason.
12 * - A loaded row whose key was `0` took the INSERT branch, so editing it wrote
13 * a copy.
14 */
15
16import { Database } from 'bun:sqlite'
17import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
18import { clearModelRegistry, configureOrm, createModel } from '../src'
19
20let db: Database
21let Post: any
22let Country: any
23const hooks: string[] = []
24
25beforeEach(() => {
26 clearModelRegistry()
27 hooks.length = 0
28 db = new Database(':memory:', { create: true })
29 configureOrm({ database: db })
30 db.run('CREATE TABLE sp_posts (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT, created_at TEXT, updated_at TEXT)')
31 db.run('CREATE TABLE sp_countries (code TEXT PRIMARY KEY, name TEXT)')
32
33 Post = createModel({
34 name: 'SpPost',
35 table: 'sp_posts',
36 primaryKey: 'id',
37 attributes: { title: { type: 'string', fillable: true } },
38 } as any)
39 Country = createModel({
40 name: 'SpCountry',
41 table: 'sp_countries',
42 primaryKey: 'code',
43 traits: { useTimestamps: false },
44 attributes: { name: { type: 'string', fillable: true } },
45 hooks: {
46 beforeCreate: () => { hooks.push('beforeCreate') },
47 afterCreate: () => { hooks.push('afterCreate') },
48 beforeUpdate: () => { hooks.push('beforeUpdate') },
49 afterUpdate: () => { hooks.push('afterUpdate') },
50 },
51 } as any)
52})
53
54afterEach(() => {
55 clearModelRegistry()
56 db.close()
57})
58
59const posts = (): any[] => db.query('SELECT id, title FROM sp_posts ORDER BY id').all() as any[]
60const countries = (): any[] => db.query('SELECT code, name FROM sp_countries ORDER BY code').all() as any[]
61
62describe('create() with a key the caller supplies', () => {
63 it('writes the row under that key', async () => {
64 // Was an UPDATE that matched nothing: the instance said id 5, the table was empty.
65 const post = await Post.create({ id: 5, title: 'five' })
66 expect(post.get('id')).toBe(5)
67 expect(posts()).toEqual([{ id: 5, title: 'five' }])
68 })
69
70 it('creates rows on a model keyed by a string', async () => {
71 const ph = await Country.create({ code: 'PH', name: 'Philippines' })
72 // The key stays the string, not the SQLite rowid the driver reports.
73 expect(ph.get('code')).toBe('PH')
74 expect(countries()).toEqual([{ code: 'PH', name: 'Philippines' }])
75 expect(hooks).toEqual(['beforeCreate', 'afterCreate'])
76 })
77
78 it('a later save() on the created instance updates it', async () => {
79 const ph = await Country.create({ code: 'PH', name: 'Philippines' })
80 hooks.length = 0
81 ph.set('name', 'Republic of the Philippines')
82 await ph.save()
83 expect(countries()).toEqual([{ code: 'PH', name: 'Republic of the Philippines' }])
84 expect(hooks).toEqual(['beforeUpdate', 'afterUpdate'])
85 })
86
87 it('still reads back a generated key when none is supplied', async () => {
88 const a = await Post.create({ title: 'a' })
89 const b = await Post.create({ title: 'b' })
90 expect([a.get('id'), b.get('id')]).toEqual([1, 2])
91 b.set('title', 'b2')
92 await b.save()
93 expect(posts()).toEqual([{ id: 1, title: 'a' }, { id: 2, title: 'b2' }])
94 })
95
96 it('treats a null key as not supplied', async () => {
97 const post = await Post.create({ id: null, title: 'auto' })
98 expect(post.get('id')).toBe(1)
99 expect(posts()).toEqual([{ id: 1, title: 'auto' }])
100 })
101
102 it('leaves a guarded key to the database', async () => {
103 clearModelRegistry()
104 const Guarded = createModel({
105 name: 'SpGuarded',
106 table: 'sp_posts',
107 primaryKey: 'id',
108 attributes: { id: { type: 'number', guarded: true }, title: { type: 'string', fillable: true } },
109 } as any) as any
110 const post = await Guarded.create({ id: 99, title: 'g' })
111 expect(post.get('id')).toBe(1)
112 expect(posts()).toEqual([{ id: 1, title: 'g' }])
113 })
114})
115
116describe('save() on a loaded row', () => {
117 it('updates a row whose key is 0 instead of copying it', async () => {
118 db.run(`INSERT INTO sp_posts (id, title) VALUES (0, 'zero')`)
119 const post = await Post.find(0)
120 post.set('title', 'zero, edited')
121 await post.save()
122 expect(posts()).toEqual([{ id: 0, title: 'zero, edited' }])
123 })
124
125 it('updates a string-keyed row read by a query', async () => {
126 db.run(`INSERT INTO sp_countries (code, name) VALUES ('JP', 'Japan')`)
127 const jp = await Country.query().where('code', 'JP').first()
128 jp.set('name', 'Nippon')
129 await jp.save()
130 expect(countries()).toEqual([{ code: 'JP', name: 'Nippon' }])
131 })
132
133 it('refuses to save changes to a row read without its key', async () => {
134 // Took the INSERT branch and wrote a copy of the row.
135 await Post.create({ title: 'a' })
136 const partial = await Post.query().select('title').first()
137 partial.set('title', 'b')
138 await expect(partial.save()).rejects.toThrow(`Cannot save a model without a primary key: 'id' was not selected`)
139 expect(posts()).toEqual([{ id: 1, title: 'a' }])
140 })
141
142 it('a save with no changes on such a row is still a no-op', async () => {
143 await Post.create({ title: 'a' })
144 const partial = await Post.query().select('title').first()
145 await partial.save()
146 expect(posts()).toEqual([{ id: 1, title: 'a' }])
147 })
148})
149
150describe('instances that are not rows yet', () => {
151 it('make() then save() inserts, with or without a key', async () => {
152 await Post.make({ title: 'made' }).save()
153 await Country.make({ code: 'FR', name: 'France' }).save()
154 expect(posts()).toEqual([{ id: 1, title: 'made' }])
155 expect(countries()).toEqual([{ code: 'FR', name: 'France' }])
156 })
157
158 it('replicate() then save() inserts a new row', async () => {
159 const original = await Post.create({ title: 'orig' })
160 const copy = original.replicate()
161 copy.set('title', 'copy')
162 await copy.save()
163 expect(posts()).toEqual([{ id: 1, title: 'orig' }, { id: 2, title: 'copy' }])
164 })
165})