From a4acde50a35ed22c963d0376ccc21ba9980f6272 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:24:59 +0000 Subject: [PATCH 1/2] fix(driver-sql): report a multi-value field left on a stale varchar/text column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A field that gains `multiple: true` materialises as a `json` column on a fresh database, but `initObjects` is additive-only: on an existing database nothing is missing, so the old varchar/text column is kept and every array written to it is stored as the stringified literal and read back as a string. Measured on live Postgres 16.13 and MySQL 8.0.46 on the pre-fix tree, `detectManagedDrift()` returned `[]` for exactly that shape. Detection only. The column is not migrated: an `ALTER TABLE ... TYPE json USING` over existing rows plus an index rebuild is a destructive migration over shipped data, and whether the platform should perform it is a separate open decision. The new `manual_column_type_change` op has no reconciler arm by design. Severity `error`, category `needs_confirm` — measured, not chosen for tone: the artifact-pinned boot gate refuses a boot for `category === 'destructive'` and nothing else, and every database this describes is already serving. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn --- ...t-detect-multi-value-base-type-mismatch.md | 63 ++++ .../schema-drift.base-type-mismatch.test.ts | 320 ++++++++++++++++++ .../drivers/driver-sql/src/schema-drift.ts | 199 +++++++++++ 3 files changed, 582 insertions(+) create mode 100644 .changeset/drift-detect-multi-value-base-type-mismatch.md create mode 100644 packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts diff --git a/.changeset/drift-detect-multi-value-base-type-mismatch.md b/.changeset/drift-detect-multi-value-base-type-mismatch.md new file mode 100644 index 0000000000..bf8b6eef9f --- /dev/null +++ b/.changeset/drift-detect-multi-value-base-type-mismatch.md @@ -0,0 +1,63 @@ +--- +'@objectstack/driver-sql': minor +--- + +Report a multi-value field left on a stale `varchar`/`text` column, instead of +letting it silently corrupt every array written to it + +A field that gains `multiple: true` materialises as a `json` column on a fresh +database, but `initObjects` is additive-only: on a database created while the +field was single-value, nothing is missing, so nothing is added and the old +`varchar`/`text` column is kept forever. The write path stringifies the array +for a json field on every non-SQLite dialect; the read path relies on the +driver's column-type-based decoding, which a stale textual column defeats. The +array goes in as the literal `["id1","id2"]` and comes back as a **string** — +so a hook copying the value into a child record's single-lookup column writes +that whole string as one id. User-filed production report, repaired by hand on a +live database. + +Until now the schema-drift detector said **nothing** about it. Measured on the +pre-fix tree against live Postgres 16.13 and MySQL 8.0.46: after the metadata +change and a reboot, `detectManagedDrift()` returned `[]` and the boot logged +zero `[schema-drift]` lines, while the very next write stored +`["user_A","user_B"]` into a `character varying(255)` column and read it back +with `typeof === 'string'`. The action vocabulary had no "the base type is +wrong" entry at all — only `relax`/`tighten_not_null`, `widen`/`narrow_varchar`, +`drop_column`, `drop_column_default` and the index ops. + +The divergence is now **detected and reported**, naming the table, the column, +the declared type, the physical type and the exact statement an operator runs by +hand — dialect-correct, and executed against both live servers by the suite +rather than merely printed. ObjectStack does **not** change the column: an +`ALTER TABLE … TYPE json USING …` over existing rows with an index drop and +rebuild is a destructive migration over shipped data, and whether the platform +should perform it is a separate, open decision. The new `manual_column_type_change` +op deliberately has no reconciler arm; `applyMigrationEntries` reports it as +skipped, which is the intended contract while that decision is open. + +Reported at severity `error` and category **`needs_confirm`**, and the category +is load-bearing rather than cosmetic. Every database this finding describes is +already serving — that is the premise of the report — and the artifact-pinned +boot gate refuses a boot for `category === 'destructive'` and nothing else +(`severity` it never reads). Measured both ways: a `destructive` entry returns +`ok=false` from that gate, this entry returns `ok=true`. Spelling it +`destructive` would have turned every affected deployment into a crash-loop on +its next restart — the report of the corruption becoming the outage. + +SQLite is deliberately excluded, and the exclusion is a measurement rather than a +scoping convenience: the same stale column reads back as a real `['x','y']` +array there, because SQLite's read path `JSON.parse`s regardless of what the +column calls itself. There is no corruption to report, and reporting it anyway +would put a permanent `error` finding on every long-lived SQLite development +database. A stale `integer`/`timestamp` column is excluded for the mirror-image +reason — the server already refuses that write loudly, so there is no silence to +break. + +Also fixed, same defect class: a multi-value field that *also* declared +`maxLength` used to produce `narrow_varchar` at severity `error`, category +**destructive** on both enforcing dialects — a finding that refuses the +artifact-pinned boot and invites `os migrate apply --allow-destructive` to +rewrite the column to `varchar(50)`, the exact opposite of the repair it needs. +`createColumn` returns at its `multiple` branch before `maxLength` is ever read, +so the emitter never asks for that width; the differ no longer does either. The +single-value width branch is untouched and pinned as untouched. diff --git a/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts b/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts new file mode 100644 index 0000000000..54041686ba --- /dev/null +++ b/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts @@ -0,0 +1,320 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11535] A field that becomes MULTI-VALUE over an existing database keeps its + * old `varchar`/`text` column, and until this suite existed nothing said so. + * + * User-filed production report: a `lookup` gained `multiple: true` on a + * long-lived Postgres database, the column stayed `character varying`, and the + * next write stored the array as the literal string `["id1","id2"]`. Reads hand + * that string back verbatim, so a hook copying the value into a child record's + * single-lookup column wrote the whole string as ONE id — silent data + * corruption, repaired by hand in production. + * + * ## What distinguishes this defect, and therefore what this suite must pin + * + * The shape produced **silence**, not a wrong answer. `detectManagedDrift()` + * returned `[]` and the boot logged nothing — measured on live Postgres 16.13 + * and MySQL 8.0.46 on the pre-fix tree. So "a finding was produced" is a weak + * assertion here; every case below pins the finding on the RIGHT COLUMN with the + * RIGHT DIAGNOSIS, and the neighbouring shapes that must stay silent are pinned + * as silent in the same breath. + * + * ## The detection half only + * + * ObjectStack does NOT migrate the column. Whether it should is the other half + * of #11535 and a live maintainer decision — a migration over existing rows plus + * an index drop/rebuild is destructive and hard to roll back. This suite pins + * the reporting, and pins that the reporting changes no deployment's ability to + * boot (see the category case, which is not a tautology — read its comment). + * + * ## Three dialects, and SQLite's absence is a MEASUREMENT + * + * SQLite never exposed this: its read path `JSON.parse`s regardless of what the + * column calls itself. So the same stale column corrupts on Postgres and MySQL + * and does not on SQLite, and the live half asserts BOTH — including that + * SQLite's silence sits next to a value that round-trips correctly, which is + * what makes the silence right rather than a missed detection. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { SqlDriver } from './sql-driver.js'; +import { diffManagedTable, manualJsonConversionSql, type PhysicalColumn, type SqlDialectName } from './schema-drift.js'; +import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; + +const MATRIX = 'multi-value base-type drift'; + +/** The column a stale database still carries, per dialect's own spelling. */ +const staleColumn = (type: string, maxLength?: number): PhysicalColumn[] => [ + { name: 'tags', type, nullable: true, maxLength }, +]; + +const diffTags = (field: Record, columns: PhysicalColumn[], dialect: SqlDialectName) => + diffManagedTable({ table: 'proj_task', fields: { tags: field }, columns, dialect }); + +// ── Half 1: the differ, on every dialect's own type spelling ──────────────── + +describe('diffManagedTable — multi-value field over a stale textual column (#11535)', () => { + it('names the column, the declared type and the physical type — Postgres spelling', () => { + const out = diffTags({ type: 'lookup', multiple: true }, staleColumn('character varying', 255), 'postgres'); + + // Not `toHaveLength(1)` alone: the defect is a MISSING report, so what is + // asserted is that the report identifies the right column and says the + // right thing about it. + expect(out).toHaveLength(1); + expect(out[0]).toMatchObject({ + table: 'proj_task', + column: 'tags', + kind: 'type_mismatch', + expected: 'json', + actual: 'character varying', + op: { type: 'manual_column_type_change', table: 'proj_task', column: 'tags', to: 'json', from: 'character varying' }, + }); + }); + + it('fires on MySQL too, carrying MySQL’s own type word', () => { + const out = diffTags({ type: 'lookup', multiple: true }, staleColumn('varchar', 255), 'mysql'); + expect(out).toHaveLength(1); + expect(out[0].actual).toBe('varchar'); + }); + + it('fires on a stale TEXT column, not only varchar — a text column takes the stringified literal just as happily', () => { + const out = diffTags({ type: 'string', multiple: true }, staleColumn('text'), 'postgres'); + expect(out).toHaveLength(1); + expect(out[0].actual).toBe('text'); + }); + + it('the message names the table, the column, both types and the hand-run remedy', () => { + const [entry] = diffTags({ type: 'lookup', multiple: true }, staleColumn('character varying', 255), 'postgres'); + // An operator reading one log line has to be able to act without opening + // the source, so every element of the diagnosis is pinned individually. + expect(entry.message).toContain('proj_task.tags'); + expect(entry.message).toContain('json'); + expect(entry.message).toContain('character varying'); + expect(entry.message).toContain('#11535'); + // The remedy is the real statement, not a gesture at one. + expect(entry.message).toContain(manualJsonConversionSql('postgres', 'proj_task', 'tags')); + // The orphaned single-value index is part of the same picture: a json + // column cannot carry a plain btree, so the remedy has to mention it. + expect(entry.message).toMatch(/btree/i); + }); + + it('the MySQL remedy is MySQL’s, not Postgres’ statement with different quotes', () => { + const [entry] = diffTags({ type: 'lookup', multiple: true }, staleColumn('varchar', 255), 'mysql'); + expect(entry.message).toContain(manualJsonConversionSql('mysql', 'proj_task', 'tags')); + // MySQL will not cast text to json implicitly — the row rewrite has to come + // first or the ALTER dies on the first legacy value. + expect(entry.message).toContain('JSON_ARRAY'); + expect(entry.message).not.toContain('json_build_array'); + }); + + // ── the shapes that must stay SILENT ───────────────────────────────────── + + it('says nothing when the column is already `json` — the healthy database', () => { + expect(diffTags({ type: 'lookup', multiple: true }, staleColumn('json'), 'postgres')).toEqual([]); + expect(diffTags({ type: 'lookup', multiple: true }, staleColumn('json'), 'mysql')).toEqual([]); + }); + + it('says nothing on SQLite, where the same stale column corrupts nothing', () => { + // Not a scoping convenience: the live half below measures that the value + // round-trips as a real array on SQLite. Reporting here would put a + // permanent `error` finding on every long-lived SQLite dev database for a + // divergence that changes no value. + expect(diffTags({ type: 'lookup', multiple: true }, staleColumn('varchar', 255), 'sqlite')).toEqual([]); + }); + + it('says nothing about a stale INTEGER column — the server already refuses that write loudly', () => { + // The finding exists to break a SILENCE. A column that rejects + // `'["a","b"]'` outright (Postgres 22P02, MySQL ER_TRUNCATED_WRONG_VALUE) + // is not silent, so there is no silence to break. + expect(diffTags({ type: 'integer', multiple: true }, staleColumn('integer'), 'postgres')).toEqual([]); + expect(diffTags({ type: 'datetime', multiple: true }, staleColumn('timestamp with time zone'), 'postgres')).toEqual([]); + }); + + it('leaves the single-value varchar-width branch (#11431) exactly where it was', () => { + // The guard added for multi-value fields must not have cost the neighbouring + // branch its reach — a fix that silences the thing next to it is a + // regression wearing a green suite. + const out = diffTags({ type: 'string', maxLength: 50 }, staleColumn('character varying', 255), 'postgres'); + expect(out).toHaveLength(1); + expect(out[0].op.type).toBe('narrow_varchar'); + }); + + it('a MULTI-VALUE field with a maxLength reports the base type ONCE, never `narrow_varchar`', () => { + // Measured on the pre-fix tree: this shape produced `narrow_varchar` at + // severity `error`, category DESTRUCTIVE on both enforcing dialects — a + // finding that refuses the artifact-pinned boot and invites + // `os migrate apply --allow-destructive` to rewrite the column to + // `varchar(50)`, the exact opposite of the repair it needs. `createColumn` + // returns at its `multiple` branch before `maxLength` is read, so the + // emitter never asks for that width and the differ must not either. + for (const dialect of ['postgres', 'mysql'] as const) { + const out = diffTags({ type: 'string', multiple: true, maxLength: 50 }, staleColumn('character varying', 255), dialect); + expect(out.map((d) => d.op.type)).toEqual(['manual_column_type_change']); + } + }); + + // ── the severity/category pin — the one that keeps deployments booting ──── + + it('is severity `error` but category `needs_confirm`, so it reports loudly and refuses NO boot', () => { + const [entry] = diffTags({ type: 'lookup', multiple: true }, staleColumn('character varying', 255), 'postgres'); + + expect(entry.severity).toBe('error'); + + // ⛔ This is not a style assertion, and `needs_confirm` is not a softer way + // of saying `destructive`. Measured downstream, on the real consumers: + // + // - `runArtifactBootMigrationGate` (packages/cli, runs on `kernel:ready` + // BEFORE the HTTP socket opens, and its refusal is a thrown boot + // failure) refuses the boot for `category === 'destructive'` and for + // nothing else — `severity` it never reads. Measured: a `destructive` + // entry returns `ok=false`, this entry returns `ok=true`. + // - Dev auto-reconcile applies `category === 'safe'` only, so this is + // never applied unattended either. + // + // Every database this finding describes is ALREADY SERVING — that is the + // premise of the report it came from. Flipping this to `destructive` would + // turn a running (if corrupt) deployment into a crash-loop on its next + // restart, i.e. the report of the corruption would become the outage. + expect(entry.category).toBe('needs_confirm'); + expect(entry.category).not.toBe('destructive'); + }); +}); + +// ── Half 2: end to end, on every provisioned dialect ──────────────────────── + +const TABLE = 'os11535_task'; +const singleValueMeta = [{ name: TABLE, fields: { name: { type: 'string' }, tags: { type: 'string' } } }]; +const multiValueMeta = [{ name: TABLE, fields: { name: { type: 'string' }, tags: { type: 'string', multiple: true } } }]; + +class DriftProbeDriver extends SqlDriver { + columnsOf(table: string) { + return this.introspectColumns(table); + } +} + +function declareBaseTypeDriftSuite(cell: DialectCell): void { + describe(`multi-value base-type drift — ${cell.label} (#11535)`, () => { + const corrupts = cell.id !== 'sqlite'; + let driver: DriftProbeDriver; + let physicalType: string; + let readBack: unknown; + + beforeAll(async () => { + driver = new DriftProbeDriver(cell.config()); + await driver.connect(); + await driver.execute(`drop table if exists ${TABLE}`).catch(() => {}); + + // 1. the OLD database: the field is single-value, so the column is textual + await driver.initObjects(singleValueMeta as any); + await driver.create(TABLE, { name: 'legacy', tags: 'a' }); + + // 2. the metadata change + reboot. `initObjects` is additive-only: nothing + // is missing, so nothing is added, and the column is never revisited. + await driver.initObjects(multiValueMeta as any); + + physicalType = (await driver.columnsOf(TABLE)).find((c) => c.name === 'tags')!.type; + + // 3. the write that corrupts (or, on SQLite, does not) + await driver.create(TABLE, { name: 'multi', tags: ['x', 'y'] }); + const rows = await driver.find(TABLE, { filters: [] } as any); + readBack = rows.find((r: any) => r.name === 'multi')!.tags; + }); + + afterAll(async () => { + await driver?.execute(`drop table if exists ${TABLE}`).catch(() => {}); + await driver?.disconnect().catch(() => {}); + }); + + it('the fixture is real: the column stayed textual while metadata declares a json column', async () => { + // Non-vacuity first. Every assertion below is about a STALE column; if the + // sync had migrated it, they would all pass for the wrong reason. + expect(physicalType).toMatch(/char|text/i); + + // And a table built from the SAME metadata on a fresh database gets json — + // which is what makes the column above stale rather than simply correct. + const fresh = `${TABLE}_fresh`; + await driver.execute(`drop table if exists ${fresh}`).catch(() => {}); + await driver.initObjects([{ name: fresh, fields: { tags: { type: 'string', multiple: true } } }] as any); + const freshType = (await driver.columnsOf(fresh)).find((c) => c.name === 'tags')!.type; + expect(freshType).toMatch(/json/i); + expect(freshType).not.toBe(physicalType); + await driver.execute(`drop table if exists ${fresh}`).catch(() => {}); + }); + + it(corrupts + ? 'the value IS corrupted here — it reads back as the stringified literal, not an array' + : 'the value is NOT corrupted here — it reads back as a real array', () => { + // This is the fact the finding exists to describe, asserted against the + // server rather than assumed from the dialect's name. It is also what + // makes SQLite's silence correct instead of a missed detection. + if (corrupts) { + expect(typeof readBack).toBe('string'); + expect(Array.isArray(readBack)).toBe(false); + expect(readBack).toBe('["x","y"]'); + } else { + expect(Array.isArray(readBack)).toBe(true); + expect(readBack).toEqual(['x', 'y']); + } + }); + + it(corrupts + ? 'detectManagedDrift() reports the stale column, naming it and both types' + : 'detectManagedDrift() stays silent, because there is nothing here to corrupt', async () => { + const drift = await driver.detectManagedDrift(); + const found = drift.filter((d) => d.op.type === 'manual_column_type_change'); + + if (!corrupts) { + expect(found).toEqual([]); + return; + } + + expect(found).toHaveLength(1); + expect(found[0].table).toBe(TABLE); + expect(found[0].column).toBe('tags'); + expect(found[0].expected).toBe('json'); + expect(found[0].actual).toBe(physicalType); + expect(found[0].severity).toBe('error'); + expect(found[0].category).toBe('needs_confirm'); + }); + + it.skipIf(!corrupts)('the remedy the finding prints actually works, and clears the finding', async () => { + // An operator-facing remedy nobody runs is a remedy that drifts into being + // wrong. This runs the emitted statement verbatim against the live server, + // over rows in every state a stale column holds. + const dialect = cell.id === 'pg' ? 'postgres' : 'mysql'; + await driver.execute( + cell.id === 'pg' + ? `insert into ${TABLE} (id, name, tags) values ('e', 'empty', '')` + : `insert into ${TABLE} (id, name, tags) values ('e', 'empty', '')`, + ); + await driver.execute(`insert into ${TABLE} (id, name) values ('n', 'nulled')`); + + for (const stmt of manualJsonConversionSql(dialect, TABLE, 'tags').split(';').map((s) => s.trim()).filter(Boolean)) { + await driver.execute(stmt); + } + + expect((await driver.columnsOf(TABLE)).find((c) => c.name === 'tags')!.type).toMatch(/json/i); + + // The finding is GONE — the report is not a permanent nag once the + // operator has acted. + const after = await driver.detectManagedDrift(); + expect(after.filter((d) => d.op.type === 'manual_column_type_change')).toEqual([]); + + // And the data is in the shape the declaration promises, for every row + // state: the corrupted array is an array again, a legacy single value has + // become a one-element array, and NULL/'' stay empty rather than becoming + // `[null]` (which `json_build_array(NULL)` would have produced). + const rows = await driver.find(TABLE, { filters: [] } as any); + const byName = new Map(rows.map((r: any) => [r.name, r.tags])); + expect(byName.get('multi')).toEqual(['x', 'y']); + expect(byName.get('legacy')).toEqual(['a']); + expect(byName.get('empty') ?? null).toBeNull(); + expect(byName.get('nulled') ?? null).toBeNull(); + }); + }); +} + +for (const cell of DIALECT_CELLS) { + declareDialectCell(cell, MATRIX, declareBaseTypeDriftSuite); +} diff --git a/packages/drivers/driver-sql/src/schema-drift.ts b/packages/drivers/driver-sql/src/schema-drift.ts index a163829f20..4d47c83ebd 100644 --- a/packages/drivers/driver-sql/src/schema-drift.ts +++ b/packages/drivers/driver-sql/src/schema-drift.ts @@ -144,6 +144,29 @@ export type DriftOp = * standing rule is report, never rewrite. */ | { type: 'drop_column_default'; table: string; column: string } + /** + * REPORT ONLY (#11535). The column's base type diverges from the one the + * declaration materialises, and **the platform deliberately does not change + * it** — an operator must, by hand. + * + * ⛔ There is NO reconciler arm for this op, and adding one is not a + * refactor. Whether ObjectStack should perform the `ALTER TABLE … TYPE … + * USING …` itself is a **live maintainer decision** (the other half of + * #11535): it is a migration over existing rows plus an index drop/rebuild, + * i.e. destructive and hard to roll back. This op exists so the divergence + * can be SEEN travelling the same plan/report road as every other finding — + * it is the absence of an automatic migration made explicit, not a + * placeholder for one. + * + * Measured consequence of having no arm, on live Postgres 16.13 (the same on + * MySQL 8.0.46): `applyDriftOpInPlace` matches no case and returns `false`, + * so `applyMigrationEntries` reports the entry as **skipped, never applied**, + * and logs it. That is the intended behaviour, not a gap to close. + * + * `from`/`to` are the physical and declared type words, carried so a renderer + * can show the divergence without re-deriving it from the message. + */ + | { type: 'manual_column_type_change'; table: string; column: string; to: string; from: string } /** * Retire the legacy platform-wide UNIQUE index on a now-tenant-scoped field * and put the composite `(tenantField, field)` in its place (#3696). The two @@ -406,6 +429,98 @@ function isCharacterColumn(type: string | undefined): boolean { return /char/i.test(String(type ?? '')); } +/** + * Is this physical column one that ACCEPTS a stringified JSON array without + * complaint — i.e. `varchar`/`char`/`text` in any dialect's spelling (#11535)? + * + * Wider than {@link isCharacterColumn} by exactly the TEXT family, and that + * width is the point rather than an accident: the write path stringifies a + * multi-value field's array, and a text column takes the literal as happily as + * a varchar does. Both are the silent-corruption shape. + * + * Deliberately NOT "anything that is not json". A stale `integer` or + * `timestamp` column under a now-multi-value field is already LOUD — Postgres + * refuses `'["a","b"]'` with `22P02 invalid input syntax`, MySQL with + * `ER_TRUNCATED_WRONG_VALUE` — so it needs no finding to become visible, and + * matching it here would report a divergence the database itself already + * refuses. The textual family is the one that says yes and corrupts. + */ +function acceptsStringifiedJson(type: string | undefined): boolean { + return /char|text/i.test(String(type ?? '')); +} + +/** + * Does a multi-value field's JSON column carry its type on THIS dialect — i.e. + * does a stale textual column silently corrupt the value (#11535)? + * + * Postgres and MySQL: yes. Measured end to end on live Postgres 16.13 and MySQL + * 8.0.46 — a field that gained `multiple: true` over a pre-existing + * `varchar(255)` column round-trips as the LITERAL STRING `["a","b"]` + * (`typeof === 'string'`, `Array.isArray === false`), because the write path + * stringifies for a json field on every non-SQLite dialect while the read path + * relies on the driver's column-type-based decoding, which a stale textual + * column defeats. + * + * SQLite: **no**, and the exclusion is measured rather than assumed. The same + * stale column reads back as a real `['a','b']` array there (the read path + * `JSON.parse`s on SQLite regardless of what the column calls itself), so there + * is no corruption to report — reporting it anyway would put a permanent + * `error` finding on every long-lived SQLite development database for a + * divergence that changes no value. SQLite's column type is an affinity label, + * not an enforced type, which is the same reason + * {@link enforcesVarcharLength} excludes it. + */ +function multiValueColumnTypeIsLoadBearing(dialect: SqlDialectName): boolean { + return dialect === 'postgres' || dialect === 'mysql'; +} + +/** + * The hand-run statement that converts a stale textual column to `json`, + * spelled for the dialect the operator is actually on. + * + * The Postgres form keeps the SHAPE of the reporter's own production workaround + * (#11535) — the three-way CASE over the three states a stale column's rows are + * actually in — with one arm changed, for a reason that was measured rather than + * reasoned: the reporter's `to_json(col)` turns a legacy single value into the + * JSON **scalar** `"a"`, under a field the metadata now declares MULTI-VALUE. On + * live Postgres 16.13 that row read back as a string with `Array.isArray === + * false`, i.e. still not the shape the declaration promises, while live MySQL + * 8.0.46's `JSON_ARRAY(col)` produced `["a"]`. `json_build_array` makes the two + * dialects hand back the same value for the same row, which is the standing rule + * here — one declaration with two enforcement answers is the defect class this + * package's conformance matrices exist to close. + * + * Both forms are EXECUTED against live servers by + * `schema-drift.base-type-mismatch.test.ts`, over rows in every state the column + * can be in (legacy single value, already-stringified array, empty string, + * NULL), and the finding is asserted to clear afterwards — an operator-facing + * remedy nobody runs is a remedy that drifts into being wrong. + */ +export function manualJsonConversionSql(dialect: SqlDialectName, table: string, column: string): string { + if (dialect === 'mysql') { + // MySQL will not cast text to json implicitly: rows holding a legacy single + // value have to become one-element arrays FIRST, or the ALTER fails with + // `ER_INVALID_JSON_TEXT` on the first non-JSON row. + return ( + `UPDATE \`${table}\` SET \`${column}\` = JSON_ARRAY(\`${column}\`) ` + + `WHERE \`${column}\` IS NOT NULL AND \`${column}\` <> '' AND LEFT(\`${column}\`, 1) <> '['; ` + + `UPDATE \`${table}\` SET \`${column}\` = NULL WHERE \`${column}\` = ''; ` + + `ALTER TABLE \`${table}\` MODIFY \`${column}\` json;` + ); + } + return ( + `ALTER TABLE "${table}" ALTER COLUMN "${column}" TYPE json USING ` + + // The `IS NULL` arm is not redundant with the `= ''` one and is not + // decoration: `json_build_array(NULL)` is `[null]`, a one-element array, so + // without it every NULL row silently gains a value. Measured on live + // Postgres 16.13 while writing this — the arm exists because the version + // without it was run and produced `[null]`. + `(CASE WHEN "${column}" IS NULL THEN NULL WHEN "${column}" = '' THEN NULL ` + + `WHEN "${column}" LIKE '[%' THEN "${column}"::json ` + + `ELSE json_build_array("${column}") END);` + ); +} + /** * Diff one table's metadata fields against its physical columns and return the * set of *drift* findings. Metadata is authoritative. @@ -518,6 +633,76 @@ export function diffManagedTable(args: { }); } + // ── base type: a multi-value field over a stale textual column (#11535) ── + // + // `createColumn` materialises a multi-value field as `table.json(name)` — + // its FIRST branch, taken before the field's type or `maxLength` is read at + // all. On a database created while the field was single-value the column is + // `varchar`/`text`, and the additive sync (`ALTER TABLE ADD COLUMN`) can + // never revisit it: nothing here is missing, so nothing is added. + // + // Until this branch existed the divergence was reported by NOTHING. + // Measured on live Postgres 16.13 and MySQL 8.0.46 on the pre-fix tree: a + // `lookup` field that gained `multiple: true` over an existing + // `character varying(255)` column produced `detectManagedDrift() === []` and + // zero `[schema-drift]` log lines, while the very next write stored the + // literal string `["user_A","user_B"]` into the column and read it back as a + // string. Downstream code that copies the value into a single-value column + // then writes that whole string as one id — the silent corruption reported + // in #11535, which a display-layer glitch it is not. + // + // ## Severity `error`, category `needs_confirm` — both MEASURED, and the + // ## category is load-bearing in a way the words do not suggest + // + // ⛔ Do NOT "correct" this to `destructive` to match how bad it sounds. + // What consumes `category` was measured, not inferred: + // + // - The artifact-pinned boot gate (`runArtifactBootMigrationGate`, on + // `kernel:ready` before the HTTP socket opens) refuses the boot for + // `category === 'destructive'` and nothing else. Measured: a + // `destructive` entry yields `ok=false`; this entry as written yields + // `ok=true`. Every database this finding describes is ALREADY SERVING — + // that is the premise of the report — so a `destructive` spelling would + // convert a running (if corrupt) deployment into a crash-loop on the + // next restart. Reporting a corruption must not be the thing that takes + // the app down. + // - Dev auto-reconcile takes `category === 'safe'` only, so + // `needs_confirm` is never applied unattended either. + // - `severity` is read by NO gate: the ordinary boot path warns on every + // entry regardless of it. It is the render weight (`✗` in + // `os migrate plan`), which is why `error` is both honest and free. + // + // The residue is stated rather than hidden: `os migrate apply` hands a + // `needs_confirm` entry to the reconciler, which — having no arm for this op + // by design — declines it (`applied=0, skipped=1`) and says so. A finding + // that is reported every time and applied never is exactly the contract + // while the automatic migration remains the maintainer's open decision. + const declaresJsonColumn = field.multiple === true; + if (declaresJsonColumn && multiValueColumnTypeIsLoadBearing(dialect) && acceptsStringifiedJson(col.type)) { + out.push({ + kind: 'type_mismatch', + remoteName: table, + table, + column: fieldName, + expected: 'json', + actual: col.type, + severity: 'error', + category: 'needs_confirm', + op: { type: 'manual_column_type_change', table, column: fieldName, to: 'json', from: col.type }, + message: + `${table}.${fieldName}: metadata declares a multi-value field (stored as \`json\`) but the ` + + `column is \`${col.type}\` — the database was created while the field was single-value and the ` + + `additive sync never migrates a column's type. Arrays are being written as the STRINGIFIED ` + + `literal (e.g. '["a","b"]') and read back as a string, so anything consuming the value ` + + `receives one opaque id instead of a list (#11535). ObjectStack will NOT change this column ` + + `for you. Migrate it by hand, in a transaction, with a backup taken first — dropping any ` + + `index on the column first, since a json column cannot carry a plain btree: ` + + `${manualJsonConversionSql(dialect, table, fieldName)} ` + + `Rows written while the column was stale may already hold a stringified array in a RELATED ` + + `single-value column; those are not repaired by the statement above.`, + }); + } + // ── varchar length (only where the dialect enforces it) ────────── // // `maxLength` must be a POSITIVE INTEGER to be a bound (#11431). Without @@ -535,12 +720,26 @@ export function diffManagedTable(args: { // disagreeing about which declarations count is the defect class #11431 // exists to close, and a differ that still honoured a malformed // `maxLength` would have re-opened it one case to the left. + // + // A MULTI-VALUE field is excluded for the same reason and by the same + // authority: `createColumn` returns at `if (field.multiple) { table.json(); + // return; }` BEFORE `maxLength` is consulted, so the emitter provably never + // gives such a field a declared width, and a differ that honours one is the + // two-halves-disagree defect #11431 exists to close — one case to the left + // again. Measured on the pre-fix tree, `{ multiple: true, maxLength: 50 }` + // over a stale `varchar(255)` column reported `narrow_varchar` at severity + // `error`, category **destructive** on both enforcing dialects: a finding + // that refuses the artifact-pinned boot and invites `os migrate apply + // --allow-destructive` to rewrite the column to `varchar(50)` — the exact + // OPPOSITE of the repair the column needs, which is `json`. That shape is + // now reported once, correctly, by the base-type branch above. const declaredMaxLength = typeof field.maxLength === 'number' && Number.isInteger(field.maxLength) && field.maxLength > 0 ? field.maxLength : undefined; if ( enforcesVarcharLength(dialect) && + !declaresJsonColumn && declaredMaxLength !== undefined && isCharacterColumn(col.type) && typeof col.maxLength === 'number' && From ecd52f335e6a0ead04b5115de7eb87eac7b959f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:51:04 +0000 Subject: [PATCH 2/2] test(driver-sql): type the drift suite's find() queries instead of casting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{ filters: [] } as any` was not merely untyped — `filters` is not a DriverQuery key at all (`where` is), so the cast was hiding a wrong shape while adding two sites to the query-options-erasure test-surface ratchet. An empty query is the typed spelling of "all rows". Part of #11535 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VK8rFDtg8eREaxBGX99Csn --- .../driver-sql/src/schema-drift.base-type-mismatch.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts b/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts index 54041686ba..440bd93150 100644 --- a/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts +++ b/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts @@ -217,7 +217,7 @@ function declareBaseTypeDriftSuite(cell: DialectCell): void { // 3. the write that corrupts (or, on SQLite, does not) await driver.create(TABLE, { name: 'multi', tags: ['x', 'y'] }); - const rows = await driver.find(TABLE, { filters: [] } as any); + const rows = await driver.find(TABLE, {}); readBack = rows.find((r: any) => r.name === 'multi')!.tags; }); @@ -305,7 +305,7 @@ function declareBaseTypeDriftSuite(cell: DialectCell): void { // state: the corrupted array is an array again, a legacy single value has // become a one-element array, and NULL/'' stay empty rather than becoming // `[null]` (which `json_build_array(NULL)` would have produced). - const rows = await driver.find(TABLE, { filters: [] } as any); + const rows = await driver.find(TABLE, {}); const byName = new Map(rows.map((r: any) => [r.name, r.tags])); expect(byName.get('multi')).toEqual(['x', 'y']); expect(byName.get('legacy')).toEqual(['a']);