ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(orm): make createModel models findable, so relations read the declared table

#1104
Merged glennmichael123 wants to merge fix/1093-relation-declared-table into main
3 files +244 -3
packages/bun-query-builder/src/model.tsmodified+6-1
@@ -41,7 +41,7 @@
4141 */
4242
4343import { createBrowserModel, isBrowser, type BrowserModelDefinition } from './browser'
44import { createModel, type ModelDefinition as OrmModelDefinition } from './orm'
44import { clearLocalModels, createModel, type ModelDefinition as OrmModelDefinition } from './orm'
4545
4646// Re-export the browser model types for convenience
4747export type { BrowserModelDefinition as ModelDefinition }
@@ -119,9 +119,14 @@ export function hasModel(name: string): boolean {
119119
120120/**
121121 * Clear all registered models. Primarily useful for testing.
122 *
123 * Also drops the models `createModel` registered internally, and the memoized
124 * relation resolutions derived from them. Leaving either behind lets a stale
125 * resolution from one test file decide a relation's table in the next.
122126 */
123127export function clearModelRegistry(): void {
124128 modelRegistry.clear()
129 clearLocalModels()
125130}
126131
127132/**
packages/bun-query-builder/src/orm.tsmodified+50-2
@@ -100,6 +100,47 @@ function assertValidOrderByColumn(name: unknown, context: string): asserts name
100100// Lazy reference to model registry to avoid circular dependency
101101// eslint-disable-next-line pickier/no-unused-vars
102102let _getModel: ((name: string) => any) | null = null
103
104/**
105 * Models built by `createModel`, keyed by definition name.
106 *
107 * `defineModel` publishes into model.ts's global registry; `createModel` never
108 * did. So every lookup below missed a `createModel` model, and each caller fell
109 * back to its own guess:
110 *
111 * - relation resolution guessed the table from the MODEL NAME, ignoring a
112 * declared `table`. Where no table existed at the guessed name that was a
113 * "no such table" crash; where one did — a plausible accident, since the
114 * guess is the conventional name — the relation silently read the WRONG
115 * TABLE and returned wrong rows.
116 * - eager-load hydration fell back to the PARENT's definition, so the related
117 * rows were shaped by the wrong model. An attribute marked `hidden: true` on
118 * the related model was serialized into a `with()` payload even though a
119 * direct query on that same model redacts it.
120 *
121 * See stacksjs/bun-query-builder#1093.
122 *
123 * Kept module-local rather than written into the public registry so that
124 * `getAllModels()` / `getModelRegistry()` / `hasModel()` keep reporting exactly
125 * what `defineModel` put there, and so two models sharing a name cannot clobber
126 * each other's public entry.
127 */
128const localModels = new Map<string, any>()
129
130function registerLocalModel(name: string, model: any): void {
131 localModels.set(name, model)
132 // Relation resolution is memoized per model+relation pair. A resolution
133 // computed before this model was registered guessed its table, so the memo
134 // has to go or registration order silently decides the answer.
135 relationCache.clear()
136}
137
138/** Drops the `createModel` models. Paired with `clearModelRegistry()`. */
139export function clearLocalModels(): void {
140 localModels.clear()
141 relationCache.clear()
142}
143
103144function getModelFromRegistry(name: string): any {
104145 if (!_getModel) {
105146 try {
@@ -109,7 +150,9 @@ function getModelFromRegistry(name: string): any {
109150 _getModel = () => undefined
110151 }
111152 }
112 return _getModel!(name)
153 // The public registry wins, so a `defineModel` facade still shadows the plain
154 // model it wraps.
155 return _getModel!(name) ?? localModels.get(name)
113156}
114157
115158// Binding helper type for SQL queries
@@ -3439,7 +3482,12 @@ function createModelInternal<const TDef extends ModelDefinition>(definition: TDe
34393482
34403483/** Create a model class from a definition with full type inference. */
34413484export function createModel<const TDef extends ModelDefinition>(definition: TDef): ModelStatic<TDef> {
3442 return createModelInternal(definition)
3485 const model = createModelInternal(definition)
3486 // Make the model findable by name, so a relation pointing at it reads its
3487 // declared `table` instead of guessing one. See #1093 and localModels above.
3488 if (definition?.name)
3489 registerLocalModel(definition.name, model)
3490 return model
34433491}
34443492
34453493export async function createTableFromModel(definition: ModelDefinition): Promise<void> {
packages/bun-query-builder/test/orm.relation-declared-table.test.tsadded+188-0
@@ -0,0 +1,188 @@
1/**
2 * Relations must read the related model's declared `table`.
3 * stacksjs/bun-query-builder#1093.
4 *
5 * `defineModel` publishes into the global model registry; `createModel` did
6 * not. Every relation lookup went through that registry, so a `createModel`
7 * model was invisible and each caller fell back to its own guess.
8 *
9 * Two distinct failures came out of that one gap, and the milder one is the
10 * only one the issue reported:
11 *
12 * 1. The table was guessed from the MODEL NAME, ignoring `table`. With no
13 * table at the guessed name that is a loud "no such table" crash. With one
14 * there — and the guess is the *conventional* name, so this is a plausible
15 * accident — the relation silently reads the wrong table and returns wrong
16 * rows.
17 * 2. Eager-load hydration fell back to the PARENT's definition, so related
18 * rows were shaped by the wrong model. An attribute marked `hidden: true`
19 * on the related model was serialized into a `with()` payload even though a
20 * direct query on that same model redacts it.
21 *
22 * The second is worse in one specific way: it does not require an unusual table
23 * name. It hits models whose naming is entirely conventional, so nothing ever
24 * warns those users that a relation is being resolved by guesswork.
25 */
26
27import { Database } from 'bun:sqlite'
28import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
29import { clearModelRegistry, configureOrm, createModel, getAllModels, hasModel } from '../src'
30
31let db: Database
32
33beforeEach(() => {
34 clearModelRegistry()
35 db = new Database(':memory:', { create: true })
36 configureOrm({ database: db })
37})
38
39afterEach(() => {
40 clearModelRegistry()
41})
42
43describe('relations resolve the declared table (#1093)', () => {
44 it('reads the declared table, not a table sitting at the guessed name', async () => {
45 db.run('CREATE TABLE wg_owners (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)')
46 db.run('CREATE TABLE wg_custom_widgets (id INTEGER PRIMARY KEY AUTOINCREMENT, owner_id INTEGER, label TEXT)')
47 // The decoy: a real table at the name the resolver would guess from
48 // `Widget`. Without the fix the relation reads THIS one and returns rows
49 // that look perfectly valid.
50 db.run('CREATE TABLE widgets (id INTEGER PRIMARY KEY AUTOINCREMENT, owner_id INTEGER, label TEXT)')
51 db.run(`INSERT INTO wg_owners (id, name) VALUES (1, 'owner-1')`)
52 db.run(`INSERT INTO wg_custom_widgets (id, owner_id, label) VALUES (1, 1, 'REAL')`)
53 db.run(`INSERT INTO widgets (id, owner_id, label) VALUES (1, 1, 'DECOY')`)
54
55 createModel({
56 name: 'Widget',
57 table: 'wg_custom_widgets',
58 primaryKey: 'id',
59 autoIncrement: true,
60 attributes: {
61 owner_id: { type: 'integer', fillable: true },
62 label: { type: 'string', fillable: true },
63 },
64 } as any)
65
66 const Owner = createModel({
67 name: 'Owner',
68 table: 'wg_owners',
69 primaryKey: 'id',
70 autoIncrement: true,
71 attributes: { name: { type: 'string', fillable: true } },
72 hasMany: ['Widget'],
73 } as any)
74
75 const rows: any = await (Owner as any).query().with('widget').get()
76 const row = typeof rows?.[0]?.toJSON === 'function' ? rows[0].toJSON() : rows?.[0]
77 const related = row?.widget
78
79 expect(Array.isArray(related)).toBe(true)
80 expect(related.map((r: any) => r.label)).toEqual(['REAL'])
81 })
82
83 it('hides the RELATED model\'s hidden attributes in a with() payload', async () => {
84 // Note the table name here IS the conventional guess for `Hidsecret`, so
85 // the table-resolution half of #1093 cannot fire. This isolates hydration:
86 // the only thing that can leak the column is being shaped by the parent's
87 // definition instead of the related model's.
88 db.run('CREATE TABLE hid_owners (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)')
89 db.run('CREATE TABLE hidsecrets (id INTEGER PRIMARY KEY AUTOINCREMENT, hidowner_id INTEGER, label TEXT, token TEXT)')
90 db.run(`INSERT INTO hid_owners (id, name) VALUES (1, 'owner-1')`)
91 db.run(`INSERT INTO hidsecrets (id, hidowner_id, label, token) VALUES (1, 1, 'ok', 'SUPER-SECRET')`)
92
93 const Secret = createModel({
94 name: 'Hidsecret',
95 table: 'hidsecrets',
96 primaryKey: 'id',
97 autoIncrement: true,
98 attributes: {
99 hidowner_id: { type: 'integer', fillable: true },
100 label: { type: 'string', fillable: true },
101 token: { type: 'string', fillable: true, hidden: true },
102 },
103 } as any)
104
105 const Owner = createModel({
106 name: 'Hidowner',
107 table: 'hid_owners',
108 primaryKey: 'id',
109 autoIncrement: true,
110 attributes: { name: { type: 'string', fillable: true } },
111 hasMany: ['Hidsecret'],
112 } as any)
113
114 // Baseline: a direct query on the related model redacts it.
115 const direct: any = await (Secret as any).query().get()
116 const directRow = typeof direct?.[0]?.toJSON === 'function' ? direct[0].toJSON() : direct?.[0]
117 expect(directRow).not.toHaveProperty('token')
118
119 // The eager-loaded payload must agree with it.
120 const rows: any = await (Owner as any).query().with('hidsecret').get()
121 const row = typeof rows?.[0]?.toJSON === 'function' ? rows[0].toJSON() : rows?.[0]
122 const related = row?.hidsecret
123
124 expect(Array.isArray(related)).toBe(true)
125 expect(related[0]).not.toHaveProperty('token')
126 expect(JSON.stringify(related)).not.toContain('SUPER-SECRET')
127 })
128})
129
130describe('createModel registration is internal (#1093)', () => {
131 it('does not add createModel models to the public registry', () => {
132 createModel({
133 name: 'PrivateOnly',
134 table: 'private_only',
135 primaryKey: 'id',
136 autoIncrement: true,
137 attributes: { name: { type: 'string', fillable: true } },
138 } as any)
139
140 // The relation resolver can find it, but the public surface is unchanged —
141 // `getAllModels()`/`hasModel()` keep reporting exactly what `defineModel`
142 // put there, so nothing downstream starts seeing new entries.
143 expect(hasModel('PrivateOnly')).toBe(false)
144 expect(getAllModels().some((m: any) => m?.getName?.() === 'PrivateOnly')).toBe(false)
145 })
146
147 it('clearModelRegistry() drops them, so resolutions do not leak across tests', async () => {
148 db.run('CREATE TABLE lk_owners (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)')
149 db.run('CREATE TABLE lk_declared (id INTEGER PRIMARY KEY AUTOINCREMENT, lkowner_id INTEGER)')
150 db.run(`INSERT INTO lk_owners (id, name) VALUES (1, 'a')`)
151
152 createModel({
153 name: 'Lkchild',
154 table: 'lk_declared',
155 primaryKey: 'id',
156 autoIncrement: true,
157 attributes: { lkowner_id: { type: 'integer', fillable: true } },
158 } as any)
159
160 const Owner = createModel({
161 name: 'Lkowner',
162 table: 'lk_owners',
163 primaryKey: 'id',
164 autoIncrement: true,
165 attributes: { name: { type: 'string', fillable: true } },
166 hasMany: ['Lkchild'],
167 } as any)
168
169 // Warms the memoized relation resolution.
170 await (Owner as any).query().with('lkchild').get()
171
172 clearModelRegistry()
173
174 // With the registration and the memo both dropped, the child is unknown
175 // again and the resolver falls back to the guessed table, which does not
176 // exist. A silent success here would mean a stale resolution survived.
177 const Owner2 = createModel({
178 name: 'Lkowner',
179 table: 'lk_owners',
180 primaryKey: 'id',
181 autoIncrement: true,
182 attributes: { name: { type: 'string', fillable: true } },
183 hasMany: ['Lkchild'],
184 } as any)
185
186 await expect((Owner2 as any).query().with('lkchild').get()).rejects.toThrow(/lkchilds/)
187 })
188})