|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * #11431 — the STRING family and its declared `maxLength`. |
| 5 | + * |
| 6 | + * ## The defect |
| 7 | + * |
| 8 | + * `createColumn` mapped `string` / `email` / `url` / `phone` / `password` with a |
| 9 | + * bare `table.string(name)`, so every one of them took knex's invented default |
| 10 | + * of 255 and the field's own `maxLength` was never read. Measured on the |
| 11 | + * pre-fix tree against live MySQL 8.0.46 and Postgres 16, through the driver's |
| 12 | + * own `initObjects`: |
| 13 | + * |
| 14 | + * wide_email email({ maxLength: 400 }) -> varchar(255) |
| 15 | + * wide_url url({ maxLength: 1024 }) -> varchar(255) |
| 16 | + * narrow_phone phone({ maxLength: 20 }) -> varchar(255) |
| 17 | + * |
| 18 | + * and a 300-character write into the `maxLength: 1024` column — legal under the |
| 19 | + * declaration, and accepted by the record validator's own `max_length` check — |
| 20 | + * was refused by both enforcing dialects: MySQL `ER_DATA_TOO_LONG`, Postgres |
| 21 | + * `22001 value too long for type character varying(255)`. |
| 22 | + * |
| 23 | + * The bound was inert in BOTH directions, which is why this file pins both. |
| 24 | + * `schema-drift.ts` has always treated `varchar(field.maxLength)` as the |
| 25 | + * expected physical shape, so before the fix a freshly created table reported |
| 26 | + * drift against itself — measured on that same table: two `widen_varchar` |
| 27 | + * (warning/safe) plus one `narrow_varchar` (error/**destructive**), on a table |
| 28 | + * with no rows in it. |
| 29 | + * |
| 30 | + * ## Why narrowing is pinned here and is NOT a destructive migration |
| 31 | + * |
| 32 | + * It reads like one, so it is worth stating with the mechanism: `createColumn` |
| 33 | + * has exactly four call sites, and every one of them is a `CREATE TABLE` or an |
| 34 | + * `ALTER TABLE ADD COLUMN` for a column that does not exist yet. The column it |
| 35 | + * sizes is always EMPTY. Narrowing an existing populated `varchar(255)` is a |
| 36 | + * different road entirely — the `narrow_varchar` drift op, category |
| 37 | + * `destructive`, behind `os migrate apply --allow-destructive` — and this |
| 38 | + * change does not touch it. `schema-drift.ts` is byte-identical. |
| 39 | + * |
| 40 | + * Opt-in for the live halves — they need real servers: |
| 41 | + * |
| 42 | + * OS_TEST_MYSQL_URL=mysql://root:root@127.0.0.1:3306/conformance \ |
| 43 | + * OS_TEST_POSTGRES_URL=postgres://postgres:postgres@127.0.0.1:5432/postgres \ |
| 44 | + * pnpm --filter @objectstack/driver-sql test |
| 45 | + */ |
| 46 | + |
| 47 | +import { describe, it, expect, afterEach } from 'vitest'; |
| 48 | +import { SqlDriver } from '../src/index.js'; |
| 49 | +import { MYSQL_CELL, PG_CELL, dialectCell, declareDialectCell } from './live-dialect-matrix.testkit.js'; |
| 50 | + |
| 51 | +const T = 'os11431_strings'; |
| 52 | +const PARENT = 'os11431_parent'; |
| 53 | + |
| 54 | +/** |
| 55 | + * One table holding every corner of the decision at once, so a change to it |
| 56 | + * cannot be green in five places and wrong in the sixth. |
| 57 | + */ |
| 58 | +const stringObject = () => ({ |
| 59 | + name: T, |
| 60 | + fields: { |
| 61 | + // ── the rule, both directions ────────────────────────────── |
| 62 | + wide_email: { type: 'email', maxLength: 400 }, |
| 63 | + wide_url: { type: 'url', maxLength: 1024 }, |
| 64 | + narrow_phone: { type: 'phone', maxLength: 20 }, |
| 65 | + narrow_password: { type: 'password', maxLength: 60 }, |
| 66 | + // ── no declaration → knex's 255, unchanged ───────────────── |
| 67 | + plain_email: { type: 'email' }, |
| 68 | + // ── a malformed declaration is not a bound ───────────────── |
| 69 | + bogus_url: { type: 'url', maxLength: 0 }, |
| 70 | + fractional_url: { type: 'url', maxLength: 12.5 }, |
| 71 | + // ── past the varchar ceiling → TEXT, never a clamp ───────── |
| 72 | + huge_url: { type: 'url', maxLength: 100000 }, |
| 73 | + // ── the three families deliberately left at 255 ──────────── |
| 74 | + a_lookup: { type: 'lookup', maxLength: 20, reference_to: PARENT }, |
| 75 | + a_user: { type: 'user', maxLength: 30 }, |
| 76 | + an_autonumber: { type: 'autonumber', maxLength: 8 }, |
| 77 | + a_secret: { type: 'secret', maxLength: 4000 }, |
| 78 | + a_select: { type: 'select', maxLength: 4000, options: ['a', 'b'] }, |
| 79 | + }, |
| 80 | +}); |
| 81 | + |
| 82 | +const parentObject = () => ({ name: PARENT, fields: { name: { type: 'text', maxLength: 64 } } }); |
| 83 | + |
| 84 | +/** `type(maxLength)` per column, normalised across the three dialects. */ |
| 85 | +async function columnShapes(driver: any, table: string): Promise<Record<string, string>> { |
| 86 | + const info: Record<string, any> = await driver.knex(table).columnInfo(); |
| 87 | + const out: Record<string, string> = {}; |
| 88 | + for (const [k, v] of Object.entries(info)) { |
| 89 | + const t = String((v as any)?.type ?? '').toLowerCase(); |
| 90 | + const n = (v as any)?.maxLength; |
| 91 | + out[k] = /char/.test(t) && n ? `varchar(${n})` : t; |
| 92 | + } |
| 93 | + return out; |
| 94 | +} |
| 95 | + |
| 96 | +// ── The emission rule, on a dialect every runner has ─────────────────────── |
| 97 | + |
| 98 | +describe('string-family columns take their declared maxLength (#11431)', () => { |
| 99 | + let driver: SqlDriver; |
| 100 | + |
| 101 | + afterEach(async () => { |
| 102 | + await driver?.disconnect().catch(() => {}); |
| 103 | + }); |
| 104 | + |
| 105 | + it('emits varchar(maxLength) in BOTH directions, and 255 without a declaration', async () => { |
| 106 | + driver = new SqlDriver(dialectCell('sqlite').config()); |
| 107 | + await driver.initObjects([parentObject(), stringObject()]); |
| 108 | + const shapes = await columnShapes(driver as any, T); |
| 109 | + |
| 110 | + // Wider than knex's default — the reported defect. |
| 111 | + expect(shapes.wide_email).toBe('varchar(400)'); |
| 112 | + expect(shapes.wide_url).toBe('varchar(1024)'); |
| 113 | + // Narrower — the same defect's other half. Empty column, nothing to truncate. |
| 114 | + expect(shapes.narrow_phone).toBe('varchar(20)'); |
| 115 | + expect(shapes.narrow_password).toBe('varchar(60)'); |
| 116 | + // Undeclared stays exactly where it was. |
| 117 | + expect(shapes.plain_email).toBe('varchar(255)'); |
| 118 | + }); |
| 119 | + |
| 120 | + it('treats a malformed maxLength as no declaration rather than guessing', async () => { |
| 121 | + driver = new SqlDriver(dialectCell('sqlite').config()); |
| 122 | + await driver.initObjects([parentObject(), stringObject()]); |
| 123 | + const shapes = await columnShapes(driver as any, T); |
| 124 | + // `varchar(0)` and `varchar(12.5)` are not DDL; neither is a bound. |
| 125 | + expect(shapes.bogus_url).toBe('varchar(255)'); |
| 126 | + expect(shapes.fractional_url).toBe('varchar(255)'); |
| 127 | + }); |
| 128 | + |
| 129 | + it('falls back to TEXT past the varchar ceiling instead of clamping', async () => { |
| 130 | + driver = new SqlDriver(dialectCell('sqlite').config()); |
| 131 | + await driver.initObjects([parentObject(), stringObject()]); |
| 132 | + const shapes = await columnShapes(driver as any, T); |
| 133 | + // ⛔ NOT varchar(16383). Clamping would reinstate the very defect being |
| 134 | + // fixed — a column narrower than the declaration, refusing legal writes. |
| 135 | + expect(shapes.huge_url).toBe('text'); |
| 136 | + }); |
| 137 | + |
| 138 | + it('leaves lookup / user / autonumber / catch-all at 255 — they do not store the declared value', async () => { |
| 139 | + driver = new SqlDriver(dialectCell('sqlite').config()); |
| 140 | + await driver.initObjects([parentObject(), stringObject()]); |
| 141 | + const shapes = await columnShapes(driver as any, T); |
| 142 | + // A lookup holds the referenced row's ID, not the declared value: a |
| 143 | + // platform id is 26 characters, so `varchar(20)` could hold none of them. |
| 144 | + expect(shapes.a_lookup).toBe('varchar(255)'); |
| 145 | + expect(shapes.a_user).toBe('varchar(255)'); |
| 146 | + // Runtime-issued; `maxLength` has no write-time counterpart on this type. |
| 147 | + expect(shapes.an_autonumber).toBe('varchar(255)'); |
| 148 | + // `secret` persists an opaque sys_secret ref; `select` a machine name. |
| 149 | + expect(shapes.a_secret).toBe('varchar(255)'); |
| 150 | + expect(shapes.a_select).toBe('varchar(255)'); |
| 151 | + }); |
| 152 | +}); |
| 153 | + |
| 154 | +// ── The halves only a length-ENFORCING server can measure ────────────────── |
| 155 | + |
| 156 | +for (const cell of [MYSQL_CELL, PG_CELL]) { |
| 157 | + declareDialectCell(cell, 'string-family maxLength (#11431)', (c) => { |
| 158 | + describe(`string-family maxLength on ${c.label} (#11431)`, () => { |
| 159 | + let driver: SqlDriver; |
| 160 | + |
| 161 | + afterEach(async () => { |
| 162 | + for (const t of [T, PARENT]) await driver?.execute(`drop table if exists ${t}`).catch(() => {}); |
| 163 | + await driver?.disconnect().catch(() => {}); |
| 164 | + }); |
| 165 | + |
| 166 | + it('accepts a write the declaration allows — the whole defect, in one assertion', async () => { |
| 167 | + driver = new SqlDriver(c.config()); |
| 168 | + for (const t of [T, PARENT]) await driver.execute(`drop table if exists ${t}`).catch(() => {}); |
| 169 | + await driver.initObjects([parentObject(), stringObject()]); |
| 170 | + |
| 171 | + const shapes = await columnShapes(driver as any, T); |
| 172 | + expect(shapes.wide_url).toBe('varchar(1024)'); |
| 173 | + expect(shapes.narrow_phone).toBe('varchar(20)'); |
| 174 | + |
| 175 | + // Pre-fix this was REFUSED — `ER_DATA_TOO_LONG` on MySQL, `22001` on |
| 176 | + // Postgres — for a value the declaration plainly permits. |
| 177 | + await driver.create(T, { id: 'r1', wide_url: 'u'.repeat(300) }); |
| 178 | + const rows: any[] = await driver.find(T, { filters: [['id', '=', 'r1']] } as any); |
| 179 | + expect(String(rows[0]?.wide_url ?? '').length).toBe(300); |
| 180 | + }); |
| 181 | + |
| 182 | + it('reports no varchar drift against a table it just created', async () => { |
| 183 | + driver = new SqlDriver(c.config()); |
| 184 | + for (const t of [T, PARENT]) await driver.execute(`drop table if exists ${t}`).catch(() => {}); |
| 185 | + await driver.initObjects([parentObject(), stringObject()]); |
| 186 | + |
| 187 | + const drift: any[] = await (driver as any).detectTableDrift(T, stringObject().fields, []); |
| 188 | + const varcharDrift = drift.filter( |
| 189 | + (d) => d.op?.type === 'widen_varchar' || d.op?.type === 'narrow_varchar', |
| 190 | + ); |
| 191 | + |
| 192 | + // The emitter and the differ finally agree about the string family. |
| 193 | + // Pre-fix this table reported two `widen_varchar` AND a |
| 194 | + // `narrow_varchar` (error/destructive) against itself, with no rows in |
| 195 | + // it. What remains is only the families this card deliberately left |
| 196 | + // alone, so the assertion names them rather than expecting zero. |
| 197 | + expect(varcharDrift.map((d) => d.column).sort()).toEqual( |
| 198 | + ['a_lookup', 'a_secret', 'a_select', 'a_user', 'an_autonumber'].sort(), |
| 199 | + ); |
| 200 | + for (const col of ['wide_email', 'wide_url', 'narrow_phone', 'narrow_password', 'plain_email']) { |
| 201 | + expect(varcharDrift.map((d) => d.column)).not.toContain(col); |
| 202 | + } |
| 203 | + |
| 204 | + // `huge_url` is TEXT, and the differ is right to say nothing about it: |
| 205 | + // it compares a numeric `col.maxLength`, which a TEXT column does not |
| 206 | + // report. A TEXT column refuses nothing the declaration allows, so |
| 207 | + // there is no divergence to plan an ALTER for. |
| 208 | + expect(varcharDrift.map((d) => d.column)).not.toContain('huge_url'); |
| 209 | + |
| 210 | + // ⛔ The malformed pair, and the reason `schema-drift.ts` is in this |
| 211 | + // diff at all. On the pre-fix tree BOTH of these reported |
| 212 | + // `narrow_varchar` at severity `error` / category `destructive` — |
| 213 | + // `maxLength: 0` takes the narrowing arm because `0 > 255` is false — |
| 214 | + // so `os migrate apply --allow-destructive` was being asked to run |
| 215 | + // `varchar(0)` and `varchar(12.5)`, DDL no server accepts. The emitter |
| 216 | + // treats a malformed bound as no bound; the differ now applies the |
| 217 | + // same predicate, which is the whole point of the fix. |
| 218 | + expect(varcharDrift.map((d) => d.column)).not.toContain('bogus_url'); |
| 219 | + expect(varcharDrift.map((d) => d.column)).not.toContain('fractional_url'); |
| 220 | + }); |
| 221 | + |
| 222 | + it('still refuses a value past the DECLARED bound — the bound binds, it did not merely move', async () => { |
| 223 | + driver = new SqlDriver(c.config()); |
| 224 | + for (const t of [T, PARENT]) await driver.execute(`drop table if exists ${t}`).catch(() => {}); |
| 225 | + await driver.initObjects([parentObject(), stringObject()]); |
| 226 | + |
| 227 | + // ⛔ The negative half. `varchar(20)` must really be 20 — a fix that |
| 228 | + // widened everything to silence the symptom would pass every |
| 229 | + // assertion above and fail this one. |
| 230 | + await expect( |
| 231 | + driver.create(T, { id: 'r2', narrow_phone: 'p'.repeat(300) }), |
| 232 | + ).rejects.toThrow(/too long|ER_DATA_TOO_LONG|22001/i); |
| 233 | + }); |
| 234 | + }); |
| 235 | + }); |
| 236 | +} |
0 commit comments