From 351768b8d8fe0e02254918e98ce140c207b0bd4f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 22:34:29 +0000 Subject: [PATCH 1/2] =?UTF-8?q?feat(cli):=20os=20migrate=20multi-value-col?= =?UTF-8?q?umns=20=E2=80=94=20operator-run=20stale=20column=20migration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator-run half of #11535, ruled C on #11700: the platform warns and ships an explicit migration an operator invokes, and never runs it for them. The statement is the one driver-sql's `manual_column_type_change` finding prints (#11720, measured against live Postgres 16.13 / MySQL 8.0.46); the command refuses to execute anything the finding does not contain verbatim. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../migrate-multi-value-columns-command.md | 5 + content/docs/deployment/cli.mdx | 74 +++ .../multi-value-columns.dry-run.test.ts | 253 ++++++++ .../multi-value-columns.no-auto-run.test.ts | 132 ++++ ...ulti-value-columns.remedy-fidelity.test.ts | 173 +++++ .../commands/migrate/multi-value-columns.ts | 603 ++++++++++++++++++ .../cli/test/json-stdout-purity.e2e.test.ts | 1 + 7 files changed, 1241 insertions(+) create mode 100644 .changeset/migrate-multi-value-columns-command.md create mode 100644 packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts create mode 100644 packages/cli/src/commands/migrate/multi-value-columns.no-auto-run.test.ts create mode 100644 packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts create mode 100644 packages/cli/src/commands/migrate/multi-value-columns.ts diff --git a/.changeset/migrate-multi-value-columns-command.md b/.changeset/migrate-multi-value-columns-command.md new file mode 100644 index 0000000000..80f90eb3c1 --- /dev/null +++ b/.changeset/migrate-multi-value-columns-command.md @@ -0,0 +1,5 @@ +--- +"@objectstack/cli": minor +--- + +New operator-run command `os migrate multi-value-columns`: migrates a stale `varchar`/`text` column to `json` where the field declares `multiple: true` — the `manual_column_type_change` drift `os migrate apply` reports and deliberately never reconciles for you (#11535, ruled C on #11700). Flags: `--apply` (default off), `--yes`/`-y`, `--force`, `--table ` (repeatable), `--database-url`, `--json`. **Dry-run contract: without `--apply` the command executes nothing at all** — it prints the exact statements and the database they would run against, opens no seam and issues no probe, and a run is verified to have left the column type and every row unchanged. `--apply` runs the statement the drift finding itself prints (Postgres: one `ALTER … USING (CASE …)` with `json_build_array`; MySQL: the two row-shaping `UPDATE`s then `ALTER … MODIFY … json`), refuses to execute anything the finding does not contain verbatim, re-runs detection afterwards and exits non-zero if the finding has not cleared. SQLite is excluded — the stale column round-trips a real array there, so the finding is never raised. Rows corrupted before the column is migrated are out of scope, and the command is never invoked automatically: nothing on the boot path reaches it. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index c53a4dc8f4..e101d9c9e1 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -543,6 +543,7 @@ diverges from the live schema, and the physical column wins at write time. |---------|-------------| | `os migrate plan` | Dry-run: show how the database has drifted from metadata, categorised safe / needs-confirm / destructive (no changes applied) | | `os migrate apply` | Reconcile the database to metadata. Applies loosening changes; destructive ones require `--allow-destructive` | +| `os migrate multi-value-columns` | Migrate a stale `varchar`/`text` column to `json` where the field declares `multiple: true` — the one drift op `apply` never reconciles for you. Dry run by default; `--apply` runs the statement the finding prints | ```bash os migrate plan # Preview drift (no changes) @@ -664,6 +665,79 @@ first. It never drops a table that is absent from your metadata, and on SQLite it reconciles via a table rebuild (copy → swap) that preserves your data. +#### `os migrate multi-value-columns` + +The one drift op `os migrate apply` will **never** apply for you. + +A field that gains `multiple: true` over a database that already exists keeps +its old `varchar` / `text` column: the additive sync adds columns, and never +changes the type of one that is already there. The write path then stores an +array as the **stringified literal** `'["a","b"]'` and reads it back as a +string, so whatever consumes the value receives one opaque id instead of a +list — a hook copying it into a child record's single-value lookup writes the +whole string as one id. `os migrate plan` reports it as +`manual_column_type_change`, at severity `error` and category `needs-confirm`, +which is why it neither refuses your boot nor is ever reconciled automatically: +changing a column's type on a serving production database, unattended, is not +something the platform will do while you are not watching. + +```bash +os migrate multi-value-columns # Dry run: the exact statements, executed NOT AT ALL +os migrate multi-value-columns --json # The same, machine-readable +os migrate multi-value-columns --apply # Run them (prompts) +os migrate multi-value-columns --apply --yes --json # CI / scripts +os migrate multi-value-columns --table crm_case # Restrict to one physical table (repeatable) +os migrate multi-value-columns --database-url postgres://… +``` + +**Take a backup first.** The dry run is the default and writes nothing at all — +not a probe, not a temporary table — so run it, read the statements it prints, +and only then re-run with `--apply`. + +The statement is the one the drift finding itself prints, per dialect, and the +command refuses to run anything else: if the finding no longer contains a +statement the command recognises, it says so and tells you to apply the +finding's statement by hand rather than falling back to SQL of its own. + +| Dialect | What runs | +|---------|-----------| +| PostgreSQL | One `ALTER TABLE … ALTER COLUMN … TYPE json USING (CASE …)`. Legacy single values become one-element arrays (`json_build_array`, **not** `to_json`, which would produce a JSON *scalar* that is still not an array); an already-stringified array is cast through; `NULL` and `''` both become `NULL` | +| MySQL | Three statements in order: `UPDATE … JSON_ARRAY(…)` over the legacy single values, `UPDATE … SET … = NULL` over the empty strings, then `ALTER TABLE … MODIFY … json`. MySQL will not cast text to json implicitly, so the rows have to move first or the `ALTER` dies on the first legacy value | +| SQLite | Nothing — and nothing is needed. SQLite's read path parses the value regardless of what the column calls itself, so the same stale column round-trips a real array. The finding is never raised there | + +After a successful run the command re-runs detection and requires the finding to +be **gone**; a run whose statements succeeded while the column is still reported +exits non-zero rather than telling you it migrated something it did not. + + +**Rollback.** The conversion is not information-preserving: both `NULL` and the +empty string become `NULL`, so once it succeeds those two states cannot be told +apart again — **restoring your backup is the only faithful rollback**, which is +why there is no `--undo`. + +Reverting only the column *type* (Postgres: `ALTER TABLE … ALTER COLUMN … TYPE +text USING …::text`) leaves JSON text in a text column; metadata still declares +the field multi-value, so the finding returns on the next boot and the +corruption resumes on the next write. Treat it as an incident stopgap, not a +rollback. + +On **PostgreSQL** the whole remedy is one statement: if it fails, the column is +untouched and there is nothing to roll back. On **MySQL** it is three, and DDL +there commits implicitly — a failure midway leaves the table partly converted. +Re-run the command: each statement skips the rows a previous run already moved, +so finishing an interrupted run is safe. + +If the `ALTER` fails naming an **index**, drop the index on that column first (a +json column cannot carry a plain btree) and re-run, then recreate it in a shape +your dialect supports for json. + + +**Rows corrupted before you migrate the column are yours to repair.** This +command converts the column and the values *in* it. A stringified array that a +hook or an integration already copied into some *other* single-value column is +not something it looks for, and it is deliberately not something it will grow +into: that repair is specific to what your automations did with the value. + #### Data migrations The commands above reconcile **schema**. A *data* migration rewrites rows, and diff --git a/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts b/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts new file mode 100644 index 0000000000..1f21826b31 --- /dev/null +++ b/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts @@ -0,0 +1,253 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11733] The dry run writes NOTHING — measured against a real database, not + * asserted about a mock. + * + * ## Why this is the pin this command most needs + * + * "Shows you what it would run" and "quietly runs it" produce the same + * successful-looking report. An operator reads the statements, sees no error, + * and concludes nothing happened — so a dry run that executed would be + * discovered by its consequences, on a production table, weeks later. A test + * asserting only that the dry run PRINTED something would pass in exactly that + * world. + * + * So the reading here is the database itself: the column's declared type and + * every row, snapshotted before and after, required IDENTICAL. + * + * ## The positive control is half the evidence + * + * An unchanged snapshot proves nothing unless the same instrument can be shown + * observing a change. Every "nothing changed" case below is paired with an + * apply run over the same fixture, through the same snapshot function, which + * must show the column type AND the rows moving. A snapshot that never moves is + * an instrument, not a result. + * + * ## What SQLite is doing in a suite about Postgres and MySQL + * + * Two different questions, deliberately split: + * + * - **is the statement right?** — answered where it can be: #11720 EXECUTES + * the real remedy against live Postgres 16.13 and MySQL 8.0.46 over four + * row states, and `multi-value-columns.remedy-fidelity.test.ts` pins that + * this command runs that exact statement. + * - **does the executor honour the dry run?** — that is dialect-independent + * control flow, and answering it needs a database this suite can actually + * open. The statements below are a SQLite-legal stand-in shaped like the + * real remedy (rewrite the values, change the column's type); they exist to + * make the change VISIBLE, and are never the statements the command builds. + * + * The real remedy is covered here too, in the last case: planned from the + * engine's own finding and dry-run against an `exec` that throws if touched. + */ + +import { describe, it, expect, beforeEach, afterAll } from 'vitest'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { SqlDriver, diffManagedTable, type PhysicalColumn } from '@objectstack/driver-sql'; +import { + runStaleColumnMigration, + planStaleColumnTargets, + type RawExec, + type StaleColumnPlan, +} from './multi-value-columns.js'; + +const TABLE = 'os11733_task'; +const dirs: string[] = []; + +let driver: SqlDriver; +let knex: any; + +/** Fresh database per case — an apply run is destructive by design. */ +async function freshFixture(): Promise { + const dir = mkdtempSync(join(tmpdir(), 'os-11733-dry-')); + dirs.push(dir); + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: join(dir, 'app.db') }, + useNullAsDefault: true, + }); + knex = (driver as any).knex; + await knex.raw(`CREATE TABLE ${TABLE} (id text primary key, tags text)`); + // The four row states a stale column is actually in — the same four #11720 + // runs the live remedy over. + await knex.raw(`INSERT INTO ${TABLE} (id, tags) VALUES ('legacy', 'a')`); + await knex.raw(`INSERT INTO ${TABLE} (id, tags) VALUES ('multi', '["x","y"]')`); + await knex.raw(`INSERT INTO ${TABLE} (id, tags) VALUES ('empty', '')`); + await knex.raw(`INSERT INTO ${TABLE} (id, tags) VALUES ('nulled', NULL)`); +} + +/** + * The two readings the dry run must leave alone: what the column CALLS ITSELF, + * and what is stored in it. + */ +async function snapshot(): Promise<{ columnType: string; rows: unknown[] }> { + const info = await knex.raw(`pragma table_info(${TABLE})`); + const tags = (info as Array<{ name: string; type: string }>).find((c) => c.name === 'tags'); + return { + columnType: String(tags?.type ?? ''), + rows: await knex.raw(`SELECT id, tags FROM ${TABLE} ORDER BY id`), + }; +} + +/** + * A SQLite-legal stand-in for the remedy: same shape (values rewritten, then + * the column's type changed), on a dialect this suite can open. NOT the + * statement the command builds — see the head note. + */ +function sqliteStandInPlan(): StaleColumnPlan { + return { + targets: [ + { + table: TABLE, + column: 'tags', + from: 'text', + to: 'json', + // Display only — the executor never branches on it. The statements + // below are SQLite's, for the reason the head note gives. + dialect: 'postgres', + statements: [ + `ALTER TABLE ${TABLE} RENAME COLUMN tags TO tags_legacy`, + `ALTER TABLE ${TABLE} ADD COLUMN tags json`, + `UPDATE ${TABLE} SET tags = CASE WHEN tags_legacy IS NULL THEN NULL ` + + `WHEN tags_legacy = '' THEN NULL WHEN substr(tags_legacy, 1, 1) = '[' THEN tags_legacy ` + + `ELSE json_array(tags_legacy) END`, + `ALTER TABLE ${TABLE} DROP COLUMN tags_legacy`, + ], + }, + ], + refusals: [], + }; +} + +const liveExec: RawExec = (sql) => knex.raw(sql); + +beforeEach(async () => { + await driver?.disconnect().catch(() => {}); + await freshFixture(); +}); + +afterAll(async () => { + await driver?.disconnect().catch(() => {}); + for (const dir of dirs) rmSync(dir, { recursive: true, force: true }); +}); + +describe('os migrate multi-value-columns — the dry run changes nothing (#11733)', () => { + it('a dry run leaves the column type and every row byte-identical, and never touches the seam', async () => { + const before = await snapshot(); + // Non-vacuity: the fixture really is the stale shape this command is about. + expect(before.columnType.toLowerCase()).toBe('text'); + expect(before.rows).toHaveLength(4); + + let seamCalls = 0; + const countingExec: RawExec = async (sql) => { + seamCalls += 1; + return knex.raw(sql); + }; + + const result = await runStaleColumnMigration({ + plan: sqliteStandInPlan(), + exec: countingExec, + apply: false, + }); + + // 1. it reported the work… + expect(result.outcomes).toHaveLength(1); + expect(result.outcomes[0].statements).toHaveLength(4); + expect(result.outcomes[0].status).toBe('planned'); + + // 2. …and ran none of it. Not one statement, not a probe. + expect(seamCalls).toBe(0); + expect(result.executedStatements).toEqual([]); + expect(result.outcomes[0].executed).toEqual([]); + + // 3. the database agrees — this is the reading that matters. + expect(await snapshot()).toEqual(before); + }); + + it('POSITIVE CONTROL: the same snapshot, the same fixture, --apply — and it moves', async () => { + // Without this case the assertion above is a claim about an instrument that + // has never been shown reading a change. + const before = await snapshot(); + + const result = await runStaleColumnMigration({ + plan: sqliteStandInPlan(), + exec: liveExec, + apply: true, + }); + + expect(result.outcomes[0].status).toBe('migrated'); + expect(result.outcomes[0].executed).toHaveLength(4); + expect(result.executedStatements).toHaveLength(4); + + const after = await snapshot(); + expect(after).not.toEqual(before); + // Both readings the dry-run case pinned as unchanged are shown changing: + expect(after.columnType.toLowerCase()).toBe('json'); + expect(after.rows).not.toEqual(before.rows); + + // And the values landed in the shape the declaration promises — including + // the two states that are easy to get wrong. + const byId = new Map( + (after.rows as Array<{ id: string; tags: string | null }>).map((r) => [r.id, r.tags]), + ); + expect(byId.get('legacy')).toBe('["a"]'); + expect(byId.get('multi')).toBe('["x","y"]'); + expect(byId.get('empty')).toBeNull(); + expect(byId.get('nulled')).toBeNull(); + }); + + it('a dry run over a plan built from the engine’s REAL finding executes nothing either', async () => { + // The stand-in above proves the control flow against a database; this + // proves the same for the statements the command actually builds, using an + // `exec` that cannot be called without failing the test. + const stale: PhysicalColumn[] = [{ name: 'tags', type: 'character varying', nullable: true, maxLength: 255 }]; + const entries = diffManagedTable({ + table: TABLE, + fields: { tags: { type: 'lookup', multiple: true } as any }, + columns: stale, + dialect: 'postgres', + }); + const plan = planStaleColumnTargets(entries); + expect(plan.targets).toHaveLength(1); // the plan is real + + const before = await snapshot(); + const result = await runStaleColumnMigration({ + plan, + apply: false, + exec: async () => { + throw new Error('the dry run executed SQL'); + }, + }); + + expect(result.executedStatements).toEqual([]); + expect(result.outcomes[0].status).toBe('planned'); + expect(await snapshot()).toEqual(before); + }); + + it('a failing statement stops THAT target where it failed, and says so', async () => { + // MySQL's three statements auto-commit one at a time, so a half-converted + // table is a real state an operator can land in. The report has to make it + // visible rather than round it up to "failed, nothing happened". + const plan = sqliteStandInPlan(); + plan.targets[0].statements = [ + `UPDATE ${TABLE} SET tags = '["a"]' WHERE id = 'legacy'`, + `ALTER TABLE ${TABLE} MODIFY tags json`, // SQLite has no MODIFY — this throws + `UPDATE ${TABLE} SET tags = NULL WHERE id = 'empty'`, + ]; + + const result = await runStaleColumnMigration({ plan, exec: liveExec, apply: true }); + + expect(result.outcomes[0].status).toBe('failed'); + expect(result.outcomes[0].executed).toEqual([plan.targets[0].statements[0]]); + expect(result.outcomes[0].error).toBeTruthy(); + + // The first statement's write is still there — that IS the half-converted + // state, and the point is that it is reported rather than hidden. + const rows = (await snapshot()).rows as Array<{ id: string; tags: string | null }>; + expect(rows.find((r) => r.id === 'legacy')?.tags).toBe('["a"]'); + expect(rows.find((r) => r.id === 'empty')?.tags).toBe(''); + }); +}); diff --git a/packages/cli/src/commands/migrate/multi-value-columns.no-auto-run.test.ts b/packages/cli/src/commands/migrate/multi-value-columns.no-auto-run.test.ts new file mode 100644 index 0000000000..dccbe14d60 --- /dev/null +++ b/packages/cli/src/commands/migrate/multi-value-columns.no-auto-run.test.ts @@ -0,0 +1,132 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11733] Nothing runs this command for you. That is the ruling, and this is + * the pin that keeps it true. + * + * Ruled C on #11700 (maintainer, 2026-08-24): the platform warns and ships an + * EXPLICIT, operator-run migration. Option A — unattended auto-migration — was + * rejected, because it was the only route on which the platform alters a + * customer's production table structure with nobody watching. The distance + * between C and A is one import: a boot path that reaches for this module + * converts the ruling without anyone re-deciding it, and it would do so + * silently, because an auto-migration that works looks like nothing at all. + * + * ## A negative result is evidence only after the instrument has produced a + * ## positive one + * + * "No file imports it" is also what a broken scanner says — a wrong root, a + * changed extension, a typo in the needle. So every absence below is preceded + * by the SAME scanner finding a call site that really exists, including one on + * the boot path itself (`commands/serve.ts` reaching for the `kernel:ready` + * migration gate, through a dynamic import, which is exactly the shape an + * accidental auto-run would take). + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** `packages/cli/src` — this package only; the scan never leaves it. */ +const SRC_ROOT = fileURLToPath(new URL('../../', import.meta.url)); + +function everyTsFile(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + everyTsFile(full, out); + } else if (entry.endsWith('.ts')) { + out.push(full); + } + } + return out; +} + +const ALL_FILES = everyTsFile(SRC_ROOT); +const relative = (file: string) => file.slice(SRC_ROOT.length).replaceAll('\\', '/'); + +/** + * Files that reference `needle` in an import position — static `from '…'` or + * dynamic `await import('…')`, both of which are how a boot path would reach + * this command. + */ +function importersOf(needle: string): string[] { + return ALL_FILES.filter((file) => { + const src = readFileSync(file, 'utf8'); + return src.includes(`'${needle}'`) || src.includes(`"${needle}"`); + }).map(relative); +} + +const isTestFile = (path: string) => /\.test\.ts$/.test(path); + +describe('the scanner works (positive controls) (#11733)', () => { + it('finds the CLI’s own boot path reaching the kernel:ready migration gate — a DYNAMIC import', () => { + // If the scanner could not see `await import('../utils/artifact-boot-migration.js')`, + // it could not see an auto-run wired the same way, and every absence below + // would be worthless. + const importers = importersOf('../utils/artifact-boot-migration.js').filter((p) => !isTestFile(p)); + expect(importers).toContain('commands/serve.ts'); + }); + + it('finds the many command files importing the shared schema-migrate boot', () => { + const importers = importersOf('../../utils/schema-migrate.js').filter((p) => !isTestFile(p)); + expect(importers.length).toBeGreaterThan(3); + expect(importers).toContain('commands/migrate/plan.ts'); + }); + + it('the file population itself is real', () => { + expect(ALL_FILES.length).toBeGreaterThan(100); + expect(ALL_FILES.map(relative)).toContain('commands/migrate/multi-value-columns.ts'); + }); +}); + +describe('nothing on the boot / reconcile path invokes this command (#11733)', () => { + it('no source file imports the migration module — its own tests are the only readers', () => { + const importers = [ + ...importersOf('./multi-value-columns.js'), + ...importersOf('../commands/migrate/multi-value-columns.js'), + ...importersOf('../../commands/migrate/multi-value-columns.js'), + ]; + // Only this command's own suites. `commands/migrate/index.ts` deliberately + // does NOT default to it either — the bare `os migrate` is the plan. + expect(importers.filter((p) => !isTestFile(p))).toEqual([]); + expect(importers.every((p) => p.startsWith('commands/migrate/multi-value-columns.'))).toBe(true); + }); + + it('the command never routes the remedy through the reconciler', () => { + // `applyMigrationEntries` is where the boot gate applies drift + // automatically. `manual_column_type_change` reaching an arm there is the + // shape that converts C back into A, so this command must not be the thing + // that calls it — it runs the engine's statement through the raw seam, + // after an explicit `--apply`. + // + // Comments are stripped first, and that is not a convenience: the command's + // own docstring EXPLAINS the reconciler's armlessness, and a reading that + // counted prose went red for the documentation while a real call would have + // looked identical. What is asserted is code. + const code = (path: string) => + readFileSync(join(SRC_ROOT, path), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^\s*\/\/.*$/gm, ''); + + // Positive control first: the symbol IS findable by this reading, in the + // command whose whole job is to call it. + expect(code('commands/migrate/apply.ts')).toContain('applyMigrationEntries'); + + expect(code('commands/migrate/multi-value-columns.ts')).not.toContain('applyMigrationEntries'); + }); + + it('the boot gate’s auto-apply set is not something this PR widened', () => { + // The gate hands `category !== 'destructive'` to the driver, so this + // finding (`needs_confirm`) DOES reach `applyMigrationEntries` at boot — + // and is declined there, because the reconciler has no arm for the op. That + // armless-by-design contract is `driver-sql`'s (#11720) and read-only here; + // what this asserts is that the CLI half of the boot path is untouched by + // this card: it still reads the category and nothing about this op. + const gate = readFileSync(join(SRC_ROOT, 'utils/artifact-boot-migration.ts'), 'utf8'); + expect(gate).toContain("d.category === 'destructive'"); // the instrument sees the real filter + expect(gate).not.toContain('manual_column_type_change'); + expect(gate).not.toContain('multi-value-columns'); + }); +}); diff --git a/packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts b/packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts new file mode 100644 index 0000000000..76f09944db --- /dev/null +++ b/packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11733] The statement `os migrate multi-value-columns` runs must be the + * statement `@objectstack/driver-sql` printed — byte for byte, per dialect. + * + * ## Why this suite is the load-bearing one + * + * The remedy was corrected TWICE by measurement while #11720 was written, and + * neither correction is recoverable by reasoning about SQL: + * + * - `to_json(col)` makes a JSON **scalar** out of a legacy single value, so + * `Array.isArray` reads `false` under a field the metadata declares + * multi-value. Measured on live Postgres 16.13; `json_build_array` is what + * makes Postgres agree with MySQL's `JSON_ARRAY`. + * - `json_build_array(NULL)` is `[null]`, a one-element array. The explicit + * `IS NULL` arm was added AFTER the version without it was run on a live + * server and observed giving every NULL row a value. + * + * The CLI carries a copy of that statement (`manualJsonConversionSql` is not on + * `@objectstack/driver-sql`'s public surface, and this card's file surface is + * read-only over that package). A copy that can drift silently would lose both + * corrections the first time the engine improves the statement, so it is held + * to the engine's own output HERE: every case below builds the finding with + * `diffManagedTable()` — the package-root export that produces the message an + * operator actually sees — and requires the CLI's statement inside it. + * + * Deleting this suite deletes the only thing keeping the copy honest. + */ + +import { describe, it, expect } from 'vitest'; +import { diffManagedTable, type ManagedDriftEntry, type PhysicalColumn } from '@objectstack/driver-sql'; +import { + multiValueJsonMigrationSql, + splitRemedyStatements, + planStaleColumnTargets, + CORRUPTING_DIALECTS, +} from './multi-value-columns.js'; + +const TABLE = 'proj_task'; +const COLUMN = 'tags'; + +/** The stale column, in each dialect's own type spelling (#11720's fixtures). */ +const STALE: Record<'postgres' | 'mysql', PhysicalColumn[]> = { + postgres: [{ name: COLUMN, type: 'character varying', nullable: true, maxLength: 255 }], + mysql: [{ name: COLUMN, type: 'varchar', nullable: true, maxLength: 255 }], +}; + +const engineFinding = (dialect: 'postgres' | 'mysql'): ManagedDriftEntry => { + const out = diffManagedTable({ + table: TABLE, + fields: { [COLUMN]: { type: 'lookup', multiple: true } as any }, + columns: STALE[dialect], + dialect, + }); + // Non-vacuity: if the engine stopped reporting this shape, every containment + // assertion below would pass against nothing. + expect(out).toHaveLength(1); + expect(out[0].op.type).toBe('manual_column_type_change'); + return out[0]; +}; + +describe('the CLI remedy is the engine remedy (#11733 / #11720)', () => { + for (const dialect of CORRUPTING_DIALECTS) { + it(`${dialect}: the statement the command would run appears verbatim in the engine's own finding`, () => { + const entry = engineFinding(dialect); + const cliSql = multiValueJsonMigrationSql(dialect, TABLE, COLUMN); + + // The whole point: not "looks similar", not "contains an ALTER" — the + // engine's message CONTAINS the CLI's statement, character for character. + expect(entry.message).toContain(cliSql); + }); + } + + it('postgres keeps both corrections measurement forced — json_build_array, and the explicit IS NULL arm', () => { + const sql = multiValueJsonMigrationSql('postgres', TABLE, COLUMN); + expect(sql).toContain('json_build_array'); + // `to_json` is the reporter's original and the wrong answer here: it yields + // a JSON scalar, not a one-element array. + expect(sql).not.toContain('to_json'); + // Without this arm every NULL row gains `[null]`. + expect(sql).toContain(`WHEN "${COLUMN}" IS NULL THEN NULL`); + expect(sql).toContain(`WHEN "${COLUMN}" = '' THEN NULL`); + }); + + it('mysql is MySQL’s own statement, not the Postgres one with different quotes', () => { + const sql = multiValueJsonMigrationSql('mysql', TABLE, COLUMN); + expect(sql).toContain('JSON_ARRAY'); + expect(sql).not.toContain('json_build_array'); + // MySQL will not cast text to json implicitly — the rows have to be moved + // BEFORE the ALTER or it dies on the first legacy value. + const statements = splitRemedyStatements(sql); + expect(statements).toHaveLength(3); + expect(statements[0]).toMatch(/^UPDATE .* JSON_ARRAY/); + expect(statements[1]).toMatch(/^UPDATE .*= NULL WHERE/); + expect(statements[2]).toMatch(/^ALTER TABLE .*MODIFY .*json$/); + }); + + it('postgres is ONE statement — which is why a failed conversion leaves the column untouched', () => { + expect(splitRemedyStatements(multiValueJsonMigrationSql('postgres', TABLE, COLUMN))).toHaveLength(1); + }); + + it('the semicolon split loses nothing — neither form carries a semicolon inside a literal', () => { + // The split is the one #11720's live suite executes the remedy with. It is + // safe because of a property of THESE statements, so the property is pinned + // rather than assumed: re-joining the parts reproduces the original. + for (const dialect of CORRUPTING_DIALECTS) { + const sql = multiValueJsonMigrationSql(dialect, TABLE, COLUMN); + const rejoined = `${splitRemedyStatements(sql).join('; ')};`; + expect(rejoined).toBe(sql.trim()); + } + }); + + it('identifiers are the finding’s own table and column, not a fixed pair', () => { + // A builder that ignored its arguments would still pass every containment + // check above if the fixture happened to use the same names. + const other = multiValueJsonMigrationSql('postgres', 'crm_case', 'watchers'); + expect(other).toContain('"crm_case"'); + expect(other).toContain('"watchers"'); + expect(other).not.toContain(TABLE); + }); +}); + +describe('planning refuses anything it cannot match to the engine (#11733)', () => { + for (const dialect of CORRUPTING_DIALECTS) { + it(`${dialect}: a real finding plans the engine's statements and names the dialect`, () => { + const plan = planStaleColumnTargets([engineFinding(dialect)]); + expect(plan.refusals).toEqual([]); + expect(plan.targets).toHaveLength(1); + expect(plan.targets[0]).toMatchObject({ table: TABLE, column: COLUMN, to: 'json', dialect }); + expect(plan.targets[0].statements).toEqual( + splitRemedyStatements(multiValueJsonMigrationSql(dialect, TABLE, COLUMN)), + ); + // The dialect is READ OFF the finding, never off a client-name table this + // package would have to keep in step with the driver's. + expect(plan.targets[0].from).toBe(dialect === 'postgres' ? 'character varying' : 'varchar'); + }); + } + + it('a finding whose message no longer carries a statement we recognise is REFUSED, not guessed at', () => { + // The failure this closes: the engine improves the remedy, the CLI copy + // goes stale, and the command runs its own outdated SQL against a customer + // database. It refuses instead — and says what to do by hand. + const entry = engineFinding('postgres'); + const mutated = { ...entry, message: entry.message.replace('json_build_array', 'to_json') }; + + const plan = planStaleColumnTargets([mutated]); + expect(plan.targets).toEqual([]); + expect(plan.refusals).toHaveLength(1); + expect(plan.refusals[0]).toMatchObject({ table: TABLE, column: COLUMN, reason: 'remedy_not_recognized' }); + expect(plan.refusals[0].detail).toContain('os migrate plan'); + }); + + it('ignores every drift op that is not this one', () => { + const others = diffManagedTable({ + table: TABLE, + fields: { [COLUMN]: { type: 'string', maxLength: 50 } as any }, + columns: STALE.postgres, + dialect: 'postgres', + }); + expect(others.map((d) => d.op.type)).toEqual(['narrow_varchar']); // the instrument found something + expect(planStaleColumnTargets(others)).toEqual({ targets: [], refusals: [] }); + }); + + it('--table narrows to the tables named, and drops the rest silently', () => { + const a = engineFinding('postgres'); + const b = { ...a, table: 'crm_case', op: { ...(a.op as any), table: 'crm_case' } } as ManagedDriftEntry; + // `b`'s message still carries `proj_task`'s statement, so it can only be + // planned if the filter lets it through — which it must not. + expect(planStaleColumnTargets([a, b], { tables: [TABLE] }).targets.map((t) => t.table)).toEqual([TABLE]); + expect(planStaleColumnTargets([a, b], { tables: ['nothing_here'] }).targets).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/migrate/multi-value-columns.ts b/packages/cli/src/commands/migrate/multi-value-columns.ts new file mode 100644 index 0000000000..352e27b89c --- /dev/null +++ b/packages/cli/src/commands/migrate/multi-value-columns.ts @@ -0,0 +1,603 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { Command, Flags } from '@oclif/core'; +import chalk from 'chalk'; +import { createInterface } from 'node:readline'; +import { + printHeader, + printSuccess, + printWarning, + printError, + printInfo, + printStep, + createTimer, + emitJson, + isExitSignal, +} from '../../utils/format.js'; +import { bootSchemaStack } from '../../utils/schema-migrate.js'; +import { OCCUPANCY_HINT, probeMigrationTarget } from '../../utils/migrate-occupancy-gate.js'; +import { describeOccupancy } from '../../utils/sqlite-occupancy.js'; +import type { ManagedDriftEntry } from '@objectstack/driver-sql'; +import type { IObjectQLEngine } from '@objectstack/spec/contracts'; + +/** The raw-SQL seam shape — same signature `@objectstack/metadata-protocol` resolves. */ +export type RawExec = (sql: string, params?: unknown[]) => Promise; + +/** The two dialects on which a stale textual column actually corrupts (measured, #11535). */ +export type CorruptingDialect = 'postgres' | 'mysql'; + +export const CORRUPTING_DIALECTS: readonly CorruptingDialect[] = ['postgres', 'mysql']; + +/** + * ── The remedy statement, taken from the engine rather than invented here ─── + * + * This is the statement `@objectstack/driver-sql`'s `manualJsonConversionSql` + * emits inside the `manual_column_type_change` finding (#11720), reproduced + * character for character. It is NOT re-derived: two of its arms were corrected + * by running the earlier version against a live server, and a fresh derivation + * loses both. + * + * - `json_build_array(col)`, never `to_json(col)`. `to_json` turns a legacy + * single value into a JSON **scalar**, so `Array.isArray` reads `false` + * under a field the metadata now declares multi-value. `json_build_array` + * makes Postgres 16.13 hand back the same value MySQL 8.0.46's `JSON_ARRAY` + * does. + * - the explicit `IS NULL` arm is load-bearing: `json_build_array(NULL)` is + * `[null]`, a one-element array. The arm was added AFTER the version + * without it was run on a live Postgres and observed giving every NULL row + * a value. + * + * ⚠️ The copy is not trusted on its word anywhere in this command. Two guards + * hold it to the engine's: + * + * 1. at RUNTIME — {@link planStaleColumnTargets} refuses to execute a + * statement the engine's own finding does not contain verbatim, so the + * only statements this command can ever run are ones `driver-sql` printed + * for that exact table, column and dialect; + * 2. at TEST TIME — `multi-value-columns.remedy-fidelity.test.ts` pins this + * output against `diffManagedTable()`'s finding for both dialects, so a + * correction landing in the engine turns this package red instead of + * leaving a stale statement behind a green suite. + * + * The copy exists because `manualJsonConversionSql` is not part of + * `@objectstack/driver-sql`'s public surface (`src/index.ts` re-exports two + * blocks from `schema-drift.js` and it is in neither) and this card's file + * surface is read-only over that package. Both guards above become unnecessary + * the day it is exported — see the PR body. + */ +export function multiValueJsonMigrationSql( + dialect: CorruptingDialect, + 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 ` + + `(CASE WHEN "${column}" IS NULL THEN NULL WHEN "${column}" = '' THEN NULL ` + + `WHEN "${column}" LIKE '[%' THEN "${column}"::json ` + + `ELSE json_build_array("${column}") END);` + ); +} + +/** + * Split the remedy into the statements a driver seam can take one at a time. + * + * The same split #11720's live suite executes the remedy with. It is not a + * general SQL splitter and does not need to be: neither dialect's form contains + * a semicolon inside a literal (`''`, `'['`), which is a property of THESE two + * statements, pinned by `multi-value-columns.remedy-fidelity.test.ts`. + * + * The MySQL form is three statements and must be run in order — the two UPDATEs + * put every row into a shape the `MODIFY … json` will accept. + */ +export function splitRemedyStatements(sql: string): string[] { + return sql + .split(';') + .map((s) => s.trim()) + .filter(Boolean); +} + +/** One stale column this command can act on. */ +export interface StaleColumnTarget { + table: string; + column: string; + /** Physical type the column still has, as the engine read it off the server. */ + from: string; + /** Type the metadata declares — always `json` for this op. */ + to: string; + dialect: CorruptingDialect; + /** Ordered statements; the whole remedy for `table.column`. */ + statements: string[]; +} + +/** A finding this command declines to act on, and why. Never silently dropped. */ +export interface StaleColumnRefusal { + table: string; + column: string; + reason: 'remedy_not_recognized'; + detail: string; +} + +export interface StaleColumnPlan { + targets: StaleColumnTarget[]; + refusals: StaleColumnRefusal[]; +} + +/** Is this a `manual_column_type_change` finding? (The only op this command touches.) */ +export function isStaleMultiValueColumn(entry: ManagedDriftEntry): boolean { + return entry?.op?.type === 'manual_column_type_change'; +} + +/** + * Turn the engine's findings into an executable plan. + * + * ## The dialect is read off the finding, not off the connection + * + * There is no client-name table here on purpose. `driver-sql` owns the mapping + * from a knex client spelling to a dialect (`postgres` / `pg` / `postgresql` + * are one dialect under three names, and getting that list wrong is a measured + * defect class — see `POSTGRES_EMIT_CLIENTS`), and a second copy in this + * package would be a copy that can disagree. So the dialect is decided by which + * dialect's statement the ENGINE's own finding contains: the two forms are + * unmistakable (backtick-quoted MySQL versus double-quoted Postgres), and a + * match is simultaneously the proof that the statement about to run is the one + * `driver-sql` printed for this exact table and column. + * + * A finding whose message contains NEITHER form is refused, never guessed at + * and never run — that is a remedy this command no longer recognises, and + * executing a statement the engine did not print is the one thing it must not + * do. SQLite lands here too if it ever produced this finding, which it does not + * (measured: SQLite reads a stale column back as a real array, so there is + * nothing to migrate and `diffManagedTable` stays silent). + */ +export function planStaleColumnTargets( + entries: ManagedDriftEntry[], + opts: { tables?: string[] } = {}, +): StaleColumnPlan { + const wanted = opts.tables && opts.tables.length > 0 ? new Set(opts.tables) : null; + const targets: StaleColumnTarget[] = []; + const refusals: StaleColumnRefusal[] = []; + + for (const entry of entries) { + if (!isStaleMultiValueColumn(entry)) continue; + const op = entry.op as { table: string; column: string; to: string; from: string }; + if (wanted && !wanted.has(op.table)) continue; + + const message = typeof entry.message === 'string' ? entry.message : ''; + const dialect = CORRUPTING_DIALECTS.find((d) => + message.includes(multiValueJsonMigrationSql(d, op.table, op.column)), + ); + + if (!dialect) { + refusals.push({ + table: op.table, + column: op.column, + reason: 'remedy_not_recognized', + detail: + `the drift finding for ${op.table}.${op.column} does not contain the remedy statement this ` + + `command knows how to run, so there is nothing here it can execute without inventing SQL. ` + + `Run "os migrate plan" and apply the statement the finding prints, by hand.`, + }); + continue; + } + + targets.push({ + table: op.table, + column: op.column, + from: op.from, + to: op.to, + dialect, + statements: splitRemedyStatements(multiValueJsonMigrationSql(dialect, op.table, op.column)), + }); + } + + return { targets, refusals }; +} + +/** What one target's execution did — or, in a dry run, did not do. */ +export interface StaleColumnOutcome { + table: string; + column: string; + dialect: CorruptingDialect; + from: string; + to: string; + statements: string[]; + /** Statements actually sent to the database. ALWAYS `[]` in a dry run. */ + executed: string[]; + status: 'planned' | 'migrated' | 'failed'; + error?: string; +} + +export interface StaleColumnRunResult { + apply: boolean; + outcomes: StaleColumnOutcome[]; + refusals: StaleColumnRefusal[]; + /** Statements sent to the database across every target. `[]` in a dry run. */ + executedStatements: string[]; +} + +/** + * Execute the plan — or, without `apply`, deliberately execute nothing. + * + * ⚠️ The dry run is the contract this function exists to keep: with + * `apply !== true` the `exec` seam is never called, not once, not for a probe. + * "Shows what it would run" and "quietly runs it" are indistinguishable to an + * operator reading a successful report, so the difference is pinned by a test + * that re-reads the column type and the rows from a real database after a dry + * run and requires both unchanged — with the same instrument shown observing + * the change an apply run makes. + * + * Failures are per target: one table's ALTER failing (an index on the column is + * the common cause — a json column cannot carry a plain btree) stops THAT + * target's remaining statements and is reported, while the other targets are + * still attempted. On MySQL the three statements are separate implicit-commit + * DDL/DML, so a target that fails midway is left partly converted; re-running + * finishes it, because each statement skips the rows the previous run already + * moved. + */ +export async function runStaleColumnMigration(args: { + plan: StaleColumnPlan; + exec: RawExec; + apply: boolean; + onStatement?: (statement: string) => void; +}): Promise { + const { plan, exec, apply } = args; + const outcomes: StaleColumnOutcome[] = []; + const executedStatements: string[] = []; + + for (const target of plan.targets) { + const outcome: StaleColumnOutcome = { + table: target.table, + column: target.column, + dialect: target.dialect, + from: target.from, + to: target.to, + statements: target.statements, + executed: [], + status: 'planned', + }; + outcomes.push(outcome); + + if (!apply) continue; + + try { + for (const statement of target.statements) { + args.onStatement?.(statement); + await exec(statement); + outcome.executed.push(statement); + executedStatements.push(statement); + } + outcome.status = 'migrated'; + } catch (error: unknown) { + outcome.status = 'failed'; + outcome.error = error instanceof Error ? error.message : String(error); + } + } + + return { apply, outcomes, refusals: plan.refusals, executedStatements }; +} + +/** + * What the operator does if it goes wrong — printed by the command and repeated + * in `content/docs/deployment/cli.mdx`. + * + * The first line is not boilerplate. The conversion is NOT information + * preserving: both `NULL` and the empty string map to `NULL`, so after a + * successful run the rows that held `''` are indistinguishable from the rows + * that held `NULL`. A type-only reversal (`ALTER … TYPE text`) therefore + * restores the column's TYPE and not its contents — the backup is the only + * faithful rollback, which is why the command refuses to pretend it has an + * `--undo`. + */ +export const ROLLBACK_NOTES: readonly string[] = [ + 'Restore the backup you took before the run. The conversion maps both NULL and the empty string to NULL, so once it succeeds those two states cannot be told apart again — no reverse statement can put them back.', + 'Reverting only the column TYPE (Postgres: ALTER TABLE … ALTER COLUMN … TYPE text USING …::text) puts the column back to text with JSON text in it. Metadata still declares the field multi-value, so the drift finding returns on the next boot and the corruption resumes on the next write. It is a stopgap for an incident, not a rollback.', + 'Postgres runs the whole remedy as ONE statement: if it fails, the column is untouched and there is nothing to roll back.', + 'MySQL runs three statements, and DDL there commits implicitly — a failure midway leaves rows partly converted. Re-run the command: the UPDATEs skip rows that already hold an array, so finishing an interrupted run is safe.', + 'If the ALTER fails naming an index, drop the index on that column first (a json column cannot carry a plain btree) and re-run; recreate it afterwards in the shape your dialect supports for json.', +]; + +async function confirm(question: string): Promise { + if (!process.stdin.isTTY) return false; // non-interactive → require --yes + const rl = createInterface({ input: process.stdin, output: process.stdout }); + try { + const answer: string = await new Promise((resolve) => rl.question(question, resolve)); + return /^y(es)?$/i.test(answer.trim()); + } finally { + rl.close(); + } +} + +/** + * `os migrate multi-value-columns` — the operator-run half of #11535. + * + * A field that gains `multiple: true` over an existing database keeps its old + * `varchar`/`text` column: the additive sync adds columns and never changes + * one's type. Arrays are then written as the STRINGIFIED literal `'["a","b"]'` + * and read back as a string, so a consumer receives one opaque id instead of a + * list. `driver-sql` reports that as `manual_column_type_change` (#11720); + * this command is how an operator acts on the report. + * + * ## It is never run for you — that is the ruling, not an omission + * + * Ruled C on #11700 (maintainer, 2026-08-24): the platform WARNS and ships an + * explicit, operator-run migration. Unattended auto-migration was rejected — + * it was the only route that had the platform altering a customer's production + * table structure with nobody watching. So the reconciler still has NO arm for + * this op: `os migrate apply` hands it to `applyMigrationEntries`, which + * declines it (`applied=0, skipped=1`). This command does not go through the + * reconciler either — it runs the engine's own statement through the driver's + * raw seam, only after the operator asked for `--apply` and confirmed. Nothing + * on the boot path invokes it; `multi-value-columns.no-auto-run.test.ts` pins + * that. + * + * ## Historical data is out of scope, by the same ruling + * + * 「11700 11693 不需要考虑历史数据，其他按照你的建议继续」 — rows corrupted + * BEFORE the column was migrated are the customer's to repair. This command + * converts the column and the values in it; it does not hunt for stringified + * arrays that a hook already copied into some other single-value column, and it + * must not grow that. + */ +export default class MigrateMultiValueColumns extends Command { + static override description = + 'Migrate a stale varchar/text column to json where the field declares multiple: true (#11535). ' + + 'Dry-run by default: prints the exact statements and the database they would run against, and writes nothing.'; + + static override examples = [ + '$ os migrate multi-value-columns', + '$ os migrate multi-value-columns --json', + '$ os migrate multi-value-columns --apply', + '$ os migrate multi-value-columns --apply --yes --json', + '$ os migrate multi-value-columns --table crm_case', + '$ os migrate multi-value-columns --database-url postgres://localhost/app', + ]; + + static override flags = { + 'database-url': Flags.string({ + description: 'Database URL to migrate (defaults to $OS_DATABASE_URL / the project DB)', + env: 'OS_DATABASE_URL', + }), + apply: Flags.boolean({ + description: 'Run the statements (default is a dry run that executes nothing at all)', + default: false, + }), + yes: Flags.boolean({ char: 'y', description: 'Skip the --apply confirmation prompt', default: false }), + force: Flags.boolean({ + description: 'Apply even when another process is using the database (SQLite occupancy check)', + default: false, + }), + table: Flags.string({ + description: 'Restrict to this physical table (repeatable; default: every stale column reported)', + multiple: true, + }), + json: Flags.boolean({ description: 'Output as JSON (implies non-interactive; requires --yes to apply)' }), + }; + + async run(): Promise { + const { flags } = await this.parse(MigrateMultiValueColumns); + const timer = createTimer(); + const apply = flags.apply; + + if (!flags.json) printHeader('Migrate · multi-value-columns'); + + // Probed before boot, so the answer is about somebody else's connections + // rather than our own pool, and before the prompt, so an operator is never + // asked to confirm a run we then refuse. + const occupancy = await probeMigrationTarget(flags['database-url']); + if (occupancy.status === 'busy' && apply && !flags.force) { + if (flags.json) { + await emitJson({ + error: 'database_busy', + database: occupancy.filename, + signal: occupancy.signal, + detail: occupancy.detail, + hint: OCCUPANCY_HINT, + }, 0, { compact: true }); + this.exit(1); + return; + } + printError(describeOccupancy(occupancy)); + printWarning(OCCUPANCY_HINT); + this.exit(1); + return; + } + if (occupancy.status === 'busy' && !flags.json) { + printWarning(apply + ? `--force: ${describeOccupancy(occupancy)} Altering the column anyway — the live process may write rows mid-migration.` + : `${describeOccupancy(occupancy)} The dry run below writes nothing.`); + } + + if (apply && !flags.yes) { + if (flags.json || !process.stdin.isTTY) { + if (flags.json) { + await emitJson({ error: 'confirmation_required', hint: 'pass --yes' }, 0, { compact: true }); + this.exit(1); + return; + } + printWarning('Apply mode changes a column type and rewrites its values. Re-run with --yes to confirm, or run without --apply to preview.'); + this.exit(1); + return; + } + const ok = await confirm( + chalk.bold('\nAlter these columns to json and rewrite their values? Take a backup first. [y/N] '), + ); + if (!ok) { + printInfo('Aborted — no changes made.'); + return; + } + } + + if (!flags.json) { + printStep(apply ? 'Booting schema stack (APPLY mode)…' : 'Booting schema stack (dry run)…'); + } + + let stack; + try { + stack = await bootSchemaStack({ + jsonOutput: flags.json, + ...(flags['database-url'] ? { databaseUrl: flags['database-url'] } : {}), + // Held back so the boot itself performs no create-table / add-column + // work — the only statements this command may run are the remedy's. + deferSchemaDdl: true, + // A dry run must not bring a database into existence either (#6743); + // an apply run needs the real target to write into. + ...(apply ? {} : { readOnlyProbe: true }), + }); + } catch (error: any) { + if (flags.json) { await emitJson({ error: error.message }, 0, { compact: true }); this.exit(1); } + printError(error.message || String(error)); + this.exit(1); + return; + } + + try { + if (!stack.driver) { + if (flags.json) { await emitJson({ error: 'no_sql_driver', targets: [] }, 0, { compact: true }); return; } + printWarning('This migration only applies to SQL drivers (Postgres / MySQL). No SQL driver is active.'); + return; + } + + const drift = await stack.driver.detectManagedDrift(); + const plan = planStaleColumnTargets(drift, flags.table ? { tables: flags.table } : {}); + + let result: StaleColumnRunResult; + let verified: boolean | null = null; + + if (!apply) { + // The dry run never resolves a seam, so there is nothing here that + // could execute by accident: the exec it is handed throws. + result = await runStaleColumnMigration({ + plan, + apply: false, + exec: async () => { + throw new Error('dry run must not execute SQL'); + }, + }); + } else { + const { resolveSeedTenancyExec, normalizeRows } = await import('@objectstack/metadata-protocol'); + const getService = (stack.kernel as { getService?: (name: string) => unknown })?.getService; + const ql = getService?.call(stack.kernel, 'objectql') as IObjectQLEngine | undefined; + const exec = resolveSeedTenancyExec(ql); + + // Loud absence, never a silent success. A driver can expose an + // `execute` that accepts every statement and performs none (#10677), + // and "migrated 3 columns" from a seam that ran nothing is the worst + // report this command could print. `select 1` must come back as a row. + const answers = exec + ? await exec('select 1 as os_seam_probe') + .then((r) => normalizeRows(r).length > 0) + .catch(() => false) + : false; + if (!exec || !answers) { + const detail = + 'The active driver exposes no usable raw SQL seam — it is either absent, or present but ' + + 'answering nothing — so the migration cannot be executed. Run "os migrate plan" and apply ' + + 'the statement the finding prints, by hand.'; + if (flags.json) { await emitJson({ error: 'no_sql_seam', detail }, 0, { compact: true }); this.exit(1); return; } + printError(detail); + this.exit(1); + return; + } + + result = await runStaleColumnMigration({ + plan, + apply: true, + exec, + onStatement: flags.json ? undefined : (s) => printStep(chalk.dim(s)), + }); + + // The finding must be GONE afterwards — the same check #11720's live + // suite makes. A migration that "succeeded" while the engine still + // reports the column has not done what it claims. + if (result.outcomes.some((o) => o.status === 'migrated')) { + const after = await stack.driver.detectManagedDrift(); + const stillStale = planStaleColumnTargets(after).targets; + verified = !result.outcomes.some( + (o) => o.status === 'migrated' && stillStale.some((t) => t.table === o.table && t.column === o.column), + ); + } + } + + const failed = result.outcomes.filter((o) => o.status === 'failed'); + + if (flags.json) { + await emitJson({ + database: stack.dbLabel, + apply, + targets: result.outcomes, + refusals: result.refusals, + verified, + rollback: ROLLBACK_NOTES, + duration: timer.elapsed(), + }); + if (failed.length > 0 || verified === false) this.exit(1); + return; + } + + printInfo(`Database: ${chalk.white(stack.dbLabel)}`); + console.log(''); + + if (result.outcomes.length === 0 && result.refusals.length === 0) { + printSuccess('No stale multi-value columns — every field declaring "multiple: true" already has a json column.'); + console.log(chalk.dim(` ${timer.display()}`)); + console.log(''); + return; + } + + for (const outcome of result.outcomes) { + const mark = outcome.status === 'migrated' ? chalk.green('✓') : outcome.status === 'failed' ? chalk.red('✗') : chalk.yellow('•'); + console.log(`${mark} ${chalk.bold(`${outcome.table}.${outcome.column}`)} ${chalk.dim(`${outcome.from} → ${outcome.to} (${outcome.dialect})`)}`); + for (const statement of outcome.statements) { + console.log(` ${chalk.cyan(statement)}`); + } + if (outcome.error) console.log(` ${chalk.red(outcome.error)}`); + console.log(''); + } + + for (const refusal of result.refusals) { + printWarning(`${refusal.table}.${refusal.column}: ${refusal.detail}`); + } + + if (!apply) { + printInfo( + `Dry run — nothing was executed. ${result.outcomes.length} column(s) would be migrated by the ` + + 'statements above. Take a backup, then re-run with --apply.', + ); + console.log(''); + console.log(chalk.bold('If it goes wrong:')); + for (const note of ROLLBACK_NOTES) console.log(` ${chalk.dim('·')} ${note}`); + } else if (failed.length > 0) { + printError(`${failed.length} column(s) could not be migrated — see the errors above.`); + console.log(''); + console.log(chalk.bold('If it goes wrong:')); + for (const note of ROLLBACK_NOTES) console.log(` ${chalk.dim('·')} ${note}`); + } else if (verified === false) { + printError('The statements ran, but the drift finding is still reported — the column has not reached json. Do not treat this as migrated.'); + } else if (result.outcomes.length > 0) { + printSuccess(`Migrated ${result.outcomes.length} column(s) to json; the drift finding no longer reports them.`); + } + + console.log(chalk.dim(` ${timer.display()}`)); + console.log(''); + if (failed.length > 0 || verified === false) this.exit(1); + } catch (error: any) { + if (isExitSignal(error)) throw error; + if (flags.json) { await emitJson({ error: error.message }, 0, { compact: true }); this.exit(1); } + printError(error.message || String(error)); + this.exit(1); + } finally { + await stack.shutdown(); + } + } +} diff --git a/packages/cli/test/json-stdout-purity.e2e.test.ts b/packages/cli/test/json-stdout-purity.e2e.test.ts index 9612f1aa13..b70b9423a0 100644 --- a/packages/cli/test/json-stdout-purity.e2e.test.ts +++ b/packages/cli/test/json-stdout-purity.e2e.test.ts @@ -83,6 +83,7 @@ const FAMILY: Record = { 'migrate apply': [], 'migrate files-to-references': [], 'migrate meta': ['--stored'], + 'migrate multi-value-columns': [], 'migrate plan': [], 'migrate recorded-by': [], 'migrate resume': [], From 694b8785c11125792440e3450b781710f7b722f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 23:52:47 +0000 Subject: [PATCH 2/2] refactor(cli,driver-sql): import the remedy statement instead of copying it Re-exports `manualJsonConversionSql` from driver-sql's index (one line) and has `os migrate multi-value-columns` import it, dropping the CLI's copy. The import is lazy and injected: no CLI production module may statically value-import a driver package (#5726), so the command `await import()`s it at the point of use and hands it to the planner, which stays synchronous. The runtime containment check survives, with a different claim: it is now only the DIALECT probe (a ManagedDriftEntry carries no dialect), matching the engine's finding against the engine's own function. Its refusal branch is still reachable. `remedy-fidelity` becomes `dialect-probe`, minus three cases that could no longer fail for any reason belonging to this package. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .../migrate-multi-value-columns-command.md | 3 +- .../multi-value-columns.dialect-probe.test.ts | 181 ++++++++++++++++++ .../multi-value-columns.dry-run.test.ts | 8 +- ...ulti-value-columns.remedy-fidelity.test.ts | 173 ----------------- .../commands/migrate/multi-value-columns.ts | 133 ++++++------- packages/drivers/driver-sql/src/index.ts | 1 + 6 files changed, 247 insertions(+), 252 deletions(-) create mode 100644 packages/cli/src/commands/migrate/multi-value-columns.dialect-probe.test.ts delete mode 100644 packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts diff --git a/.changeset/migrate-multi-value-columns-command.md b/.changeset/migrate-multi-value-columns-command.md index 80f90eb3c1..27abd9d454 100644 --- a/.changeset/migrate-multi-value-columns-command.md +++ b/.changeset/migrate-multi-value-columns-command.md @@ -1,5 +1,6 @@ --- "@objectstack/cli": minor +"@objectstack/driver-sql": minor --- -New operator-run command `os migrate multi-value-columns`: migrates a stale `varchar`/`text` column to `json` where the field declares `multiple: true` — the `manual_column_type_change` drift `os migrate apply` reports and deliberately never reconciles for you (#11535, ruled C on #11700). Flags: `--apply` (default off), `--yes`/`-y`, `--force`, `--table ` (repeatable), `--database-url`, `--json`. **Dry-run contract: without `--apply` the command executes nothing at all** — it prints the exact statements and the database they would run against, opens no seam and issues no probe, and a run is verified to have left the column type and every row unchanged. `--apply` runs the statement the drift finding itself prints (Postgres: one `ALTER … USING (CASE …)` with `json_build_array`; MySQL: the two row-shaping `UPDATE`s then `ALTER … MODIFY … json`), refuses to execute anything the finding does not contain verbatim, re-runs detection afterwards and exits non-zero if the finding has not cleared. SQLite is excluded — the stale column round-trips a real array there, so the finding is never raised. Rows corrupted before the column is migrated are out of scope, and the command is never invoked automatically: nothing on the boot path reaches it. +New operator-run command `os migrate multi-value-columns`: migrates a stale `varchar`/`text` column to `json` where the field declares `multiple: true` — the `manual_column_type_change` drift `os migrate apply` reports and deliberately never reconciles for you (#11535, ruled C on #11700). Flags: `--apply` (default off), `--yes`/`-y`, `--force`, `--table ` (repeatable), `--database-url`, `--json`. **Dry-run contract: without `--apply` the command executes nothing at all** — it prints the exact statements and the database they would run against, opens no seam and issues no probe, and a run is verified to have left the column type and every row unchanged. `--apply` runs `@objectstack/driver-sql`'s own `manualJsonConversionSql` — newly re-exported from that package's index for this consumer, its only other change — i.e. the statement the drift finding itself prints (Postgres: one `ALTER … USING (CASE …)` with `json_build_array`; MySQL: the two row-shaping `UPDATE`s then `ALTER … MODIFY … json`), refuses to execute anything the finding does not contain verbatim, re-runs detection afterwards and exits non-zero if the finding has not cleared. SQLite is excluded — the stale column round-trips a real array there, so the finding is never raised. Rows corrupted before the column is migrated are out of scope, and the command is never invoked automatically: nothing on the boot path reaches it. diff --git a/packages/cli/src/commands/migrate/multi-value-columns.dialect-probe.test.ts b/packages/cli/src/commands/migrate/multi-value-columns.dialect-probe.test.ts new file mode 100644 index 0000000000..ce53c80f1f --- /dev/null +++ b/packages/cli/src/commands/migrate/multi-value-columns.dialect-probe.test.ts @@ -0,0 +1,181 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11733] The command runs the ENGINE's statement, and works out WHICH dialect + * it is running by reading the engine's own finding. + * + * ## What this file used to be, and why it is smaller + * + * It began as a fidelity suite: `manualJsonConversionSql` was not on + * `@objectstack/driver-sql`'s public surface, so the command carried a copy of + * the statement and this suite held the copy to the engine's, byte for byte. + * The one-line re-export landed with this card, the copy is gone, and with it + * every assertion whose only job was to compare two spellings of one statement. + * Three cases were deleted rather than left behind: + * + * - "identifiers are the finding's own table and column" — it tested the + * CLI's builder handling its arguments. There is no CLI builder now; the + * call site's argument passing is covered by the plan cases below, which + * compare a real plan against `manualJsonConversionSql(dialect, …)`. + * - "postgres keeps both corrections measurement forced" and the content half + * of the MySQL case (`JSON_ARRAY` present, `json_build_array` absent). Both + * now assert `driver-sql`'s CONTENT from a consumer's suite. They are not + * vacuous — the engine could change that text — but that is precisely the + * problem: they could only ever fail for a reason that has nothing to do + * with this package, turning a deliberate engine correction into a red CLI + * suite. `driver-sql` owns those, and pins them in + * `schema-drift.base-type-mismatch.test.ts`, where they are also EXECUTED + * against live Postgres 16.13 and MySQL 8.0.46. + * + * ## What is left is not fidelity, and can still fail + * + * Two claims, both about this package: + * + * 1. **the coupling the dialect probe reads** — the finding's message still + * EMBEDS the remedy. Nothing in the CLI can keep that true, and everything + * in the CLI depends on it: the probe decides Postgres from MySQL by which + * dialect's statement the message contains, because a `ManagedDriftEntry` + * carries no dialect and a client-spelling table copied out of the driver + * could only disagree with it. If the engine ever stops interpolating the + * statement, these go red here — where the consumer that would silently + * lose its dialect lives. + * 2. **the split, and the refusal** — how this command turns one engine + * statement into the statements a seam takes, and what it does with a + * finding it cannot read a dialect from. + */ + +import { describe, it, expect } from 'vitest'; +import { + diffManagedTable, + manualJsonConversionSql, + type ManagedDriftEntry, + type PhysicalColumn, +} from '@objectstack/driver-sql'; +import { splitRemedyStatements, planStaleColumnTargets, CORRUPTING_DIALECTS } from './multi-value-columns.js'; + +const TABLE = 'proj_task'; +const COLUMN = 'tags'; + +/** Exactly what the command hands the planner in `run()`. */ +const SQL = { sql: manualJsonConversionSql }; + +/** The stale column, in each dialect's own type spelling (#11720's fixtures). */ +const STALE: Record<'postgres' | 'mysql', PhysicalColumn[]> = { + postgres: [{ name: COLUMN, type: 'character varying', nullable: true, maxLength: 255 }], + mysql: [{ name: COLUMN, type: 'varchar', nullable: true, maxLength: 255 }], +}; + +const engineFinding = (dialect: 'postgres' | 'mysql'): ManagedDriftEntry => { + const out = diffManagedTable({ + table: TABLE, + fields: { [COLUMN]: { type: 'lookup', multiple: true } as any }, + columns: STALE[dialect], + dialect, + }); + // Non-vacuity: if the engine stopped reporting this shape, every assertion + // below would pass against nothing. + expect(out).toHaveLength(1); + expect(out[0].op.type).toBe('manual_column_type_change'); + return out[0]; +}; + +describe('the coupling the dialect probe depends on (#11733)', () => { + for (const dialect of CORRUPTING_DIALECTS) { + it(`${dialect}: the finding's message still EMBEDS the remedy, which is what the probe matches on`, () => { + // Not "the CLI agrees with the engine" — there is one function now, so + // that could not fail. This is the engine's MESSAGE against the engine's + // FUNCTION: it fails the day the message stops carrying the statement, + // which is the day this command can no longer tell Postgres from MySQL. + expect(engineFinding(dialect).message).toContain(manualJsonConversionSql(dialect, TABLE, COLUMN)); + }); + } + + it('the two dialect forms are distinguishable — a probe reading one cannot match the other', () => { + // The premise of reading the dialect off the message. If the forms were + // substrings of one another the probe would resolve the wrong dialect and + // run the wrong DDL, so this is asserted rather than assumed. + const pg = manualJsonConversionSql('postgres', TABLE, COLUMN); + const my = manualJsonConversionSql('mysql', TABLE, COLUMN); + expect(pg).not.toBe(my); + expect(engineFinding('postgres').message).not.toContain(my); + expect(engineFinding('mysql').message).not.toContain(pg); + }); +}); + +describe('splitting the engine statement into what a seam can run (#11733)', () => { + it('mysql is three statements, in the order that makes the ALTER survivable', () => { + const statements = splitRemedyStatements(manualJsonConversionSql('mysql', TABLE, COLUMN)); + expect(statements).toHaveLength(3); + expect(statements[0]).toMatch(/^UPDATE .* JSON_ARRAY/); + expect(statements[1]).toMatch(/^UPDATE .*= NULL WHERE/); + expect(statements[2]).toMatch(/^ALTER TABLE .*MODIFY .*json$/); + }); + + it('postgres is ONE statement — which is why a failed conversion leaves the column untouched', () => { + // The rollback notes state this as a fact about Postgres; it is a fact + // about the STATEMENT, so it is read off the statement. + expect(splitRemedyStatements(manualJsonConversionSql('postgres', TABLE, COLUMN))).toHaveLength(1); + }); + + it('the semicolon split loses nothing — neither form carries a semicolon inside a literal', () => { + // The split is the one #11720's live suite executes the remedy with. It is + // safe because of a property of THESE statements, so the property is pinned + // rather than assumed: re-joining the parts reproduces the original. + for (const dialect of CORRUPTING_DIALECTS) { + const sql = manualJsonConversionSql(dialect, TABLE, COLUMN); + expect(`${splitRemedyStatements(sql).join('; ')};`).toBe(sql.trim()); + } + }); +}); + +describe('planning refuses anything it cannot read a dialect from (#11733)', () => { + for (const dialect of CORRUPTING_DIALECTS) { + it(`${dialect}: a real finding plans the engine's statements and names the dialect`, () => { + const plan = planStaleColumnTargets([engineFinding(dialect)], SQL); + expect(plan.refusals).toEqual([]); + expect(plan.targets).toHaveLength(1); + expect(plan.targets[0]).toMatchObject({ table: TABLE, column: COLUMN, to: 'json', dialect }); + // Also the call site's argument passing: these are the statements for + // THIS table and column, not for a pair fixed anywhere in the command. + expect(plan.targets[0].statements).toEqual( + splitRemedyStatements(manualJsonConversionSql(dialect, TABLE, COLUMN)), + ); + expect(plan.targets[0].from).toBe(dialect === 'postgres' ? 'character varying' : 'varchar'); + }); + } + + it('a finding whose message carries no statement we can read a dialect from is REFUSED', () => { + // The failure this closes: the engine rewords the message, the probe can no + // longer tell Postgres from MySQL, and the command picks one anyway and + // runs the wrong dialect's DDL against a customer's table. It refuses + // instead — and says what to do by hand. + const entry = engineFinding('postgres'); + const mutated = { ...entry, message: entry.message.replace('json_build_array', 'to_json') }; + + const plan = planStaleColumnTargets([mutated], SQL); + expect(plan.targets).toEqual([]); + expect(plan.refusals).toHaveLength(1); + expect(plan.refusals[0]).toMatchObject({ table: TABLE, column: COLUMN, reason: 'remedy_not_recognized' }); + expect(plan.refusals[0].detail).toContain('os migrate plan'); + }); + + it('ignores every drift op that is not this one', () => { + const others = diffManagedTable({ + table: TABLE, + fields: { [COLUMN]: { type: 'string', maxLength: 50 } as any }, + columns: STALE.postgres, + dialect: 'postgres', + }); + expect(others.map((d) => d.op.type)).toEqual(['narrow_varchar']); // the instrument found something + expect(planStaleColumnTargets(others, SQL)).toEqual({ targets: [], refusals: [] }); + }); + + it('--table narrows to the tables named, and drops the rest silently', () => { + const a = engineFinding('postgres'); + const b = { ...a, table: 'crm_case', op: { ...(a.op as any), table: 'crm_case' } } as ManagedDriftEntry; + // `b`'s message still carries `proj_task`'s statement, so it can only be + // planned if the filter lets it through — which it must not. + expect(planStaleColumnTargets([a, b], { ...SQL, tables: [TABLE] }).targets.map((t) => t.table)).toEqual([TABLE]); + expect(planStaleColumnTargets([a, b], { ...SQL, tables: ['nothing_here'] }).targets).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts b/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts index 1f21826b31..c5b6e92f45 100644 --- a/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts +++ b/packages/cli/src/commands/migrate/multi-value-columns.dry-run.test.ts @@ -30,8 +30,8 @@ * * - **is the statement right?** — answered where it can be: #11720 EXECUTES * the real remedy against live Postgres 16.13 and MySQL 8.0.46 over four - * row states, and `multi-value-columns.remedy-fidelity.test.ts` pins that - * this command runs that exact statement. + * row states, and the command runs that very function + * (`manualJsonConversionSql`) rather than a statement of its own. * - **does the executor honour the dry run?** — that is dialect-independent * control flow, and answering it needs a database this suite can actually * open. The statements below are a SQLite-legal stand-in shaped like the @@ -46,7 +46,7 @@ import { describe, it, expect, beforeEach, afterAll } from 'vitest'; import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { SqlDriver, diffManagedTable, type PhysicalColumn } from '@objectstack/driver-sql'; +import { SqlDriver, diffManagedTable, manualJsonConversionSql, type PhysicalColumn } from '@objectstack/driver-sql'; import { runStaleColumnMigration, planStaleColumnTargets, @@ -210,7 +210,7 @@ describe('os migrate multi-value-columns — the dry run changes nothing (#11733 columns: stale, dialect: 'postgres', }); - const plan = planStaleColumnTargets(entries); + const plan = planStaleColumnTargets(entries, { sql: manualJsonConversionSql }); expect(plan.targets).toHaveLength(1); // the plan is real const before = await snapshot(); diff --git a/packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts b/packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts deleted file mode 100644 index 76f09944db..0000000000 --- a/packages/cli/src/commands/migrate/multi-value-columns.remedy-fidelity.test.ts +++ /dev/null @@ -1,173 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * [#11733] The statement `os migrate multi-value-columns` runs must be the - * statement `@objectstack/driver-sql` printed — byte for byte, per dialect. - * - * ## Why this suite is the load-bearing one - * - * The remedy was corrected TWICE by measurement while #11720 was written, and - * neither correction is recoverable by reasoning about SQL: - * - * - `to_json(col)` makes a JSON **scalar** out of a legacy single value, so - * `Array.isArray` reads `false` under a field the metadata declares - * multi-value. Measured on live Postgres 16.13; `json_build_array` is what - * makes Postgres agree with MySQL's `JSON_ARRAY`. - * - `json_build_array(NULL)` is `[null]`, a one-element array. The explicit - * `IS NULL` arm was added AFTER the version without it was run on a live - * server and observed giving every NULL row a value. - * - * The CLI carries a copy of that statement (`manualJsonConversionSql` is not on - * `@objectstack/driver-sql`'s public surface, and this card's file surface is - * read-only over that package). A copy that can drift silently would lose both - * corrections the first time the engine improves the statement, so it is held - * to the engine's own output HERE: every case below builds the finding with - * `diffManagedTable()` — the package-root export that produces the message an - * operator actually sees — and requires the CLI's statement inside it. - * - * Deleting this suite deletes the only thing keeping the copy honest. - */ - -import { describe, it, expect } from 'vitest'; -import { diffManagedTable, type ManagedDriftEntry, type PhysicalColumn } from '@objectstack/driver-sql'; -import { - multiValueJsonMigrationSql, - splitRemedyStatements, - planStaleColumnTargets, - CORRUPTING_DIALECTS, -} from './multi-value-columns.js'; - -const TABLE = 'proj_task'; -const COLUMN = 'tags'; - -/** The stale column, in each dialect's own type spelling (#11720's fixtures). */ -const STALE: Record<'postgres' | 'mysql', PhysicalColumn[]> = { - postgres: [{ name: COLUMN, type: 'character varying', nullable: true, maxLength: 255 }], - mysql: [{ name: COLUMN, type: 'varchar', nullable: true, maxLength: 255 }], -}; - -const engineFinding = (dialect: 'postgres' | 'mysql'): ManagedDriftEntry => { - const out = diffManagedTable({ - table: TABLE, - fields: { [COLUMN]: { type: 'lookup', multiple: true } as any }, - columns: STALE[dialect], - dialect, - }); - // Non-vacuity: if the engine stopped reporting this shape, every containment - // assertion below would pass against nothing. - expect(out).toHaveLength(1); - expect(out[0].op.type).toBe('manual_column_type_change'); - return out[0]; -}; - -describe('the CLI remedy is the engine remedy (#11733 / #11720)', () => { - for (const dialect of CORRUPTING_DIALECTS) { - it(`${dialect}: the statement the command would run appears verbatim in the engine's own finding`, () => { - const entry = engineFinding(dialect); - const cliSql = multiValueJsonMigrationSql(dialect, TABLE, COLUMN); - - // The whole point: not "looks similar", not "contains an ALTER" — the - // engine's message CONTAINS the CLI's statement, character for character. - expect(entry.message).toContain(cliSql); - }); - } - - it('postgres keeps both corrections measurement forced — json_build_array, and the explicit IS NULL arm', () => { - const sql = multiValueJsonMigrationSql('postgres', TABLE, COLUMN); - expect(sql).toContain('json_build_array'); - // `to_json` is the reporter's original and the wrong answer here: it yields - // a JSON scalar, not a one-element array. - expect(sql).not.toContain('to_json'); - // Without this arm every NULL row gains `[null]`. - expect(sql).toContain(`WHEN "${COLUMN}" IS NULL THEN NULL`); - expect(sql).toContain(`WHEN "${COLUMN}" = '' THEN NULL`); - }); - - it('mysql is MySQL’s own statement, not the Postgres one with different quotes', () => { - const sql = multiValueJsonMigrationSql('mysql', TABLE, COLUMN); - expect(sql).toContain('JSON_ARRAY'); - expect(sql).not.toContain('json_build_array'); - // MySQL will not cast text to json implicitly — the rows have to be moved - // BEFORE the ALTER or it dies on the first legacy value. - const statements = splitRemedyStatements(sql); - expect(statements).toHaveLength(3); - expect(statements[0]).toMatch(/^UPDATE .* JSON_ARRAY/); - expect(statements[1]).toMatch(/^UPDATE .*= NULL WHERE/); - expect(statements[2]).toMatch(/^ALTER TABLE .*MODIFY .*json$/); - }); - - it('postgres is ONE statement — which is why a failed conversion leaves the column untouched', () => { - expect(splitRemedyStatements(multiValueJsonMigrationSql('postgres', TABLE, COLUMN))).toHaveLength(1); - }); - - it('the semicolon split loses nothing — neither form carries a semicolon inside a literal', () => { - // The split is the one #11720's live suite executes the remedy with. It is - // safe because of a property of THESE statements, so the property is pinned - // rather than assumed: re-joining the parts reproduces the original. - for (const dialect of CORRUPTING_DIALECTS) { - const sql = multiValueJsonMigrationSql(dialect, TABLE, COLUMN); - const rejoined = `${splitRemedyStatements(sql).join('; ')};`; - expect(rejoined).toBe(sql.trim()); - } - }); - - it('identifiers are the finding’s own table and column, not a fixed pair', () => { - // A builder that ignored its arguments would still pass every containment - // check above if the fixture happened to use the same names. - const other = multiValueJsonMigrationSql('postgres', 'crm_case', 'watchers'); - expect(other).toContain('"crm_case"'); - expect(other).toContain('"watchers"'); - expect(other).not.toContain(TABLE); - }); -}); - -describe('planning refuses anything it cannot match to the engine (#11733)', () => { - for (const dialect of CORRUPTING_DIALECTS) { - it(`${dialect}: a real finding plans the engine's statements and names the dialect`, () => { - const plan = planStaleColumnTargets([engineFinding(dialect)]); - expect(plan.refusals).toEqual([]); - expect(plan.targets).toHaveLength(1); - expect(plan.targets[0]).toMatchObject({ table: TABLE, column: COLUMN, to: 'json', dialect }); - expect(plan.targets[0].statements).toEqual( - splitRemedyStatements(multiValueJsonMigrationSql(dialect, TABLE, COLUMN)), - ); - // The dialect is READ OFF the finding, never off a client-name table this - // package would have to keep in step with the driver's. - expect(plan.targets[0].from).toBe(dialect === 'postgres' ? 'character varying' : 'varchar'); - }); - } - - it('a finding whose message no longer carries a statement we recognise is REFUSED, not guessed at', () => { - // The failure this closes: the engine improves the remedy, the CLI copy - // goes stale, and the command runs its own outdated SQL against a customer - // database. It refuses instead — and says what to do by hand. - const entry = engineFinding('postgres'); - const mutated = { ...entry, message: entry.message.replace('json_build_array', 'to_json') }; - - const plan = planStaleColumnTargets([mutated]); - expect(plan.targets).toEqual([]); - expect(plan.refusals).toHaveLength(1); - expect(plan.refusals[0]).toMatchObject({ table: TABLE, column: COLUMN, reason: 'remedy_not_recognized' }); - expect(plan.refusals[0].detail).toContain('os migrate plan'); - }); - - it('ignores every drift op that is not this one', () => { - const others = diffManagedTable({ - table: TABLE, - fields: { [COLUMN]: { type: 'string', maxLength: 50 } as any }, - columns: STALE.postgres, - dialect: 'postgres', - }); - expect(others.map((d) => d.op.type)).toEqual(['narrow_varchar']); // the instrument found something - expect(planStaleColumnTargets(others)).toEqual({ targets: [], refusals: [] }); - }); - - it('--table narrows to the tables named, and drops the rest silently', () => { - const a = engineFinding('postgres'); - const b = { ...a, table: 'crm_case', op: { ...(a.op as any), table: 'crm_case' } } as ManagedDriftEntry; - // `b`'s message still carries `proj_task`'s statement, so it can only be - // planned if the filter lets it through — which it must not. - expect(planStaleColumnTargets([a, b], { tables: [TABLE] }).targets.map((t) => t.table)).toEqual([TABLE]); - expect(planStaleColumnTargets([a, b], { tables: ['nothing_here'] }).targets).toEqual([]); - }); -}); diff --git a/packages/cli/src/commands/migrate/multi-value-columns.ts b/packages/cli/src/commands/migrate/multi-value-columns.ts index 352e27b89c..f85643f536 100644 --- a/packages/cli/src/commands/migrate/multi-value-columns.ts +++ b/packages/cli/src/commands/migrate/multi-value-columns.ts @@ -29,65 +29,33 @@ export type CorruptingDialect = 'postgres' | 'mysql'; export const CORRUPTING_DIALECTS: readonly CorruptingDialect[] = ['postgres', 'mysql']; /** - * ── The remedy statement, taken from the engine rather than invented here ─── + * ── The remedy statement comes FROM THE ENGINE, at the point of use ───────── * - * This is the statement `@objectstack/driver-sql`'s `manualJsonConversionSql` - * emits inside the `manual_column_type_change` finding (#11720), reproduced - * character for character. It is NOT re-derived: two of its arms were corrected - * by running the earlier version against a live server, and a fresh derivation - * loses both. + * `manualJsonConversionSql` is `@objectstack/driver-sql`'s own builder — the + * one whose output `manual_column_type_change` prints — re-exported from that + * package for this command (#11733) and imported here rather than reproduced. + * Two of its arms were corrected by running the earlier version against a live + * server (`json_build_array` over `to_json`, which yields a JSON *scalar*; and + * the explicit `IS NULL` arm, because `json_build_array(NULL)` is `[null]`), so + * a second copy of it in this package could only ever be a copy that goes + * stale. There is now exactly one definition, in the package that measured it. * - * - `json_build_array(col)`, never `to_json(col)`. `to_json` turns a legacy - * single value into a JSON **scalar**, so `Array.isArray` reads `false` - * under a field the metadata now declares multi-value. `json_build_array` - * makes Postgres 16.13 hand back the same value MySQL 8.0.46's `JSON_ARRAY` - * does. - * - the explicit `IS NULL` arm is load-bearing: `json_build_array(NULL)` is - * `[null]`, a one-element array. The arm was added AFTER the version - * without it was run on a live Postgres and observed giving every NULL row - * a value. - * - * ⚠️ The copy is not trusted on its word anywhere in this command. Two guards - * hold it to the engine's: - * - * 1. at RUNTIME — {@link planStaleColumnTargets} refuses to execute a - * statement the engine's own finding does not contain verbatim, so the - * only statements this command can ever run are ones `driver-sql` printed - * for that exact table, column and dialect; - * 2. at TEST TIME — `multi-value-columns.remedy-fidelity.test.ts` pins this - * output against `diffManagedTable()`'s finding for both dialects, so a - * correction landing in the engine turns this package red instead of - * leaving a stale statement behind a green suite. - * - * The copy exists because `manualJsonConversionSql` is not part of - * `@objectstack/driver-sql`'s public surface (`src/index.ts` re-exports two - * blocks from `schema-drift.js` and it is in neither) and this card's file - * surface is read-only over that package. Both guards above become unnecessary - * the day it is exported — see the PR body. + * It arrives through {@link RemedySqlBuilder} rather than a module-level import + * for a mechanical reason, not a stylistic one: no CLI production module may + * STATICALLY value-import a driver package. oclif `import()`s every command + * module on every invocation while building its command table, so one static + * driver import makes an unbuilt `driver-sql/dist` print a `MODULE_NOT_FOUND` + * block for each of the nine commands sharing that chain, in front of whatever + * the operator actually ran, and drops all nine from the table (#5726, pinned + * by `schema-migrate.lazy-driver-import.test.ts`). The command therefore + * `await import(...)`s the driver at the point of use and hands the builder in, + * which also keeps {@link planStaleColumnTargets} synchronous and pure. */ -export function multiValueJsonMigrationSql( +export type RemedySqlBuilder = ( dialect: CorruptingDialect, 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 ` + - `(CASE WHEN "${column}" IS NULL THEN NULL WHEN "${column}" = '' THEN NULL ` + - `WHEN "${column}" LIKE '[%' THEN "${column}"::json ` + - `ELSE json_build_array("${column}") END);` - ); -} +) => string; /** * Split the remedy into the statements a driver seam can take one at a time. @@ -95,7 +63,7 @@ export function multiValueJsonMigrationSql( * The same split #11720's live suite executes the remedy with. It is not a * general SQL splitter and does not need to be: neither dialect's form contains * a semicolon inside a literal (`''`, `'['`), which is a property of THESE two - * statements, pinned by `multi-value-columns.remedy-fidelity.test.ts`. + * statements, pinned by `multi-value-columns.dialect-probe.test.ts`. * * The MySQL form is three statements and must be run in order — the two UPDATEs * put every row into a shape the `MODIFY … json` will accept. @@ -143,26 +111,35 @@ export function isStaleMultiValueColumn(entry: ManagedDriftEntry): boolean { * * ## The dialect is read off the finding, not off the connection * - * There is no client-name table here on purpose. `driver-sql` owns the mapping - * from a knex client spelling to a dialect (`postgres` / `pg` / `postgresql` - * are one dialect under three names, and getting that list wrong is a measured - * defect class — see `POSTGRES_EMIT_CLIENTS`), and a second copy in this - * package would be a copy that can disagree. So the dialect is decided by which - * dialect's statement the ENGINE's own finding contains: the two forms are - * unmistakable (backtick-quoted MySQL versus double-quoted Postgres), and a - * match is simultaneously the proof that the statement about to run is the one - * `driver-sql` printed for this exact table and column. + * A `ManagedDriftEntry` does not carry a dialect, and there is deliberately no + * client-name table here: `driver-sql` owns the mapping from a knex client + * spelling to a dialect (`postgres` / `pg` / `postgresql` are one dialect under + * three names, and getting that list wrong is a measured defect class — see + * `POSTGRES_EMIT_CLIENTS`), so a second copy of it in this package could only + * ever disagree with it. The dialect is therefore decided by WHICH dialect's + * statement the engine's own finding message contains; the two forms are + * unmistakable, backtick-quoted MySQL against double-quoted Postgres. + * + * ## What the containment check claims, now that `sql` IS the engine's function + * + * While this command carried its own copy of the statement, this check was also + * the guard that the copy had not gone stale. That job is gone — `sql` is + * `manualJsonConversionSql` itself (#11733), so the two sides cannot disagree, + * and a test asserting they match would be a test that cannot fail. * - * A finding whose message contains NEITHER form is refused, never guessed at - * and never run — that is a remedy this command no longer recognises, and - * executing a statement the engine did not print is the one thing it must not - * do. SQLite lands here too if it ever produced this finding, which it does not - * (measured: SQLite reads a stale column back as a real array, so there is - * nothing to migrate and `diffManagedTable` stays silent). + * The check stays because the OTHER job it was doing is the one it now does + * alone: it reads the dialect, by matching the engine's finding against the + * engine's own function. Its failure branch remains reachable and load-bearing. + * A finding whose message no longer embeds the remedy — a message the engine + * reworded, an entry forwarded without one — yields no dialect, and the command + * REFUSES it rather than guess which of two dialects' DDL to run against a + * customer's table. SQLite would land there too if it ever produced this + * finding, which it does not: it reads a stale column back as a real array, so + * `diffManagedTable` stays silent (measured, #11720). */ export function planStaleColumnTargets( entries: ManagedDriftEntry[], - opts: { tables?: string[] } = {}, + opts: { sql: RemedySqlBuilder; tables?: string[] }, ): StaleColumnPlan { const wanted = opts.tables && opts.tables.length > 0 ? new Set(opts.tables) : null; const targets: StaleColumnTarget[] = []; @@ -175,7 +152,7 @@ export function planStaleColumnTargets( const message = typeof entry.message === 'string' ? entry.message : ''; const dialect = CORRUPTING_DIALECTS.find((d) => - message.includes(multiValueJsonMigrationSql(d, op.table, op.column)), + message.includes(opts.sql(d, op.table, op.column)), ); if (!dialect) { @@ -197,7 +174,7 @@ export function planStaleColumnTargets( from: op.from, to: op.to, dialect, - statements: splitRemedyStatements(multiValueJsonMigrationSql(dialect, op.table, op.column)), + statements: splitRemedyStatements(opts.sql(dialect, op.table, op.column)), }); } @@ -468,8 +445,16 @@ export default class MigrateMultiValueColumns extends Command { return; } + // Lazily, at the point of use: a static value import of a driver package + // in a command module costs every OTHER command its place in oclif's + // table when the driver is not built (#5726). + const { manualJsonConversionSql } = await import('@objectstack/driver-sql'); + const drift = await stack.driver.detectManagedDrift(); - const plan = planStaleColumnTargets(drift, flags.table ? { tables: flags.table } : {}); + const plan = planStaleColumnTargets(drift, { + sql: manualJsonConversionSql, + ...(flags.table ? { tables: flags.table } : {}), + }); let result: StaleColumnRunResult; let verified: boolean | null = null; @@ -522,7 +507,7 @@ export default class MigrateMultiValueColumns extends Command { // reports the column has not done what it claims. if (result.outcomes.some((o) => o.status === 'migrated')) { const after = await stack.driver.detectManagedDrift(); - const stillStale = planStaleColumnTargets(after).targets; + const stillStale = planStaleColumnTargets(after, { sql: manualJsonConversionSql }).targets; verified = !result.outcomes.some( (o) => o.status === 'migrated' && stillStale.some((t) => t.table === o.table && t.column === o.column), ); diff --git a/packages/drivers/driver-sql/src/index.ts b/packages/drivers/driver-sql/src/index.ts index 99d71b7231..ce67dfa0ba 100644 --- a/packages/drivers/driver-sql/src/index.ts +++ b/packages/drivers/driver-sql/src/index.ts @@ -76,6 +76,7 @@ export { isSyncReproducibleIndex, legacyUniqueIndexNames, legacyUniqueReplacements, + manualJsonConversionSql, normalizeDeclaredIndex, parseIndexDdl, uniqueIndexesFromFields,