From 863bc905edb77d8c973e959024cd840982fc9d04 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 00:36:09 +0000 Subject: [PATCH] =?UTF-8?q?fix(driver-sql):=20make=20the=20SQLite=20Field.?= =?UTF-8?q?json=20codec=20injective=20=E2=80=94=20one=20encoding=20across?= =?UTF-8?q?=20all=20three=20dialects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `formatInput` now `JSON.stringify`s every `Field.json` value on every dialect and `formatOutput` parses it back, deleting the SQLite branch rather than adding one. Postgres and MySQL are untouched: this makes SQLite match what they already did. Measured live (SQLite / PG 16.13 / MySQL 8.0.46), stored cell read back through a separate raw catalog query: PG and MySQL were 17/17 faithful, SQLite 13/17 type-changed, through three mechanisms — the read-side parse, SQLite's NUMERIC affinity on a `json`-declared column eating number-like strings before storage, and native booleans landing as INTEGER 1/0. The declared contract decides which dialect is right: `json`'s stored contract is `z.unknown()`, an explicitly open contract that admits both `123` and `'123'` as legal values of one field. `backfillCanonicalJsonEncoding` converges existing SQLite rows on `syncSchema`, in the same shape and posture as the datetime/time storage-format backfills beside it. It converts the one unambiguous on-disk class (TEXT that is not valid JSON) and refuses to guess at the two that the pre-fix encoding made ambiguous. Part of #12380 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --- .changeset/sqlite-json-injective-codec.md | 109 ++++ .../sql-driver-12380-json-roundtrip.test.ts | 472 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 193 ++++++- 3 files changed, 755 insertions(+), 19 deletions(-) create mode 100644 .changeset/sqlite-json-injective-codec.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-12380-json-roundtrip.test.ts diff --git a/.changeset/sqlite-json-injective-codec.md b/.changeset/sqlite-json-injective-codec.md new file mode 100644 index 0000000000..1a44e740c3 --- /dev/null +++ b/.changeset/sqlite-json-injective-codec.md @@ -0,0 +1,109 @@ +--- +"@objectstack/driver-sql": minor +--- + +fix(driver-sql): make the SQLite `Field.json` codec injective — one encoding across all three dialects (#12380) + +**BREAKING** storage-format change for `Field.json` columns on SQLite (and the +SQLite-backed `driver-turso` / `driver-sqlite-wasm`, which inherit this codec), +shipped as `minor` under the repo's launch-window convention for breaking +changes. Postgres and MySQL are **untouched** — this makes SQLite match what +they have always done. + +`formatInput` now `JSON.stringify`s every `Field.json` value on every dialect, +and `formatOutput` parses it back. That **deletes a dialect branch rather than +adding one**. + +## What was wrong + +Measured 2026-08-26 through the driver boundary on live SQLite, live Postgres +16.13 and live MySQL 8.0.46, with each stored cell read back through a separate +raw catalog query: **Postgres and MySQL were 17/17 faithful; SQLite was 13/17 +type-changed.** Three independent mechanisms, only two of them reversible: + +1. **Read-side.** `formatOutput` `JSON.parse`s every string in a json column, so + a stored string whose *content* is valid JSON came back type-changed — + `'true'` → boolean, `'null'` → null, `'[]'` → array, `'{"a":1}'` → object. +2. **Write-side.** The column is declared type `json`, which contains none of + `INT`/`CHAR`/`CLOB`/`TEXT`/`BLOB`/`REAL`/`FLOA`/`DOUB`, so SQLite's affinity + rules fall through to **NUMERIC** and a bound number-like string was converted + to INTEGER/REAL *before storage*: `'123'`, `' 123 '`, `'0123'`, `'1e5'`, + `'1.0'`, `'-0'` were destroyed on disk. ⛔ Not reversible. +3. **Native booleans.** `true` was stored as INTEGER 1 and read back as the + number `1` — `formatOutput`'s `booleanFields` pass is keyed to declared + `Field.boolean` *columns*, not to booleans inside a json payload. + +The contract decides which dialect is right, not strictness: `json`'s stored +contract is `z.unknown()` because *"openness is now an explicit decision, not an +accident of nobody checking"* (`packages/spec/src/data/field-value.zod.ts`). An +explicitly-open contract admits both `123` and `'123'` as legal values of one +field, so no driver may collapse them onto one representation. + +The live consumer is `sys_setting.value`, which is `Field.json`, and the settings +service persists verbatim and reads back with no re-coercion by declared type — +so the driver's answer is what the caller gets, on the dialect tenant +environments actually run. + +## What changes on disk, and what does not + +The DDL is unchanged — the column is still declared `json`, so NUMERIC affinity +is still in force. The encoded form defeats it because a string's encoding +carries its quotes (`'123'` → `"123"`, which is not a numeric literal). Pinned +live rather than reasoned. + +For **new** writes the on-disk delta is exactly two classes: + +- **strings** are now quoted JSON text; +- **booleans** are now TEXT `true`/`false` instead of INTEGER `1`/`0`. + +Objects, arrays, `null` and **numbers** are byte-identical to before (`123` bound +as a number and `"123"` bound as text both land as INTEGER `123`). + +⚠️ An out-of-band reader of a SQLite file — anything reading the table with its +own SQL rather than through this driver — now sees quoted JSON text where it saw +a bare value. + +## The migration, and the limits of what it can recover + +`backfillCanonicalJsonEncoding` runs on `syncSchema`/`initObjects` for existing +tables, the same posture and shape as the `backfillCanonicalDatetimes` and +`backfillCanonicalTimes` storage-format migrations beside it: one `UPDATE` per +column, failures logged and swallowed, correctness never contingent on it having +run. It converts the **one on-disk class the pre-fix encoding left unambiguous** — +a TEXT cell that is not valid JSON, which nothing but a stored plain string could +have produced — into its quoted form. Idempotent by construction: the `WHERE` is +the exact complement of the `SET`'s output, so a converted row cannot match again +and re-running costs one scan and zero writes. + +⛔ **It does not guess, because the rest cannot be guessed**, and two classes are +therefore left exactly as they are: + +- **INTEGER/REAL cells.** A number, a boolean, and a number-like string eaten by + NUMERIC affinity are the *same bytes* on disk — `123` the number and `'123'` + the string are one INTEGER `123`. No migration can know which was written. +- **TEXT cells that already parse.** A stored object `{"a":1}` and a stored + *string* `'{"a":1}'` were byte-identical before this change. Re-quoting them + would turn every legacy object and array into a string — corrupting the common + case to guess at the rare one. + +⇒ Those rows read after this change exactly as they read before it. **The class +stops growing; it is not retroactively repaired.** Maintainer ruling 2026-08-26, +with that cost accepted explicitly. + +The migration changes **no read**: a legacy plain string reads back as that +string before it runs (via `formatOutput`'s parse fallback, kept for exactly this +reason and now documented as the pre-#12380 read-side repair) and after it runs. +It is a canonicalisation that makes the on-disk format uniform and injective +going forward, not a repair of something that reads wrong today. + +## What upgraders may notice + +Values that were being **corrupted** now read back correctly. Code that adapted +to the corruption is what changes underneath: a boolean `Field.json` value that +read back as `1` now reads back as `true`, and a string whose content is valid +JSON now reads back as that string instead of the structure it looked like. +Filters are unaffected — every scalar comparison operator on a json column is +already refused by the driver (`JSON_COLUMN_INCOMPATIBLE_OPERATORS`), so no +predicate could have been keyed to the old stored text. + + diff --git a/packages/drivers/driver-sql/src/sql-driver-12380-json-roundtrip.test.ts b/packages/drivers/driver-sql/src/sql-driver-12380-json-roundtrip.test.ts new file mode 100644 index 0000000000..b7a5c74740 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-12380-json-roundtrip.test.ts @@ -0,0 +1,472 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12380] A `Field.json` column round-trips FAITHFULLY — what you wrote is what + * you read back — on every dialect this driver speaks. + * + * ## What was measured, and why the contract decides it + * + * Measured 2026-08-26 through the driver boundary on live SQLite (better-sqlite3), + * live Postgres 16.13 (server TZ `Asia/Shanghai`) and live MySQL 8.0.46 (server + * TZ `+08:00`), with the stored cell read back through a SEPARATE RAW QUERY + * against the catalog (`typeof()` / `json_typeof()` / `json_type()`), never + * through the emitted DDL. Before the fix: **Postgres and MySQL 17/17 faithful, + * SQLite 13/17 type-changed.** + * + * `json`'s stored contract is `z.unknown()` — deliberately open + * (`spec/src/data/field-value.zod.ts`: *"openness is now an explicit decision, + * not an accident of nobody checking"*). An explicitly-open contract admits BOTH + * `123` and `'123'` as legal values of one field, so no driver has license to + * collapse them onto one representation. That is what makes Postgres and MySQL + * right and SQLite wrong here — a contract argument, not a strictness one. + * Maintainer ruling 2026-08-26: make SQLite injective, deleting a dialect branch + * rather than adding one. + * + * ## Three mechanisms, and only two of them were ever reversible + * + * 1. **Read-side.** `formatOutput` `JSON.parse`s every string in a json column, + * so a stored string whose CONTENT parses came back type-changed. + * 2. **Write-side.** The column is declared type `json`, which contains none of + * `INT`/`CHAR`/`CLOB`/`TEXT`/`BLOB`/`REAL`/`FLOA`/`DOUB`, so SQLite's + * affinity rules fall through to NUMERIC and a bound number-like string was + * converted to INTEGER/REAL **before storage**. ⛔ Not reversible. + * 3. **Native booleans.** `true` was bound as INTEGER 1 and read back as the + * number `1`; `formatOutput`'s `booleanFields` pass is keyed to declared + * `Field.boolean` COLUMNS, not to booleans inside a json payload. + * + * §3 is the one that decides the design and is pinned LIVE rather than reasoned: + * the declared column type is unchanged, so NUMERIC affinity is still in force — + * and the suite proves it is, in the same statement in which it proves the + * driver's encoded form defeats it. + * + * ## Assertion conventions + * + * Values are asserted with `toStrictEqual` **and** an explicit `typeof` pin. The + * before-state is a WRONG TYPE carrying a right-looking value: `'123'` read back + * as `123` passes `toEqual`-style coercion and every truthiness pin, which is + * exactly how this survived to be found by reading the code. A round-trip pin + * that does not compare types is not a round-trip pin. + * + * Postgres and MySQL were faithful BEFORE this change and are the regression + * control: if the fix ever moves the defect onto them instead of closing it, + * §1 goes red on those cells first. + * + * ⛔ Deliberately NOT here: the cross-driver `VALUE_ROUNDTRIP` conformance + * case-set. It is ruled in but sequenced SECOND (it is cross-driver by design + * and would ship red on SQLite by construction if it landed before this fix), + * and this suite consumes no `CASE_SETS` marker, so the conformance census does + * not score it. It states its dialect stance through `DIALECT_CELLS` anyway. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { DriverQuery } from '@objectstack/spec/contracts'; +import { SqlDriver } from './sql-driver.js'; +import { DIALECT_CELLS, declareDialectCell, dialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const TABLE = 'json_roundtrip_12380'; +const LEGACY_TABLE = 'json_legacy_12380'; + +const FIELDS = { + label: { type: 'string' }, + val: { type: 'json' }, +} as const; + +/** + * The boundary set. Every STRING here has content that is valid JSON or is + * number-like (or both) — the two classes the pre-fix encoding destroyed — plus + * the ordinary strings that always worked, as controls. `wrote` is the exact JS + * value handed to `create()`; the read must be `toStrictEqual` to it. + */ +const CASES: Array<{ label: string; wrote: unknown; note: string }> = [ + // ── strings whose content is valid JSON (mechanism 1) ────────────────────── + { label: 's_true', wrote: 'true', note: 'string, parses as boolean' }, + { label: 's_false', wrote: 'false', note: 'string, parses as boolean' }, + { label: 's_null', wrote: 'null', note: 'string, parses as null' }, + { label: 's_arr', wrote: '[]', note: 'string, parses as array' }, + { label: 's_obj', wrote: '{"a":1}', note: 'string, parses as object' }, + { label: 's_quoted', wrote: '"quoted"', note: 'string, parses as string' }, + // ── number-like strings (mechanism 2 — irreversible before the fix) ──────── + { label: 's_123', wrote: '123', note: 'string, number-like' }, + { label: 's_pad', wrote: ' 123 ', note: 'string, number-like with padding' }, + { label: 's_0123', wrote: '0123', note: 'string, leading zero' }, + { label: 's_1e5', wrote: '1e5', note: 'string, exponent form' }, + { label: 's_1p0', wrote: '1.0', note: 'string, trailing zero' }, + { label: 's_neg0', wrote: '-0', note: 'string, negative zero' }, + // ── ordinary strings — controls that were faithful before ───────────────── + { label: 's_empty', wrote: '', note: 'empty string' }, + { label: 's_tz', wrote: 'America/New_York', note: 'ordinary string' }, + { label: 's_bad', wrote: '{bad json', note: 'string that does not parse' }, + { label: 's_nan', wrote: 'NaN', note: 'string, not valid JSON' }, + // ── native (non-string) JSON values ─────────────────────────────────────── + { label: 'n_true', wrote: true, note: 'native boolean (mechanism 3)' }, + { label: 'n_false', wrote: false, note: 'native boolean (mechanism 3)' }, + { label: 'n_int', wrote: 123, note: 'native number' }, + { label: 'n_real', wrote: 1.5, note: 'native number' }, + { label: 'n_null', wrote: null, note: 'native null' }, + { label: 'n_obj', wrote: { a: 1 }, note: 'native object' }, + { label: 'n_arr', wrote: [1, 2], note: 'native array' }, + { label: 'n_str', wrote: 'plain', note: 'native plain string' }, +]; + +/** + * The pairs that must stay DISTINGUISHABLE ON DISK: the string form of a value + * against the native form it looks like. Three of these six collided on SQLite + * before the fix (`'123'`/`123`, `'[]'`/`[]`, `'{"a":1}'`/`{a:1}`) and a fourth + * collided on read (`'null'`/`null`); all six were distinct on PG and MySQL. + */ +const COLLISION_PAIRS: Array<[string, string]> = [ + ['s_123', 'n_int'], + ['s_arr', 'n_arr_empty'], + ['s_obj', 'n_obj'], + ['s_true', 'n_true'], + ['s_null', 'n_null'], + ['s_quoted', 'n_quoted'], +]; + +/** Extra rows that exist only to be the native half of a collision pair. */ +const PAIR_ROWS: Array<{ label: string; wrote: unknown }> = [ + { label: 'n_arr_empty', wrote: [] }, + { label: 'n_quoted', wrote: 'quoted' }, +]; + +/** knex's raw result shape differs per client; this is the only place that knows. */ +function rowsOf(cell: DialectCell, res: any): any[] { + if (cell.id === 'pg') return res?.rows ?? []; + if (cell.id === 'mysql') return Array.isArray(res) ? (res[0] ?? []) : []; + return Array.isArray(res) ? res : (res?.rows ?? []); +} + +/** + * Read one cell's STORED form through a separate raw query — the storage class + * (or JSON type) the server reports, plus the stored text. Never the DDL. + */ +async function diskCell( + driver: SqlDriver, + cell: DialectCell, + table: string, + label: string, +): Promise<{ t: string | null; v: string | null }> { + const sql = + cell.id === 'pg' + ? `select json_typeof("val") as t, "val"::text as v from "${table}" where "label" = ?` + : cell.id === 'mysql' + ? `select json_type(\`val\`) as t, cast(\`val\` as char) as v from \`${table}\` where \`label\` = ?` + : `select typeof("val") as t, cast("val" as text) as v from "${table}" where "label" = ?`; + const rows = rowsOf(cell, await driver.execute(sql, [label])); + expect(rows, `no disk row for ${label}`).toHaveLength(1); + return { t: rows[0].t ?? null, v: rows[0].v ?? null }; +} + +/** The physical column type, read from the CATALOG rather than the emitted DDL. */ +async function catalogType(driver: SqlDriver, cell: DialectCell, table: string): Promise { + const sql = + cell.id === 'pg' + ? `select udt_name as ty from information_schema.columns + where table_schema = current_schema() and table_name = ? and column_name = 'val'` + : cell.id === 'mysql' + ? `select data_type as ty from information_schema.columns + where table_schema = database() and table_name = ? and column_name = 'val'` + : `select type as ty from pragma_table_info(?) where name = 'val'`; + const rows = rowsOf(cell, await driver.execute(sql, [table])); + expect(rows, `no catalog row for ${table}.val`).toHaveLength(1); + return String(rows[0].ty).toLowerCase(); +} + +function declareRoundTrip(cell: DialectCell): void { +describe(`[#12380] driver-sql — Field.json round-trips faithfully (${cell.label})`, () => { + let driver: SqlDriver; + + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.initObjects([{ name: TABLE, fields: { ...FIELDS } }]); + for (const c of [...CASES, ...PAIR_ROWS]) { + await driver.create(TABLE, { label: c.label, val: c.wrote }, { bypassTenantAudit: true }); + } + }, 60_000); + + afterAll(async () => { + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver.disconnect(); + }); + + // The fixture read back rather than trusted: a seed that dropped or folded a + // row would turn every assertion below into a test of the wrong table. + it('the fixture is one row per case', async () => { + const rows = (await driver.find(TABLE, {})) as Array<{ label: string }>; + expect(rows.map((r) => r.label).sort()).toEqual( + [...CASES, ...PAIR_ROWS].map((c) => c.label).sort(), + ); + }); + + // ─── §1 The round trip itself — value AND type ─────────────────────────── + + for (const c of CASES) { + it(`round-trips ${c.label} (${c.note})`, async () => { + const rows = (await driver.find(TABLE, { where: { label: c.label } } as DriverQuery)) as any[]; + expect(rows).toHaveLength(1); + const read = rows[0].val; + // The type pin comes first: `'123'` read back as `123` is the exact + // before-state, and it survives every value-only comparison. + expect(typeof read, `typeof for ${c.label}`).toBe(typeof c.wrote); + expect(read, `value for ${c.label}`).toStrictEqual(c.wrote); + }); + } + + it('every case in the boundary set round-trips — the whole matrix at once', async () => { + const rows = (await driver.find(TABLE, {})) as any[]; + const byLabel = new Map(rows.map((r) => [r.label, r.val])); + const unfaithful = CASES.filter( + (c) => + typeof byLabel.get(c.label) !== typeof c.wrote || + JSON.stringify(byLabel.get(c.label)) !== JSON.stringify(c.wrote), + ).map((c) => `${c.label}: wrote ${JSON.stringify(c.wrote)} (${typeof c.wrote}), read ${JSON.stringify(byLabel.get(c.label))} (${typeof byLabel.get(c.label)})`); + expect(unfaithful, `${CASES.length - unfaithful.length}/${CASES.length} faithful`).toEqual([]); + }); + + // ─── §2 Distinguishable ON DISK, read through a separate raw query ─────── + + it('a string and the native value it looks like are distinct ON DISK', async () => { + const collisions: string[] = []; + for (const [strLabel, nativeLabel] of COLLISION_PAIRS) { + const a = await diskCell(driver, cell, TABLE, strLabel); + const b = await diskCell(driver, cell, TABLE, nativeLabel); + if (a.t === b.t && a.v === b.v) { + collisions.push(`${strLabel} vs ${nativeLabel}: both ${a.t} ${JSON.stringify(a.v)}`); + } + } + expect(collisions, `${collisions.length}/${COLLISION_PAIRS.length} collided`).toEqual([]); + }); + + it('a string and the native value it looks like are distinct ON READ', async () => { + const rows = (await driver.find(TABLE, {})) as any[]; + const byLabel = new Map(rows.map((r) => [r.label, r.val])); + for (const [strLabel, nativeLabel] of COLLISION_PAIRS) { + const s = byLabel.get(strLabel); + const n = byLabel.get(nativeLabel); + expect(typeof s, `${strLabel} must read as a string`).toBe('string'); + expect(s === n, `${strLabel} and ${nativeLabel} read identically`).toBe(false); + } + }); + + // ─── §3 The column type, and the affinity it still carries ─────────────── + + it('the physical column type is `json`, read from the catalog', async () => { + expect(await catalogType(driver, cell, TABLE)).toBe('json'); + }); +}); +} + +/** + * ⚠️ SQLite only, and the point of the whole design: the DDL is UNCHANGED, so + * the `json`-declared column still has NUMERIC affinity. This proves the + * affinity is still in force AND that the driver's encoded form survives it — + * both in one statement, so neither half can be true of a different column. + */ +function declareAffinity(): void { +describe('[#12380] SQLite NUMERIC affinity is still in force, and the encoding defeats it', () => { + const cell = dialectCell('sqlite'); + const T = 'json_affinity_12380'; + let driver: SqlDriver; + + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + await driver.execute(`drop table if exists ${T}`).catch(() => {}); + await driver.initObjects([{ name: T, fields: { ...FIELDS } }]); + }, 60_000); + + afterAll(async () => { + await driver.execute(`drop table if exists ${T}`).catch(() => {}); + await driver.disconnect(); + }); + + it('the declared type is still `json` — no DDL change was needed', async () => { + expect(await catalogType(driver, cell, T)).toBe('json'); + }); + + it.each(['123', ' 123 ', '0123', '1e5', '1.0', '-0'])( + 'raw %j becomes a numeric storage class, but its JSON encoding stays TEXT', + async (raw) => { + const bare = `bare_${raw}`; + const enc = `enc_${raw}`; + // ONE statement, one column, two bindings: the pre-fix form and the form + // `formatInput` now produces. Bound through raw SQL so nothing but SQLite's + // own affinity rule can be responsible for the difference. + await driver.execute( + `insert into "${T}" ("id", "label", "val") values (?, ?, ?), (?, ?, ?)`, + [`i_${bare}`, bare, raw, `i_${enc}`, enc, JSON.stringify(raw)], + ); + const bareDisk = await diskCell(driver, cell, T, bare); + const encDisk = await diskCell(driver, cell, T, enc); + // The affinity is REAL — this is the mechanism the card could not repair. + expect(['integer', 'real'], `bare ${raw} must be eaten by NUMERIC affinity`).toContain(bareDisk.t); + // …and the quotes defeat it, with no DDL change. + expect(encDisk.t, `encoded ${raw} must stay TEXT`).toBe('text'); + expect(encDisk.v, `encoded ${raw} bytes`).toBe(JSON.stringify(raw)); + expect(JSON.parse(encDisk.v!), `encoded ${raw} parses back`).toBe(raw); + }, + ); + + it('a native number still lands as a numeric storage class — unchanged by this fix', async () => { + await driver.create(T, { label: 'num', val: 4200 }, { bypassTenantAudit: true }); + const d = await diskCell(driver, cell, T, 'num'); + expect(d.t).toBe('integer'); + const rows = (await driver.find(T, { where: { label: 'num' } } as DriverQuery)) as any[]; + expect(rows[0].val).toBe(4200); + }); +}); +} + +/** + * ⚠️ SQLite only. The storage-format migration: what it converts, what it + * refuses to guess at, that it changes no read, and that it is idempotent. + * + * Legacy rows are planted through raw SQL with the values the PRE-fix + * `formatInput` would have bound — so the same affinity rule that produced the + * legacy corpus produces this one. + */ +function declareMigration(): void { +describe('[#12380] the storage-format migration over legacy SQLite json rows', () => { + const cell = dialectCell('sqlite'); + let driver: SqlDriver; + + /** label → the value the PRE-fix formatInput would have BOUND for it. */ + const LEGACY: Array<{ label: string; bound: unknown; wasWritten: string }> = [ + { label: 'l_plain', bound: 'America/New_York', wasWritten: "the string 'America/New_York'" }, + { label: 'l_empty', bound: '', wasWritten: "the empty string" }, + { label: 'l_bad', bound: '{bad json', wasWritten: "the string '{bad json'" }, + { label: 'l_obj', bound: '{"a":1}', wasWritten: 'an object {a:1} — OR the string \'{"a":1}\'' }, + { label: 'l_arr', bound: '["x"]', wasWritten: "an array ['x'] — OR the string '[\"x\"]'" }, + { label: 'l_strtrue', bound: 'true', wasWritten: "the string 'true'" }, + { label: 'l_int', bound: 123, wasWritten: 'the number 123 — OR the string \'123\'' }, + { label: 'l_bool', bound: 1, wasWritten: 'the boolean true — OR the number 1' }, + ]; + + async function plant(): Promise { + for (const r of LEGACY) { + await driver.execute( + `insert into "${LEGACY_TABLE}" ("id", "label", "val") values (?, ?, ?)`, + [`i_${r.label}`, r.label, r.bound as any], + ); + } + } + + async function snapshot(): Promise> { + const out: Record = {}; + for (const r of LEGACY) out[r.label] = await diskCell(driver, cell, LEGACY_TABLE, r.label); + return out; + } + + async function readAll(): Promise> { + const rows = (await driver.find(LEGACY_TABLE, {})) as any[]; + return new Map(rows.map((r) => [r.label, r.val])); + } + + let beforeDisk: Record; + let beforeRead: Map; + let afterDisk: Record; + let afterRead: Map; + let twiceDisk: Record; + + beforeAll(async () => { + driver = new SqlDriver(cell.config()); + await driver.execute(`drop table if exists ${LEGACY_TABLE}`).catch(() => {}); + // Pass 1 CREATES the table, so the backfill correctly does nothing. + await driver.initObjects([{ name: LEGACY_TABLE, fields: { ...FIELDS } }]); + await plant(); + beforeDisk = await snapshot(); + beforeRead = await readAll(); + // Pass 2 finds the table EXISTING, which is what runs the migration. + await driver.initObjects([{ name: LEGACY_TABLE, fields: { ...FIELDS } }]); + afterDisk = await snapshot(); + afterRead = await readAll(); + // Pass 3 — idempotence. + await driver.initObjects([{ name: LEGACY_TABLE, fields: { ...FIELDS } }]); + twiceDisk = await snapshot(); + }, 120_000); + + afterAll(async () => { + await driver.execute(`drop table if exists ${LEGACY_TABLE}`).catch(() => {}); + await driver.disconnect(); + }); + + it('the legacy corpus really is in the pre-fix storage form', () => { + // The plain strings are bare TEXT — not valid JSON, which is what makes + // them the one unambiguous class. + expect(beforeDisk.l_plain).toEqual({ t: 'text', v: 'America/New_York' }); + expect(beforeDisk.l_bad).toEqual({ t: 'text', v: '{bad json' }); + // …and the number-like/boolean ones were already eaten by NUMERIC affinity. + expect(beforeDisk.l_int.t).toBe('integer'); + expect(beforeDisk.l_bool.t).toBe('integer'); + }); + + it('CONVERTS the one unambiguous class: bare TEXT that is not valid JSON', () => { + expect(afterDisk.l_plain.v).toBe('"America/New_York"'); + expect(afterDisk.l_empty.v).toBe('""'); + expect(afterDisk.l_bad.v).toBe('"{bad json"'); + for (const label of ['l_plain', 'l_empty', 'l_bad']) { + expect(afterDisk[label].t, `${label} stays TEXT`).toBe('text'); + expect(afterDisk[label].v, `${label} was rewritten`).not.toBe(beforeDisk[label].v); + } + }); + + it('⛔ REFUSES to guess: INTEGER/REAL cells are left exactly as they are', () => { + // `123` the number, `'123'` the string and the boolean `true` are the same + // bytes on disk. Guessing would corrupt two of the three. + expect(afterDisk.l_int).toEqual(beforeDisk.l_int); + expect(afterDisk.l_bool).toEqual(beforeDisk.l_bool); + }); + + it('⛔ REFUSES to guess: TEXT cells that already parse are left exactly as they are', () => { + // Re-quoting these would turn every legacy object and array into a string — + // corrupting the common case to guess at the rare one. + expect(afterDisk.l_obj).toEqual(beforeDisk.l_obj); + expect(afterDisk.l_arr).toEqual(beforeDisk.l_arr); + expect(afterDisk.l_strtrue).toEqual(beforeDisk.l_strtrue); + }); + + it('changes NO read — every legacy row reads back exactly as it did before', () => { + for (const r of LEGACY) { + expect(afterRead.get(r.label), `${r.label} (${r.wasWritten})`).toStrictEqual( + beforeRead.get(r.label), + ); + } + // Spelled out for the two that matter most: the converted row still reads as + // its string, and the unrecoverable row still reads as the number it became. + expect(afterRead.get('l_plain')).toBe('America/New_York'); + expect(afterRead.get('l_int')).toBe(123); + expect(afterRead.get('l_obj')).toStrictEqual({ a: 1 }); + }); + + it('is IDEMPOTENT — a second run rewrites nothing and double-encodes nothing', () => { + expect(twiceDisk).toEqual(afterDisk); + // The specific failure a naive migration has: `"America/New_York"` becoming + // `"\"America/New_York\""` on the second pass. + expect(twiceDisk.l_plain.v).toBe('"America/New_York"'); + }); + + it('a table this call CREATED is skipped — no scan, nothing to converge', async () => { + const fresh = 'json_fresh_12380'; + await driver.execute(`drop table if exists ${fresh}`).catch(() => {}); + await driver.initObjects([{ name: fresh, fields: { ...FIELDS } }]); + await driver.create(fresh, { label: 'x', val: 'America/New_York' }, { bypassTenantAudit: true }); + // Written by the NEW codec, so it is already canonical. + expect((await diskCell(driver, cell, fresh, 'x')).v).toBe('"America/New_York"'); + await driver.execute(`drop table if exists ${fresh}`).catch(() => {}); + }); +}); +} + +// A matrix that silently finds zero cells reports OK — assert the axis is real +// before iterating it. +describe('[#12380] the dialect axis this suite runs', () => { + it('runs every dialect this driver speaks', () => { + expect(DIALECT_CELLS.map((c) => c.id)).toEqual(['sqlite', 'pg', 'mysql']); + }); +}); + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, 'json value round-trip', declareRoundTrip); +} +declareAffinity(); +declareMigration(); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 981b9c2021..d1bcf0e42f 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -9356,6 +9356,9 @@ export class SqlDriver implements IDataDriver { await this.backfillCanonicalDatetimes(tableName, exists); // #3994: the `Field.time` twin of the line above. await this.backfillCanonicalTimes(tableName, exists); + // #12380: converge this table's `Field.json` columns on the injective + // JSON-text storage form the rewritten `formatInput` now writes. + await this.backfillCanonicalJsonEncoding(tableName, exists); // #3942: the MySQL twin — widen legacy `TIMESTAMP` columns to `DATETIME(3)`. if (exists) await this.migrateMysqlDatetimeColumns(tableName, obj.fields ?? {}); // #3994: widen legacy MySQL `TIME` columns to `TIME(3)`. @@ -9452,6 +9455,104 @@ export class SqlDriver implements IDataDriver { } } + /** + * Converge one table's `Field.json` columns on the injective JSON-text + * storage form (#12380) — the `Field.json` twin of + * {@link backfillCanonicalDatetimes}, built the same way for the same reasons. + * + * SQLite only. Postgres and MySQL never had the defect: their half of + * `formatInput` has always been an unconditional `JSON.stringify`, so their + * rows are already in this exact format and there is nothing on disk to + * rewrite (measured 17/17 faithful on PG 16.13 and MySQL 8.0.46). + * + * ## What it converts, and the one class it converts + * + * ONE `UPDATE` per column. `json_quote()` is SQLite's own spelling of + * `JSON.stringify` over a scalar, so "what the canonical form means" has a + * single definition per dialect and the migration cannot drift from the codec + * it exists to serve. + * + * The `WHERE` names the ONLY on-disk class the pre-fix encoding left + * unambiguous — a TEXT cell that is not valid JSON. Nothing but a stored + * plain string could have produced one: `JSON.stringify` of an object or an + * array is always valid JSON, and every other input either stayed a primitive + * storage class or already parses. + * + * ⛔ **It does not guess, because the rest cannot be guessed.** Two classes are + * left exactly as they are, and both are named here rather than discovered: + * + * - **INTEGER/REAL cells** (`typeof` is not `'text'`). A number, a boolean, + * and a number-like string eaten by NUMERIC affinity are the SAME BYTES on + * disk — `123` the number and `'123'` the string are one INTEGER 123, and a + * boolean `true` is one INTEGER 1 alongside the number 1. No migration can + * know which was written, so this one asks nothing of them. This is the + * class the ruling accepts as unrecoverable: it stops growing (new writes + * encode injectively), it is not repaired. + * - **TEXT cells that DO parse.** A stored object `{"a":1}` and a stored + * STRING `'{"a":1}'` were byte-identical before this change, and both read + * back as the object under the pre-fix read path. Re-quoting them would + * turn every legacy object and array into a string — corrupting the common + * case to guess at the rare one. Left alone, they read after this change + * exactly as they read before it. + * + * ⇒ **This migration changes no read.** A legacy plain string reads back as + * that string before it runs (via `formatOutput`'s parse fallback) and after + * it runs (because `"…"` parses back to it). It is a canonicalisation that + * makes the on-disk format uniform and injective going forward — so a string + * written TOMORROW whose content is valid JSON is distinguishable from the + * structure it looks like — not a repair of what reads wrong today. + * + * ## Idempotent by construction, not by convention + * + * The `WHERE` is the exact complement of the `SET`'s output: `json_quote(X)` + * of a TEXT value is a quoted JSON string, for which `json_valid()` is 1, so + * a converted row cannot match the predicate again. Re-running costs one scan + * and zero writes — the same "a converged table is a no-op" property + * {@link backfillCanonicalDatetimes} has, and pinned the same way. + * + * ⚠️ An out-of-band reader of the SQLite file sees quoted JSON text where it + * saw a bare value. That is the accepted cost of the format, recorded here so + * it is read rather than discovered. + * + * Failures are logged and swallowed, exactly as in the datetime twin: the + * rows simply stay in the legacy form, `formatOutput`'s parse fallback keeps + * reading them correctly, and correctness never becomes contingent on a + * migration having run. That also covers a SQLite build without the JSON + * functions and a `skipSchemaSync` deployment that never reaches this path. + */ + protected async backfillCanonicalJsonEncoding(table: string, tableExisted: boolean): Promise { + const fields = this.jsonFields[table]; + if (!this.isSqlite || !fields || fields.length === 0) return; + // A table created by this very call is empty, so every json column in it is + // canonical without a single row being read. + if (!tableExisted) return; + + for (const field of fields) { + try { + const res = await this.knex.raw( + `update ?? set ?? = json_quote(??) where typeof(??) = 'text' and json_valid(??) = 0`, + [table, field, field, field, field], + ); + const converted = (res as any)?.changes ?? 0; + if (converted) { + this.logger.info?.( + `[sql-driver] canonicalised json storage (#12380) for ${table}.${field}`, + { rowsConverted: converted }, + ); + } + } catch (err) { + // Correctness does not depend on this succeeding: `formatOutput` keeps + // its parse fallback precisely so an un-migrated row still reads back + // as the string it is. + this.logger.warn( + `[sql-driver] could not canonicalise json storage for ${table}.${field}; ` + + `reads stay correct via formatOutput's parse fallback`, + { error: err instanceof Error ? err.message : String(err) }, + ); + } + } + } + /** * Converge one table's `Field.time` columns on the canonical time-of-day text * form (#3994) — the `Field.time` twin of {@link backfillCanonicalDatetimes}, @@ -14865,28 +14966,52 @@ export class SqlDriver implements IDataDriver { } } - // JSON field serialisation: PostgreSQL native jsonb columns require - // valid JSON for ALL values (strings, numbers, booleans, objects). - // SQLite stores JSON as plain TEXT so only objects/arrays need - // stringification (better-sqlite3 can only bind primitives). + // ── JSON field serialisation: ONE encoding, every dialect (#12380) ────── + // + // `JSON.stringify` unconditionally, which is what Postgres and MySQL have + // always done here. This DELETES the SQLite branch rather than adding one, + // and the reason is the declared contract, not strictness: `json`'s stored + // contract is `z.unknown()` (`spec/src/data/field-value.zod.ts`, "openness + // is now an explicit decision, not an accident of nobody checking"), so + // `123` the number and `'123'` the string are BOTH legal values of one + // field and no driver has license to collapse them onto one representation. + // + // What the deleted branch did, measured on live SQLite/PG 16.13/MySQL + // 8.0.46 before this change (PG and MySQL: 17/17 faithful; SQLite: 13/17 + // type-changed). It stored a non-object AS-IS, which broke the round trip + // through two INDEPENDENT mechanisms: + // + // 1. READ-side. `formatOutput` `JSON.parse`s every string in a json + // column, so a stored string whose CONTENT is valid JSON came back + // type-changed: `'true'` → boolean, `'null'` → null, `'[]'` → array, + // `'{"a":1}'` → object. Reversible — the bytes were still on disk. + // 2. WRITE-side, and NOT reversible. The column is declared type `json`, + // which contains none of `INT`/`CHAR`/`CLOB`/`TEXT`/`BLOB`/`REAL`/ + // `FLOA`/`DOUB`, so SQLite's affinity rules fall through to **NUMERIC** + // — and a bound number-like string is converted to INTEGER/REAL BEFORE + // storage. `'0123'`, `' 123 '`, `'1e5'`, `'1.0'` and `'-0'` were + // destroyed on disk, indistinguishable from the numbers they became. + // + // Stringifying closes BOTH without touching the DDL, because the encoded + // form of a string carries its quotes (`'123'` → `"123"`) and `"123"` is + // not a well-formed numeric literal, so NUMERIC affinity leaves it TEXT. + // Measured, not reasoned — see `sql-driver-12380-json-roundtrip.test.ts`. + // + // The on-disk delta for NEW writes is therefore exactly two classes: + // strings (now quoted) and booleans (now TEXT `true`/`false` instead of + // INTEGER 1/0, which is mechanism (3) the read path could never repair — + // `formatOutput`'s `booleanFields` pass is keyed to declared + // `Field.boolean` COLUMNS, not to booleans living inside a json payload). + // Objects, arrays and null are byte-identical to before, and so are + // numbers: `123` bound as a number and `"123"` bound as text both land as + // INTEGER 123 under NUMERIC affinity. Existing rows are converged by + // {@link backfillCanonicalJsonEncoding}. const jsonFields = this.jsonFields[object]; if (jsonFields && jsonFields.length > 0) { for (const field of jsonFields) { if (copy[field] === undefined || copy[field] === null) continue; - if (this.isSqlite) { - // SQLite: only objects/arrays need JSON.stringify; primitives - // are stored as-is and re-parsed on read by formatOutput. - if (typeof copy[field] === 'object') { - if (!copied) { copy = { ...copy }; copied = true; } - copy[field] = JSON.stringify(copy[field]); - } - } else { - // PostgreSQL: every value must be valid JSON so the native - // jsonb column accepts it. JSON.stringify wraps strings in - // quotes, leaves numbers/booleans unchanged as literals. - if (!copied) { copy = { ...copy }; copied = true; } - copy[field] = JSON.stringify(copy[field]); - } + if (!copied) { copy = { ...copy }; copied = true; } + copy[field] = JSON.stringify(copy[field]); } } @@ -14926,6 +15051,36 @@ export class SqlDriver implements IDataDriver { } if (this.isSqlite) { + // The exact inverse of `formatInput`'s `JSON.stringify` (#12380). Postgres + // and MySQL need no arm here because their clients already parse a native + // `json`/`jsonb` column; SQLite hands back the stored TEXT, so the driver + // parses it. One codec, three dialects, same answer. + // + // Only strings are parsed, and that is load-bearing rather than + // incidental: a json NUMBER is stored under NUMERIC affinity as INTEGER/ + // REAL and comes back as a JS number, which is already the value that was + // written — parsing is neither possible nor needed for it. + // + // ── Why the catch survives, and what it now means ─────────────────────── + // + // Nothing this driver writes can reach it: every new value on disk is the + // output of `JSON.stringify`, so it parses by construction. It is the + // READ-SIDE REPAIR for rows written before #12380 — a pre-fix plain string + // was stored raw (`America/New_York`), and re-quoting it is exactly what + // {@link backfillCanonicalJsonEncoding} does on the next `syncSchema`. + // Keeping it here is the same posture `backfillCanonicalDatetimes` takes: + // correctness must NEVER be contingent on a migration having run, so an + // un-migrated (or un-migratable, e.g. `skipSchemaSync`) deployment reads a + // legacy plain string back as that same string — byte-identical to what it + // read before this change — instead of throwing mid-row. + // + // ⚠️ It cannot repair the two classes the pre-fix encoding made ambiguous, + // and must not pretend to: a legacy TEXT cell that DOES parse (a stored + // object, or a stored string whose content was valid JSON) and a legacy + // INTEGER/REAL cell (a number, a boolean, or a number-like string eaten by + // NUMERIC affinity) are collisions already resolved on disk. Those rows + // read exactly as they read before this change — the class stops growing; + // it is not retroactively repaired. See the ruling recorded on #12380. const jsonFields = this.jsonFields[object]; if (jsonFields && jsonFields.length > 0) { for (const field of jsonFields) { @@ -14933,7 +15088,7 @@ export class SqlDriver implements IDataDriver { try { data[field] = JSON.parse(data[field]); } catch { - // keep as string + // Pre-#12380 row: keep the raw string, which IS its value. } } }