From 09cc81330ea5f507f5958a8c532f7a491ee46ab0 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:01:23 +0000 Subject: [PATCH 1/2] fix(cli,driver-sql): os migrate plan lists the datetime storage convergence (#3954) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The datetime canonicalisation (#3912/#3942) added two steps to initObjects' physical path: a row-rewriting backfill on SQLite and a TIMESTAMP → DATETIME(3) column rebuild on MySQL. Both already respected the DDL deferral, so plan performed neither and apply performed both — the behaviour was never wrong. The reporting was. PendingSchemaWork could only express create_table / add_columns, so an operator saw a plan listing two added columns, confirmed it, and apply additionally rewrote every row of a datetime column — or took a metadata lock to rebuild one on a large table. The plan promises to show what apply will do, so anything added to initObjects' physical path has to be representable there; the type now says so. - Two new kinds, `normalize_datetime_storage` and `widen_datetime_columns`, plus an optional `rows` carrying how much data the step touches: row-writes for the backfill, the table's size for the rebuild, because `ALTER … MODIFY` is a full rebuild holding a metadata lock and that is the number deciding "now" versus "in a maintenance window". - previewDeferredSchemaWork measures both without performing either. Each probe reuses the exact predicate its migration uses — the backfill's whole WHERE, the widening's own information_schema filter, now extracted to legacyMysqlTimestampColumns and shared — so the plan and the apply cannot name different sets. A probe that cannot run is swallowed to an unlisted step, never to a failed plan: under-reporting costs one line, throwing would cost the create/add entries too. - The CLI renders them under their own heading rather than folding them into the additive section, whose "created when you apply" framing carries an implicit promise that the work is never data-losing. That promise must not silently come to cover a row rewrite. summarizePendingSchemaWork — the line read just before typing y — never omits in-place work, and is unchanged when there is none. The convergence probe counts rows, so plan is more than metadata lookups on an un-migrated database. That is the right trade for a command the operator ran to be told the size of the job, and it is paid by plan/apply only, never on a normal boot. Tests: a SQLite suite covering listed/measured/not-performed, the add_columns combination the gap was worst in, idempotence after flush, and the swallowed-probe path (3 of 7 fail without the driver change); CLI renderer tests pinning the two-section split and the summary line; and two cases added to the opt-in MySQL suite, verified against a real MariaDB 10.11 — plan lists 3 columns and 5 rows without touching the schema, apply performs exactly that, re-plan is empty. Closes #3954 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- ...migrate-plan-lists-datetime-convergence.md | 31 ++++ .../schema-migrate.pending-render.test.ts | 108 ++++++++++++ packages/cli/src/utils/schema-migrate.ts | 73 ++++++-- packages/plugins/driver-sql/src/index.ts | 2 + .../plugins/driver-sql/src/schema-drift.ts | 54 +++++- .../sql-driver-datetime-mysql-storage.test.ts | 65 +++++++ ...iver-deferred-datetime-convergence.test.ts | 161 ++++++++++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 135 ++++++++++++--- 8 files changed, 586 insertions(+), 43 deletions(-) create mode 100644 .changeset/migrate-plan-lists-datetime-convergence.md create mode 100644 packages/cli/src/utils/schema-migrate.pending-render.test.ts create mode 100644 packages/plugins/driver-sql/src/sql-driver-deferred-datetime-convergence.test.ts diff --git a/.changeset/migrate-plan-lists-datetime-convergence.md b/.changeset/migrate-plan-lists-datetime-convergence.md new file mode 100644 index 0000000000..fef0e8d7d9 --- /dev/null +++ b/.changeset/migrate-plan-lists-datetime-convergence.md @@ -0,0 +1,31 @@ +--- +"@objectstack/driver-sql": minor +"@objectstack/cli": patch +--- + +fix(cli,driver-sql): `os migrate plan` lists the datetime storage convergence (#3954) + +The datetime canonicalisation (#3912/#3942) added two steps to `initObjects`' +physical path: a row-rewriting backfill on SQLite and a `TIMESTAMP` → +`DATETIME(3)` column rebuild on MySQL. Both already respected the DDL deferral, +so `plan` performed neither and `apply` performed both — the behaviour was never +wrong. The reporting was. + +`PendingSchemaWork` could only express `create_table` / `add_columns`, so an +operator saw a plan listing two added columns, confirmed it, and `apply` +additionally rewrote every row of a datetime column — or took a metadata lock to +rebuild one on a large table. The plan promises to show what apply will do. + +- `PendingSchemaWork.kind` gains `normalize_datetime_storage` and + `widen_datetime_columns`, plus an optional `rows` carrying how much data the + step touches: row-writes for the backfill, the table's size for the rebuild — + the number that decides "now" versus "in a maintenance window". +- `previewDeferredSchemaWork()` measures both without performing either, reusing + the exact predicate each migration uses (the backfill's whole `WHERE`, the + widening's own `information_schema` filter) so the plan and the apply cannot + name different sets. A probe that cannot run is swallowed to "unlisted", never + to a failed plan. +- The CLI renders them under their own heading rather than folding them into the + additive section, whose "created when you apply" framing carries an implicit + promise that the work is never data-losing. `summarizePendingSchemaWork` — the + line read just before typing `y` — never omits in-place work. diff --git a/packages/cli/src/utils/schema-migrate.pending-render.test.ts b/packages/cli/src/utils/schema-migrate.pending-render.test.ts new file mode 100644 index 0000000000..bf992e4b5f --- /dev/null +++ b/packages/cli/src/utils/schema-migrate.pending-render.test.ts @@ -0,0 +1,108 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3954 — how `os migrate plan` renders the pending work. + * + * The additive section carries an implicit promise: what it lists is created, + * never data-losing, and needs no `--allow-destructive` thought. The datetime + * convergence (#3912/#3942) rewrites rows and rebuilds columns, so folding it + * into that section would quietly extend the promise to cover a table rewrite. + * These tests pin the split, and pin that the summary line — the one an operator + * reads before typing "yes" — never omits in-place work. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { + renderPendingSchemaWork, + summarizePendingSchemaWork, + type PendingSchemaWork, +} from './schema-migrate.js'; + +let lines: string[]; + +beforeEach(() => { + lines = []; + vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { + lines.push(args.map(String).join(' ')); + }); +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +const out = () => lines.join('\n'); + +const ADDITIVE: PendingSchemaWork[] = [ + { table: 'widgets', kind: 'create_table', columns: ['sku', 'qty'] }, + { table: 'orders', kind: 'add_columns', columns: ['note'] }, +]; + +const IN_PLACE: PendingSchemaWork[] = [ + { table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'], rows: 1234567 }, + { table: 'legacy', kind: 'widen_datetime_columns', columns: ['at', 'created_at'], rows: 42 }, +]; + +describe('renderPendingSchemaWork (#3954)', () => { + it('renders nothing at all when there is nothing pending', () => { + renderPendingSchemaWork([]); + expect(out()).toBe(''); + }); + + it('keeps the additive section exactly as it was when only additive work is pending', () => { + renderPendingSchemaWork(ADDITIVE); + expect(out()).toContain('New (additive — created when you apply)'); + expect(out()).toContain('widgets'); + expect(out()).toContain('[create_table, 2 column(s)]'); + expect(out()).toContain('[add_columns: note]'); + // No second heading appears when there is no in-place work. + expect(out()).not.toContain('In place'); + }); + + it('puts the datetime convergence under its OWN heading, not the additive one', () => { + renderPendingSchemaWork(IN_PLACE); + expect(out()).toContain('In place (existing rows converged when you apply)'); + // The additive heading claims the work is never data-losing; a row rewrite + // must never be listed beneath it. + expect(out()).not.toContain('New (additive'); + }); + + it('names the columns and the size of each in-place step', () => { + renderPendingSchemaWork(IN_PLACE); + expect(out()).toContain('normalize_datetime_storage: at'); + expect(out()).toContain('1,234,567 row update(s)'); + expect(out()).toContain('widen_datetime_columns: at, created_at'); + // A MySQL widen is ALTER … MODIFY — a rebuild, said outright. + expect(out()).toContain('42 row table rebuild'); + }); + + it('shows both sections when both kinds are pending', () => { + renderPendingSchemaWork([...ADDITIVE, ...IN_PLACE]); + expect(out()).toContain('New (additive — created when you apply)'); + expect(out()).toContain('In place (existing rows converged when you apply)'); + }); + + it('reads an unmeasured count as unknown rather than zero', () => { + renderPendingSchemaWork([{ table: 'evt', kind: 'normalize_datetime_storage', columns: ['at'] }]); + expect(out()).toContain('? row update(s)'); + expect(out()).not.toContain('0 row update(s)'); + }); +}); + +describe('summarizePendingSchemaWork (#3954)', () => { + it('is unchanged for purely additive work', () => { + expect(summarizePendingSchemaWork(ADDITIVE)).toBe('1 table(s) to create, 1 column(s) to add'); + }); + + it('is unchanged when nothing is pending', () => { + expect(summarizePendingSchemaWork([])).toBe('0 table(s) to create, 0 column(s) to add'); + }); + + it('never omits in-place work — this is the line read before confirming', () => { + const summary = summarizePendingSchemaWork([...ADDITIVE, ...IN_PLACE]); + expect(summary).toContain('1 table(s) to create'); + expect(summary).toContain('1 column(s) to add'); + expect(summary).toContain('3 datetime column(s) to converge in place'); + expect(summary).toContain('~1,234,609 rows'); + }); +}); diff --git a/packages/cli/src/utils/schema-migrate.ts b/packages/cli/src/utils/schema-migrate.ts index 4f058e87cf..b967c51152 100644 --- a/packages/cli/src/utils/schema-migrate.ts +++ b/packages/cli/src/utils/schema-migrate.ts @@ -14,6 +14,7 @@ */ import chalk from 'chalk'; import type { ManagedDriftEntry, DriftCategory, PendingSchemaWork } from '@objectstack/driver-sql'; +import { isInPlaceSchemaWork } from '@objectstack/driver-sql'; import { describeDriverConnection } from './connection-display.js'; export type { PendingSchemaWork }; @@ -251,23 +252,59 @@ export function summarize(drift: ManagedDriftEntry[]): string { } /** - * Render the additive work the boot sync was held back from doing (#3917). + * Render the work the boot sync was held back from doing (#3917), in two + * sections split by whether it touches existing data (#3954). * - * Deliberately its own section rather than a `DriftCategory`: this is not - * divergence between metadata and an existing column — it is the create/add - * that used to happen silently at boot, now shown before it runs. Purely - * additive and never data-losing, so it carries no `--allow-destructive` gate. + * Deliberately its own block rather than a `DriftCategory`: this is not + * divergence between metadata and an existing column — it is what used to + * happen silently at boot, now shown before it runs. + * + * The split matters. The additive section tells the operator the work is never + * data-losing, and that promise must not quietly come to cover the datetime + * convergence, which rewrites rows (SQLite) or rebuilds a column (MySQL). Those + * get their own heading, and their row counts, because "how long will this hold + * the table" is the question they raise and the additive kinds do not. */ export function renderPendingSchemaWork(pending: PendingSchemaWork[]): void { if (pending.length === 0) return; - console.log(` ${chalk.bold('New (additive — created when you apply)')}`); - for (const p of pending) { - const detail = p.kind === 'create_table' - ? `[create_table, ${p.columns.length} column(s)]` - : `[add_columns: ${p.columns.join(', ')}]`; - console.log(` ${chalk.cyan('+')} ${chalk.cyan(p.table)} ${chalk.dim(detail)}`); + + const additive = pending.filter((p) => !isInPlaceSchemaWork(p.kind)); + const inPlace = pending.filter((p) => isInPlaceSchemaWork(p.kind)); + + if (additive.length > 0) { + console.log(` ${chalk.bold('New (additive — created when you apply)')}`); + for (const p of additive) { + const detail = p.kind === 'create_table' + ? `[create_table, ${p.columns.length} column(s)]` + : `[add_columns: ${p.columns.join(', ')}]`; + console.log(` ${chalk.cyan('+')} ${chalk.cyan(p.table)} ${chalk.dim(detail)}`); + } + console.log(''); + } + + if (inPlace.length > 0) { + console.log(` ${chalk.bold('In place (existing rows converged when you apply)')}`); + for (const p of inPlace) { + const label = p.kind === 'normalize_datetime_storage' + ? 'normalize_datetime_storage' + : 'widen_datetime_columns'; + // A MySQL widen is `ALTER … MODIFY`, i.e. a full table rebuild holding a + // metadata lock — worth saying outright, not just implying via the count. + const cost = p.kind === 'widen_datetime_columns' + ? `${formatRows(p.rows)} row table rebuild` + : `${formatRows(p.rows)} row update(s)`; + console.log( + ` ${chalk.yellow('~')} ${chalk.yellow(p.table)} ` + + `${chalk.dim(`[${label}: ${p.columns.join(', ')} — ${cost}]`)}`, + ); + } + console.log(''); } - console.log(''); +} + +/** `rows` is optional on the type; an unmeasured count reads as unknown, not zero. */ +function formatRows(rows: number | undefined): string { + return rows === undefined ? '?' : rows.toLocaleString('en-US'); } export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string { @@ -275,5 +312,15 @@ export function summarizePendingSchemaWork(pending: PendingSchemaWork[]): string const columns = pending .filter((p) => p.kind === 'add_columns') .reduce((n, p) => n + p.columns.length, 0); - return `${creates} table(s) to create, ${columns} column(s) to add`; + const parts = [`${creates} table(s) to create`, `${columns} column(s) to add`]; + + // Only mentioned when there is some, so the common in-sync summary is + // unchanged — but never omitted when there is, which is the #3954 point. + const inPlace = pending.filter((p) => isInPlaceSchemaWork(p.kind)); + if (inPlace.length > 0) { + const cols = inPlace.reduce((n, p) => n + p.columns.length, 0); + const rows = inPlace.reduce((n, p) => n + (p.rows ?? 0), 0); + parts.push(`${cols} datetime column(s) to converge in place (~${formatRows(rows)} rows)`); + } + return parts.join(', '); } diff --git a/packages/plugins/driver-sql/src/index.ts b/packages/plugins/driver-sql/src/index.ts index edb983fff9..964accf2ba 100644 --- a/packages/plugins/driver-sql/src/index.ts +++ b/packages/plugins/driver-sql/src/index.ts @@ -21,6 +21,7 @@ export { diffManagedIndexes, expectedIndexes, isIndexDriftOp, + isInPlaceSchemaWork, isManagedIndexName, legacyUniqueIndexNames, legacyUniqueReplacements, @@ -38,6 +39,7 @@ export type { ExpectedIndex, LegacyUniqueReplacement, PendingSchemaWork, + PendingSchemaWorkKind, FieldDef as DriftFieldDef, } from './schema-drift.js'; diff --git a/packages/plugins/driver-sql/src/schema-drift.ts b/packages/plugins/driver-sql/src/schema-drift.ts index 05cdca7399..f328def2b9 100644 --- a/packages/plugins/driver-sql/src/schema-drift.ts +++ b/packages/plugins/driver-sql/src/schema-drift.ts @@ -91,20 +91,60 @@ export type DriftOp = }; /** - * Physical schema work the *additive* boot sync is holding back (#3917). + * Physical work the boot sync is holding back (#3917). * * Distinct from {@link DriftOp}: drift is divergence between metadata and an * EXISTING column/index that only a deliberate reconcile may resolve, whereas - * this is the create-table / add-column work `initObjects` performs on its own - * — captured rather than executed while the driver runs with DDL deferred, so - * `os migrate plan` can show it and `os migrate apply` can gate it behind the - * confirmation prompt. + * this is the work `initObjects` performs on its own — captured rather than + * executed while the driver runs with DDL deferred, so `os migrate plan` can + * show it and `os migrate apply` can gate it behind the confirmation prompt. + * + * The plan's promise is that it shows what `apply` will do, so **anything added + * to `initObjects`' physical path has to be representable here** — otherwise an + * operator confirms a two-column plan and `apply` additionally rewrites a table. + * That is the gap #3954 closed for the datetime convergence; keep it closed. */ export interface PendingSchemaWork { table: string; - kind: 'create_table' | 'add_columns'; - /** Declared columns for a create; the missing ones for an add. */ + kind: PendingSchemaWorkKind; + /** + * Declared columns for a create; the missing ones for an add; the columns + * being converged for the two datetime steps. + */ columns: string[]; + /** + * How much data the step touches, when that is knowable up front and worth + * knowing — absent for the additive kinds, which touch none. + * + * For `normalize_datetime_storage` it is the number of row-writes (summed + * across `columns`, since each is its own `UPDATE`). For + * `widen_datetime_columns` it is the table's row count, because MySQL's + * `ALTER … MODIFY` is a full rebuild holding a metadata lock — which is the + * number that decides "now" versus "in a maintenance window". + */ + rows?: number; +} + +/** + * What kind of physical work a {@link PendingSchemaWork} entry represents. + * + * The first two are purely additive and never touch existing rows. The datetime + * pair is NOT: `normalize_datetime_storage` rewrites rows in place (the SQLite + * canonical-UTC backfill) and `widen_datetime_columns` rebuilds a column (the + * MySQL `TIMESTAMP` → `DATETIME(3)` widening) — both from #3912/#3942. They are + * rendered under their own heading for that reason: the additive section tells + * the operator the work is never data-losing, and that claim must not silently + * come to cover a row rewrite. + */ +export type PendingSchemaWorkKind = + | 'create_table' + | 'add_columns' + | 'normalize_datetime_storage' + | 'widen_datetime_columns'; + +/** True for the kinds that rewrite or rebuild existing data rather than adding to it. */ +export function isInPlaceSchemaWork(kind: PendingSchemaWorkKind): boolean { + return kind === 'normalize_datetime_storage' || kind === 'widen_datetime_columns'; } /** Ops that act on an index rather than a column — reconciled without a table rebuild. */ diff --git a/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts b/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts index 08b4f0a3fd..6c3c6e87d4 100644 --- a/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts +++ b/packages/plugins/driver-sql/src/sql-driver-datetime-mysql-storage.test.ts @@ -224,6 +224,71 @@ describe.skipIf(!URL)('MySQL TIMESTAMP → DATETIME(3) migration (#3942)', () => }); }); +describe.skipIf(!URL)('os migrate plan lists the MySQL widening (#3954)', () => { + const LEGACY = 'os3954_legacy'; + const SHAPE = { name: LEGACY, fields: { label: { type: 'string' }, at: { type: 'datetime' } } }; + let driver: SqlDriver; + + beforeEach(async () => { + const legacy = rawDriver(); + await legacy.execute(`drop table if exists ${LEGACY}`); + await legacy.execute( + `create table ${LEGACY} ( + id varchar(255) not null primary key, + created_at timestamp null default current_timestamp, + updated_at timestamp null default current_timestamp, + label varchar(255) null, + at timestamp null + )`, + ); + await legacy.execute(`set time_zone = '+00:00'`); + for (let i = 0; i < 5; i++) { + await legacy.execute(`insert into ${LEGACY} (id, label, at) values (?, ?, ?)`, [ + `e${i}`, `e${i}`, '2026-03-20 12:00:00', + ]); + } + await legacy.disconnect(); + driver = new SqlDriver({ client: 'mysql2', connection: URL }); + }); + + afterEach(async () => { + await driver.execute(`drop table if exists ${LEGACY}`).catch(() => {}); + await driver.disconnect(); + }); + + it('reports the widening, its columns and the table size — without performing it', async () => { + driver.setDeferredDdl(true); + await driver.initObjects([SHAPE]); + + const pending = await driver.previewDeferredSchemaWork(); + const widen = pending.filter((p) => p.kind === 'widen_datetime_columns'); + expect(widen).toHaveLength(1); + expect(widen[0].table).toBe(LEGACY); + // The audit columns are declared datetime too, so all three are rebuilt. + expect([...widen[0].columns].sort()).toEqual(['at', 'created_at', 'updated_at']); + // `ALTER … MODIFY` is a full rebuild holding a metadata lock, so the table's + // size is the number that decides "now" versus "in a maintenance window". + expect(widen[0].rows).toBe(5); + + // Planning must not have touched the schema. + expect((await columnTypes(driver, LEGACY)).at).toBe('timestamp'); + }); + + it('applies exactly what it planned, and then finds nothing left', async () => { + driver.setDeferredDdl(true); + await driver.initObjects([SHAPE]); + const planned = await driver.previewDeferredSchemaWork(); + expect(planned.some((p) => p.kind === 'widen_datetime_columns')).toBe(true); + + await driver.flushDeferredSchemaDdl(); + expect((await columnTypes(driver, LEGACY)).at).toBe('datetime(3)'); + + driver.setDeferredDdl(true); + await driver.initObjects([SHAPE]); + expect(await driver.previewDeferredSchemaWork()).toEqual([]); + }); +}); + // ── helpers ───────────────────────────────────────────────────────────────── /** mysql2 hands back `[rows, fields]`; normalise to just the rows. */ diff --git a/packages/plugins/driver-sql/src/sql-driver-deferred-datetime-convergence.test.ts b/packages/plugins/driver-sql/src/sql-driver-deferred-datetime-convergence.test.ts new file mode 100644 index 0000000000..32fc21da3b --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-deferred-datetime-convergence.test.ts @@ -0,0 +1,161 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3954 — `os migrate plan` must list the datetime storage convergence. + * + * The datetime canonicalisation (#3912/#3942) added two steps to `initObjects`' + * physical path: a row-rewriting backfill on SQLite and a column rebuild on + * MySQL. Both correctly respect the DDL deferral, so `plan` performs neither and + * `apply` performs both — the behaviour was never wrong. + * + * What was wrong is the reporting. `PendingSchemaWork` could only express + * `create_table` / `add_columns`, so an operator saw a plan listing two added + * columns, confirmed it, and `apply` additionally rewrote every row of a + * datetime column. The plan promises to show what apply will do. + * + * These tests pin three things: the step is listed, it is measured, and it is + * still not *performed* by the preview. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from './sql-driver.js'; +import { LegacyStorageDriver } from './legacy-datetime-storage.testkit.js'; + +const EVT = { + name: 'evt', + fields: { + label: { type: 'text' }, + at: { type: 'datetime' }, + seen_at: { type: 'datetime' }, + }, +} as any; + +const make = (Ctor: new (cfg: any) => T): T => + new Ctor({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }); + +/** A database an older build wrote: legacy datetime forms, backfill not yet run. */ +async function seedLegacyDatabase(driver: LegacyStorageDriver) { + await driver.initObjects([EVT]); + await driver.seedLegacyRows('evt', 'at', [ + { id: 'e1', label: 'a', at: Date.parse('2026-03-20T12:00:00.000Z'), seen_at: '2026-03-20T12:00:00.000Z' }, + { id: 'e2', label: 'b', at: '2026-03-20 13:00:00', seen_at: '2026-03-20T13:00:00.000Z' }, + { id: 'e3', label: 'c', at: '2026-03-20T14:00:00+08:00', seen_at: '2026-03-20T06:00:00.000Z' }, + ]); + // `seen_at` was written canonical, so only `at` should be reported. + driver.forgetCanonical('evt', 'seen_at'); +} + +describe('os migrate plan lists the datetime convergence (#3954)', () => { + let driver: LegacyStorageDriver; + + afterEach(async () => { + await driver.disconnect(); + }); + + it('reports the pending backfill, naming the columns and the row count', async () => { + driver = make(LegacyStorageDriver); + await seedLegacyDatabase(driver); + + driver.setDeferredDdl(true); + await driver.initObjects([EVT]); + const pending = await driver.previewDeferredSchemaWork(); + + const converge = pending.filter((p) => p.kind === 'normalize_datetime_storage'); + expect(converge).toHaveLength(1); + expect(converge[0].table).toBe('evt'); + // Only the column that actually holds non-canonical rows. + expect(converge[0].columns).toEqual(['at']); + expect(converge[0].rows).toBe(3); + }); + + it('does not PERFORM the backfill while previewing it', async () => { + driver = make(LegacyStorageDriver); + await seedLegacyDatabase(driver); + const before = await driver.storedForms('evt', 'at'); + + driver.setDeferredDdl(true); + await driver.initObjects([EVT]); + await driver.previewDeferredSchemaWork(); + + // The whole point of the deferral: plan measures, apply changes. + expect(await driver.storedForms('evt', 'at')).toEqual(before); + }); + + it('flushing the deferral converges the rows the plan named', async () => { + driver = make(LegacyStorageDriver); + await seedLegacyDatabase(driver); + + driver.setDeferredDdl(true); + await driver.initObjects([EVT]); + const planned = await driver.previewDeferredSchemaWork(); + expect(planned.some((p) => p.kind === 'normalize_datetime_storage')).toBe(true); + + await driver.flushDeferredSchemaDdl(); + + for (const row of await driver.storedForms('evt', 'at')) { + expect(String(row.value)).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/); + } + // And re-planning now finds nothing left to converge. + driver.setDeferredDdl(true); + await driver.initObjects([EVT]); + const after = await driver.previewDeferredSchemaWork(); + expect(after.filter((p) => p.kind === 'normalize_datetime_storage')).toEqual([]); + }); + + it('reports nothing for a database that is already canonical', async () => { + driver = make(LegacyStorageDriver); + await driver.initObjects([EVT]); + await driver.create('evt', { id: 'ok', label: 'ok', at: '2026-03-20T12:00:00.000Z' }, { bypassTenantAudit: true }); + + driver.setDeferredDdl(true); + await driver.initObjects([EVT]); + expect(await driver.previewDeferredSchemaWork()).toEqual([]); + }); + + it('reports nothing for a table that does not exist yet — it is created empty', async () => { + driver = make(LegacyStorageDriver); + driver.setDeferredDdl(true); + await driver.initObjects([EVT]); + + const pending = await driver.previewDeferredSchemaWork(); + expect(pending.map((p) => p.kind)).toEqual(['create_table']); + }); + + it('reports the convergence ALONGSIDE an add_columns on the same table', async () => { + // The combination the gap was worst in: a plan that lists one added column + // while apply also rewrites the table. + driver = make(LegacyStorageDriver); + await seedLegacyDatabase(driver); + + const widened = { ...EVT, fields: { ...EVT.fields, note: { type: 'text' } } }; + driver.setDeferredDdl(true); + await driver.initObjects([widened]); + const pending = await driver.previewDeferredSchemaWork(); + + expect(pending.map((p) => p.kind).sort()).toEqual(['add_columns', 'normalize_datetime_storage']); + expect(pending.find((p) => p.kind === 'add_columns')!.columns).toEqual(['note']); + }); + + it('survives a probe that cannot run, rather than failing the plan', async () => { + // Same posture as the migrations themselves, which log and swallow: a probe + // that cannot run costs an UNLISTED step, while throwing would cost the whole + // plan — including the create/add entries that have nothing to do with it. + class BrokenProbe extends LegacyStorageDriver { + protected override sqliteCanonicalDatetimeSql(): string { + throw new Error('probe exploded'); + } + } + const broken = make(BrokenProbe); + try { + await seedLegacyDatabase(broken); + broken.setDeferredDdl(true); + await broken.initObjects([{ ...EVT, fields: { ...EVT.fields, note: { type: 'text' } } }]); + + const pending = await broken.previewDeferredSchemaWork(); + expect(pending.map((p) => p.kind)).toEqual(['add_columns']); + } finally { + await broken.disconnect(); + } + driver = make(LegacyStorageDriver); // satisfy afterEach + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 2c8f93d246..2b4e31c4ab 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -2731,6 +2731,43 @@ export class SqlDriver implements IDataDriver { } } + /** + * The `Field.datetime` (and audit) columns of `table` that MySQL still stores + * as a legacy `TIMESTAMP`, with the nullability each must keep. + * + * Shared by {@link migrateMysqlDatetimeColumns}, which widens them, and + * {@link previewDatetimeConvergence}, which reports them into `os migrate + * plan` (#3954) — so what the plan lists and what apply does are the same set + * by construction, not by two filters that agree today. + * + * `[]` on any dialect but MySQL, and on a table declaring no datetime column. + */ + protected async legacyMysqlTimestampColumns( + table: string, + fields: Record, + ): Promise> { + if (!this.isMysql) return []; + const candidates = new Set(AUDIT_TIMESTAMP_COLUMNS); + for (const [name, field] of Object.entries(fields)) { + if ((field?.type ?? 'string') === 'datetime' && !field?.multiple) candidates.add(name); + } + if (candidates.size === 0) return []; + + const res: any = await this.knex.raw( + `select column_name, is_nullable from information_schema.columns + where table_schema = database() and table_name = ? and data_type = 'timestamp'`, + [table], + ); + // mysql2 returns [rows, fields]; column names vary in case by server. + const rows: any[] = Array.isArray(res?.[0]) ? res[0] : (res?.rows ?? res ?? []); + return rows + .map((r) => ({ + name: String(r.COLUMN_NAME ?? r.column_name ?? ''), + nullable: String(r.IS_NULLABLE ?? r.is_nullable ?? 'YES').toUpperCase() !== 'NO', + })) + .filter((c) => c.name && candidates.has(c.name)); + } + /** * Widen a table's legacy MySQL `TIMESTAMP` datetime columns to `DATETIME(3)` * (#3942) — the MySQL counterpart of {@link backfillCanonicalDatetimes}. @@ -2758,26 +2795,8 @@ export class SqlDriver implements IDataDriver { fields: Record, ): Promise { if (!this.isMysql) return; - const candidates = new Set(AUDIT_TIMESTAMP_COLUMNS); - for (const [name, field] of Object.entries(fields)) { - if ((field?.type ?? 'string') === 'datetime' && !field?.multiple) candidates.add(name); - } - if (candidates.size === 0) return; - try { - const res: any = await this.knex.raw( - `select column_name, is_nullable from information_schema.columns - where table_schema = database() and table_name = ? and data_type = 'timestamp'`, - [table], - ); - // mysql2 returns [rows, fields]; column names vary in case by server. - const rows: any[] = Array.isArray(res?.[0]) ? res[0] : (res?.rows ?? res ?? []); - const legacy = rows - .map((r) => ({ - name: String(r.COLUMN_NAME ?? r.column_name ?? ''), - nullable: String(r.IS_NULLABLE ?? r.is_nullable ?? 'YES').toUpperCase() !== 'NO', - })) - .filter((c) => c.name && candidates.has(c.name)); + const legacy = await this.legacyMysqlTimestampColumns(table, fields); if (legacy.length === 0) return; for (const col of legacy) { @@ -2835,15 +2854,23 @@ export class SqlDriver implements IDataDriver { /** * What the deferred sync *would* do, without doing it. * - * Read-only: `hasTable` + `columnInfo`, the same two probes the additive sync - * uses to decide between create and alter. Tables and columns that already - * match metadata produce no entry, so an in-sync database returns `[]`. + * Read-only: `hasTable` + `columnInfo` decide between create and alter (the + * same two probes the additive sync uses), then + * {@link previewDatetimeConvergence} asks whether the datetime storage steps + * have anything left to do. Tables that already match metadata and hold + * canonical data produce no entry, so an in-sync database returns `[]`. + * + * The convergence probe COUNTS rows, so this is more than metadata lookups on + * a database that has not been migrated yet. That is the right trade for a + * command the operator ran to be told the size of the job — and it is paid + * once, by `plan`/`apply`, never on a normal boot. */ async previewDeferredSchemaWork(): Promise { const out: PendingSchemaWork[] = []; for (const [tableName, obj] of this.deferredSchemaObjects) { const declared = Object.keys(obj.fields ?? {}); if (!(await this.knex.schema.hasTable(tableName))) { + // A table that does not exist yet is created empty, so nothing to converge. out.push({ table: tableName, kind: 'create_table', columns: declared }); continue; } @@ -2852,11 +2879,73 @@ export class SqlDriver implements IDataDriver { if (missing.length > 0) { out.push({ table: tableName, kind: 'add_columns', columns: missing }); } + out.push(...(await this.previewDatetimeConvergence(tableName, obj.fields ?? {}, existing))); } - out.sort((a, b) => a.table.localeCompare(b.table)); + out.sort((a, b) => a.table.localeCompare(b.table) || a.kind.localeCompare(b.kind)); return out; } + /** + * The datetime storage-convergence work {@link backfillCanonicalDatetimes} and + * {@link migrateMysqlDatetimeColumns} would do for `table` — measured, not + * performed (#3954). + * + * Both steps run inside `initObjects` alongside the create/alter path, so + * `apply` performs them; without an entry here `plan` would show a two-column + * change and `apply` would additionally rewrite every row of a datetime column + * or rebuild one on a large table. The plan promises to show what apply does. + * + * Each probe reuses the very predicate its migration uses, so the preview + * cannot claim work the migration will not do (or miss work it will): + * - SQLite counts rows matching `col IS NOT ` — the backfill's + * entire `WHERE`. + * - MySQL lists the candidate columns still typed `timestamp` — the + * migration's own `information_schema` filter. + * + * Failures are swallowed to `[]`, matching the migrations themselves: a probe + * that cannot run must not fail the plan, and under-reporting here costs an + * unlisted step rather than a wrong one. + */ + protected async previewDatetimeConvergence( + table: string, + fields: Record, + existingColumns: Set, + ): Promise { + try { + if (this.isSqlite) { + const declared = [...(this.datetimeFields[table] ?? [])].filter((c) => existingColumns.has(c)); + if (declared.length === 0) return []; + const canonical = this.sqliteCanonicalDatetimeSql('??'); + const columns: string[] = []; + let rows = 0; + for (const field of declared) { + const res: any = await this.knex.raw( + `select count(*) as n from ?? where ?? is not null and ?? is not ${canonical}`, + [table, field, field, field, field, field, field], + ); + const n = Number((Array.isArray(res) ? res[0] : res)?.n ?? 0); + if (n > 0) { columns.push(field); rows += n; } + } + return columns.length === 0 + ? [] + : [{ table, kind: 'normalize_datetime_storage', columns, rows }]; + } + + if (this.isMysql) { + const legacy = await this.legacyMysqlTimestampColumns(table, fields); + if (legacy.length === 0) return []; + const res: any = await this.knex.raw(`select count(*) as n from ??`, [table]); + const counted = Array.isArray(res?.[0]) ? res[0] : (res?.rows ?? res ?? []); + const rows = Number((counted[0] as any)?.n ?? (counted as any)?.n ?? 0); + return [{ table, kind: 'widen_datetime_columns', columns: legacy.map((c) => c.name), rows }]; + } + + return []; + } catch { + return []; + } + } + /** * Run the deferred sync and disarm the deferral. Returns the work that was * outstanding (captured before the DDL ran, so the caller can report what it From 7da1454e083acf9e21f60d24ffb95dd7acc7f781 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 00:03:31 +0000 Subject: [PATCH 2/2] docs(cli): show the in-place section in the documented migrate-plan output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `deployment/cli.mdx` prints a sample of what `os migrate plan` renders before the [y/N] prompt, under the heading "everything apply is about to do is on screen". Adding a second section to that output without adding it here would have made the doc quietly incomplete in exactly the way #3954 is about. Also spells out what separates the two: New never touches a row, In place rewrites existing data and therefore carries a row count — and on MySQL is a table rebuild holding a metadata lock. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ --- content/docs/deployment/cli.mdx | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index 2b54421c19..b0e5156a76 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -511,12 +511,27 @@ performed. So `plan` really is a dry run, and everything `apply` is about to do + crm_quote [create_table, 9 column(s)] + crm_contact [add_columns: nickname, region] + In place (existing rows converged when you apply) + ~ crm_contact [normalize_datetime_storage: signed_at — 1,240 row update(s)] + Safe (loosening — applied without --allow-destructive) ✓ crm_contact.email [relax_not_null] ``` Answering `n` leaves the database exactly as it was. +The two upper sections differ in a way worth reading carefully. **New** is +purely additive — it creates tables and columns and never touches a row. **In +place** rewrites existing data: the storage-form convergence a `Field.datetime` +column needs when the database predates the canonical UTC storage (ADR-0053 +addendum D-B1..D-B4). It carries a row count because that is the number +deciding whether to run it now; on MySQL it reads `widen_datetime_columns` and +is an `ALTER … MODIFY` table rebuild that holds a metadata lock for its +duration. + +Both are safe to apply — the convergence preserves every stored instant and is +idempotent — but only the second takes time proportional to your data. + #### Occupancy check (SQLite) A running `os dev` / `os serve` holding the same SQLite file open is the usual