ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(orm): give a declared column type the storage type it asks for

#1106
Merged glennmichael123 wants to merge fix/1094-integer-column-type into main
5 files +322 -33
packages/bun-query-builder/src/column-types.tsadded+98-0
@@ -0,0 +1,98 @@
1/**
2 * The one place a declared attribute type becomes a storage type.
3 *
4 * Two paths build SQLite columns from a model and they disagreed:
5 *
6 * - the migration path (`SQLiteDriver.getColumnType`) understands the full
7 * canonical set — integer, bigint, float, double, decimal, date, json, …
8 * - `createTableFromModel` in orm.ts understood exactly two, `number` and
9 * `boolean`, and sent everything else to the `TEXT` default.
10 *
11 * So `type: 'integer'` — the spelling used in this library's own docs and model
12 * examples — produced a TEXT column. Integers then stored and read back as
13 * strings, and under SQLite's text collation `'10' < '9'`, so `ORDER BY` and
14 * range filters on an integer attribute returned the wrong rows with no error
15 * anywhere. See stacksjs/bun-query-builder#1094.
16 *
17 * A shared mapping is the fix rather than a second copy of the switch, because
18 * a second copy is what produced the divergence in the first place.
19 *
20 * This module is a leaf: it imports only types, so either path can use it
21 * without a runtime cycle.
22 */
23
24import type { NormalizedColumnType } from './migrations'
25
26/**
27 * Whether a canonical type stores a number.
28 *
29 * Used to decide when the `*_id` naming heuristic may coerce a column to
30 * INTEGER. Declared text types must never be coerced by a name heuristic —
31 * external ids are frequently strings (tickers, wallet addresses, hashes).
32 */
33export function isNumericPlanType(type: string | undefined): boolean {
34 return type === 'integer' || type === 'bigint' || type === 'float' || type === 'double' || type === 'decimal'
35}
36
37/**
38 * Map a declared attribute type onto the canonical set.
39 *
40 * Accepts the spellings the model layer and the validation-rule reader already
41 * accept, so `type: 'int'`, `'bool'` and `'timestamp'` land where their long
42 * forms do. Returns `undefined` for anything unrecognised, which callers treat
43 * as "no opinion" rather than guessing.
44 */
45export function normalizeAttributeType(raw: unknown): NormalizedColumnType | undefined {
46 if (typeof raw !== 'string')
47 return undefined
48
49 switch (raw.toLowerCase()) {
50 case 'string': return 'string'
51 case 'text': return 'text'
52 case 'integer':
53 case 'int': return 'integer'
54 // `number` is an integer here, deliberately, and for the reason spelled out
55 // at the rule-based normalizer in migrations.ts: it is what nearly every
56 // count, coordinate, port and foreign key is declared as. Reading it as
57 // `decimal` was tried and reverted (cdd35a3, 3ba6dd2) because it reshaped
58 // every such column. Both paths have to agree on this or a model migrates
59 // into one column type and is created as another.
60 case 'number': return 'integer'
61 case 'bigint': return 'bigint'
62 case 'float': return 'float'
63 case 'double': return 'double'
64 case 'decimal': return 'decimal'
65 case 'boolean':
66 case 'bool': return 'boolean'
67 case 'date': return 'date'
68 case 'datetime':
69 case 'timestamp': return 'datetime'
70 case 'timestamptz':
71 case 'timestamp_tz': return 'timestamptz'
72 case 'json': return 'json'
73 case 'enum': return 'enum'
74 default: return undefined
75 }
76}
77
78/**
79 * The SQLite storage class for a canonical type.
80 *
81 * `enum` is TEXT here; the migration driver additionally emits a CHECK
82 * constraint, which needs the column name and a quoter and so stays with the
83 * driver.
84 */
85export function sqliteAffinityFor(type: NormalizedColumnType | undefined): 'TEXT' | 'INTEGER' | 'REAL' {
86 switch (type) {
87 case 'boolean': // SQLite stores booleans as 0/1
88 case 'integer':
89 case 'bigint':
90 return 'INTEGER'
91 case 'float':
92 case 'double':
93 case 'decimal':
94 return 'REAL'
95 default:
96 return 'TEXT'
97 }
98}
packages/bun-query-builder/src/drivers/sqlite.tsmodified+17-23
@@ -1,15 +1,18 @@
11import type { ColumnPlan, IndexPlan, RebuildTableSpec, TablePlan } from '../migrations'
22import type { DateBucketGrain } from './postgres'
3import { isNumericPlanType, sqliteAffinityFor } from '../column-types'
34import { qualifiedIndexName } from './index-name'
45
56/**
67 * Whether a plan type belongs to the numeric family. Used by the `_id`
78 * safety net (numeric ids must store as INTEGER on SQLite) — non-numeric
89 * declared types must never be coerced by the name heuristic.
10 *
11 * Defined in column-types.ts now, so the ORM's create-table path applies the
12 * same rule from the same source. Re-exported because this is where callers
13 * already import it from.
914 */
10export function isNumericPlanType(type: string | undefined): boolean {
11 return type === 'integer' || type === 'bigint' || type === 'float' || type === 'double' || type === 'decimal'
12}
15export { isNumericPlanType }
1316
1417export interface DialectDriver {
1518 createEnumType: (enumTypeName: string, values: string[]) => string
@@ -53,27 +56,18 @@ export class SQLiteDriver implements DialectDriver {
5356 return 'INTEGER'
5457 }
5558
56 switch (column.type) {
57 case 'string': return 'TEXT'
58 case 'text': return 'TEXT'
59 case 'boolean': return 'INTEGER' // SQLite uses INTEGER for booleans (0/1)
60 case 'integer': return 'INTEGER'
61 case 'bigint': return 'INTEGER'
62 case 'float': return 'REAL'
63 case 'double': return 'REAL'
64 case 'decimal': return 'REAL'
65 case 'date': return 'TEXT'
66 case 'datetime': return 'TEXT'
67 case 'timestamptz': return 'TEXT'
68 case 'json': return 'TEXT'
69 case 'enum':
70 if (column.enumValues && column.enumValues.length > 0) {
71 const enumValues = column.enumValues.map(v => `'${v.replace(/'/g, '\'\'')}'`).join(', ')
72 return `TEXT CHECK (${this.quoteIdentifier(column.name)} IN (${enumValues}))`
73 }
74 return 'TEXT'
75 default: return 'TEXT'
59 // An enum is TEXT plus a CHECK, which needs the column name and a quoter,
60 // so it stays here. Every other type comes from the shared mapping — this
61 // path and the ORM's create-table path drifting apart is #1094.
62 if (column.type === 'enum') {
63 if (column.enumValues && column.enumValues.length > 0) {
64 const enumValues = column.enumValues.map(v => `'${v.replace(/'/g, '\'\'')}'`).join(', ')
65 return `TEXT CHECK (${this.quoteIdentifier(column.name)} IN (${enumValues}))`
66 }
67 return 'TEXT'
7668 }
69
70 return sqliteAffinityFor(column.type)
7771 }
7872
7973 private getPrimaryKeyType(column: ColumnPlan): string {
packages/bun-query-builder/src/orm.tsmodified+15-5
@@ -28,6 +28,7 @@ import { Database, type SQLQueryBindings } from 'bun:sqlite'
2828import type { FactoryFaker } from './faker-compat'
2929import type { SupportedDialect } from './types'
3030import type { RelationCardinality } from './type-inference'
31import { isNumericPlanType, normalizeAttributeType, sqliteAffinityFor } from './column-types'
3132import { config, isMysqlLike } from './config'
3233import type { DriverConnection } from './db'
3334import { getOrCreateBunSql } from './db'
@@ -3465,11 +3466,20 @@ export async function createTableFromModel(definition: ModelDefinition): Promise
34653466 const name = toSnakeCase(attrName)
34663467 if (emitted.has(name)) continue
34673468 emitted.add(name)
3468 let colType = 'TEXT'
3469 if (attr.type === 'number') colType = 'INTEGER'
3470 else if (attr.type === 'boolean') colType = 'INTEGER'
3471 // Safety net: foreign key columns must always be INTEGER
3472 if (name.endsWith('_id')) colType = 'INTEGER'
3469 // Same mapping the migration path uses. This understood only `number` and
3470 // `boolean` and sent everything else to TEXT, so `type: 'integer'` — the
3471 // spelling in this library's own docs — created a TEXT column and integers
3472 // read back as strings. See #1094 and column-types.ts.
3473 const declaredType = normalizeAttributeType(attr.type)
3474 let colType: string = sqliteAffinityFor(declaredType)
3475 // Safety net: a numeric foreign-key column stores as INTEGER, so float
3476 // storage cannot corrupt an id (11.0 for 11). Conditional on the declared
3477 // type, matching SQLiteDriver.getColumnType: external ids are frequently
3478 // strings (tickers, wallet addresses, hashes) and a declared text type has
3479 // to win over a name heuristic. An attribute that declares no type at all
3480 // keeps the old INTEGER default rather than silently changing shape.
3481 if (name.endsWith('_id') && (declaredType === undefined || isNumericPlanType(declaredType)))
3482 colType = 'INTEGER'
34733483 let colDef = `${name} ${colType}`
34743484 if (attr.unique) colDef += ' UNIQUE'
34753485 // Inline FK constraints for SQLite CREATE TABLE
packages/bun-query-builder/test/orm.create-table-column-types.test.tsadded+189-0
@@ -0,0 +1,189 @@
1/**
2 * `createTableFromModel` must give a declared attribute type the same storage
3 * type the migration path would. stacksjs/bun-query-builder#1094.
4 *
5 * It understood exactly two spellings — `number` and `boolean` — and sent
6 * everything else to the `TEXT` default. So `type: 'integer'`, the spelling
7 * used in this library's own docs and model examples, produced a TEXT column.
8 *
9 * Nothing errors when that happens. Integers store and read back as strings,
10 * and under SQLite's text collation `'10' < '9'`, so `ORDER BY` and range
11 * filters on an integer attribute return the wrong rows quietly. `WHERE n > 9`
12 * returned zero rows where one was correct.
13 *
14 * The cross-path test below is the one that keeps this fixed: the bug was not
15 * a wrong entry in a table, it was two tables that disagreed, so asserting the
16 * ORM's output alone would let them drift apart again.
17 */
18
19import type { ColumnPlan } from '../src/migrations'
20import { Database } from 'bun:sqlite'
21import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
22import { clearModelRegistry, configureOrm, createModel, createTableFromModel } from '../src'
23import { normalizeAttributeType, sqliteAffinityFor } from '../src/column-types'
24import { SQLiteDriver } from '../src/drivers/sqlite'
25
26let db: Database
27
28beforeEach(() => {
29 clearModelRegistry()
30 db = new Database(':memory:', { create: true })
31 configureOrm({ database: db })
32})
33
34afterEach(() => {
35 clearModelRegistry()
36})
37
38/** Column name -> declared SQLite type, from the table the ORM actually built. */
39async function columnTypesFor(attributes: Record<string, any>): Promise<Record<string, string>> {
40 const M = createModel({
41 name: 'Ctrow',
42 table: 'ct_rows',
43 primaryKey: 'id',
44 autoIncrement: true,
45 attributes,
46 } as any)
47
48 await createTableFromModel((M as any).getDefinition())
49
50 const out: Record<string, string> = {}
51 for (const c of db.query('PRAGMA table_info(ct_rows)').all() as any[]) {
52 if (c.name !== 'id')
53 out[c.name] = String(c.type)
54 }
55 return out
56}
57
58describe('createTableFromModel column types (#1094)', () => {
59 it('maps every declared type to its storage type', async () => {
60 const types = await columnTypesFor({
61 a_string: { type: 'string', fillable: true },
62 a_text: { type: 'text', fillable: true },
63 a_integer: { type: 'integer', fillable: true },
64 a_int: { type: 'int', fillable: true },
65 a_number: { type: 'number', fillable: true },
66 a_bigint: { type: 'bigint', fillable: true },
67 a_float: { type: 'float', fillable: true },
68 a_double: { type: 'double', fillable: true },
69 a_decimal: { type: 'decimal', fillable: true },
70 a_boolean: { type: 'boolean', fillable: true },
71 a_bool: { type: 'bool', fillable: true },
72 a_date: { type: 'date', fillable: true },
73 a_datetime: { type: 'datetime', fillable: true },
74 a_json: { type: 'json', fillable: true },
75 })
76
77 expect(types).toEqual({
78 a_string: 'TEXT',
79 a_text: 'TEXT',
80 a_integer: 'INTEGER',
81 a_int: 'INTEGER',
82 // `number` is an integer by long-standing decision — see column-types.ts.
83 a_number: 'INTEGER',
84 a_bigint: 'INTEGER',
85 a_float: 'REAL',
86 a_double: 'REAL',
87 a_decimal: 'REAL',
88 a_boolean: 'INTEGER',
89 a_bool: 'INTEGER',
90 a_date: 'TEXT',
91 a_datetime: 'TEXT',
92 a_json: 'TEXT',
93 })
94 })
95
96 it('agrees with the migration driver for every canonical type', () => {
97 // The anti-drift check. #1094 was two mappings disagreeing, so pinning only
98 // one of them would let the next divergence through.
99 const driver = new SQLiteDriver()
100 const canonical = [
101 'string',
102 'text',
103 'boolean',
104 'integer',
105 'bigint',
106 'float',
107 'double',
108 'decimal',
109 'date',
110 'datetime',
111 'timestamptz',
112 'json',
113 ] as const
114
115 for (const type of canonical) {
116 const plan: ColumnPlan = {
117 name: 'c',
118 type,
119 isPrimaryKey: false,
120 isUnique: false,
121 isNullable: true,
122 hasDefault: false,
123 }
124 // `addColumn` renders `"c" <TYPE> ...`; take the type it chose.
125 const rendered = driver.addColumn('t', plan)
126 const driverType = rendered.match(/"c"\s+(TEXT|INTEGER|REAL)/)?.[1]
127
128 expect(driverType, `driver type for ${type}`).toBeDefined()
129 expect(sqliteAffinityFor(type), `shared mapping disagrees with driver for '${type}'`).toBe(driverType as any)
130 }
131 })
132
133 it('stores an integer attribute as a number, so comparisons are numeric', async () => {
134 const M = createModel({
135 name: 'Ctnum',
136 table: 'ct_nums',
137 primaryKey: 'id',
138 autoIncrement: true,
139 attributes: { name: { type: 'string', fillable: true }, n: { type: 'integer', fillable: true } },
140 } as any)
141 await createTableFromModel((M as any).getDefinition())
142
143 await (M as any).create({ name: 'nine', n: 9 })
144 await (M as any).create({ name: 'ten', n: 10 })
145
146 // Under TEXT affinity this returned [] — '10' sorts before '9'.
147 const gt = db.query('SELECT name FROM ct_nums WHERE n > 9').all() as any[]
148 expect(gt.map(r => r.name)).toEqual(['ten'])
149
150 const [{ t }] = db.query('SELECT typeof(n) t FROM ct_nums LIMIT 1').all() as any[]
151 expect(t).toBe('integer')
152 })
153
154 describe('the *_id naming heuristic', () => {
155 it('keeps a numeric id as INTEGER', async () => {
156 const types = await columnTypesFor({ owner_id: { type: 'integer', fillable: true } })
157 expect(types.owner_id).toBe('INTEGER')
158 })
159
160 it('does not coerce an id that is declared text', async () => {
161 // External ids are frequently strings — tickers, wallet addresses,
162 // hashes. A declared type has to beat a name heuristic, and the
163 // migration path already worked this way.
164 const types = await columnTypesFor({ ticker_id: { type: 'string', fillable: true } })
165 expect(types.ticker_id).toBe('TEXT')
166 })
167
168 it('still defaults an id with no declared type to INTEGER', async () => {
169 const types = await columnTypesFor({ legacy_id: { fillable: true } })
170 expect(types.legacy_id).toBe('INTEGER')
171 })
172 })
173})
174
175describe('normalizeAttributeType (#1094)', () => {
176 it('accepts the short spellings alongside the long ones', () => {
177 expect(normalizeAttributeType('int')).toBe('integer')
178 expect(normalizeAttributeType('bool')).toBe('boolean')
179 expect(normalizeAttributeType('timestamp')).toBe('datetime')
180 expect(normalizeAttributeType('INTEGER')).toBe('integer')
181 })
182
183 it('returns undefined for anything it does not recognise', () => {
184 // Callers treat this as "no opinion" and fall back, rather than guessing.
185 expect(normalizeAttributeType('wat')).toBeUndefined()
186 expect(normalizeAttributeType(undefined)).toBeUndefined()
187 expect(normalizeAttributeType(42)).toBeUndefined()
188 })
189})
packages/bun-query-builder/test/orm.or-where-precedence.test.tsmodified+3-5
@@ -67,11 +67,9 @@ describe('ORM orWhere grouping (#1083)', () => {
6767
6868 // Was 15: every `alter_*` row came back regardless of batch.
6969 expect(rows.length).toBe(5)
70 // Number(): `type: 'integer'` currently generates a TEXT column in
71 // createTableFromModel, so this reads back as '1'. Unrelated to #1083 —
72 // coerced rather than asserted on, so this test fails for its own reason
73 // or not at all.
74 expect(rows.every(r => Number(r.get('batch')) === 1)).toBe(true)
70 // `type: 'integer'` produces an INTEGER column since #1094, so this reads
71 // back as a number and can be asserted directly rather than coerced.
72 expect(rows.every(r => r.get('batch') === 1)).toBe(true)
7573 })
7674
7775 it('brackets a leading OR run against the AND that follows', async () => {