ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix: an unbounded string column is text on Postgres, not varchar(255)

#1191
Merged glennmichael123 wants to merge fix/postgres-unbounded-string-is-text into main
5 files +89 -15
packages/bun-query-builder/src/drivers/postgres.tsmodified+8-1
@@ -104,7 +104,14 @@ export class PostgresDriver implements DialectDriver {
104104
105105 private getColumnType(column: ColumnPlan): string {
106106 switch (column.type) {
107 case 'string': return `varchar(${column.maxLength ?? 255})`
107 // An unbounded `schema.string()` is `text`, not `varchar(255)`. The 255
108 // default is a MySQL idiom: there, TEXT cannot be indexed without a
109 // prefix length, so a bounded varchar is worth defaulting to. Postgres
110 // stores `text` and `varchar(n)` identically and indexes both, so the
111 // default bought nothing and cost correctness - a column the model never
112 // bounded rejected its own writes with "value too long for type
113 // character varying(255)". A declared `.max(n)` is still honoured.
114 case 'string': return column.maxLength === undefined ? 'text' : `varchar(${column.maxLength})`
108115 case 'text': return 'text'
109116 case 'boolean': return 'boolean'
110117 case 'integer': return 'integer'
packages/bun-query-builder/src/migrations.tsmodified+26-4
@@ -1615,8 +1615,14 @@ export function canonicalStorageType(col: ColumnPlan, dialect: SupportedDialect)
16151615 // invisible: both are `string`, the diff saw nothing, and the column stayed
16161616 // varchar(50) while the validator accepted 100 characters - every longer
16171617 // write then failed with "value too long for type character varying(50)".
1618 // Must agree with PostgresDriver.getColumnType: an unbounded string is
1619 // `text` there, and a live `text` column introspects back as `type: 'text'`.
1620 // Reporting `varchar(255)` for the model side would diff against that
1621 // forever, re-ALTERing an unchanged column on every reconciliation.
16181622 if (col.type === 'string')
1619 return `varchar(${col.maxLength ?? 255})`
1623 return dialect === 'postgres' && col.maxLength === undefined
1624 ? 'text'
1625 : `varchar(${col.maxLength ?? 255})`
16201626 // Likewise a decimal's precision and scale, which used to be a hard-coded
16211627 // (10,2) nothing could change.
16221628 if (col.type === 'decimal')
@@ -1624,6 +1630,22 @@ export function canonicalStorageType(col: ColumnPlan, dialect: SupportedDialect)
16241630 return col.type
16251631}
16261632
1633/**
1634 * How many characters a string column accepts.
1635 *
1636 * "No bound declared" is not one number across dialects: on Postgres it is
1637 * `text`, which has no limit, and on MySQL it is `varchar(255)`, because
1638 * MySQL TEXT cannot be indexed without a prefix length. Folding both into 255
1639 * made dropping a bound on Postgres look like a narrowing to 255 when it is
1640 * really a widening to `text`, which would have flagged a safe ALTER
1641 * destructive. A caller that passes no dialect keeps the single old default.
1642 */
1643function stringCharacterBound(col: ColumnPlan, dialect?: SupportedDialect): number {
1644 if (col.maxLength !== undefined)
1645 return col.maxLength
1646 return dialect === 'postgres' ? Number.POSITIVE_INFINITY : 255
1647}
1648
16271649/**
16281650 * Whether a change keeps the physical type but tightens a bound on it, so
16291651 * existing values may no longer fit (a varchar(100) becoming varchar(50), a
@@ -1631,9 +1653,9 @@ export function canonicalStorageType(col: ColumnPlan, dialect: SupportedDialect)
16311653 * Postgres and strict MySQL refuse the ALTER when a row is too long; MySQL in
16321654 * a permissive mode truncates. Either way it is not a free change.
16331655 */
1634export function isNarrowingChange(from: ColumnPlan, to: ColumnPlan): boolean {
1656export function isNarrowingChange(from: ColumnPlan, to: ColumnPlan, dialect?: SupportedDialect): boolean {
16351657 if (from.type === 'string' && to.type === 'string')
1636 return (to.maxLength ?? 255) < (from.maxLength ?? 255)
1658 return stringCharacterBound(to, dialect) < stringCharacterBound(from, dialect)
16371659 if (from.type === 'decimal' && to.type === 'decimal') {
16381660 // Lossless only when neither the fractional digits nor the integer
16391661 // digits (precision - scale) shrink.
@@ -2425,7 +2447,7 @@ export function generateDiffOperations(previous: MigrationPlan | undefined, next
24252447 kind: 'modify_column',
24262448 table: curr.table,
24272449 column: colName,
2428 destructive: (prevCol.type !== currCol.type && !isLosslessTypeChange(prevCol.type, currCol.type)) || isNarrowingChange(prevCol, currCol),
2450 destructive: (prevCol.type !== currCol.type && !isLosslessTypeChange(prevCol.type, currCol.type)) || isNarrowingChange(prevCol, currCol, dialect),
24292451 sql: modifyColumnStatement,
24302452 })
24312453 info(`-- Detected column change: ${curr.table}.${colName} (${prevCol.type} -> ${currCol.type})`)
packages/bun-query-builder/test/migrations.attributes.test.tsmodified+8-4
@@ -1627,8 +1627,10 @@ describe('migrations - complex model attributes', () => {
16271627
16281628 describe('dialect-specific SQL generation', () => {
16291629 it('generates correct PostgreSQL types', () => {
1630 // Note: validation rule { name: 'text' } maps to 'string' type (varchar(255))
1631 // The 'text' column type is only inferred from long default strings (>255 chars)
1630 // An unbounded string is `text` on Postgres; only a declared `.max(n)`
1631 // produces `varchar(n)`. This test used to pin varchar(255) for `str`,
1632 // which was the defect: a column the model never bounded rejected its
1633 // own writes at 256 characters.
16321634 const longString = 'x'.repeat(300)
16331635 const models = defineModels({
16341636 AllTypes: {
@@ -1637,6 +1639,7 @@ describe('migrations - complex model attributes', () => {
16371639 attributes: {
16381640 id: { validation: { rule: {} } },
16391641 str: { validation: { rule: { name: 'string' } } },
1642 code: { validation: { rule: { name: 'string', rules: [{ name: 'max', params: { max: 40 } }] } } },
16401643 txt: { default: longString, validation: { rule: {} } }, // Use long default to get text type
16411644 int_col: { validation: { rule: { name: 'integer' } } },
16421645 big: { validation: { rule: { name: 'bigint' } } },
@@ -1654,8 +1657,9 @@ describe('migrations - complex model attributes', () => {
16541657 const plan = buildMigrationPlan(models as any, { dialect: 'postgres' })
16551658 const sql = generateSql(plan).join('\n').toLowerCase()
16561659
1657 expect(sql).toContain('varchar')
1658 expect(sql).toContain('text') // From long default string
1660 expect(sql).toContain('"str" text') // unbounded: no varchar(255) default
1661 expect(sql).toContain('"code" varchar(40)') // bounded: the declared max
1662 expect(sql).toContain('"txt" text') // From long default string
16591663 expect(sql).toContain('integer')
16601664 expect(sql).toContain('bigint')
16611665 expect(sql).toContain('timestamp')
packages/bun-query-builder/test/migrations.postgres-varchar-length.test.tsmodified+35-3
@@ -25,7 +25,22 @@ const handle = (plan: MigrationPlan): ColumnPlan => plan.tables[0].columns.find(
2525describe('postgres varchar length', () => {
2626 it('carries the bound into the canonical storage type', () => {
2727 expect(canonicalStorageType(handle(modelPlan(50)), 'postgres')).toBe('varchar(50)')
28 expect(canonicalStorageType(handle(modelPlan()), 'postgres')).toBe('varchar(255)')
28 })
29
30 // An unbounded column is `text` here, so the model side has to say `text`
31 // too. A live `text` column introspects back as `type: 'text'`; reporting
32 // `varchar(255)` for the model would diff against that on every run and
33 // re-ALTER a column nobody touched.
34 it('an unbounded string is text, and agrees with what the driver emits', () => {
35 expect(canonicalStorageType(handle(modelPlan()), 'postgres')).toBe('text')
36 expect(canonicalStorageType({ name: 'handle', type: 'text' } as ColumnPlan, 'postgres')).toBe('text')
37 })
38
39 // MySQL keeps the 255 default: TEXT there cannot be indexed without a
40 // prefix length, so the bound is worth having.
41 it('leaves the MySQL default alone', () => {
42 expect(canonicalStorageType(handle(modelPlan()), 'mysql')).toBe('VARCHAR(255)')
43 expect(canonicalStorageType(handle(modelPlan(50)), 'mysql')).toBe('VARCHAR(50)')
2944 })
3045
3146 it('widening max(50) -> max(100) emits ALTER COLUMN ... TYPE varchar(100), non-destructively', () => {
@@ -48,9 +63,26 @@ describe('postgres varchar length', () => {
4863 expect(operations).toEqual([])
4964 })
5065
51 it('an unbounded string and varchar(255) are the same column', () => {
66 // These used to be the same column, which is what made an unbounded
67 // `schema.string()` reject its own writes at 256 characters. They are now
68 // different, and dropping the bound widens to text rather than narrowing.
69 it('dropping a bound widens varchar(255) to text, non-destructively', () => {
70 const { operations } = generateDiffOperations(modelPlan(255), modelPlan())
71 const change = operations.find(op => op.kind === 'modify_column' && op.column === 'handle')
72 expect(change).toBeDefined()
73 expect(change!.sql).toContain('ALTER COLUMN "handle" TYPE text')
74 expect(change!.destructive).toBe(false)
75 })
76
77 it('adding a bound to an unbounded column is destructive, since text may not fit', () => {
5278 const { operations } = generateDiffOperations(modelPlan(), modelPlan(255))
53 expect(operations).toEqual([])
79 const change = operations.find(op => op.kind === 'modify_column' && op.column === 'handle')
80 expect(change!.sql).toContain('TYPE varchar(255)')
81 expect(change!.destructive).toBe(true)
82 })
83
84 it('an unchanged unbounded column produces no change', () => {
85 expect(generateDiffOperations(modelPlan(), modelPlan()).operations).toEqual([])
5486 })
5587
5688 it('introspection reads the live bound so a live diff compares lengths too', () => {
packages/bun-query-builder/test/varchar-width.test.tsmodified+12-3
@@ -5,6 +5,11 @@ import { buildMigrationPlan } from '../src/migrations'
55// A bounded string column (`.max(n)`) should become `varchar(n)` instead of the
66// default `varchar(255)`, so tight columns don't over-reserve. Indexable
77// bounded identifiers may exceed 255; content-sized values become `text`.
8//
9// An UNBOUNDED string splits by dialect, which is the contrast the two DDL
10// tests below draw: `varchar(255)` on MySQL, where TEXT cannot be indexed
11// without a prefix length, and `text` on Postgres, which stores and indexes
12// both the same way and so gains nothing from a bound the model never set.
813const mkStr = (max?: number) => ({
914 validation: { rule: { name: 'string', rules: max ? [{ name: 'max', params: { max } }] : [] } },
1015})
@@ -22,7 +27,7 @@ const models = {
2227 id: mkStr(32),
2328 country: mkStr(2),
2429 mailbox: mkStr(320),
25 path: mkStr(), // unbounded → varchar(255)
30 path: mkStr(), // unbounded → varchar(255) on MySQL, text on Postgres
2631 bio: mkStr(5000), // > 255 → text
2732 payload: mkTypedStr(16_000), // explicit type must not hide validator width
2833 },
@@ -56,11 +61,15 @@ describe('varchar width from .max()', () => {
5661 expect(ddl).toContain('`payload` text')
5762 })
5863
59 it('emits varchar(n) in Postgres DDL', () => {
64 it('emits varchar(n) in Postgres DDL, and text for an unbounded column', () => {
6065 const ddl = getDialectDriver('postgres' as any).createTable(plan('postgres'))
6166 expect(ddl).toContain('"id" varchar(32)')
6267 expect(ddl).toContain('"country" varchar(2)')
6368 expect(ddl).toContain('"mailbox" varchar(320)')
64 expect(ddl).toContain('"path" varchar(255)')
69 // Not varchar(255): nothing bounded this column, and a Postgres varchar
70 // rejects its own writes past the bound while text would have taken them.
71 expect(ddl).toContain('"path" text')
72 expect(ddl).toContain('"bio" text')
73 expect(ddl).toContain('"payload" text')
6574 })
6675})