From 5c5b55af111947c5aa2b3d99222feb03572ec962 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 22:05:57 -0400 Subject: [PATCH 1/8] fix(migrate-ts): let Postgres canonicalise expected column defaults and the column-rename CHECK carry --- CHANGELOG.md | 18 +++ docs/features/migrations-and-drift.md | 7 + .../migrate-ts/src/check-expr-compare.ts | 12 +- .../packages/migrate-ts/src/diff/index.ts | 73 +++++++-- .../migrate-ts/src/expr-canon-probe.ts | 43 ++++- .../packages/migrate-ts/src/expr-canon.ts | 32 +++- .../src/snapshot/adopt-generated-spelling.ts | 15 +- .../test/integration/pg-default-canon.test.ts | 149 ++++++++++++++++++ .../integration/pg-rename-carry-canon.test.ts | 83 ++++++++++ .../diff-rename-carries-constraints.test.ts | 9 ++ 10 files changed, 412 insertions(+), 29 deletions(-) create mode 100644 server/typescript/packages/migrate-ts/test/integration/pg-default-canon.test.ts create mode 100644 server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index fcdcecd1e1..adcc470c5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -299,6 +299,24 @@ here.** so. The three flags it never read (`--server`, `--client`, `--out`) are still accepted but ignored and hidden from help, so the old invocation still reaches the pointer. +- **Postgres column `DEFAULT`s that Postgres re-spells no longer read as drift.** `NOW()`, + `current_date`, a date literal on a `timestamptz`, `'1e3'` on a double, `'007'` on an integer + and a composite expression such as `now() + interval '1 day'` were reported by `meta verify + --db` as `change-column-default` on every run, and `meta migrate --from-db` proposed a + `SET DEFAULT` for each, because the comparison was the classifier's text against the text + Postgres hands back. The same expression-canon probe that answers CHECKs now answers defaults: + inside its rolled-back transaction it sets each expected default on the scratch copy and reads + `pg_get_expr`, and a literal on a `timestamptz` is compared in the reading session's + `TimeZone`. A real change (another value, another instant, another function) is still reported. + The classifier is still the comparison for SQLite, offline runs, read-only connections and a + default that does not compile against the live column. `baseline --from-db` records the + authored default where the probe proved it equal, so the offline plan starts clean too. + A column rename that carries a CHECK is now decided the same way: the scratch copy renames the + column first, so a rename plus a literal-case change (`'OPEN'` to `'open'`) is not folded into a + plain `RENAME CONSTRAINT`. The anonymous-CHECK match on SQLite and D1 shares the comparison. + Separately, the rename rewrite no longer touches a cast type, a function name or the inside of + an `E'..'` literal when a column of that word is renamed. + - **`@default: "now"` is the current time on Postgres, not the table's creation time.** The expected schema stored it as the literal `'now'`, and Postgres converts that literal to a timestamp once, when it creates the column's `DEFAULT`, so every row got the same value. It diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 81088da4b7..32dca50f0b 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -403,6 +403,13 @@ The normaliser cannot tell a precedence change from a bracket-only change (it de bracket), so on the fallback path `(a OR b) AND c` and `a OR (b AND c)` compare as equal. A fallback never reports drift of its own: a probe that fails changes nothing about the answer. +The probe answers column defaults the same way. Postgres re-spells a default (`NOW()` becomes +`now()`, `current_date` becomes `CURRENT_DATE`, a date literal on a `timestamptz` becomes a full +timestamp read in the session's `TimeZone`, `'1e3'` on a double becomes `'1000'`), so for a column +with a default on both sides the expected one is set on the scratch table and `pg_get_expr` is +read back. A column rename that carries a CHECK is answered on a scratch copy whose columns are +renamed first. + `meta migrate baseline --from-db` on Postgres closes the offline gap. Where the live database proves a CHECK, or an expression index's key or predicate, is what the metadata would generate, the snapshot records the **generated** text rather than Postgres's spelling, so the offline plan diff --git a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts index e43a3a35c4..941f686986 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -495,7 +495,7 @@ export function stripCheckWrapper(def: string): string { * a constraint has after `RENAME COLUMN`, which rewrites it in place on both engines. * Matches a double-quoted identifier or a bare word token, and never touches the inside of * a single-quoted literal (a regex or IN-list value that happens to spell a column name is - * data, not a reference). Used only to COMPARE: a miss leaves the drop+add pair it would + * data, not a reference), a word after `::` (a cast type) or a word before `(` (a function). Used only to COMPARE: a miss leaves the drop+add pair it would * have paired exactly as it was — gated — so it fails closed. */ export function renameExprIdentifiers(expr: string, renames: ReadonlyMap): string { @@ -505,10 +505,13 @@ export function renameExprIdentifiers(expr: string, renames: ReadonlyMap { + await carryThroughColumnRenames(changes, args.dialect, (id) => { const live = predecessors.get(id) ?? actualTables.get(id); return live === undefined ? NO_TEXT_COLUMNS : textColumnsOf(live); + }, args.exprCanonProbe === undefined ? undefined : { + probe: args.exprCanonProbe, + liveOf: (id) => predecessors.get(id) ?? actualTables.get(id), + expectedOf: (id) => expectedTables.get(id), }); // Strip the rename-detection side-channel fields before status assignment / return. @@ -413,12 +417,14 @@ async function probeExprCanon( } const checks = new Map(); const indexes = new Map(); + const defaults = new Map(); for (const [id, expected] of expectedTables) { const live = actualTables.get(id) ?? liveOf.get(id); if (live === undefined) continue; const liveChecks = new Map(live.checks.map((c) => [c.name, c.expression])); const liveIndexes = new Map(live.indexes.map((ix) => [ix.name, ix])); - // Both lookups are by expected descriptor, so the probe is asked only about those the + const liveColumns = new Map(live.columns.map((c) => [c.name, c])); + // Every lookup is by expected descriptor, so the probe is asked only about those the // comparison could not settle by identical text. const unsettled: TableDescriptor = { ...expected, @@ -427,14 +433,21 @@ async function probeExprCanon( const l = liveIndexes.get(ix.name); return l === undefined || ix.expr !== l.expr || ix.where !== l.where; }), + // A default is only a spelling question when both sides have one; a missing side is + // a real add or drop. + columns: expected.columns.filter((c) => { + const l = liveColumns.get(c.name)?.default; + return c.default !== undefined && l !== undefined && !columnDefaultsEqual(c.default, l); + }), }; - if (unsettled.checks.length === 0 && unsettled.indexes.length === 0) continue; + if (unsettled.checks.length === 0 && unsettled.indexes.length === 0 && unsettled.columns.length === 0) continue; const answer = await probe(live, unsettled); if (answer === undefined) continue; for (const [c, text] of answer.checks) checks.set(c, text); for (const [ix, text] of answer.indexes) indexes.set(ix, text); + for (const [c, d] of answer.defaults) defaults.set(c, d); } - return { checks, indexes }; + return { checks, indexes, defaults }; } /** @@ -451,7 +464,7 @@ function diffTablePair( dialect: Dialect | undefined, canon: ExprCanon, ): void { - diffTableColumns(expected, actual, changes, dialect); + diffTableColumns(expected, actual, changes, dialect, canon); diffTableIndexes(expected, actual, changes, canon); diffTableForeignKeys(expected, actual, changes, dialect); // CHECK constraints on existing tables are evolved whenever a dialect is @@ -525,6 +538,12 @@ function foldNameOnlyChanges( return tableChanges.filter((c) => !folded.has(c)); } +function idOfChange(c: Change): string | undefined { + return "table" in c && typeof c.table === "string" + ? tableIdentity({ name: c.table, ...schemaSpread(c.schema) }) + : undefined; +} + /** * Pair each drop+add of a CHECK / FK / index that a `rename-column` in the same table * explains — the add is the drop with the renamed columns respelled — and mark it carried. @@ -543,11 +562,16 @@ function foldNameOnlyChanges( * * Anything that is not EXACTLY the old rule under the new names stays a gated drop+add. */ -function carryThroughColumnRenames( +async function carryThroughColumnRenames( changes: Change[], dialect: Dialect | undefined, textColumnsOfTable: (tableId: string) => ReadonlySet, -): void { + postgres?: { + probe: ExprCanonProbe; + liveOf: (tableId: string) => TableDescriptor | undefined; + expectedOf: (tableId: string) => TableDescriptor | undefined; + }, +): Promise { const renamesByTable = new Map>(); const renameChangeByTable = new Map>(); for (const c of changes) { @@ -559,13 +583,28 @@ function carryThroughColumnRenames( } if (renamesByTable.size === 0) return; - const idOf = (c: Change): string | undefined => - "table" in c && typeof c.table === "string" - ? tableIdentity({ name: c.table, ...schemaSpread(c.schema) }) - : undefined; + const idOf = idOfChange; const mapCols = (cols: readonly string[], m: ReadonlyMap): string[] => cols.map((col) => m.get(col) ?? col); + // Postgres: the add half of a CHECK pair mentions the NEW column name, which the live table + // does not have yet, so the Pass 1 probe could not compile it. Ask again on a scratch copy + // whose columns are renamed first, so the comparison below is Postgres's spelling against + // Postgres's (the live body respelled for the rename) rather than two strings. + const checkCanon = new Map(); + if (postgres !== undefined && dialect === "postgres") { + for (const [table, renames] of renamesByTable) { + const live = postgres.liveOf(table); + const expected = postgres.expectedOf(table); + if (live === undefined || expected === undefined) continue; + const adds = changes.flatMap((c) => (c.kind === "add-check" && idOfChange(c) === table ? [c.check] : [])); + if (adds.length === 0) continue; + const answer = await postgres.probe(live, { ...expected, checks: adds, indexes: [], columns: [] }, renames); + for (const [c, text] of answer?.checks ?? []) checkCanon.set(c, text); + } + } + const canon: ExprCanon = { ...NO_EXPR_CANON, checks: checkCanon }; + const paired = new Set(); const pairs: { drop: Change; add: Change; from: string; to: string; index: boolean; table: string }[] = []; for (const d of changes) { @@ -579,7 +618,8 @@ function carryThroughColumnRenames( const lowerRenames = new Map([...renames].map(([from, to]) => [from.toLowerCase(), to.toLowerCase()])); const textColumns = new Set([...textColumnsOfTable(table)].map((col) => lowerRenames.get(col) ?? col)); const a = changes.find((c): c is Extract => - c.kind === "add-check" && sameTable(c) && checkExprEquals(c.check.expression, want, textColumns)); + c.kind === "add-check" && sameTable(c) + && checkMatchesLive(canon, c.check, { ...d.restore!, expression: want }, textColumns)); if (a) { paired.add(d).add(a); pairs.push({ drop: d, add: a, from: d.check, to: a.check.name, index: false, table }); } } else if (d.kind === "drop-fk" && d.restore !== undefined) { const want: FkDescriptor = { ...d.restore, columns: mapCols(d.restore.columns, renames) }; @@ -688,6 +728,7 @@ function diffTableColumns( actual: TableDescriptor, changes: Change[], dialect?: Dialect, + canon: ExprCanon = NO_EXPR_CANON, ): void { const table = expected.name; const sx = schemaSpread(expected.schema); @@ -755,7 +796,7 @@ function diffTableColumns( const toDefault = skipIdentityDefaultDiff ? ac.default : ec.default; if (ac.default !== undefined) typeChange.fromDefault = ac.default; if (toDefault !== undefined) typeChange.toDefault = toDefault; - } else if (!skipIdentityDefaultDiff && !columnDefaultsEqual(ec.default, ac.default)) { + } else if (!skipIdentityDefaultDiff && !columnDefaultMatchesLive(canon, ec, ac)) { const change: Change = { kind: "change-column-default", table, ...sx, column: name, status: ALLOWED, @@ -936,7 +977,7 @@ function diffTableChecks( continue; } if (exprFallback) { - const match = actual.checks.find((a) => !consumed.has(a) && checkExprEquals(ec.expression, a.expression)); + const match = actual.checks.find((a) => !consumed.has(a) && checkMatchesLive(canon, ec, a)); if (match) { consumed.add(match); continue; } // An anonymous/name-mismatched check in the pre-1.0.9 null-safe spelling: the same // rule, re-spelled once so an older SQLite can open the database. diff --git a/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts b/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts index 9871f3c3be..615f444747 100644 --- a/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts +++ b/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts @@ -9,10 +9,10 @@ import type { Kysely } from "kysely"; import { sql } from "kysely"; import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; -import { indexDefKeyExpr } from "./introspect/postgres.js"; +import { indexDefKeyExpr, parsePgDefault } from "./introspect/postgres.js"; import { stripCheckWrapper } from "./check-expr-compare.js"; import type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; -import type { CheckDescriptor, IndexDescriptor } from "./types.js"; +import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor } from "./types.js"; import { quote, rolledBack } from "./view-replace-probe.js"; const TABLE = "mo_check_probe"; @@ -20,6 +20,11 @@ const CONSTRAINT = "mo_check_probe_c"; const INDEX = "mo_check_probe_i"; const SAVEPOINT = "mo_check_probe_sp"; +/** A default as it must be spelled after `SET DEFAULT`: a literal is quoted, an expression is the author's text. */ +function defaultSql(d: ColumnDefault): string { + return d.kind === "expr" ? d.value : `'${d.value.replace(/'/g, "''")}'`; +} + /** SQLSTATE that means no table will ever probe on this connection: the transaction is read-only. */ const READ_ONLY_SQLSTATE = "25006"; @@ -64,7 +69,7 @@ export function postgresExprCanonProbe(db: Kysely>): Exp // Set once the server refuses a scratch table because it is read-only, so a standby is asked // once rather than once per table. Any other refusal (such as no SELECT on one table) is per table. let unavailable = false; - return async (live, expected) => { + return async (live, expected, renames) => { if (unavailable) return undefined; const checks = expected.checks.filter((c) => probeable(c.expression)); const indexes = expected.indexes.filter( @@ -72,7 +77,13 @@ export function postgresExprCanonProbe(db: Kysely>): Exp && probeable(ix.expr) && probeable(ix.where) && (ix.using === undefined || ACCESS_METHOD.test(ix.using)), ); - if (checks.length === 0 && indexes.length === 0) return undefined; + // Only a column with a default on both sides can differ in spelling alone; a missing side + // is a real add or drop. The scratch table copies the LIVE column, so the default is + // coerced to the column's live type, exactly as it is stored today. + const liveColumns = new Map(live.columns.map((c) => [c.name, c])); + const defaults = expected.columns.filter((c) => + c.default !== undefined && liveColumns.get(c.name)?.default !== undefined && probeable(c.default.value)); + if (checks.length === 0 && indexes.length === 0 && defaults.length === 0) return undefined; const liveTable = `${quote(live.schema ?? DEFAULT_DB_SCHEMA_POSTGRES)}.${quote(live.name)}`; const scratch = `pg_temp.${quote(TABLE)}`; try { @@ -84,6 +95,28 @@ export function postgresExprCanonProbe(db: Kysely>): Exp if (code === READ_ONLY_SQLSTATE) unavailable = true; throw err; } + // Two phases through scratch names, so a swap (a -> b, b -> a) cannot collide. + const moves = [...(renames ?? [])]; + for (const [i, [from]] of moves.entries()) { + await sql.raw(`ALTER TABLE ${scratch} RENAME COLUMN ${quote(from)} TO ${quote(`mo_rename_${i}`)}`).execute(q); + } + for (const [i, [, to]] of moves.entries()) { + await sql.raw(`ALTER TABLE ${scratch} RENAME COLUMN ${quote(`mo_rename_${i}`)} TO ${quote(to)}`).execute(q); + } + const defaultAnswers = new Map(); + for (const col of defaults) { + const raw = await attempt(q, async () => { + await sql.raw(`ALTER TABLE ${scratch} ALTER COLUMN ${quote(col.name)} SET DEFAULT ${defaultSql(col.default!)}`).execute(q); + const rows = await sql<{ def: string | null }>` + SELECT pg_get_expr(d.adbin, d.adrelid) AS def FROM pg_attrdef d + JOIN pg_attribute a ON a.attrelid = d.adrelid AND a.attnum = d.adnum + WHERE d.adrelid = ${scratch}::regclass AND a.attname = ${col.name} + `.execute(q); + return rows.rows[0]?.def; + }); + const stored = raw === undefined || raw === null ? undefined : parsePgDefault(raw); + if (stored !== undefined) defaultAnswers.set(col, stored); + } const checkAnswers = new Map(); for (const check of checks) { const def = await attempt(q, async () => { @@ -114,7 +147,7 @@ export function postgresExprCanonProbe(db: Kysely>): Exp if (ix.where !== undefined && stored.predicate !== null) canon.where = stored.predicate; indexAnswers.set(ix, canon); } - return { checks: checkAnswers, indexes: indexAnswers }; + return { checks: checkAnswers, indexes: indexAnswers, defaults: defaultAnswers }; }); } catch { return undefined; diff --git a/server/typescript/packages/migrate-ts/src/expr-canon.ts b/server/typescript/packages/migrate-ts/src/expr-canon.ts index a855f1f651..81f9aa1ead 100644 --- a/server/typescript/packages/migrate-ts/src/expr-canon.ts +++ b/server/typescript/packages/migrate-ts/src/expr-canon.ts @@ -14,8 +14,9 @@ // against the live columns) yields no entry, and the comparison falls back to the string // normaliser exactly as it was. -import type { CheckDescriptor, IndexDescriptor, TableDescriptor } from "./types.js"; +import type { CheckDescriptor, ColumnDescriptor, ColumnDefault, IndexDescriptor, TableDescriptor } from "./types.js"; import { checkExprEquals, normalizeCheckExpr } from "./check-expr-compare.js"; +import { columnDefaultsEqual } from "./column-default.js"; /** Postgres's stored form of the expression parts of one expected index. */ export interface IndexExprCanon { @@ -33,9 +34,14 @@ export interface IndexExprCanon { export interface ExprCanon { readonly checks: ReadonlyMap; readonly indexes: ReadonlyMap; + /** + * Each expected column DEFAULT as Postgres stores it (`pg_get_expr(adbin, adrelid)`, read + * through the introspector's own `parsePgDefault`), keyed by the expected column. + */ + readonly defaults: ReadonlyMap; } -export const NO_EXPR_CANON: ExprCanon = { checks: new Map(), indexes: new Map() }; +export const NO_EXPR_CANON: ExprCanon = { checks: new Map(), indexes: new Map(), defaults: new Map() }; /** * Ask the database how it would store `expected`'s CHECK constraints and index expressions @@ -46,6 +52,12 @@ export const NO_EXPR_CANON: ExprCanon = { checks: new Map(), indexes: new Map() export type ExprCanonProbe = ( live: TableDescriptor, expected: TableDescriptor, + /** + * Live column renames (old name → new name) the scratch table applies before probing, so + * an expected expression that names a renamed column compiles against the table as it + * will be after the migration. + */ + renames?: ReadonlyMap, ) => Promise; /** @@ -71,3 +83,19 @@ export function indexExprMatchesLive( ): boolean { return stored !== undefined ? stored === live : normalizeCheckExpr(expected) === normalizeCheckExpr(live); } + +/** + * The same decision for one column DEFAULT. With the database's answer, Postgres's spelling + * of the expected default is compared with the live one (both read by `parsePgDefault`, so + * `NOW()`/`now()`, `current_date`/`CURRENT_DATE`, a date literal on a timestamptz, `'1e3'` + * on a double and `'007'` on an integer are equal when Postgres stores them equal, and a + * real change still differs); without it, the classifier's `kind` and `value` as written. + */ +export function columnDefaultMatchesLive( + canon: ExprCanon, + expected: ColumnDescriptor, + live: ColumnDescriptor, +): boolean { + const stored = canon.defaults.get(expected); + return columnDefaultsEqual(stored ?? expected.default, live.default); +} diff --git a/server/typescript/packages/migrate-ts/src/snapshot/adopt-generated-spelling.ts b/server/typescript/packages/migrate-ts/src/snapshot/adopt-generated-spelling.ts index 8894145362..62288dacbe 100644 --- a/server/typescript/packages/migrate-ts/src/snapshot/adopt-generated-spelling.ts +++ b/server/typescript/packages/migrate-ts/src/snapshot/adopt-generated-spelling.ts @@ -1,5 +1,5 @@ // adopt-generated-spelling.ts — what `baseline --from-db` writes for a CHECK / index -// expression it can prove equal to the one migrate generates. +// expression or a column DEFAULT it can prove equal to the one migrate generates. // // An introspected Postgres snapshot carries Postgres's spelling (`(resource_kind = 'space'::text)`) // of every expression. The OFFLINE plan has no database to ask, so it compares that text with the @@ -12,7 +12,8 @@ import type { ExprCanonProbe } from "../expr-canon.js"; import { qualifiedDbName } from "../qualified-name.js"; -import type { CheckDescriptor, IndexDescriptor, SchemaSnapshot, TableDescriptor } from "../types.js"; +import { columnDefaultsEqual } from "../column-default.js"; +import type { CheckDescriptor, ColumnDescriptor, IndexDescriptor, SchemaSnapshot, TableDescriptor } from "../types.js"; export async function adoptGeneratedSpelling( live: SchemaSnapshot, @@ -31,6 +32,14 @@ export async function adoptGeneratedSpelling( async function adoptTable(live: TableDescriptor, expected: TableDescriptor, probe: ExprCanonProbe): Promise { const answer = await probe(live, expected); if (answer === undefined) return live; + const expectedColumns = new Map(expected.columns.map((c) => [c.name, c] as const)); + const columns = live.columns.map((ac): ColumnDescriptor => { + const ec = expectedColumns.get(ac.name); + const stored = ec === undefined ? undefined : answer.defaults.get(ec); + return ec?.default !== undefined && stored !== undefined && columnDefaultsEqual(stored, ac.default) + ? { ...ac, default: ec.default } + : ac; + }); const expectedChecks = new Map(expected.checks.map((c) => [c.name, c] as const)); const expectedIndexes = new Map(expected.indexes.map((ix) => [ix.name, ix] as const)); const checks = live.checks.map((ac): CheckDescriptor => { @@ -46,5 +55,5 @@ async function adoptTable(live: TableDescriptor, expected: TableDescriptor, prob if (ei.where !== undefined && ai.where !== undefined && stored.where === ai.where) adopted.where = ei.where; return adopted; }); - return { ...live, checks, indexes }; + return { ...live, columns, checks, indexes }; } diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-default-canon.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-default-canon.test.ts new file mode 100644 index 0000000000..36cc603bbd --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-default-canon.test.ts @@ -0,0 +1,149 @@ +/** + * Column DEFAULTs: Postgres, not a regex classifier, decides whether the expected default is + * the stored one. + * + * Postgres re-spells a default at parse analysis (`NOW()` → `now()`, `current_date` → + * `CURRENT_DATE`, a date literal on a timestamptz → `'2024-01-01 00:00:00+00'::timestamp with + * time zone`, `'1e3'` on a double → `'1000'::double precision`, `'007'` on an integer → `7`), + * so comparing the author's text with the live text reported all of them as drift forever. + * The oracle is the live engine: a table is created from SQL, the expected side is the same + * table with the author's spelling, and the pair is diffed with and without the probe. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ + +import { test, expect, beforeAll, afterAll, describe } from "bun:test"; +import { Pool } from "pg"; +import { Kysely, PostgresDialect, sql } from "kysely"; +import { introspectPostgres } from "../../src/introspect/postgres.js"; +import { diff } from "../../src/diff/index.js"; +import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; +import { adoptGeneratedSpelling } from "../../src/snapshot/adopt-generated-spelling.js"; +import type { ColumnDefault, SchemaSnapshot } from "../../src/types.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const TABLE = "default_canon_t"; + +type Db = Kysely>; + +/** The author's spelling of each default, by column: what the model declares. */ +const AUTHORED: Record = { + b: { kind: "expr", value: "NOW()" }, + c: { kind: "expr", value: "current_date" }, + d: { kind: "literal", value: "2024-01-01" }, + i: { kind: "literal", value: "1e3" }, + j: { kind: "literal", value: "007" }, + k: { kind: "expr", value: "now() + interval '1 day'" }, +}; + +describe("column DEFAULT canon (live PG)", () => { + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + const connect = (options?: string): { pool: Pool; db: Db } => { + const pool = new Pool({ connectionString: PG_URL, ...(options !== undefined ? { options } : {}) }); + return { pool, db: new Kysely>({ dialect: new PostgresDialect({ pool }) }) }; + }; + let pool: Pool; + let db: Db; + + const create = (q: Db): Promise => + sql.raw(` + DROP TABLE IF EXISTS ${TABLE}; + CREATE TABLE ${TABLE} ( + id bigserial PRIMARY KEY, + b timestamptz DEFAULT NOW(), c date DEFAULT current_date, d timestamptz DEFAULT '2024-01-01', + i double precision DEFAULT '1e3', j integer DEFAULT '007', k timestamptz DEFAULT now() + interval '1 day', + s text DEFAULT 'a' + )`).execute(q); + + beforeAll(async () => { + ({ pool, db } = connect()); + await create(db); + }); + afterAll(async () => { + await sql.raw(`DROP TABLE IF EXISTS ${TABLE}`).execute(db); + await pool.end(); + }); + + /** The live schema, and a copy declaring `defaults` (column → default) over the live ones. */ + const declared = async (conn: Db, defaults: Record): Promise<{ live: SchemaSnapshot; expected: SchemaSnapshot }> => { + const live = await introspectPostgres(conn); + const expected = structuredClone(live); + const t = expected.tables.find((x) => x.name === TABLE)!; + for (const [col, d] of Object.entries(defaults)) t.columns.find((c) => c.name === col)!.default = d; + return { live, expected }; + }; + const defaultChanges = async (conn: Db, defaults: Record, probe: boolean) => { + const { live, expected } = await declared(conn, defaults); + const d = await diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + ...(probe ? { exprCanonProbe: postgresExprCanonProbe(conn) } : {}), + }); + return d.changes.filter((c) => c.kind === "change-column-default").map((c) => (c as { column: string }).column).sort(); + }; + + test("the string classifier reports every re-spelled default as drift (the defect)", async () => { + expect(await defaultChanges(db, AUTHORED, false)).toEqual(["b", "c", "d", "i", "j", "k"]); + }); + + test("with the probe, NOW(), current_date, a date literal, 1e3, 007 and a composite expression read clean", async () => { + expect(await defaultChanges(db, AUTHORED, true)).toEqual([]); + }); + + test("a real default change is still reported", async () => { + const changed: Record = { + ...AUTHORED, + s: { kind: "literal", value: "b" }, + j: { kind: "literal", value: "008" }, + i: { kind: "literal", value: "1001" }, + d: { kind: "literal", value: "2024-01-02" }, + k: { kind: "expr", value: "now() + interval '2 days'" }, + b: { kind: "expr", value: "clock_timestamp()" }, + }; + expect(await defaultChanges(db, changed, true)).toEqual(["b", "d", "i", "j", "k", "s"]); + }); + + test("an expression that does not compile against the table keeps the classifier's answer", async () => { + expect(await defaultChanges(db, { s: { kind: "expr", value: "no_such_function(1)" } }, true)).toEqual(["s"]); + }); + + test("the probe leaves nothing behind", async () => { + await defaultChanges(db, AUTHORED, true); + const left = await sql<{ n: number }>`SELECT count(*)::int AS n FROM pg_class WHERE relname = 'mo_check_probe' AND relpersistence = 't'`.execute(db); + expect(left.rows[0]!.n).toBe(0); + }); + + describe("session TimeZone", () => { + for (const zone of ["UTC", "America/New_York", "Asia/Kolkata"]) { + test(`a timestamptz literal default reads clean when created and read in ${zone}`, async () => { + const { pool: p, db: conn } = connect(`-c TimeZone=${zone}`); + try { + await create(conn); + expect(await defaultChanges(conn, { d: AUTHORED["d"]! }, true)).toEqual([]); + // …and a different instant is still a change. + expect(await defaultChanges(conn, { d: { kind: "literal", value: "2024-01-02" } }, true)).toEqual(["d"]); + } finally { + await p.end(); + } + }); + } + test("restore the shared table", async () => { + await create(db); + }); + }); + + test("baseline --from-db records the authored default for one the probe proved equal", async () => { + const { live, expected } = await declared(db, { ...AUTHORED, s: { kind: "literal", value: "b" } }); + const adopted = await adoptGeneratedSpelling(live, expected, postgresExprCanonProbe(db)); + const cols = adopted.tables.find((t) => t.name === TABLE)!.columns; + for (const [col, d] of Object.entries(AUTHORED)) expect(cols.find((c) => c.name === col)!.default).toEqual(d); + // A different default keeps what the database has: recording the authored one would hide it. + expect(cols.find((c) => c.name === "s")!.default).toEqual({ kind: "literal", value: "a" }); + + // The offline plan then compares like with like, and still reports the real change. + const offline = await diff({ expected, actual: adopted, dialect: "postgres", scopeSchemas: ["public"] }); + expect(offline.changes.filter((c) => c.kind === "change-column-default").map((c) => (c as { column: string }).column)).toEqual(["s"]); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts new file mode 100644 index 0000000000..c146b2c128 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts @@ -0,0 +1,83 @@ +/** + * A column rename carries the CHECK named after it only when Postgres says the rule is the + * same. The pairing used to compare the author's text with the live text through the string + * normaliser, which folds a literal-case change (`'OPEN'` → `'open'`) into a plain + * RENAME CONSTRAINT, so the stale values survived the migration. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ + +import { test, expect, beforeAll, afterAll, describe } from "bun:test"; +import { Pool } from "pg"; +import { Kysely, PostgresDialect, sql } from "kysely"; +import { introspectPostgres } from "../../src/introspect/postgres.js"; +import { diff } from "../../src/diff/index.js"; +import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const TABLE = "rename_carry_t"; + +describe("column rename carry (live PG)", () => { + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + let pool: Pool; + let db: Kysely>; + + beforeAll(async () => { + pool = new Pool({ connectionString: PG_URL }); + db = new Kysely>({ dialect: new PostgresDialect({ pool }) }); + await sql.raw(` + DROP TABLE IF EXISTS ${TABLE}; + CREATE TABLE ${TABLE} (id bigserial PRIMARY KEY, status text, + CONSTRAINT ${TABLE}_status_chk CHECK (status IN ('OPEN', 'CLOSED')))`).execute(db); + }); + afterAll(async () => { + await sql.raw(`DROP TABLE IF EXISTS ${TABLE}`).execute(db); + await pool.end(); + }); + + const run = async (expression: string, probe: boolean) => { + const live = await introspectPostgres(db); + const expected = structuredClone(live); + const t = expected.tables.find((x) => x.name === TABLE)!; + t.columns.find((c) => c.name === "status")!.name = "state"; + t.checks = [{ name: `${TABLE}_state_chk`, expression }]; + return diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + renames: [{ kind: "column", table: TABLE, from: "status", to: "state" }], + ...(probe ? { exprCanonProbe: postgresExprCanonProbe(db) } : {}), + }); + }; + const kinds = (r: Awaited>) => r.changes.map((c) => c.kind).sort(); + + test("the same rule under the new column name folds into the rename", async () => { + const r = await run(`"state" IN ('OPEN', 'CLOSED')`, true); + expect(kinds(r)).toEqual(["rename-column"]); + expect((r.changes[0] as { constraintRenames?: unknown }).constraintRenames).toEqual([ + { from: `${TABLE}_status_chk`, to: `${TABLE}_state_chk` }, + ]); + }); + + test("a rename plus a literal-case change is NOT folded: the stale values must be rewritten", async () => { + const r = await run(`"state" IN ('open', 'closed')`, true); + expect(kinds(r)).toEqual(["add-check", "drop-check", "rename-column"]); + }); + + test("a rename plus a changed rule is not folded either", async () => { + const r = await run(`"state" IN ('OPEN')`, true); + expect(kinds(r)).toEqual(["add-check", "drop-check", "rename-column"]); + }); + + test("the probe compiles an expected expression against the renamed columns", async () => { + const live = (await introspectPostgres(db)).tables.find((t) => t.name === TABLE)!; + const check = { name: `${TABLE}_state_chk`, expression: `"state" IN ('OPEN', 'CLOSED')` }; + const probe = postgresExprCanonProbe(db); + const renames = new Map([["status", "state"]]); + const answer = await probe(live, { ...live, columns: [], indexes: [], checks: [check] }, renames); + expect(answer?.checks.get(check)).toBe("(state = ANY (ARRAY['OPEN'::text, 'CLOSED'::text]))"); + // Without the rename the column does not exist yet, so there is no answer for it. + expect((await probe(live, { ...live, columns: [], indexes: [], checks: [check] }))?.checks.size ?? 0).toBe(0); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/unit/diff-rename-carries-constraints.test.ts b/server/typescript/packages/migrate-ts/test/unit/diff-rename-carries-constraints.test.ts index e32a189954..e3754c4de1 100644 --- a/server/typescript/packages/migrate-ts/test/unit/diff-rename-carries-constraints.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/diff-rename-carries-constraints.test.ts @@ -109,4 +109,13 @@ describe("renameExprIdentifiers", () => { expect(renameExprIdentifiers(`rating_note = 'rating' OR "rating" IS NULL`, m)) .toBe(`rating_note = 'rating' OR "stars" IS NULL`); }); + test("leaves a cast type and a function name alone even when a column of that name is renamed", () => { + const date = new Map([["date", "day"]]); + expect(renameExprIdentifiers(`date >= 'x'::date AND date(created) = date`, date)) + .toBe(`day >= 'x'::date AND date(created) = day`); + }); + test("leaves the inside of an E'..' literal alone, including a backslash-escaped quote", () => { + expect(renameExprIdentifiers(`note = E'it\\'s rating' AND rating > 1`, m)) + .toBe(`note = E'it\\'s rating' AND stars > 1`); + }); }); From 677fd16fc5aa187df7379186e0acf96bf87fee75 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 22:34:17 -0400 Subject: [PATCH 2/8] feat(migrate-ts,cli): decide Postgres expression drift against a scratch (shadow) database first --- CHANGELOG.md | 15 +- docs/features/migrations-and-drift.md | 33 +++- .../packages/cli/src/commands/migrate.ts | 29 ++- .../packages/cli/src/commands/verify.ts | 18 +- server/typescript/packages/cli/src/index.ts | 18 ++ .../typescript/packages/cli/src/lib/args.ts | 16 ++ .../typescript/packages/cli/src/lib/config.ts | 6 + .../typescript/packages/cli/src/lib/shadow.ts | 87 +++++++++ .../cli/test/__snapshots__/cli.test.ts.snap | 12 ++ .../packages/cli/test/shadow-opener.test.ts | 73 ++++++++ .../packages/migrate-ts/src/diff/index.ts | 23 ++- .../packages/migrate-ts/src/drift/drift.ts | 4 + .../packages/migrate-ts/src/index.ts | 4 +- .../packages/migrate-ts/src/shadow-canon.ts | 101 +++++++++++ .../packages/migrate-ts/src/types.ts | 11 ++ .../test/integration/pg-shadow-canon.test.ts | 168 ++++++++++++++++++ server/typescript/packages/sdk/src/config.ts | 2 + 17 files changed, 605 insertions(+), 15 deletions(-) create mode 100644 server/typescript/packages/cli/src/lib/shadow.ts create mode 100644 server/typescript/packages/cli/test/shadow-opener.test.ts create mode 100644 server/typescript/packages/migrate-ts/src/shadow-canon.ts create mode 100644 server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index adcc470c5d..59fc33d750 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -299,6 +299,18 @@ here.** so. The three flags it never read (`--server`, `--client`, `--out`) are still accepted but ignored and hidden from help, so the old invocation still reaches the pointer. +- **Postgres drift is decided by a scratch (shadow) database first.** `meta verify --db` and + `meta migrate` (`--from-db`, `--apply`, and `baseline --from-db`) now apply the model's expected + schema to a throwaway database, as Atlas, Prisma and migra do, read it with the introspector that + reads the live database, and compare CHECKs, index expressions and predicates, and column + defaults in Postgres's own spelling. The scratch database is `--shadow-db `, else + `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl`; with none, a database is `CREATE`d on the + `--db` server when the role may and always dropped afterwards; `--no-shadow-db` skips both. The + schema is applied in a rolled-back transaction, so an explicit URL is left empty. When no scratch + database is available the rolled-back temp-object probe on the target answers (it needs write + privileges and takes brief locks), and SQLite and offline plans keep the string normaliser. The + output says which path ran. See `docs/features/migrations-and-drift.md`. + - **Postgres column `DEFAULT`s that Postgres re-spells no longer read as drift.** `NOW()`, `current_date`, a date literal on a `timestamptz`, `'1e3'` on a double, `'007'` on an integer and a composite expression such as `now() + interval '1 day'` were reported by `meta verify @@ -308,8 +320,7 @@ here.** inside its rolled-back transaction it sets each expected default on the scratch copy and reads `pg_get_expr`, and a literal on a `timestamptz` is compared in the reading session's `TimeZone`. A real change (another value, another instant, another function) is still reported. - The classifier is still the comparison for SQLite, offline runs, read-only connections and a - default that does not compile against the live column. `baseline --from-db` records the + The classifier is still the comparison for SQLite and offline runs. `baseline --from-db` records the authored default where the probe proved it equal, so the offline plan starts clean too. A column rename that carries a CHECK is now decided the same way: the scratch copy renames the column first, so a rename plus a literal-case change (`'OPEN'` to `'open'`) is not folded into a diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 32dca50f0b..14d2d3f905 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -377,17 +377,40 @@ Postgres rewrites an expression before it stores it (`"k" IN ('x')` becomes `k = `= ANY (ARRAY[…])`, and casts and brackets are added), so the text `meta migrate` generates never comes back from the database as written. On Postgres, a plan against the database (`meta verify --db`, `meta migrate --from-db` and `--apply`) therefore **asks Postgres** instead of -guessing. For each table it creates a **temporary table with the live table's columns** inside a -transaction it rolls back (a savepoint, when the connection is already in one), adds the expected +guessing. There are three ways it asks, tried in this order, and the output says which one ran. + +**1. A scratch (shadow) database, the primary path.** The practice Atlas, Prisma and migra share: +the model's full expected schema (the DDL `meta migrate` emits for an empty database) is applied to +a throwaway Postgres database, in one transaction that is always rolled back, and read with the same +introspector that reads your live database. The expected CHECKs, index expressions and predicates, +and column defaults are then Postgres's own spelling of the same catalog, compared with the live +one as plain strings. No per-shape rule is involved, so `NOW()`/`now()`, `current_date`, a date +literal on a `timestamptz` (read in the session `TimeZone`), `'1e3'` on a double, `'007'` on an +integer, `now() + interval '1 day'`, a regrouped `NOT` and a precedence change are all decided by +the database. A real change of any of them is still reported. The scratch database comes from: + +| Source | How | +|---|---| +| An explicit URL | `--shadow-db ` on `verify` and `migrate`, else `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl` in `.metaobjects/config.json`. Used as given and never dropped; nothing is left in it because the schema is applied inside a rolled-back transaction. | +| `CREATE DATABASE` | With none of the above, a database named `mo_shadow_` is created on the `--db` server when the role may, and **always dropped** afterwards, including on error. | +| Off | `--no-shadow-db` skips both. | + +If no scratch database can be opened, or the expected schema does not apply to it (an FK to a table +outside the run's scope, a missing extension), the next path runs. + +**2. The temp-object probe, the first fallback.** For each table it creates a **temporary table with +the live table's columns** on the target itself, inside a transaction it rolls back (a savepoint, when the connection is already in one), adds the expected CHECK to it, creates each expected expression or partial-index predicate as a temporary index, and reads back `pg_get_constraintdef` / `pg_get_indexdef` / `pg_get_expr`. Those are compared with -the live definitions as plain strings. Nothing persists, and nothing is written to your table. +the live definitions as plain strings. Nothing persists, and nothing is written to your table, but +the probe **needs write privileges on the target** (it creates temporary objects) and **takes brief +locks** on it, which is why a scratch database is preferred. Two expressions that mean different things never compare equal: a precedence change (`(a OR b) AND c` against `a OR (b AND c)`), a regrouped `NOT`, and a changed enum-member or regex literal are all drift. -Where the question cannot be asked, the comparison falls back to a **string normaliser** -(`check-expr-compare.ts`), which rewrites both texts toward one shape. The fallback applies to: +**3. The string normaliser, the last fallback.** Where neither question can be asked, the comparison uses it +(`check-expr-compare.ts`), which rewrites both texts toward one shape. It always applies to: - SQLite and D1, which store the text as written; - the default **offline** `meta migrate`, which diffs the committed snapshot and has no database; diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index 5f0200ddd6..2709e95b41 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -10,6 +10,7 @@ import { formatMigrateResultJson } from "../lib/output-json.js"; import type { OutputFormat } from "../lib/format.js"; import { emitStructured, narrate, toonEncode } from "../lib/format.js"; import { buildKyselyFromUrl, inferDialect, redactUrl } from "../lib/kysely.js"; +import { describeExprCanonPath, shadowOpener } from "../lib/shadow.js"; import { log } from "../lib/log.js"; import { loadMemory, resolveCollection, resolveConfigDir, type Collection } from "@metaobjectsdev/sdk"; import { loadMemoryOptionsFrom, loadMetaobjectsConfig, RemovedNamingConfigError, resolveGenConfigDir } from "../lib/load-metaobjects-config.js"; @@ -58,6 +59,8 @@ import { type TableDescriptor, postgresViewReplaceProbe, postgresExprCanonProbe, + postgresShadowCanon, + type ShadowCanonSource, adoptGeneratedSpelling, } from "@metaobjectsdev/migrate-ts"; import { @@ -99,6 +102,10 @@ SUBCOMMANDS: MIGRATE FLAGS: --db DB connection URL (required for live-introspect / --apply / --rollback) Supports: file:, libsql:, postgres:, postgresql: + --shadow-db Postgres: a scratch database for the drift comparison. Default: CREATE + DATABASE on the --db server (dropped afterwards), else the temp-object + probe on the target. Also SHADOW_DATABASE_URL / migrate.shadowDatabaseUrl. + --no-shadow-db Never open a scratch database (use the probe on the target). --dialect sqlite|postgres|d1 Optional dialect override (auto-detected from URL scheme) --migration-format default|flyway @@ -765,6 +772,13 @@ export async function migrateCommand( return 2; } warnDecimalPrecision(metadata, collection, kysely.dialect); + // Postgres: a scratch database answers first how each expected expression is stored (lib/shadow.ts). + const shadowOpen = kysely.dialect === "postgres" + ? shadowOpener( + { url: config.shadowDatabaseUrl, disabled: config.shadowDisabled === true }, + { url: config.databaseUrl, handle: kysely }, + ) + : undefined; let exitCode = 0; let writtenPaths: string[] = []; @@ -831,6 +845,7 @@ export async function migrateCommand( // ...and the same for CHECK / index expressions: Postgres says how it stores the expected // text, so a stored rewrite no string rule knows is not proposed as a drop and re-add. ...(kysely.dialect === "postgres" ? { exprCanonProbe: postgresExprCanonProbe(kysely.db) } : {}), + ...(shadowOpen !== undefined ? { exprCanonShadow: postgresShadowCanon(shadowOpen) } : {}), allow: tokensToAllowOptions(config.allow), renames: config.renames, // #258 — adopting a live DB whose PRIMARY KEY differs from the metadata identity @@ -930,6 +945,7 @@ export async function migrateCommand( // no-op — output will say "No schema changes" } else { notes.push(...viewRecreateNotes(diffResult.changes)); + if (diffResult.exprCanonPath !== undefined) notes.push(describeExprCanonPath(diffResult.exprCanonPath)); let emitted: EmitResult | undefined; try { emitted = emit(diffResult.changes, { @@ -1157,11 +1173,13 @@ async function withGeneratedSpelling( dialect: Dialect, metaRoot: string, genRoot: string, + shadow?: ShadowCanonSource, ): Promise { try { const metadata = await loadBaselineMetadata(metaRoot, genRoot); const expected = baselineFromMetadata(metadata, dialect, buildProjectionViews(metadata, { dialect })); - return await adoptGeneratedSpelling(snapshot, expected, postgresExprCanonProbe(db)); + const probe = (shadow !== undefined ? await shadow(expected) : undefined) ?? postgresExprCanonProbe(db); + return await adoptGeneratedSpelling(snapshot, expected, probe); } catch { return snapshot; } @@ -1205,7 +1223,14 @@ export async function runBaseline( snapshot = await introspect(kysely.db, kysely.dialect); if (config.format === "flyway") snapshot = withoutFlywayHistory(snapshot); if (kysely.dialect === "postgres") { - snapshot = await withGeneratedSpelling(snapshot, kysely.db, config.dialect, metaRoot, genRoot); + const shadowOpen = shadowOpener( + { url: config.shadowDatabaseUrl, disabled: config.shadowDisabled === true }, + { url: config.databaseUrl, handle: kysely }, + ); + snapshot = await withGeneratedSpelling( + snapshot, kysely.db, config.dialect, metaRoot, genRoot, + shadowOpen !== undefined ? postgresShadowCanon(shadowOpen) : undefined, + ); } } finally { await kysely.close(); diff --git a/server/typescript/packages/cli/src/commands/verify.ts b/server/typescript/packages/cli/src/commands/verify.ts index 35f26d29da..3a23f4524c 100644 --- a/server/typescript/packages/cli/src/commands/verify.ts +++ b/server/typescript/packages/cli/src/commands/verify.ts @@ -46,6 +46,8 @@ import { lintNodeNames } from "../lib/name-lint.js"; import { lintDeprecatedReferences } from "../lib/deprecation-lint.js"; import { lintDuplicateFields, lintReferenceFields } from "../lib/field-lint.js"; import { FileSource } from "@metaobjectsdev/metadata/core"; +import { shadowOpener } from "../lib/shadow.js"; +import { describeExprCanonPath } from "../lib/shadow.js"; import { resolveD1Config, resolveMigrateConfig, type ResolvedMigrateConfig } from "../lib/config.js"; import { buildWranglerExecuteArgs, @@ -88,6 +90,7 @@ import { type ExprCanonProbe, postgresViewReplaceProbe, postgresExprCanonProbe, + postgresShadowCanon, withoutFlywayHistory, } from "@metaobjectsdev/migrate-ts"; import { @@ -203,7 +206,9 @@ export async function verifyCommand( log.error(REMOVED_VERIFY_DOCS); return 2; } - const migrateFlags: MigrateFlags = { ...EMPTY_MIGRATE_FLAGS, format: flags.migrationFormat }; + const migrateFlags: MigrateFlags = { + ...EMPTY_MIGRATE_FLAGS, format: flags.migrationFormat, shadowDb: flags.shadowDb, noShadowDb: flags.noShadowDb === true, + }; // ADR-0021 D2 — explicit verify subverbs. Each flag selects one drift mode; // any combination runs each and the overall exit code is the MAX (non-zero on @@ -1351,6 +1356,14 @@ export async function verifyCommand( const expectedViews = buildProjectionViews(root, { dialect: kysely.dialect }); // One probe for the whole run, so a server that refuses scratch tables is asked once. const exprCanonProbe = kysely.dialect === "postgres" ? postgresExprCanonProbe(kysely.db) : undefined; + // ...but a scratch database answers first, when one can be opened (see lib/shadow.ts). + const shadowOpen = kysely.dialect === "postgres" + ? shadowOpener( + { url: migrateConfig.shadowDatabaseUrl, disabled: migrateConfig.shadowDisabled === true }, + { url: flags.db as string, handle: kysely }, + ) + : undefined; + const exprCanonShadow = shadowOpen !== undefined ? postgresShadowCanon(shadowOpen) : undefined; let driftResult; let actual: SchemaSnapshot; try { @@ -1369,6 +1382,7 @@ export async function verifyCommand( ...(kysely.dialect === "postgres" ? { viewReplaceProbe: postgresViewReplaceProbe(kysely.db) } : {}), // ...and asks Postgres, not a string rewrite, how it stores each expected CHECK and index expression. ...(exprCanonProbe !== undefined ? { exprCanonProbe } : {}), + ...(exprCanonShadow !== undefined ? { exprCanonShadow } : {}), ...(schemaScope !== undefined ? { inScope: schemaScope } : {}), // The import exclusion reaches scopeExpectedSchema through this option bag — // there is no direct call on this path to attach it to. @@ -1380,6 +1394,8 @@ export async function verifyCommand( return 1; } + if (driftResult.exprCanonPath !== undefined) say(describeExprCanonPath(driftResult.exprCanonPath)); + const snapshotDrift = driftResult.changes.length === 0 ? await checkCommittedSnapshot(actual, kysely.dialect, kysely.displayUrl, driftResult, migrateConfig, exprCanonProbe) diff --git a/server/typescript/packages/cli/src/index.ts b/server/typescript/packages/cli/src/index.ts index 6e81cf7e3d..f8d341eb50 100644 --- a/server/typescript/packages/cli/src/index.ts +++ b/server/typescript/packages/cli/src/index.ts @@ -119,6 +119,12 @@ VERIFY FLAGS (ADR-0021 D2 — explicit subverbs; combine any; exit 1 on ANY drif --db Schema drift — live DB URL enables the schema-drift gate. Supports: file:, libsql:, postgres:, postgresql:. Omit to skip. D1 has no URL — use --dialect d1 / --d1 instead. + --shadow-db Postgres: a scratch database to apply the expected schema to, so CHECKs, + index expressions and defaults are compared in Postgres's own spelling. + Default: CREATE DATABASE on the --db server (dropped afterwards), else the + temp-object probe on the target. Also SHADOW_DATABASE_URL or + migrate.shadowDatabaseUrl in .metaobjects/config.json. + --no-shadow-db Never open a scratch database (use the probe on the target instead). --prompts Directory of provider-resolved template text (default: prompts) --dialect sqlite|postgres|d1 Optional override (auto-detected from --db URL scheme) --allow Accepted for parity with 'migrate'; does NOT affect the @@ -161,6 +167,12 @@ MIGRATE FLAGS: --db DB connection URL (required, or set DATABASE_URL or config) Supports: file:, libsql:, postgres:, postgresql: D1 has no URL — use --dialect d1 / --d1 instead. + --shadow-db Postgres: a scratch database to apply the expected schema to, so CHECKs, + index expressions and defaults are compared in Postgres's own spelling. + Default: CREATE DATABASE on the --db server (dropped afterwards), else the + temp-object probe on the target. Also SHADOW_DATABASE_URL or + migrate.shadowDatabaseUrl in .metaobjects/config.json. + --no-shadow-db Never open a scratch database (use the probe on the target instead). --dialect sqlite|postgres|d1 Optional override (auto-detected from URL scheme) --out-dir Migration directory (default: ./.metaobjects/migrations) --slug Required when changes are present (e.g., add-user-shipping) @@ -332,6 +344,12 @@ FLAGS: --db Schema drift — live DB URL enables the schema-drift gate. Supports: file:, libsql:, postgres:, postgresql: D1 has no URL — use --dialect d1 / --d1 instead. + --shadow-db Postgres: a scratch database to apply the expected schema to, so CHECKs, + index expressions and defaults are compared in Postgres's own spelling. + Default: CREATE DATABASE on the --db server (dropped afterwards), else the + temp-object probe on the target. Also SHADOW_DATABASE_URL or + migrate.shadowDatabaseUrl in .metaobjects/config.json. + --no-shadow-db Never open a scratch database (use the probe on the target instead). --replay Migration-chain drift — replay the committed chain into an EMPTY throwaway database and assert it applies. Needs no --db: the engine is in-process (PGlite for postgres, a temp file for diff --git a/server/typescript/packages/cli/src/lib/args.ts b/server/typescript/packages/cli/src/lib/args.ts index f2dbca7528..a068e2fda6 100644 --- a/server/typescript/packages/cli/src/lib/args.ts +++ b/server/typescript/packages/cli/src/lib/args.ts @@ -344,6 +344,10 @@ export interface VerifyFlags { prompts: string | undefined; /** Live DB connection URL; when present, enables the schema-drift gate. */ db: string | undefined; + /** Postgres scratch database for the drift comparison (see `MigrateFlags.shadowDb`). */ + shadowDb?: string | undefined; + /** Never use a scratch database (see `MigrateFlags.noShadowDb`). */ + noShadowDb?: boolean; /** Optional dialect override (auto-detected from --db URL scheme otherwise). */ dialect: Dialect | undefined; /** Destructive-change permissions; only affects how drift is described. */ @@ -496,6 +500,8 @@ export interface VerifyFlags { export const VERIFY_OPTIONS = { prompts: { type: "string" }, db: { type: "string" }, + "shadow-db": { type: "string" }, + "no-shadow-db": { type: "boolean", default: false }, dialect: { type: "string" }, allow: { type: "string", multiple: true }, "skip-schema": { type: "boolean", default: false }, @@ -586,6 +592,8 @@ export function parseVerifyArgs(argv: string[]): VerifyFlags { return { prompts: values.prompts, db: values.db as string | undefined, + ...(values["shadow-db"] !== undefined ? { shadowDb: values["shadow-db"] as string } : {}), + ...(values["no-shadow-db"] ? { noShadowDb: true } : {}), dialect: dialect as Dialect | undefined, allow: allowTokens as AllowToken[], skipSchema: !!values["skip-schema"], @@ -676,6 +684,10 @@ export interface MigrateFlags { yes: boolean; /** Use live-DB introspection instead of the committed snapshot (legacy/adoption). */ fromDb: boolean; + /** `--shadow-db `: the Postgres scratch database the expected schema is applied to. */ + shadowDb?: string | undefined; + /** `--no-shadow-db`: never use a scratch database (not even a CREATE DATABASE one). */ + noShadowDb?: boolean; /** `migrate baseline` subcommand: seed the snapshot, emit no migration. */ baseline: boolean; /** `migrate apply-pending` subcommand: replay committed migration files, no diff. */ @@ -695,6 +707,8 @@ export const MIGRATE_OPTIONS = { "rename-column": { type: "string", multiple: true }, "dry-run": { type: "boolean", default: false }, "from-db": { type: "boolean", default: false }, + "shadow-db": { type: "string" }, + "no-shadow-db": { type: "boolean", default: false }, "d1": { type: "string" }, "remote": { type: "boolean", default: false }, "apply": { type: "boolean", default: false }, @@ -751,6 +765,8 @@ export function parseMigrateArgs(argv: string[]): MigrateFlags { rollback: values.rollback as string | undefined, yes: !!values.yes, fromDb: !!values["from-db"], + ...(values["shadow-db"] !== undefined ? { shadowDb: values["shadow-db"] as string } : {}), + ...(values["no-shadow-db"] ? { noShadowDb: true } : {}), baseline, applyPending, }; diff --git a/server/typescript/packages/cli/src/lib/config.ts b/server/typescript/packages/cli/src/lib/config.ts index cb16a37c20..420f4b1e45 100644 --- a/server/typescript/packages/cli/src/lib/config.ts +++ b/server/typescript/packages/cli/src/lib/config.ts @@ -39,6 +39,10 @@ export interface ResolvedD1Config { export interface ResolvedMigrateConfig { outDir: string; databaseUrl: string | undefined; + /** Postgres: an explicit scratch database URL (flag > `SHADOW_DATABASE_URL` > config). */ + shadowDatabaseUrl?: string | undefined; + /** `--no-shadow-db`: never open a scratch database. */ + shadowDisabled?: boolean; dialect: Dialect | undefined; /** Output-format adapter (#192): "default" homegrown layout, or "flyway" V__/U__. */ format: MigrateFormat; @@ -137,6 +141,8 @@ export async function resolveMigrateConfig( return { outDir: flags.outDir ?? cfgBlock.outDir ?? MIGRATE_DEFAULTS.outDir, databaseUrl: flags.db ?? envUrl ?? cfgBlock.databaseUrl ?? MIGRATE_DEFAULTS.databaseUrl, + shadowDatabaseUrl: flags.shadowDb ?? process.env.SHADOW_DATABASE_URL ?? cfgBlock.shadowDatabaseUrl, + shadowDisabled: flags.noShadowDb === true, dialect: flags.dialect ?? cfgBlock.dialect ?? MIGRATE_DEFAULTS.dialect, format: flags.format ?? cfgBlock.format ?? MIGRATE_DEFAULTS.format, onAmbiguous: flags.onAmbiguous ?? cfgBlock.onAmbiguous ?? MIGRATE_DEFAULTS.onAmbiguous, diff --git a/server/typescript/packages/cli/src/lib/shadow.ts b/server/typescript/packages/cli/src/lib/shadow.ts new file mode 100644 index 0000000000..c319bb6740 --- /dev/null +++ b/server/typescript/packages/cli/src/lib/shadow.ts @@ -0,0 +1,87 @@ +// shadow.ts — where the Postgres scratch (shadow) database comes from. +// +// `meta verify --db` and `meta migrate --from-db` apply the model's expected schema to a +// scratch database and compare it with the live one in Postgres's own spelling +// (migrate-ts shadow-canon.ts). Three sources, in order: +// 1. an explicit URL (`--shadow-db`, `SHADOW_DATABASE_URL`, `migrate.shadowDatabaseUrl`), +// used as given and never dropped (the expected schema is applied in a transaction that +// is rolled back, so nothing is left in it); +// 2. a database CREATEd on the target server when the role may, named per run and always +// dropped afterwards, including on error; +// 3. none: the caller falls back to the temp-object probe. +// `--no-shadow-db` skips straight to 3. + +import { randomBytes } from "node:crypto"; +import { sql } from "kysely"; +import type { ShadowHandle, ShadowOpener } from "@metaobjectsdev/migrate-ts"; +import { buildKyselyFromUrl, type KyselyHandle } from "./kysely.js"; + +export interface ShadowSettings { + /** Explicit scratch database URL, or undefined to create one on the target server. */ + url: string | undefined; + /** `--no-shadow-db`. */ + disabled: boolean; +} + +/** The URL of database `name` on the same server as `targetUrl` (same credentials and options). */ +export function urlForDatabase(targetUrl: string, name: string): string { + const u = new URL(targetUrl); + u.pathname = `/${encodeURIComponent(name)}`; + return u.toString(); +} + +const quoteIdent = (s: string): string => `"${s.replace(/"/g, '""')}"`; + +/** + * The opener for a Postgres target, or `undefined` when scratch databases are switched off. + * `target` is the already-open connection to the live database, used for CREATE/DROP DATABASE. + */ +export function shadowOpener( + settings: ShadowSettings, + target: { url: string; handle: KyselyHandle }, +): ShadowOpener | undefined { + if (settings.disabled) return undefined; + if (settings.url !== undefined) { + const explicit = settings.url; + return async (): Promise => { + const h = await buildKyselyFromUrl(explicit, "postgres"); + return { db: h.db, close: async () => { await h.close(); } }; + }; + } + return async (): Promise => { + const name = `mo_shadow_${randomBytes(6).toString("hex")}`; + await sql.raw(`CREATE DATABASE ${quoteIdent(name)}`).execute(target.handle.db); + let h: KyselyHandle | undefined; + const drop = async (): Promise => { + try { await h?.close(); } catch { /* the drop below is what matters */ } + try { + await sql.raw(`DROP DATABASE IF EXISTS ${quoteIdent(name)} WITH (FORCE)`).execute(target.handle.db); + } catch { + // Before Postgres 13 there is no WITH (FORCE); our own connection is closed by now. + try { await sql.raw(`DROP DATABASE IF EXISTS ${quoteIdent(name)}`).execute(target.handle.db); } catch { /* best effort */ } + } + }; + try { + h = await buildKyselyFromUrl(urlForDatabase(target.url, name), "postgres"); + } catch (err) { + await drop(); + throw err; + } + const opened = h; + return { db: opened.db, close: drop }; + }; +} + +/** One line saying which comparison decided CHECK, index-expression and column-default equality. */ +export function describeExprCanonPath(path: "shadow" | "probe" | "normaliser"): string { + switch (path) { + case "shadow": + return "Postgres expressions compared against a scratch (shadow) database."; + case "probe": + return "Postgres expressions compared via the temp-object probe on the target database (no scratch database was " + + "available): it needs write privileges and takes brief locks on the target. Pass --shadow-db to avoid that."; + case "normaliser": + return "Postgres expressions compared as text (no scratch database or probe was available); a re-spelled " + + "CHECK, index expression or default may read as drift."; + } +} diff --git a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap index 9fac28ca28..73334aacda 100644 --- a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap +++ b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap @@ -99,6 +99,12 @@ VERIFY FLAGS (ADR-0021 D2 — explicit subverbs; combine any; exit 1 on ANY drif --db Schema drift — live DB URL enables the schema-drift gate. Supports: file:, libsql:, postgres:, postgresql:. Omit to skip. D1 has no URL — use --dialect d1 / --d1 instead. + --shadow-db Postgres: a scratch database to apply the expected schema to, so CHECKs, + index expressions and defaults are compared in Postgres's own spelling. + Default: CREATE DATABASE on the --db server (dropped afterwards), else the + temp-object probe on the target. Also SHADOW_DATABASE_URL or + migrate.shadowDatabaseUrl in .metaobjects/config.json. + --no-shadow-db Never open a scratch database (use the probe on the target instead). --prompts Directory of provider-resolved template text (default: prompts) --dialect sqlite|postgres|d1 Optional override (auto-detected from --db URL scheme) --allow Accepted for parity with 'migrate'; does NOT affect the @@ -141,6 +147,12 @@ MIGRATE FLAGS: --db DB connection URL (required, or set DATABASE_URL or config) Supports: file:, libsql:, postgres:, postgresql: D1 has no URL — use --dialect d1 / --d1 instead. + --shadow-db Postgres: a scratch database to apply the expected schema to, so CHECKs, + index expressions and defaults are compared in Postgres's own spelling. + Default: CREATE DATABASE on the --db server (dropped afterwards), else the + temp-object probe on the target. Also SHADOW_DATABASE_URL or + migrate.shadowDatabaseUrl in .metaobjects/config.json. + --no-shadow-db Never open a scratch database (use the probe on the target instead). --dialect sqlite|postgres|d1 Optional override (auto-detected from URL scheme) --out-dir Migration directory (default: ./.metaobjects/migrations) --slug Required when changes are present (e.g., add-user-shipping) diff --git a/server/typescript/packages/cli/test/shadow-opener.test.ts b/server/typescript/packages/cli/test/shadow-opener.test.ts new file mode 100644 index 0000000000..7a05f19ed1 --- /dev/null +++ b/server/typescript/packages/cli/test/shadow-opener.test.ts @@ -0,0 +1,73 @@ +/** + * Where the Postgres scratch database comes from: an explicit URL, else a database CREATEd on + * the target server and always dropped, else none. The live-server cases skip gracefully when + * MIGRATE_TS_PG_URL is not set. + */ +import { describe, expect, test } from "bun:test"; +import { sql } from "kysely"; +import { parseMigrateArgs, parseVerifyArgs } from "../src/lib/args.js"; +import { buildKyselyFromUrl } from "../src/lib/kysely.js"; +import { describeExprCanonPath, shadowOpener, urlForDatabase } from "../src/lib/shadow.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; + +describe("shadow flags", () => { + test("--shadow-db and --no-shadow-db parse on migrate and verify", () => { + expect(parseMigrateArgs(["--shadow-db", "postgres://h/s"]).shadowDb).toBe("postgres://h/s"); + expect(parseMigrateArgs(["--no-shadow-db"]).noShadowDb).toBe(true); + const v = parseVerifyArgs(["--db", "postgres://h/d", "--shadow-db", "postgres://h/s"]); + expect(v.shadowDb).toBe("postgres://h/s"); + expect(parseVerifyArgs(["--db", "postgres://h/d", "--no-shadow-db"]).noShadowDb).toBe(true); + }); +}); + +describe("shadowOpener", () => { + test("urlForDatabase keeps the server, credentials and options, and swaps the database", () => { + expect(urlForDatabase("postgres://u:p@h:5433/live?sslmode=require", "mo_shadow_ab")) + .toBe("postgres://u:p@h:5433/mo_shadow_ab?sslmode=require"); + }); + test("disabled opens nothing", () => { + expect(shadowOpener({ url: undefined, disabled: true }, { url: "postgres://h/d", handle: {} as never })).toBeUndefined(); + }); + test("each source names itself", () => { + expect(describeExprCanonPath("shadow")).toContain("scratch"); + expect(describeExprCanonPath("probe")).toContain("write privileges"); + expect(describeExprCanonPath("normaliser")).toContain("as text"); + }); + + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + const shadowDbs = async (target: Awaited>): Promise => + (await sql<{ datname: string }>`SELECT datname FROM pg_database WHERE datname LIKE 'mo_shadow_%'`.execute(target.db)) + .rows.map((r) => r.datname); + + test("without a URL it CREATEs a database on the target and drops it on close", async () => { + const target = await buildKyselyFromUrl(PG_URL, "postgres"); + try { + const before = await shadowDbs(target); + const open = shadowOpener({ url: undefined, disabled: false }, { url: PG_URL, handle: target })!; + const handle = (await open())!; + expect((await shadowDbs(target)).length).toBe(before.length + 1); + await sql`SELECT 1`.execute(handle.db); + await handle.close(); + expect(await shadowDbs(target)).toEqual(before); + } finally { + await target.close(); + } + }); + + test("an explicit URL is opened as given and never dropped", async () => { + const target = await buildKyselyFromUrl(PG_URL, "postgres"); + try { + const open = shadowOpener({ url: PG_URL, disabled: false }, { url: PG_URL, handle: target })!; + const handle = (await open())!; + await handle.close(); + // The explicit database is still there: the target itself answers. + await sql`SELECT 1`.execute(target.db); + } finally { + await target.close(); + } + }); +}); diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index 2604039ce8..861f534e40 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -3,7 +3,7 @@ import type { ViewDescriptor, ViewChangeReason, DependentRelation, Change, ChangeStatus, DiffResult, AllowOptions, AmbiguousCallback, Dialect, - CheckDescriptor, ColumnDefault, NameChange, DeclaredRename, + CheckDescriptor, ColumnDefault, NameChange, DeclaredRename, ExprCanonPath, } from "../types.js"; import type { SqlType } from "../sql-type.js"; import { sqlTypeEquals } from "../sql-type.js"; @@ -13,6 +13,7 @@ import { detectColumnRenames, detectTableRenames } from "./rename-heuristic.js"; import { firstViewSqlDifference, viewSqlEquals } from "../view-sql-compare.js"; import { viewReplaceIsLegal, type ViewReplaceProbe } from "../view-replace.js"; import { liveViewReadsTable, sqlNamesIdentifier } from "../view-reads.js"; +import type { ShadowCanonSource } from "../shadow-canon.js"; import { checkExprEquals, checkExprRespelledNullSafe, renameExprIdentifiers } from "../check-expr-compare.js"; import { checkMatchesLive, columnDefaultMatchesLive, indexExprMatchesLive, NO_EXPR_CANON, type ExprCanon, type ExprCanonProbe, type IndexExprCanon } from "../expr-canon.js"; import { isPgAutoSequenceDefault } from "../pg-identity-default.js"; @@ -89,6 +90,12 @@ export interface DiffArgs { * the live columns), the normaliser decides exactly as it always has. */ exprCanonProbe?: ExprCanonProbe; + /** + * Postgres: the primary source of that answer, a scratch (shadow) database the expected + * schema is applied to (see shadow-canon.ts). Tried first; when it resolves no probe, + * `exprCanonProbe` answers, and without one the string normaliser does. + */ + exprCanonShadow?: ShadowCanonSource; /** * #258 — refuse (throw {@link PrimaryKeyChangeError}) when an existing table's live * PRIMARY KEY differs from the metadata identity. There is no primary-key change kind @@ -254,7 +261,14 @@ export async function diff( // any table present on both sides (diffRenamedTables), and its columns are never // scanned as orphaned drop/add pairs by the column heuristic in Pass 3. await detectTableRenames(changes, args.onAmbiguous, args.renames); - const canon = await probeExprCanon(args.exprCanonProbe, args.dialect, changes, expectedTables, actualTables); + let exprCanonPath: ExprCanonPath = "normaliser"; + let canonProbe: ExprCanonProbe | undefined; + if (args.dialect === "postgres") { + canonProbe = args.exprCanonShadow === undefined ? undefined : await args.exprCanonShadow(args.expected); + if (canonProbe !== undefined) exprCanonPath = "shadow"; + else if (args.exprCanonProbe !== undefined) { canonProbe = args.exprCanonProbe; exprCanonPath = "probe"; } + } + const canon = await probeExprCanon(canonProbe, args.dialect, changes, expectedTables, actualTables); const { renamed, predecessors } = diffRenamedTables(changes, expectedTables, actualTables, args.dialect, canon); // Pass 2: tables in both → compare columns/indexes/FKs @@ -297,8 +311,8 @@ export async function diff( await carryThroughColumnRenames(changes, args.dialect, (id) => { const live = predecessors.get(id) ?? actualTables.get(id); return live === undefined ? NO_TEXT_COLUMNS : textColumnsOf(live); - }, args.exprCanonProbe === undefined ? undefined : { - probe: args.exprCanonProbe, + }, canonProbe === undefined ? undefined : { + probe: canonProbe, liveOf: (id) => predecessors.get(id) ?? actualTables.get(id), expectedOf: (id) => expectedTables.get(id), }); @@ -332,6 +346,7 @@ export async function diff( changes, blocked: changes.filter((c) => c.status.state === "blocked"), hazards: changes.flatMap((c) => dataHazardOf(c) ?? []), + ...(args.dialect === "postgres" ? { exprCanonPath } : {}), }; } diff --git a/server/typescript/packages/migrate-ts/src/drift/drift.ts b/server/typescript/packages/migrate-ts/src/drift/drift.ts index 498f8cfc13..d4715432d1 100644 --- a/server/typescript/packages/migrate-ts/src/drift/drift.ts +++ b/server/typescript/packages/migrate-ts/src/drift/drift.ts @@ -26,6 +26,7 @@ import { postgresViewReplaceProbe } from "../view-replace-probe.js"; import type { ViewReplaceProbe } from "../view-replace.js"; import { postgresExprCanonProbe } from "../expr-canon-probe.js"; import type { ExprCanonProbe } from "../expr-canon.js"; +import type { ShadowCanonSource } from "../shadow-canon.js"; export interface ComputeDriftOptions { /** @@ -59,6 +60,8 @@ export interface ComputeDriftOptions { * the string normaliser decides. */ exprCanonProbe?: ExprCanonProbe; + /** Postgres: the shadow-database source tried before `exprCanonProbe` (see shadow-canon.ts). */ + exprCanonShadow?: ShadowCanonSource; /** * Per-command scope (`migrate.scope`): objects whose declaring FQN this predicate * rejects are governed by somebody else. They leave the expected side AND are @@ -149,6 +152,7 @@ export async function computeDriftFromActual( ...(opts?.ignoreTables !== undefined ? { ignoreTables: opts.ignoreTables } : {}), ...(opts?.viewReplaceProbe !== undefined ? { viewReplaceProbe: opts.viewReplaceProbe } : {}), ...(opts?.exprCanonProbe !== undefined ? { exprCanonProbe: opts.exprCanonProbe } : {}), + ...(opts?.exprCanonShadow !== undefined ? { exprCanonShadow: opts.exprCanonShadow } : {}), }); return { ...result, diff --git a/server/typescript/packages/migrate-ts/src/index.ts b/server/typescript/packages/migrate-ts/src/index.ts index e192e43983..15cf4d00c6 100644 --- a/server/typescript/packages/migrate-ts/src/index.ts +++ b/server/typescript/packages/migrate-ts/src/index.ts @@ -76,7 +76,7 @@ export type { ViewDescriptor, ViewChangeReason, FkAction, NameChange, Change, ChangeKind, ChangeStatus, AllowOptions, AmbiguousChange, AmbiguousResolution, AmbiguousCallback, - DiffResult, DataHazard, DeclaredRename, EmitResult, Dialect, + DiffResult, DataHazard, DeclaredRename, EmitResult, Dialect, ExprCanonPath, } from "./types.js"; export type { DiffArgs } from "./diff/index.js"; export type { EmitOptions } from "./emit/index.js"; @@ -97,6 +97,8 @@ export type { ExactViewColumn, ViewReplaceEvidence, ViewReplaceProbe } from "./v // Postgres's own spelling of an expected CHECK / index expression is asked of the database the // same way (rolled-back scratch table); the string normaliser covers whatever it cannot answer. export { postgresExprCanonProbe } from "./expr-canon-probe.js"; +export { postgresShadowCanon } from "./shadow-canon.js"; +export type { ShadowCanonSource, ShadowHandle, ShadowOpener } from "./shadow-canon.js"; export type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; // `baseline --from-db`: record the generated text for an expression the probe proved equal. export { adoptGeneratedSpelling } from "./snapshot/adopt-generated-spelling.js"; diff --git a/server/typescript/packages/migrate-ts/src/shadow-canon.ts b/server/typescript/packages/migrate-ts/src/shadow-canon.ts new file mode 100644 index 0000000000..b6c81395a3 --- /dev/null +++ b/server/typescript/packages/migrate-ts/src/shadow-canon.ts @@ -0,0 +1,101 @@ +// shadow-canon.ts — the primary Postgres answer to "how does Postgres spell this?". +// +// The practice Atlas, Prisma and migra share: apply the model's full expected schema to a +// scratch (shadow) database, introspect it with the very introspector that reads the live +// database, and compare. Both sides are then Postgres's own spelling of the same catalog, so +// no per-shape rule is needed for a CHECK, an index expression or predicate, or a column +// DEFAULT. +// +// The DDL is the one `meta migrate` emits for an empty database, run inside ONE transaction +// that is always rolled back, so nothing is left behind even in a shared shadow database. +// Where no shadow database can be opened, or the schema does not apply to it (an FK to a +// table outside the run's scope, a missing extension), the caller falls back to the +// temp-object probe (expr-canon-probe.ts), and from there to the string normaliser. + +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { diff } from "./diff/index.js"; +import { emit } from "./emit/index.js"; +import { introspectPostgres } from "./introspect/postgres.js"; +import { qualifiedDbName } from "./qualified-name.js"; +import type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; +import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor, SchemaSnapshot } from "./types.js"; + +/** A connection to a scratch Postgres database, and how to release it. */ +export interface ShadowHandle { + db: Kysely>; + /** Release the connection and drop the database if the opener created it. Must not throw. */ + close(): Promise; +} + +/** Opens a shadow database, or resolves `undefined` when none is available. May throw; that reads as unavailable. */ +export type ShadowOpener = () => Promise; + +/** + * Given the expected schema of a run, resolves the probe that answers from a shadow database, + * or `undefined` when no shadow is available (the caller then falls back). + */ +export type ShadowCanonSource = (expected: SchemaSnapshot) => Promise; + +const ROLLBACK = new Error("shadow canon: always rolled back"); + +export function postgresShadowCanon(open: ShadowOpener): ShadowCanonSource { + return async (expected) => { + let handle: ShadowHandle | undefined; + try { + handle = await open(); + } catch { + return undefined; + } + if (handle === undefined) return undefined; + try { + const { changes } = await diff({ expected, actual: { tables: [], views: [] }, dialect: "postgres" }); + const { up } = emit(changes, { dialect: "postgres" }); + let shadow: SchemaSnapshot | undefined; + try { + await handle.db.transaction().execute(async (trx) => { + await sql.raw(up).execute(trx); + shadow = await introspectPostgres(trx as unknown as Kysely>); + throw ROLLBACK; + }); + } catch (err) { + if (err !== ROLLBACK) return undefined; + } + return shadow === undefined ? undefined : probeOver(shadow); + } catch { + return undefined; + } finally { + await handle.close(); + } + }; +} + +/** The answer for each expected descriptor, read off the shadow table of the same name. */ +function probeOver(shadow: SchemaSnapshot): ExprCanonProbe { + const tables = new Map(shadow.tables.map((t) => [qualifiedDbName(t), t] as const)); + return async (_live, expected) => { + const st = tables.get(qualifiedDbName(expected)); + if (st === undefined) return undefined; + const checks = new Map(); + for (const c of expected.checks) { + const s = st.checks.find((x) => x.name === c.name); + if (s !== undefined) checks.set(c, s.expression); + } + const indexes = new Map(); + for (const ix of expected.indexes) { + const s = st.indexes.find((x) => x.name === ix.name); + if (s === undefined) continue; + const canon: IndexExprCanon = {}; + if (ix.expr !== undefined && s.expr !== undefined) canon.expr = s.expr; + if (ix.where !== undefined && s.where !== undefined) canon.where = s.where; + indexes.set(ix, canon); + } + const defaults = new Map(); + for (const col of expected.columns) { + const d = st.columns.find((x) => x.name === col.name)?.default; + if (col.default !== undefined && d !== undefined) defaults.set(col, d); + } + const answer: ExprCanon = { checks, indexes, defaults }; + return answer; + }; +} diff --git a/server/typescript/packages/migrate-ts/src/types.ts b/server/typescript/packages/migrate-ts/src/types.ts index 8caf6e675d..e00b7e1ea0 100644 --- a/server/typescript/packages/migrate-ts/src/types.ts +++ b/server/typescript/packages/migrate-ts/src/types.ts @@ -526,8 +526,19 @@ export interface DiffResult { * writes the preparation step above the statement. See {@link DataHazard}. */ hazards: DataHazard[]; + /** Postgres: which comparison decided CHECK, index-expression and column-default equality. */ + exprCanonPath?: ExprCanonPath; } +/** + * Where Postgres's own spelling of an expected expression came from. + * - `shadow`: the expected schema applied to a scratch database (shadow-canon.ts). + * - `probe`: temp objects in a rolled-back transaction on the target itself (needs write + * privileges, and takes brief locks on it). + * - `normaliser`: no database was asked; the string comparison decided. + */ +export type ExprCanonPath = "shadow" | "probe" | "normaliser"; + /** * A change whose apply depends on the data already in the table. * diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts new file mode 100644 index 0000000000..5f04f13b68 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts @@ -0,0 +1,168 @@ +/** + * The shadow-database path: the expected schema is applied to a scratch database, introspected + * by the same introspector as the live one, and the two are compared in Postgres's own + * spelling. One round trip covers every shape the string comparison used to get wrong (CHECK + * variants, the five re-spelled defaults, a composite default, index expressions and + * predicates, a view), and a real change of each kind must still be reported. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ + +import { test, expect, beforeAll, afterAll, describe } from "bun:test"; +import { Pool } from "pg"; +import { Kysely, PostgresDialect, sql } from "kysely"; +import { introspectPostgres } from "../../src/introspect/postgres.js"; +import { diff } from "../../src/diff/index.js"; +import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; +import { postgresShadowCanon, type ShadowOpener } from "../../src/shadow-canon.js"; +import type { SchemaSnapshot, TableDescriptor } from "../../src/types.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const TABLE = "shadow_canon_t"; +const SHADOW_DB = "mo_shadow_canon_test"; + +type Db = Kysely>; + +describe("shadow-database canon (live PG)", () => { + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + const connect = (url: string): { pool: Pool; db: Db } => { + const pool = new Pool({ connectionString: url }); + return { pool, db: new Kysely>({ dialect: new PostgresDialect({ pool }) }) }; + }; + const shadowUrl = (): string => { + const u = new URL(PG_URL); + u.pathname = `/${SHADOW_DB}`; + return u.toString(); + }; + let main: { pool: Pool; db: Db }; + let closes = 0; + + const opener: ShadowOpener = async () => { + const s = connect(shadowUrl()); + return { db: s.db, close: async () => { closes++; await s.db.destroy(); } }; + }; + + beforeAll(async () => { + main = connect(PG_URL); + await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); + await sql.raw(`CREATE DATABASE ${SHADOW_DB}`).execute(main.db); + await sql.raw(` + DROP VIEW IF EXISTS ${TABLE}_v; DROP TABLE IF EXISTS ${TABLE}; + CREATE TABLE ${TABLE} ( + id bigint PRIMARY KEY, a int, b int, status text, code varchar(16), + ts timestamptz DEFAULT NOW(), dt date DEFAULT current_date, ts2 timestamptz DEFAULT '2024-01-01', + dbl double precision DEFAULT '1e3', n integer DEFAULT '007', soon timestamptz DEFAULT now() + interval '1 day', + label text DEFAULT 'a', + CONSTRAINT sc_prec_chk CHECK ((a = 1 OR b = 1) AND status = 'x'), + CONSTRAINT sc_in_chk CHECK (status IN ('OPEN', 'CLOSED')), + CONSTRAINT sc_re_chk CHECK (code ~ '^[a-z]+$'), + CONSTRAINT sc_ns_chk CHECK (a IS NOT DISTINCT FROM b) + ); + CREATE INDEX sc_expr_idx ON ${TABLE} (lower(status)); + CREATE INDEX sc_part_idx ON ${TABLE} (id) WHERE (a = 1 OR b = 1) AND status = 'x'; + CREATE VIEW ${TABLE}_v AS SELECT id, status FROM ${TABLE}`).execute(main.db); + }); + afterAll(async () => { + await sql.raw(`DROP VIEW IF EXISTS ${TABLE}_v; DROP TABLE IF EXISTS ${TABLE}`).execute(main.db); + await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); + await main.pool.end(); + }); + + const tableOf = (s: SchemaSnapshot): TableDescriptor => s.tables.find((t) => t.name === TABLE)!; + + /** The expected side: the live schema with the author's spelling (and any real change) laid over it. */ + const declared = async (edit: (t: TableDescriptor) => void): Promise<{ live: SchemaSnapshot; expected: SchemaSnapshot }> => { + const live = await introspectPostgres(main.db); + const expected = structuredClone(live); + edit(tableOf(expected)); + return { live, expected }; + }; + const AUTHORED = (t: TableDescriptor): void => { + const col = (n: string) => t.columns.find((c) => c.name === n)!; + col("ts").default = { kind: "expr", value: "NOW()" }; + col("dt").default = { kind: "expr", value: "current_date" }; + col("ts2").default = { kind: "literal", value: "2024-01-01" }; + col("dbl").default = { kind: "literal", value: "1e3" }; + col("n").default = { kind: "literal", value: "007" }; + col("soon").default = { kind: "expr", value: "now() + interval '1 day'" }; + const chk = (n: string) => t.checks.find((c) => c.name === n)!; + chk("sc_prec_chk").expression = `("a" = 1 OR "b" = 1) AND "status" = 'x'`; + chk("sc_in_chk").expression = `"status" IN ('OPEN', 'CLOSED')`; + chk("sc_re_chk").expression = `"code" ~ '^[a-z]+$'`; + chk("sc_ns_chk").expression = `"a" IS NOT DISTINCT FROM "b"`; + const ix = (n: string) => t.indexes.find((i) => i.name === n)!; + ix("sc_expr_idx").expr = `lower("status")`; + ix("sc_part_idx").where = `("a" = 1 OR "b" = 1) AND "status" = 'x'`; + }; + const changeKinds = async (edit: (t: TableDescriptor) => void, how: "shadow" | "probe" | "none") => { + const { live, expected } = await declared(edit); + const r = await diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + ...(how === "shadow" ? { exprCanonShadow: postgresShadowCanon(opener) } : {}), + ...(how === "probe" ? { exprCanonProbe: postgresExprCanonProbe(main.db) } : {}), + }); + return { kinds: r.changes.map((c) => c.kind).sort(), path: r.exprCanonPath }; + }; + + test("the string comparison alone reports the authored spellings as drift (the defect)", async () => { + const r = await changeKinds(AUTHORED, "none"); + expect(r.path).toBe("normaliser"); + expect(r.kinds).toContain("change-column-default"); + }); + + test("the shadow path reads every authored spelling as clean", async () => { + const before = closes; + const r = await changeKinds(AUTHORED, "shadow"); + expect(r).toEqual({ kinds: [], path: "shadow" }); + expect(closes).toBe(before + 1); + }); + + test("the shadow leaves nothing behind in the shadow database", async () => { + const s = connect(shadowUrl()); + try { + const left = await sql<{ n: number }>`SELECT count(*)::int AS n FROM pg_class WHERE relname LIKE ${`${TABLE}%`}`.execute(s.db); + expect(left.rows[0]!.n).toBe(0); + } finally { + await s.pool.end(); + } + }); + + test("a real change of each kind is still reported", async () => { + const cases: Array<[string, (t: TableDescriptor) => void, string[]]> = [ + ["default value", (t) => { AUTHORED(t); t.columns.find((c) => c.name === "label")!.default = { kind: "literal", value: "b" }; }, ["change-column-default"]], + ["default instant", (t) => { AUTHORED(t); t.columns.find((c) => c.name === "ts2")!.default = { kind: "literal", value: "2024-01-02" }; }, ["change-column-default"]], + ["default expression", (t) => { AUTHORED(t); t.columns.find((c) => c.name === "soon")!.default = { kind: "expr", value: "now() + interval '2 days'" }; }, ["change-column-default"]], + ["check literal case", (t) => { AUTHORED(t); t.checks.find((c) => c.name === "sc_in_chk")!.expression = `"status" IN ('open', 'closed')`; }, ["add-check", "drop-check"]], + ["check precedence", (t) => { AUTHORED(t); t.checks.find((c) => c.name === "sc_prec_chk")!.expression = `"a" = 1 OR ("b" = 1 AND "status" = 'x')`; }, ["add-check", "drop-check"]], + ["index expression", (t) => { AUTHORED(t); t.indexes.find((i) => i.name === "sc_expr_idx")!.expr = `upper("status")`; }, ["add-index", "drop-index"]], + ["index predicate", (t) => { AUTHORED(t); t.indexes.find((i) => i.name === "sc_part_idx")!.where = `"a" = 1 OR ("b" = 1 AND "status" = 'x')`; }, ["add-index", "drop-index"]], + ]; + for (const [label, edit, want] of cases) { + const r = await changeKinds(edit, "shadow"); + expect({ label, path: r.path, kinds: r.kinds }).toEqual({ label, path: "shadow", kinds: want }); + } + }); + + test("a schema that does not apply to the shadow falls back to the probe, then the normaliser", async () => { + const { live, expected } = await declared(AUTHORED); + tableOf(expected).foreignKeys.push({ name: "sc_fk", columns: ["a"], refTable: "no_such_table", refColumns: ["id"], onDelete: "restrict" }); + const r = await diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + exprCanonShadow: postgresShadowCanon(opener), exprCanonProbe: postgresExprCanonProbe(main.db), + }); + expect(r.exprCanonPath).toBe("probe"); + expect(r.changes.map((c) => c.kind)).toEqual(["add-fk"]); + }); + + test("an opener that cannot open reads as unavailable", async () => { + const { live, expected } = await declared(AUTHORED); + const r = await diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + exprCanonShadow: postgresShadowCanon(async () => { throw new Error("no CREATE DATABASE"); }), + }); + expect(r.exprCanonPath).toBe("normaliser"); + }); +}); diff --git a/server/typescript/packages/sdk/src/config.ts b/server/typescript/packages/sdk/src/config.ts index d77887016b..a39c274631 100644 --- a/server/typescript/packages/sdk/src/config.ts +++ b/server/typescript/packages/sdk/src/config.ts @@ -67,6 +67,8 @@ const MigrateFormatEnum = z.enum(["default", "flyway"]); const MigrateBlock = z.object({ outDir: z.string(), databaseUrl: z.string(), + /** Postgres: a scratch database `meta verify --db` and `meta migrate` apply the expected schema to. */ + shadowDatabaseUrl: z.string(), dialect: DialectEnum, format: MigrateFormatEnum, onAmbiguous: OnAmbiguousEnum, From 3712375d29d98e2f9ab8c76dbf8b41127b5ca2e0 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 22:38:15 -0400 Subject: [PATCH 3/8] feat(migrate-ts): make the shadow path a schema-wide diff of introspect(shadow) against introspect(live) --- CHANGELOG.md | 6 +- docs/features/migrations-and-drift.md | 12 ++- .../packages/cli/src/commands/migrate.ts | 14 +-- .../packages/cli/src/commands/verify.ts | 6 +- .../packages/migrate-ts/src/diff/index.ts | 19 ++-- .../packages/migrate-ts/src/drift/drift.ts | 8 +- .../packages/migrate-ts/src/index.ts | 5 +- .../packages/migrate-ts/src/shadow-canon.ts | 23 ++-- .../migrate-ts/src/shadow-expected.ts | 63 +++++++++++ .../test/integration/pg-shadow-canon.test.ts | 8 +- .../integration/pg-shadow-schema-wide.test.ts | 100 ++++++++++++++++++ 11 files changed, 221 insertions(+), 43 deletions(-) create mode 100644 server/typescript/packages/migrate-ts/src/shadow-expected.ts create mode 100644 server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 59fc33d750..f6a10ffbea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -302,8 +302,10 @@ here.** - **Postgres drift is decided by a scratch (shadow) database first.** `meta verify --db` and `meta migrate` (`--from-db`, `--apply`, and `baseline --from-db`) now apply the model's expected schema to a throwaway database, as Atlas, Prisma and migra do, read it with the introspector that - reads the live database, and compare CHECKs, index expressions and predicates, and column - defaults in Postgres's own spelling. The scratch database is `--shadow-db `, else + reads the live database, and diff the two catalogs, so tables, columns and types, defaults, + CHECKs, constraints and their actions, and indexes are all compared in Postgres's own spelling + with no per-shape rule (renames, `@unmanaged`, scope and a key's `identity` stay inputs to the + diff; views keep their model fingerprint; comments are not read back). The scratch database is `--shadow-db `, else `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl`; with none, a database is `CREATE`d on the `--db` server when the role may and always dropped afterwards; `--no-shadow-db` skips both. The schema is applied in a rolled-back transaction, so an explicit URL is left empty. When no scratch diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 14d2d3f905..8d609bdfd7 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -382,9 +382,15 @@ guessing. There are three ways it asks, tried in this order, and the output says **1. A scratch (shadow) database, the primary path.** The practice Atlas, Prisma and migra share: the model's full expected schema (the DDL `meta migrate` emits for an empty database) is applied to a throwaway Postgres database, in one transaction that is always rolled back, and read with the same -introspector that reads your live database. The expected CHECKs, index expressions and predicates, -and column defaults are then Postgres's own spelling of the same catalog, compared with the live -one as plain strings. No per-shape rule is involved, so `NOW()`/`now()`, `current_date`, a date +introspector that reads your live database. Drift is then the **same diff run on two catalogs**, +`introspect(shadow)` as the expected side against `introspect(live)`, for every object in the run's +scope: tables, columns and types, nullability, defaults, CHECKs, primary/foreign/unique constraints +and their actions, and indexes. Both sides are Postgres's own spelling, so a shape nobody wrote a +rule for (`int4` against `integer`, `ON DELETE SET NULL`) needs none. Model-only knowledge stays an +input to that diff, not a second comparison: renames, `@unmanaged` and scope are applied as always, +a column's `identity` is carried over from the model (a legacy `serial` key is not drift against an +`increment` key), and views keep their model fingerprint. Column and table comments are not read +back from the database, so a changed description is not drift. No per-shape rule is involved, so `NOW()`/`now()`, `current_date`, a date literal on a `timestamptz` (read in the session `TimeZone`), `'1e3'` on a double, `'007'` on an integer, `now() + interval '1 day'`, a regrouped `NOT` and a precedence change are all decided by the database. A real change of any of them is still reported. The scratch database comes from: diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index 2709e95b41..02a14ede62 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -59,8 +59,9 @@ import { type TableDescriptor, postgresViewReplaceProbe, postgresExprCanonProbe, - postgresShadowCanon, - type ShadowCanonSource, + postgresShadowSchema, + type ShadowSchemaSource, + canonProbeOverShadow, adoptGeneratedSpelling, } from "@metaobjectsdev/migrate-ts"; import { @@ -845,7 +846,7 @@ export async function migrateCommand( // ...and the same for CHECK / index expressions: Postgres says how it stores the expected // text, so a stored rewrite no string rule knows is not proposed as a drop and re-add. ...(kysely.dialect === "postgres" ? { exprCanonProbe: postgresExprCanonProbe(kysely.db) } : {}), - ...(shadowOpen !== undefined ? { exprCanonShadow: postgresShadowCanon(shadowOpen) } : {}), + ...(shadowOpen !== undefined ? { shadowSchema: postgresShadowSchema(shadowOpen) } : {}), allow: tokensToAllowOptions(config.allow), renames: config.renames, // #258 — adopting a live DB whose PRIMARY KEY differs from the metadata identity @@ -1173,12 +1174,13 @@ async function withGeneratedSpelling( dialect: Dialect, metaRoot: string, genRoot: string, - shadow?: ShadowCanonSource, + shadow?: ShadowSchemaSource, ): Promise { try { const metadata = await loadBaselineMetadata(metaRoot, genRoot); const expected = baselineFromMetadata(metadata, dialect, buildProjectionViews(metadata, { dialect })); - const probe = (shadow !== undefined ? await shadow(expected) : undefined) ?? postgresExprCanonProbe(db); + const shadowCatalog = shadow !== undefined ? await shadow(expected) : undefined; + const probe = shadowCatalog !== undefined ? canonProbeOverShadow(shadowCatalog) : postgresExprCanonProbe(db); return await adoptGeneratedSpelling(snapshot, expected, probe); } catch { return snapshot; @@ -1229,7 +1231,7 @@ export async function runBaseline( ); snapshot = await withGeneratedSpelling( snapshot, kysely.db, config.dialect, metaRoot, genRoot, - shadowOpen !== undefined ? postgresShadowCanon(shadowOpen) : undefined, + shadowOpen !== undefined ? postgresShadowSchema(shadowOpen) : undefined, ); } } finally { diff --git a/server/typescript/packages/cli/src/commands/verify.ts b/server/typescript/packages/cli/src/commands/verify.ts index 3a23f4524c..9932846fdb 100644 --- a/server/typescript/packages/cli/src/commands/verify.ts +++ b/server/typescript/packages/cli/src/commands/verify.ts @@ -90,7 +90,7 @@ import { type ExprCanonProbe, postgresViewReplaceProbe, postgresExprCanonProbe, - postgresShadowCanon, + postgresShadowSchema, withoutFlywayHistory, } from "@metaobjectsdev/migrate-ts"; import { @@ -1363,7 +1363,7 @@ export async function verifyCommand( { url: flags.db as string, handle: kysely }, ) : undefined; - const exprCanonShadow = shadowOpen !== undefined ? postgresShadowCanon(shadowOpen) : undefined; + const shadowSchema = shadowOpen !== undefined ? postgresShadowSchema(shadowOpen) : undefined; let driftResult; let actual: SchemaSnapshot; try { @@ -1382,7 +1382,7 @@ export async function verifyCommand( ...(kysely.dialect === "postgres" ? { viewReplaceProbe: postgresViewReplaceProbe(kysely.db) } : {}), // ...and asks Postgres, not a string rewrite, how it stores each expected CHECK and index expression. ...(exprCanonProbe !== undefined ? { exprCanonProbe } : {}), - ...(exprCanonShadow !== undefined ? { exprCanonShadow } : {}), + ...(shadowSchema !== undefined ? { shadowSchema } : {}), ...(schemaScope !== undefined ? { inScope: schemaScope } : {}), // The import exclusion reaches scopeExpectedSchema through this option bag — // there is no direct call on this path to attach it to. diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index 861f534e40..1c3b9f45d4 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -13,7 +13,7 @@ import { detectColumnRenames, detectTableRenames } from "./rename-heuristic.js"; import { firstViewSqlDifference, viewSqlEquals } from "../view-sql-compare.js"; import { viewReplaceIsLegal, type ViewReplaceProbe } from "../view-replace.js"; import { liveViewReadsTable, sqlNamesIdentifier } from "../view-reads.js"; -import type { ShadowCanonSource } from "../shadow-canon.js"; +import { expectedFromShadow, shadowIdentityProbe, type ShadowSchemaSource } from "../shadow-expected.js"; import { checkExprEquals, checkExprRespelledNullSafe, renameExprIdentifiers } from "../check-expr-compare.js"; import { checkMatchesLive, columnDefaultMatchesLive, indexExprMatchesLive, NO_EXPR_CANON, type ExprCanon, type ExprCanonProbe, type IndexExprCanon } from "../expr-canon.js"; import { isPgAutoSequenceDefault } from "../pg-identity-default.js"; @@ -91,11 +91,12 @@ export interface DiffArgs { */ exprCanonProbe?: ExprCanonProbe; /** - * Postgres: the primary source of that answer, a scratch (shadow) database the expected - * schema is applied to (see shadow-canon.ts). Tried first; when it resolves no probe, + * Postgres: the primary path. A scratch (shadow) database the expected schema is applied to; + * when it resolves a catalog, drift is the diff of that catalog against `actual` (see + * shadow-expected.ts) and `exprCanonProbe` is not consulted. When it resolves none, * `exprCanonProbe` answers, and without one the string normaliser does. */ - exprCanonShadow?: ShadowCanonSource; + shadowSchema?: ShadowSchemaSource; /** * #258 — refuse (throw {@link PrimaryKeyChangeError}) when an existing table's live * PRIMARY KEY differs from the metadata identity. There is no primary-key change kind @@ -164,9 +165,14 @@ export async function diff( optsMaybe?: Omit, ): Promise { // Normalize args. - const args: DiffArgs = isDiffArgs(argsOrExpected) + const given: DiffArgs = isDiffArgs(argsOrExpected) ? argsOrExpected : { expected: argsOrExpected, actual: actualMaybe!, ...(optsMaybe ?? {}) }; + // Postgres, primary path: the expected side becomes the shadow database's own catalog. + const shadow = given.dialect === "postgres" && given.shadowSchema !== undefined + ? await given.shadowSchema(given.expected) + : undefined; + const args: DiffArgs = shadow !== undefined ? { ...given, expected: expectedFromShadow(shadow, given.expected) } : given; const changes: Change[] = []; @@ -264,8 +270,7 @@ export async function diff( let exprCanonPath: ExprCanonPath = "normaliser"; let canonProbe: ExprCanonProbe | undefined; if (args.dialect === "postgres") { - canonProbe = args.exprCanonShadow === undefined ? undefined : await args.exprCanonShadow(args.expected); - if (canonProbe !== undefined) exprCanonPath = "shadow"; + if (shadow !== undefined) { canonProbe = shadowIdentityProbe; exprCanonPath = "shadow"; } else if (args.exprCanonProbe !== undefined) { canonProbe = args.exprCanonProbe; exprCanonPath = "probe"; } } const canon = await probeExprCanon(canonProbe, args.dialect, changes, expectedTables, actualTables); diff --git a/server/typescript/packages/migrate-ts/src/drift/drift.ts b/server/typescript/packages/migrate-ts/src/drift/drift.ts index d4715432d1..6c76415350 100644 --- a/server/typescript/packages/migrate-ts/src/drift/drift.ts +++ b/server/typescript/packages/migrate-ts/src/drift/drift.ts @@ -26,7 +26,7 @@ import { postgresViewReplaceProbe } from "../view-replace-probe.js"; import type { ViewReplaceProbe } from "../view-replace.js"; import { postgresExprCanonProbe } from "../expr-canon-probe.js"; import type { ExprCanonProbe } from "../expr-canon.js"; -import type { ShadowCanonSource } from "../shadow-canon.js"; +import type { ShadowSchemaSource } from "../shadow-expected.js"; export interface ComputeDriftOptions { /** @@ -60,8 +60,8 @@ export interface ComputeDriftOptions { * the string normaliser decides. */ exprCanonProbe?: ExprCanonProbe; - /** Postgres: the shadow-database source tried before `exprCanonProbe` (see shadow-canon.ts). */ - exprCanonShadow?: ShadowCanonSource; + /** Postgres: the shadow-database catalog the diff runs against first (see shadow-expected.ts). */ + shadowSchema?: ShadowSchemaSource; /** * Per-command scope (`migrate.scope`): objects whose declaring FQN this predicate * rejects are governed by somebody else. They leave the expected side AND are @@ -152,7 +152,7 @@ export async function computeDriftFromActual( ...(opts?.ignoreTables !== undefined ? { ignoreTables: opts.ignoreTables } : {}), ...(opts?.viewReplaceProbe !== undefined ? { viewReplaceProbe: opts.viewReplaceProbe } : {}), ...(opts?.exprCanonProbe !== undefined ? { exprCanonProbe: opts.exprCanonProbe } : {}), - ...(opts?.exprCanonShadow !== undefined ? { exprCanonShadow: opts.exprCanonShadow } : {}), + ...(opts?.shadowSchema !== undefined ? { shadowSchema: opts.shadowSchema } : {}), }); return { ...result, diff --git a/server/typescript/packages/migrate-ts/src/index.ts b/server/typescript/packages/migrate-ts/src/index.ts index 15cf4d00c6..5255a85eb5 100644 --- a/server/typescript/packages/migrate-ts/src/index.ts +++ b/server/typescript/packages/migrate-ts/src/index.ts @@ -97,8 +97,9 @@ export type { ExactViewColumn, ViewReplaceEvidence, ViewReplaceProbe } from "./v // Postgres's own spelling of an expected CHECK / index expression is asked of the database the // same way (rolled-back scratch table); the string normaliser covers whatever it cannot answer. export { postgresExprCanonProbe } from "./expr-canon-probe.js"; -export { postgresShadowCanon } from "./shadow-canon.js"; -export type { ShadowCanonSource, ShadowHandle, ShadowOpener } from "./shadow-canon.js"; +export { postgresShadowSchema, canonProbeOverShadow } from "./shadow-canon.js"; +export type { ShadowHandle, ShadowOpener } from "./shadow-canon.js"; +export type { ShadowSchemaSource } from "./shadow-expected.js"; export type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; // `baseline --from-db`: record the generated text for an expression the probe proved equal. export { adoptGeneratedSpelling } from "./snapshot/adopt-generated-spelling.js"; diff --git a/server/typescript/packages/migrate-ts/src/shadow-canon.ts b/server/typescript/packages/migrate-ts/src/shadow-canon.ts index b6c81395a3..9e34b26fe8 100644 --- a/server/typescript/packages/migrate-ts/src/shadow-canon.ts +++ b/server/typescript/packages/migrate-ts/src/shadow-canon.ts @@ -2,9 +2,8 @@ // // The practice Atlas, Prisma and migra share: apply the model's full expected schema to a // scratch (shadow) database, introspect it with the very introspector that reads the live -// database, and compare. Both sides are then Postgres's own spelling of the same catalog, so -// no per-shape rule is needed for a CHECK, an index expression or predicate, or a column -// DEFAULT. +// database, and diff the two catalogs (shadow-expected.ts). Both sides are then Postgres's own +// spelling of the same catalog, so no per-shape rule is needed for anything in scope. // // The DDL is the one `meta migrate` emits for an empty database, run inside ONE transaction // that is always rolled back, so nothing is left behind even in a shared shadow database. @@ -19,6 +18,7 @@ import { emit } from "./emit/index.js"; import { introspectPostgres } from "./introspect/postgres.js"; import { qualifiedDbName } from "./qualified-name.js"; import type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; +import type { ShadowSchemaSource } from "./shadow-expected.js"; import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor, SchemaSnapshot } from "./types.js"; /** A connection to a scratch Postgres database, and how to release it. */ @@ -31,15 +31,14 @@ export interface ShadowHandle { /** Opens a shadow database, or resolves `undefined` when none is available. May throw; that reads as unavailable. */ export type ShadowOpener = () => Promise; -/** - * Given the expected schema of a run, resolves the probe that answers from a shadow database, - * or `undefined` when no shadow is available (the caller then falls back). - */ -export type ShadowCanonSource = (expected: SchemaSnapshot) => Promise; const ROLLBACK = new Error("shadow canon: always rolled back"); -export function postgresShadowCanon(open: ShadowOpener): ShadowCanonSource { +/** + * The shadow catalog for a run: `expected` applied to a scratch database and introspected, or + * `undefined` when no shadow is available or the schema does not apply (the caller falls back). + */ +export function postgresShadowSchema(open: ShadowOpener): ShadowSchemaSource { return async (expected) => { let handle: ShadowHandle | undefined; try { @@ -61,7 +60,7 @@ export function postgresShadowCanon(open: ShadowOpener): ShadowCanonSource { } catch (err) { if (err !== ROLLBACK) return undefined; } - return shadow === undefined ? undefined : probeOver(shadow); + return shadow; } catch { return undefined; } finally { @@ -70,8 +69,8 @@ export function postgresShadowCanon(open: ShadowOpener): ShadowCanonSource { }; } -/** The answer for each expected descriptor, read off the shadow table of the same name. */ -function probeOver(shadow: SchemaSnapshot): ExprCanonProbe { +/** An {@link ExprCanonProbe} reading each expected descriptor off the shadow table of the same name (used by `baseline --from-db`). */ +export function canonProbeOverShadow(shadow: SchemaSnapshot): ExprCanonProbe { const tables = new Map(shadow.tables.map((t) => [qualifiedDbName(t), t] as const)); return async (_live, expected) => { const st = tables.get(qualifiedDbName(expected)); diff --git a/server/typescript/packages/migrate-ts/src/shadow-expected.ts b/server/typescript/packages/migrate-ts/src/shadow-expected.ts new file mode 100644 index 0000000000..18d199f33a --- /dev/null +++ b/server/typescript/packages/migrate-ts/src/shadow-expected.ts @@ -0,0 +1,63 @@ +// shadow-expected.ts — turn a shadow database's catalog into the EXPECTED side of the diff. +// +// On the shadow path drift is the diff of introspect(shadow) against introspect(live): the +// same introspector reads both, so every object in scope (tables, columns and types, +// nullability, defaults, CHECKs, PK/FK/unique constraints and their actions, indexes) is in +// Postgres's own spelling on both sides and no per-shape rule is needed. Model-only knowledge +// stays an INPUT to the same diff rather than a second comparison: +// - renames, @unmanaged and scope are `diff` arguments, applied as always; +// - a column's `identity` and a table's/column's `description` are carried over from the +// model, because the catalog cannot say which spelling of an auto-generated key the model +// asked for (a legacy `serial` column is deliberately not drift against an `increment` key); +// - views keep the model's own descriptor, since their drift check is a fingerprint the +// model computed, not a comparison of Postgres's deparsed body. + +import { qualifiedDbName } from "./qualified-name.js"; +import type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; +import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor, SchemaSnapshot, TableDescriptor } from "./types.js"; + +/** A shadow database's introspected catalog for `expected`'s run, or `undefined` when none is available. */ +export type ShadowSchemaSource = (expected: SchemaSnapshot) => Promise; + +/** + * `expected` re-read from `shadow`: each model table replaced by the shadow's table of the + * same name (a table the shadow lacks is kept as modelled), with the model-only fields laid back. + */ +export function expectedFromShadow(shadow: SchemaSnapshot, expected: SchemaSnapshot): SchemaSnapshot { + const byId = new Map(shadow.tables.map((t) => [qualifiedDbName(t), t] as const)); + const tables = expected.tables.map((model): TableDescriptor => { + const st = byId.get(qualifiedDbName(model)); + if (st === undefined) return model; + const modelCols = new Map(model.columns.map((c) => [c.name, c] as const)); + const columns = st.columns.map((c): ColumnDescriptor => { + const m = modelCols.get(c.name); + return { + ...c, + ...(m?.identity !== undefined ? { identity: m.identity } : {}), + ...(m?.description !== undefined ? { description: m.description } : {}), + }; + }); + return { ...st, columns, ...(model.description !== undefined ? { description: model.description } : {}) }; + }); + return { ...expected, tables }; +} + +/** + * The expression-canon answer when `expected` is already Postgres's spelling: each + * expression is its own canon, so the comparison with the live one is a plain string equality + * and the string normaliser (which cannot see a precedence change) is never consulted. + */ +export const shadowIdentityProbe: ExprCanonProbe = async (_live, expected) => { + const checks = new Map(expected.checks.map((c) => [c, c.expression] as const)); + const indexes = new Map(); + for (const ix of expected.indexes) { + const canon: IndexExprCanon = {}; + if (ix.expr !== undefined) canon.expr = ix.expr; + if (ix.where !== undefined) canon.where = ix.where; + indexes.set(ix, canon); + } + const defaults = new Map(); + for (const col of expected.columns) if (col.default !== undefined) defaults.set(col, col.default); + const answer: ExprCanon = { checks, indexes, defaults }; + return answer; +}; diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts index 5f04f13b68..da70cfbee1 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts @@ -14,7 +14,7 @@ import { Kysely, PostgresDialect, sql } from "kysely"; import { introspectPostgres } from "../../src/introspect/postgres.js"; import { diff } from "../../src/diff/index.js"; import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; -import { postgresShadowCanon, type ShadowOpener } from "../../src/shadow-canon.js"; +import { postgresShadowSchema, type ShadowOpener } from "../../src/shadow-canon.js"; import type { SchemaSnapshot, TableDescriptor } from "../../src/types.js"; const PG_URL = process.env["MIGRATE_TS_PG_URL"]; @@ -101,7 +101,7 @@ describe("shadow-database canon (live PG)", () => { const { live, expected } = await declared(edit); const r = await diff({ expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], - ...(how === "shadow" ? { exprCanonShadow: postgresShadowCanon(opener) } : {}), + ...(how === "shadow" ? { shadowSchema: postgresShadowSchema(opener) } : {}), ...(how === "probe" ? { exprCanonProbe: postgresExprCanonProbe(main.db) } : {}), }); return { kinds: r.changes.map((c) => c.kind).sort(), path: r.exprCanonPath }; @@ -151,7 +151,7 @@ describe("shadow-database canon (live PG)", () => { tableOf(expected).foreignKeys.push({ name: "sc_fk", columns: ["a"], refTable: "no_such_table", refColumns: ["id"], onDelete: "restrict" }); const r = await diff({ expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], - exprCanonShadow: postgresShadowCanon(opener), exprCanonProbe: postgresExprCanonProbe(main.db), + shadowSchema: postgresShadowSchema(opener), exprCanonProbe: postgresExprCanonProbe(main.db), }); expect(r.exprCanonPath).toBe("probe"); expect(r.changes.map((c) => c.kind)).toEqual(["add-fk"]); @@ -161,7 +161,7 @@ describe("shadow-database canon (live PG)", () => { const { live, expected } = await declared(AUTHORED); const r = await diff({ expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], - exprCanonShadow: postgresShadowCanon(async () => { throw new Error("no CREATE DATABASE"); }), + shadowSchema: postgresShadowSchema(async () => { throw new Error("no CREATE DATABASE"); }), }); expect(r.exprCanonPath).toBe("normaliser"); }); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts new file mode 100644 index 0000000000..fb17c05cdd --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts @@ -0,0 +1,100 @@ +/** + * The shadow path is a catalog-to-catalog diff, not a list of expression rules. Nothing in + * this file has a special case anywhere in migrate-ts: an `int4` column against a model + * `integer`, and an FK `ON DELETE SET NULL`, read clean through the shadow path because + * Postgres (not a rule) says what they are, and each is still reported when it really differs. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ + +import { test, expect, beforeAll, afterAll, describe } from "bun:test"; +import { Pool } from "pg"; +import { Kysely, PostgresDialect, sql } from "kysely"; +import { introspectPostgres } from "../../src/introspect/postgres.js"; +import { diff } from "../../src/diff/index.js"; +import { postgresShadowSchema } from "../../src/shadow-canon.js"; +import type { FkDescriptor, SchemaSnapshot } from "../../src/types.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const SHADOW_DB = "mo_shadow_wide_test"; + +type Db = Kysely>; + +describe("shadow path is schema-wide (live PG)", () => { + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + const connect = (url: string): { pool: Pool; db: Db } => { + const pool = new Pool({ connectionString: url }); + return { pool, db: new Kysely>({ dialect: new PostgresDialect({ pool }) }) }; + }; + let main: { pool: Pool; db: Db }; + const shadow = postgresShadowSchema(async () => { + const u = new URL(PG_URL); + u.pathname = `/${SHADOW_DB}`; + const s = connect(u.toString()); + return { db: s.db, close: async () => { await s.db.destroy(); } }; + }); + + beforeAll(async () => { + main = connect(PG_URL); + await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); + await sql.raw(`CREATE DATABASE ${SHADOW_DB}`).execute(main.db); + await sql.raw(` + DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent; + CREATE TABLE sc2_parent (id int4 PRIMARY KEY, label character varying(20) NOT NULL); + CREATE TABLE sc2_child ( + id int4 PRIMARY KEY, parent_id integer, + CONSTRAINT sc2_child_parent_fk FOREIGN KEY (parent_id) REFERENCES sc2_parent (id) ON DELETE SET NULL + )`).execute(main.db); + }); + afterAll(async () => { + await sql.raw(`DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent`).execute(main.db); + await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); + await main.pool.end(); + }); + + /** The model, written by hand (not read from the live database). */ + const model = (over?: { onDelete?: FkDescriptor["onDelete"]; parentIdBits?: 32 | 64; labelLength?: number }): SchemaSnapshot => ({ + views: [], + tables: [ + { + name: "sc2_parent", schema: "public", indexes: [], foreignKeys: [], checks: [], primaryKey: ["id"], + columns: [ + { name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false }, + { name: "label", sqlType: { kind: "text", maxLength: over?.labelLength ?? 20 }, nullable: false }, + ], + }, + { + name: "sc2_child", schema: "public", indexes: [], checks: [], primaryKey: ["id"], + columns: [ + { name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false }, + { name: "parent_id", sqlType: { kind: "integer", bits: over?.parentIdBits ?? 32 }, nullable: true }, + ], + foreignKeys: [{ + name: "sc2_child_parent_fk", columns: ["parent_id"], refTable: "sc2_parent", refColumns: ["id"], + onDelete: over?.onDelete ?? "set-null", + }], + }, + ], + }); + const run = async (expected: SchemaSnapshot, withShadow: boolean) => { + const live = await introspectPostgres(main.db); + const r = await diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + ...(withShadow ? { shadowSchema: shadow } : {}), + }); + return { kinds: r.changes.filter((c) => "table" in c && String(c.table).startsWith("sc2_")).map((c) => c.kind).sort(), path: r.exprCanonPath }; + }; + + test("int4 against integer, varchar spelling and an FK ON DELETE SET NULL read clean", async () => { + expect(await run(model(), true)).toEqual({ kinds: [], path: "shadow" }); + }); + + test("each is still reported when it really differs", async () => { + expect((await run(model({ onDelete: "cascade" }), true)).kinds).toEqual(["add-fk", "drop-fk"]); + expect((await run(model({ parentIdBits: 64 }), true)).kinds).toEqual(["change-column-type"]); + expect((await run(model({ labelLength: 40 }), true)).kinds).toEqual(["change-column-type"]); + }); +}); From a77abb9932303616a7f284dc01fa1fee8fe8c31e Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 23:20:43 -0400 Subject: [PATCH 4/8] no-mistakes(review): Make shadow path catalog-wide, strict, shared and reported --- CHANGELOG.md | 25 ++-- docs/features/migrations-and-drift.md | 36 ++++-- .../packages/cli/src/commands/migrate.ts | 66 ++++++----- .../packages/cli/src/commands/verify.ts | 42 ++++--- server/typescript/packages/cli/src/index.ts | 3 - .../typescript/packages/cli/src/lib/allow.ts | 2 + .../typescript/packages/cli/src/lib/args.ts | 8 -- .../typescript/packages/cli/src/lib/config.ts | 3 - .../typescript/packages/cli/src/lib/output.ts | 3 +- .../typescript/packages/cli/src/lib/shadow.ts | 109 ++++++++++++------ .../cli/test/__snapshots__/cli.test.ts.snap | 2 - .../packages/cli/test/shadow-opener.test.ts | 56 ++++++--- .../packages/migrate-ts/src/diff/index.ts | 56 ++++++++- .../packages/migrate-ts/src/diff/status.ts | 12 ++ .../packages/migrate-ts/src/emit/postgres.ts | 25 +++- .../packages/migrate-ts/src/emit/sqlite.ts | 9 ++ .../packages/migrate-ts/src/errors.ts | 4 + .../packages/migrate-ts/src/expr-canon.ts | 5 + .../packages/migrate-ts/src/index.ts | 2 +- .../migrate-ts/src/introspect/postgres.ts | 40 +++++++ .../packages/migrate-ts/src/shadow-canon.ts | 58 +++++++--- .../migrate-ts/src/shadow-expected.ts | 47 +++----- .../packages/migrate-ts/src/types.ts | 14 ++- .../integration/pg-rename-carry-canon.test.ts | 54 ++++++--- .../test/integration/pg-shadow-canon.test.ts | 33 +++++- .../integration/pg-shadow-schema-wide.test.ts | 88 +++++++++++--- 26 files changed, 581 insertions(+), 221 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6a10ffbea..0b54a8dcde 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -303,15 +303,24 @@ here.** `meta migrate` (`--from-db`, `--apply`, and `baseline --from-db`) now apply the model's expected schema to a throwaway database, as Atlas, Prisma and migra do, read it with the introspector that reads the live database, and diff the two catalogs, so tables, columns and types, defaults, - CHECKs, constraints and their actions, and indexes are all compared in Postgres's own spelling - with no per-shape rule (renames, `@unmanaged`, scope and a key's `identity` stay inputs to the - diff; views keep their model fingerprint; comments are not read back). The scratch database is `--shadow-db `, else + identity, CHECKs, constraints and their actions, indexes, view bodies and comments are all + compared in Postgres's own spelling with no per-shape rule (renames, `@unmanaged` and scope stay + inputs to the diff). On that path the introspector reads identity (`attidentity`) and comments + (`pg_description`) from both catalogs, a GENERATED identity added or dropped is planned as + `change-column-identity` (dropping one is gated behind `--allow drop-identity-default`), a + changed comment as `change-comment`, and a view's deparsed body is compared with the live one; a + view that does not apply to the scratch database is compared by its fingerprint, without costing + the run its table comparison. The scratch database is `--shadow-db `, else `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl`; with none, a database is `CREATE`d on the - `--db` server when the role may and always dropped afterwards; `--no-shadow-db` skips both. The - schema is applied in a rolled-back transaction, so an explicit URL is left empty. When no scratch - database is available the rolled-back temp-object probe on the target answers (it needs write - privileges and takes brief locks), and SQLite and offline plans keep the string normaliser. The - output says which path ran. See `docs/features/migrations-and-drift.md`. + `--db` server when the role may and always dropped afterwards. A run opens it once, so + `meta verify --db` compares the committed snapshot on the same scratch database. The schema is + applied in a rolled-back transaction, so an explicit URL is left empty, and an explicit one that + cannot be opened or applied is an error naming the cause. When a `CREATE`d one is not available, + the rolled-back temp-object probe on the target answers (it needs write privileges and takes + brief locks), and SQLite and offline plans keep the string normaliser. Every Postgres run says + which path ran and, on a fallback, why. The Postgres emitter no longer writes a `DEFAULT` beside + `GENERATED BY DEFAULT AS IDENTITY` (a snapshot of a legacy `serial` column carries both, and + Postgres refuses the pair). See `docs/features/migrations-and-drift.md`. - **Postgres column `DEFAULT`s that Postgres re-spells no longer read as drift.** `NOW()`, `current_date`, a date literal on a `timestamptz`, `'1e3'` on a double, `'007'` on an integer diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 8d609bdfd7..6328a49995 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -377,20 +377,29 @@ Postgres rewrites an expression before it stores it (`"k" IN ('x')` becomes `k = `= ANY (ARRAY[…])`, and casts and brackets are added), so the text `meta migrate` generates never comes back from the database as written. On Postgres, a plan against the database (`meta verify --db`, `meta migrate --from-db` and `--apply`) therefore **asks Postgres** instead of -guessing. There are three ways it asks, tried in this order, and the output says which one ran. +guessing. There are three ways it asks, tried in this order, and every Postgres run of +`meta verify --db`, `meta migrate` and `meta migrate baseline --from-db` says which one ran. **1. A scratch (shadow) database, the primary path.** The practice Atlas, Prisma and migra share: the model's full expected schema (the DDL `meta migrate` emits for an empty database) is applied to a throwaway Postgres database, in one transaction that is always rolled back, and read with the same introspector that reads your live database. Drift is then the **same diff run on two catalogs**, `introspect(shadow)` as the expected side against `introspect(live)`, for every object in the run's -scope: tables, columns and types, nullability, defaults, CHECKs, primary/foreign/unique constraints -and their actions, and indexes. Both sides are Postgres's own spelling, so a shape nobody wrote a -rule for (`int4` against `integer`, `ON DELETE SET NULL`) needs none. Model-only knowledge stays an -input to that diff, not a second comparison: renames, `@unmanaged` and scope are applied as always, -a column's `identity` is carried over from the model (a legacy `serial` key is not drift against an -`increment` key), and views keep their model fingerprint. Column and table comments are not read -back from the database, so a changed description is not drift. No per-shape rule is involved, so `NOW()`/`now()`, `current_date`, a date +scope: tables, columns and types, nullability, defaults, identity, CHECKs, primary/foreign/unique +constraints and their actions, indexes, view bodies and comments. Both sides are Postgres's own +spelling, so a shape nobody wrote a rule for (`int4` against `integer`, `ON DELETE SET NULL`, a +view body written differently from Postgres's deparse) needs none. Model-only knowledge stays an +input to that diff, not a second comparison: renames, `@unmanaged` and scope are applied as always. +Identity is read from `pg_attribute.attidentity` on both sides (a legacy `serial` column also reads +as an `increment` key, so it is not drift against one); a GENERATED identity added or dropped is a +`change-column-identity`, and dropping one is gated behind `--allow drop-identity-default`. Table +and column comments are read from `pg_description` on both sides, and a changed one is a +`change-comment` (`COMMENT ON … IS …`). A view's body is compared as Postgres's deparse +(`pg_get_viewdef`) of the shadow's copy against the live one; each view is created under its own +savepoint, so a view whose body reads something only the live database has is left out of the +shadow and compared by its fingerprint instead, without costing the run its table comparison. +Off the shadow path identity and comments are not compared, and views are compared by fingerprint. +No per-shape rule is involved, so `NOW()`/`now()`, `current_date`, a date literal on a `timestamptz` (read in the session `TimeZone`), `'1e3'` on a double, `'007'` on an integer, `now() + interval '1 day'`, a regrouped `NOT` and a precedence change are all decided by the database. A real change of any of them is still reported. The scratch database comes from: @@ -399,10 +408,15 @@ the database. A real change of any of them is still reported. The scratch databa |---|---| | An explicit URL | `--shadow-db ` on `verify` and `migrate`, else `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl` in `.metaobjects/config.json`. Used as given and never dropped; nothing is left in it because the schema is applied inside a rolled-back transaction. | | `CREATE DATABASE` | With none of the above, a database named `mo_shadow_` is created on the `--db` server when the role may, and **always dropped** afterwards, including on error. | -| Off | `--no-shadow-db` skips both. | -If no scratch database can be opened, or the expected schema does not apply to it (an FK to a table -outside the run's scope, a missing extension), the next path runs. +A run opens its scratch database once, and every comparison it makes uses it: `meta verify --db` +compares the committed snapshot with the database on the same scratch database as the drift check. + +An explicitly configured scratch database that cannot be opened, or that the expected schema does +not apply to (an FK to a table outside the run's scope, a missing extension), is an **error** naming +the cause: you asked for a scratch database so the target would not be written to, so the run does +not fall back to the probe on the target. A `CREATE DATABASE` that the role may not run, or that the +schema does not apply to, falls back to the next path, and the output says why. **2. The temp-object probe, the first fallback.** For each table it creates a **temporary table with the live table's columns** on the target itself, inside a transaction it rolls back (a savepoint, when the connection is already in one), adds the expected diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index 02a14ede62..e1da8f7ec5 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -10,7 +10,7 @@ import { formatMigrateResultJson } from "../lib/output-json.js"; import type { OutputFormat } from "../lib/format.js"; import { emitStructured, narrate, toonEncode } from "../lib/format.js"; import { buildKyselyFromUrl, inferDialect, redactUrl } from "../lib/kysely.js"; -import { describeExprCanonPath, shadowOpener } from "../lib/shadow.js"; +import { describeExprCanonPath, shadowSession } from "../lib/shadow.js"; import { log } from "../lib/log.js"; import { loadMemory, resolveCollection, resolveConfigDir, type Collection } from "@metaobjectsdev/sdk"; import { loadMemoryOptionsFrom, loadMetaobjectsConfig, RemovedNamingConfigError, resolveGenConfigDir } from "../lib/load-metaobjects-config.js"; @@ -59,8 +59,8 @@ import { type TableDescriptor, postgresViewReplaceProbe, postgresExprCanonProbe, - postgresShadowSchema, type ShadowSchemaSource, + type ExprCanonPath, canonProbeOverShadow, adoptGeneratedSpelling, } from "@metaobjectsdev/migrate-ts"; @@ -106,7 +106,6 @@ MIGRATE FLAGS: --shadow-db Postgres: a scratch database for the drift comparison. Default: CREATE DATABASE on the --db server (dropped afterwards), else the temp-object probe on the target. Also SHADOW_DATABASE_URL / migrate.shadowDatabaseUrl. - --no-shadow-db Never open a scratch database (use the probe on the target). --dialect sqlite|postgres|d1 Optional dialect override (auto-detected from URL scheme) --migration-format default|flyway @@ -773,12 +772,9 @@ export async function migrateCommand( return 2; } warnDecimalPrecision(metadata, collection, kysely.dialect); - // Postgres: a scratch database answers first how each expected expression is stored (lib/shadow.ts). - const shadowOpen = kysely.dialect === "postgres" - ? shadowOpener( - { url: config.shadowDatabaseUrl, disabled: config.shadowDisabled === true }, - { url: config.databaseUrl, handle: kysely }, - ) + // Postgres: a scratch database is the expected side of the drift diff (lib/shadow.ts). + const shadow = kysely.dialect === "postgres" + ? shadowSession(config.shadowDatabaseUrl, config.databaseUrl) : undefined; let exitCode = 0; @@ -846,7 +842,7 @@ export async function migrateCommand( // ...and the same for CHECK / index expressions: Postgres says how it stores the expected // text, so a stored rewrite no string rule knows is not proposed as a drop and re-add. ...(kysely.dialect === "postgres" ? { exprCanonProbe: postgresExprCanonProbe(kysely.db) } : {}), - ...(shadowOpen !== undefined ? { shadowSchema: postgresShadowSchema(shadowOpen) } : {}), + ...(shadow !== undefined ? { shadowSchema: shadow.schema } : {}), allow: tokensToAllowOptions(config.allow), renames: config.renames, // #258 — adopting a live DB whose PRIMARY KEY differs from the metadata identity @@ -942,11 +938,13 @@ export async function migrateCommand( // STAGE_ORDER sequences drop-view before and create-view after any column change // a view reads. There is no separate view-migration emitter, and unchanged views // produce no change (introspect reads the actual body, diff compares it). + if (diffResult.exprCanonPath !== undefined) { + notes.push(describeExprCanonPath(diffResult.exprCanonPath, diffResult.shadowUnavailable)); + } if (diffResult.changes.length === 0) { // no-op — output will say "No schema changes" } else { notes.push(...viewRecreateNotes(diffResult.changes)); - if (diffResult.exprCanonPath !== undefined) notes.push(describeExprCanonPath(diffResult.exprCanonPath)); let emitted: EmitResult | undefined; try { emitted = emit(diffResult.changes, { @@ -1084,6 +1082,7 @@ export async function migrateCommand( } } } finally { + await shadow?.close(); try { await kysely.close(); } catch (err) { @@ -1174,16 +1173,25 @@ async function withGeneratedSpelling( dialect: Dialect, metaRoot: string, genRoot: string, - shadow?: ShadowSchemaSource, -): Promise { + shadow: ShadowSchemaSource, +): Promise<{ snapshot: SchemaSnapshot; path?: ExprCanonPath; shadowUnavailable?: string }> { + let expected: SchemaSnapshot; try { const metadata = await loadBaselineMetadata(metaRoot, genRoot); - const expected = baselineFromMetadata(metadata, dialect, buildProjectionViews(metadata, { dialect })); - const shadowCatalog = shadow !== undefined ? await shadow(expected) : undefined; - const probe = shadowCatalog !== undefined ? canonProbeOverShadow(shadowCatalog) : postgresExprCanonProbe(db); - return await adoptGeneratedSpelling(snapshot, expected, probe); + expected = baselineFromMetadata(metadata, dialect, buildProjectionViews(metadata, { dialect })); + } catch { + return { snapshot }; + } + // An explicitly configured scratch database that cannot be used rejects here, and is fatal. + const answer = await shadow(expected); + const chosen = "catalog" in answer + ? { path: "shadow" as const, probe: canonProbeOverShadow(answer.catalog) } + : { path: "probe" as const, probe: postgresExprCanonProbe(db), shadowUnavailable: answer.unavailable }; + const why = chosen.shadowUnavailable !== undefined ? { shadowUnavailable: chosen.shadowUnavailable } : {}; + try { + return { snapshot: await adoptGeneratedSpelling(snapshot, expected, chosen.probe), path: chosen.path, ...why }; } catch { - return snapshot; + return { snapshot, path: chosen.path, ...why }; } } @@ -1225,14 +1233,20 @@ export async function runBaseline( snapshot = await introspect(kysely.db, kysely.dialect); if (config.format === "flyway") snapshot = withoutFlywayHistory(snapshot); if (kysely.dialect === "postgres") { - const shadowOpen = shadowOpener( - { url: config.shadowDatabaseUrl, disabled: config.shadowDisabled === true }, - { url: config.databaseUrl, handle: kysely }, - ); - snapshot = await withGeneratedSpelling( - snapshot, kysely.db, config.dialect, metaRoot, genRoot, - shadowOpen !== undefined ? postgresShadowSchema(shadowOpen) : undefined, - ); + const shadow = shadowSession(config.shadowDatabaseUrl, config.databaseUrl); + let adopted; + try { + adopted = await withGeneratedSpelling(snapshot, kysely.db, config.dialect, metaRoot, genRoot, shadow.schema); + } catch (err) { + log.error(`migrate baseline: ${describeError(err)}`); + return 2; + } finally { + await shadow.close(); + } + snapshot = adopted.snapshot; + if (adopted.path !== undefined) { + narrate(fmt, `migrate baseline: ${describeExprCanonPath(adopted.path, adopted.shadowUnavailable)}`); + } } } finally { await kysely.close(); diff --git a/server/typescript/packages/cli/src/commands/verify.ts b/server/typescript/packages/cli/src/commands/verify.ts index 9932846fdb..991aebeaea 100644 --- a/server/typescript/packages/cli/src/commands/verify.ts +++ b/server/typescript/packages/cli/src/commands/verify.ts @@ -46,8 +46,7 @@ import { lintNodeNames } from "../lib/name-lint.js"; import { lintDeprecatedReferences } from "../lib/deprecation-lint.js"; import { lintDuplicateFields, lintReferenceFields } from "../lib/field-lint.js"; import { FileSource } from "@metaobjectsdev/metadata/core"; -import { shadowOpener } from "../lib/shadow.js"; -import { describeExprCanonPath } from "../lib/shadow.js"; +import { describeExprCanonPath, shadowSession, type ShadowSession } from "../lib/shadow.js"; import { resolveD1Config, resolveMigrateConfig, type ResolvedMigrateConfig } from "../lib/config.js"; import { buildWranglerExecuteArgs, @@ -90,7 +89,7 @@ import { type ExprCanonProbe, postgresViewReplaceProbe, postgresExprCanonProbe, - postgresShadowSchema, + type ShadowSchemaSource, withoutFlywayHistory, } from "@metaobjectsdev/migrate-ts"; import { @@ -207,7 +206,7 @@ export async function verifyCommand( return 2; } const migrateFlags: MigrateFlags = { - ...EMPTY_MIGRATE_FLAGS, format: flags.migrationFormat, shadowDb: flags.shadowDb, noShadowDb: flags.noShadowDb === true, + ...EMPTY_MIGRATE_FLAGS, format: flags.migrationFormat, shadowDb: flags.shadowDb, }; // ADR-0021 D2 — explicit verify subverbs. Each flag selects one drift mode; @@ -1344,6 +1343,7 @@ export async function verifyCommand( return 1; } + let shadow: ShadowSession | undefined; try { const allow = tokensToAllowOptions(flags.allow); // Resolve the migrate config once for this whole path: `tryLoadConfig` is uncached, @@ -1356,14 +1356,12 @@ export async function verifyCommand( const expectedViews = buildProjectionViews(root, { dialect: kysely.dialect }); // One probe for the whole run, so a server that refuses scratch tables is asked once. const exprCanonProbe = kysely.dialect === "postgres" ? postgresExprCanonProbe(kysely.db) : undefined; - // ...but a scratch database answers first, when one can be opened (see lib/shadow.ts). - const shadowOpen = kysely.dialect === "postgres" - ? shadowOpener( - { url: migrateConfig.shadowDatabaseUrl, disabled: migrateConfig.shadowDisabled === true }, - { url: flags.db as string, handle: kysely }, - ) + // ...but a scratch database answers first, when one can be opened (see lib/shadow.ts): + // one for the run, shared by the drift diff and the committed-snapshot diff. + shadow = kysely.dialect === "postgres" + ? shadowSession(migrateConfig.shadowDatabaseUrl, flags.db as string) : undefined; - const shadowSchema = shadowOpen !== undefined ? postgresShadowSchema(shadowOpen) : undefined; + const shadowSchema = shadow?.schema; let driftResult; let actual: SchemaSnapshot; try { @@ -1394,15 +1392,24 @@ export async function verifyCommand( return 1; } - if (driftResult.exprCanonPath !== undefined) say(describeExprCanonPath(driftResult.exprCanonPath)); + if (driftResult.exprCanonPath !== undefined) { + say(describeExprCanonPath(driftResult.exprCanonPath, driftResult.shadowUnavailable)); + } - const snapshotDrift = - driftResult.changes.length === 0 - ? await checkCommittedSnapshot(actual, kysely.dialect, kysely.displayUrl, driftResult, migrateConfig, exprCanonProbe) + let snapshotDrift: string[]; + try { + snapshotDrift = driftResult.changes.length === 0 + ? await checkCommittedSnapshot(actual, kysely.dialect, kysely.displayUrl, driftResult, migrateConfig, exprCanonProbe, shadowSchema) : []; + } catch (err) { + schemaRunError = describeError(err); + log.error(`verify: ${schemaRunError}`); + return 1; + } return reportSchemaDrift(driftResult, [...ledgerDrift, ...snapshotDrift], kysely.displayUrl); } finally { + await shadow?.close(); try { await kysely.close(); } catch (err) { @@ -1528,6 +1535,8 @@ export async function verifyCommand( // Postgres: the snapshot may hold either spelling of an expression (generated, or // Postgres's own from a `baseline --from-db`); the database settles which is equal. exprCanonProbe?: ExprCanonProbe, + // ...and a scratch database, when the run has one, settles it first (the probe is the fallback). + shadowSchema?: ShadowSchemaSource, ): Promise { if (dialect === "d1") return []; // d1 keeps migrations Wrangler-native; no offline snapshot // The migrations dir comes from migrate's OWN precedence (flag > config > default) @@ -1567,6 +1576,7 @@ export async function verifyCommand( // MERGED with the out-of-scope set, and a second key would silently drop that half. dialect, ...(exprCanonProbe !== undefined ? { exprCanonProbe } : {}), + ...(shadowSchema !== undefined ? { shadowSchema } : {}), }); // A view recreated only around a table change matches the snapshot (D3) — not a difference. const changes = withoutViewRecreates(result.changes); @@ -1870,6 +1880,8 @@ const DRIFT_PRESENTATION: Record Directory of provider-resolved template text (default: prompts) --dialect sqlite|postgres|d1 Optional override (auto-detected from --db URL scheme) --allow Accepted for parity with 'migrate'; does NOT affect the @@ -172,7 +171,6 @@ MIGRATE FLAGS: Default: CREATE DATABASE on the --db server (dropped afterwards), else the temp-object probe on the target. Also SHADOW_DATABASE_URL or migrate.shadowDatabaseUrl in .metaobjects/config.json. - --no-shadow-db Never open a scratch database (use the probe on the target instead). --dialect sqlite|postgres|d1 Optional override (auto-detected from URL scheme) --out-dir Migration directory (default: ./.metaobjects/migrations) --slug Required when changes are present (e.g., add-user-shipping) @@ -349,7 +347,6 @@ FLAGS: Default: CREATE DATABASE on the --db server (dropped afterwards), else the temp-object probe on the target. Also SHADOW_DATABASE_URL or migrate.shadowDatabaseUrl in .metaobjects/config.json. - --no-shadow-db Never open a scratch database (use the probe on the target instead). --replay Migration-chain drift — replay the committed chain into an EMPTY throwaway database and assert it applies. Needs no --db: the engine is in-process (PGlite for postgres, a temp file for diff --git a/server/typescript/packages/cli/src/lib/allow.ts b/server/typescript/packages/cli/src/lib/allow.ts index 24e5ad8dfe..b30f22f908 100644 --- a/server/typescript/packages/cli/src/lib/allow.ts +++ b/server/typescript/packages/cli/src/lib/allow.ts @@ -75,6 +75,8 @@ export function describeChange(c: Change): string { } case "change-column-nullable": return `${c.table}.${c.column} (${c.from ? "NULL" : "NOT NULL"} → ${c.to ? "NULL" : "NOT NULL"})`; case "change-column-default": return `${c.table}.${c.column}`; + case "change-column-identity": return `${c.table}.${c.column} (identity ${c.from ?? "none"} → ${c.to ?? "none"})`; + case "change-comment": return `${c.column !== undefined ? `${c.table}.${c.column}` : c.table} (comment)`; case "add-index": return `${c.table} idx ${c.index.name}`; case "drop-index": return `${c.table} idx ${c.index}`; case "add-fk": return `${c.table} fk ${c.fk.name}`; diff --git a/server/typescript/packages/cli/src/lib/args.ts b/server/typescript/packages/cli/src/lib/args.ts index a068e2fda6..3f43b21544 100644 --- a/server/typescript/packages/cli/src/lib/args.ts +++ b/server/typescript/packages/cli/src/lib/args.ts @@ -346,8 +346,6 @@ export interface VerifyFlags { db: string | undefined; /** Postgres scratch database for the drift comparison (see `MigrateFlags.shadowDb`). */ shadowDb?: string | undefined; - /** Never use a scratch database (see `MigrateFlags.noShadowDb`). */ - noShadowDb?: boolean; /** Optional dialect override (auto-detected from --db URL scheme otherwise). */ dialect: Dialect | undefined; /** Destructive-change permissions; only affects how drift is described. */ @@ -501,7 +499,6 @@ export const VERIFY_OPTIONS = { prompts: { type: "string" }, db: { type: "string" }, "shadow-db": { type: "string" }, - "no-shadow-db": { type: "boolean", default: false }, dialect: { type: "string" }, allow: { type: "string", multiple: true }, "skip-schema": { type: "boolean", default: false }, @@ -593,7 +590,6 @@ export function parseVerifyArgs(argv: string[]): VerifyFlags { prompts: values.prompts, db: values.db as string | undefined, ...(values["shadow-db"] !== undefined ? { shadowDb: values["shadow-db"] as string } : {}), - ...(values["no-shadow-db"] ? { noShadowDb: true } : {}), dialect: dialect as Dialect | undefined, allow: allowTokens as AllowToken[], skipSchema: !!values["skip-schema"], @@ -686,8 +682,6 @@ export interface MigrateFlags { fromDb: boolean; /** `--shadow-db `: the Postgres scratch database the expected schema is applied to. */ shadowDb?: string | undefined; - /** `--no-shadow-db`: never use a scratch database (not even a CREATE DATABASE one). */ - noShadowDb?: boolean; /** `migrate baseline` subcommand: seed the snapshot, emit no migration. */ baseline: boolean; /** `migrate apply-pending` subcommand: replay committed migration files, no diff. */ @@ -708,7 +702,6 @@ export const MIGRATE_OPTIONS = { "dry-run": { type: "boolean", default: false }, "from-db": { type: "boolean", default: false }, "shadow-db": { type: "string" }, - "no-shadow-db": { type: "boolean", default: false }, "d1": { type: "string" }, "remote": { type: "boolean", default: false }, "apply": { type: "boolean", default: false }, @@ -766,7 +759,6 @@ export function parseMigrateArgs(argv: string[]): MigrateFlags { yes: !!values.yes, fromDb: !!values["from-db"], ...(values["shadow-db"] !== undefined ? { shadowDb: values["shadow-db"] as string } : {}), - ...(values["no-shadow-db"] ? { noShadowDb: true } : {}), baseline, applyPending, }; diff --git a/server/typescript/packages/cli/src/lib/config.ts b/server/typescript/packages/cli/src/lib/config.ts index 420f4b1e45..2ca74b01e0 100644 --- a/server/typescript/packages/cli/src/lib/config.ts +++ b/server/typescript/packages/cli/src/lib/config.ts @@ -41,8 +41,6 @@ export interface ResolvedMigrateConfig { databaseUrl: string | undefined; /** Postgres: an explicit scratch database URL (flag > `SHADOW_DATABASE_URL` > config). */ shadowDatabaseUrl?: string | undefined; - /** `--no-shadow-db`: never open a scratch database. */ - shadowDisabled?: boolean; dialect: Dialect | undefined; /** Output-format adapter (#192): "default" homegrown layout, or "flyway" V__/U__. */ format: MigrateFormat; @@ -142,7 +140,6 @@ export async function resolveMigrateConfig( outDir: flags.outDir ?? cfgBlock.outDir ?? MIGRATE_DEFAULTS.outDir, databaseUrl: flags.db ?? envUrl ?? cfgBlock.databaseUrl ?? MIGRATE_DEFAULTS.databaseUrl, shadowDatabaseUrl: flags.shadowDb ?? process.env.SHADOW_DATABASE_URL ?? cfgBlock.shadowDatabaseUrl, - shadowDisabled: flags.noShadowDb === true, dialect: flags.dialect ?? cfgBlock.dialect ?? MIGRATE_DEFAULTS.dialect, format: flags.format ?? cfgBlock.format ?? MIGRATE_DEFAULTS.format, onAmbiguous: flags.onAmbiguous ?? cfgBlock.onAmbiguous ?? MIGRATE_DEFAULTS.onAmbiguous, diff --git a/server/typescript/packages/cli/src/lib/output.ts b/server/typescript/packages/cli/src/lib/output.ts index e16167b842..7e622a1b58 100644 --- a/server/typescript/packages/cli/src/lib/output.ts +++ b/server/typescript/packages/cli/src/lib/output.ts @@ -218,7 +218,8 @@ export function formatMigrateResult(result: MigrateResultShape, _opts: FormatOpt const changeEntries = Object.entries(result.changeCounts).filter(([, v]) => v > 0); if (changeEntries.length === 0 && result.blocked.length === 0 && result.ambiguous.length === 0) { - return `${header}\n\n No schema changes.\n`; + const noteLines = (result.notes ?? []).map((n) => ` Note: ${n}\n`).join(""); + return `${header}\n\n No schema changes.\n${noteLines}`; } if (changeEntries.length > 0) { diff --git a/server/typescript/packages/cli/src/lib/shadow.ts b/server/typescript/packages/cli/src/lib/shadow.ts index c319bb6740..d639398f55 100644 --- a/server/typescript/packages/cli/src/lib/shadow.ts +++ b/server/typescript/packages/cli/src/lib/shadow.ts @@ -1,27 +1,20 @@ // shadow.ts — where the Postgres scratch (shadow) database comes from. // -// `meta verify --db` and `meta migrate --from-db` apply the model's expected schema to a -// scratch database and compare it with the live one in Postgres's own spelling -// (migrate-ts shadow-canon.ts). Three sources, in order: +// `meta verify --db`, `meta migrate --from-db` and `meta migrate baseline --from-db` apply the +// model's expected schema to a scratch database and compare it with the live one in +// Postgres's own spelling (migrate-ts shadow-canon.ts). Sources, in order: // 1. an explicit URL (`--shadow-db`, `SHADOW_DATABASE_URL`, `migrate.shadowDatabaseUrl`), // used as given and never dropped (the expected schema is applied in a transaction that -// is rolled back, so nothing is left in it); +// is rolled back, so nothing is left in it). One that cannot be used is an error; // 2. a database CREATEd on the target server when the role may, named per run and always // dropped afterwards, including on error; -// 3. none: the caller falls back to the temp-object probe. -// `--no-shadow-db` skips straight to 3. +// 3. none: the caller falls back to the temp-object probe, and says why. +// A run opens its scratch database once, however many comparisons it makes. import { randomBytes } from "node:crypto"; import { sql } from "kysely"; -import type { ShadowHandle, ShadowOpener } from "@metaobjectsdev/migrate-ts"; -import { buildKyselyFromUrl, type KyselyHandle } from "./kysely.js"; - -export interface ShadowSettings { - /** Explicit scratch database URL, or undefined to create one on the target server. */ - url: string | undefined; - /** `--no-shadow-db`. */ - disabled: boolean; -} +import { postgresShadowSchema, type ExprCanonPath, type ShadowHandle, type ShadowOpener, type ShadowSchemaSource } from "@metaobjectsdev/migrate-ts"; +import { buildKyselyFromUrl, redactUrl, type KyselyHandle } from "./kysely.js"; /** The URL of database `name` on the same server as `targetUrl` (same credentials and options). */ export function urlForDatabase(targetUrl: string, name: string): string { @@ -32,37 +25,60 @@ export function urlForDatabase(targetUrl: string, name: string): string { const quoteIdent = (s: string): string => `"${s.replace(/"/g, '""')}"`; +const messageOf = (err: unknown): string => (err instanceof Error ? err.message : String(err)); + +/** Connect to `url`, proving the connection with a round trip (a Kysely pool connects lazily). */ +async function connect(url: string): Promise { + const h = await buildKyselyFromUrl(url, "postgres"); + try { + await sql`SELECT 1`.execute(h.db); + } catch (err) { + try { await h.close(); } catch { /* the connection error is what matters */ } + throw err; + } + return h; +} + /** - * The opener for a Postgres target, or `undefined` when scratch databases are switched off. - * `target` is the already-open connection to the live database, used for CREATE/DROP DATABASE. + * Opens the scratch database for a Postgres target. `url` is the explicit setting, if any; + * `targetUrl` is the live database, on whose server a database is CREATEd otherwise. CREATE + * and DROP DATABASE run on a connection of the opener's own, so the drop does not depend on + * the caller's connection to the target still being open. */ -export function shadowOpener( - settings: ShadowSettings, - target: { url: string; handle: KyselyHandle }, -): ShadowOpener | undefined { - if (settings.disabled) return undefined; - if (settings.url !== undefined) { - const explicit = settings.url; +export function shadowOpener(url: string | undefined, targetUrl: string): ShadowOpener { + if (url !== undefined) { return async (): Promise => { - const h = await buildKyselyFromUrl(explicit, "postgres"); + let h: KyselyHandle; + try { + h = await connect(url); + } catch (err) { + throw new Error(`${redactUrl(url)}: ${messageOf(err)}`); + } return { db: h.db, close: async () => { await h.close(); } }; }; } return async (): Promise => { const name = `mo_shadow_${randomBytes(6).toString("hex")}`; - await sql.raw(`CREATE DATABASE ${quoteIdent(name)}`).execute(target.handle.db); + const admin = await connect(targetUrl); + try { + await sql.raw(`CREATE DATABASE ${quoteIdent(name)}`).execute(admin.db); + } catch (err) { + try { await admin.close(); } catch { /* the refusal is what matters */ } + throw new Error(`CREATE DATABASE on the --db server failed: ${messageOf(err)}`); + } let h: KyselyHandle | undefined; const drop = async (): Promise => { try { await h?.close(); } catch { /* the drop below is what matters */ } try { - await sql.raw(`DROP DATABASE IF EXISTS ${quoteIdent(name)} WITH (FORCE)`).execute(target.handle.db); + await sql.raw(`DROP DATABASE IF EXISTS ${quoteIdent(name)} WITH (FORCE)`).execute(admin.db); } catch { // Before Postgres 13 there is no WITH (FORCE); our own connection is closed by now. - try { await sql.raw(`DROP DATABASE IF EXISTS ${quoteIdent(name)}`).execute(target.handle.db); } catch { /* best effort */ } + try { await sql.raw(`DROP DATABASE IF EXISTS ${quoteIdent(name)}`).execute(admin.db); } catch { /* best effort */ } } + try { await admin.close(); } catch { /* the database is gone; nothing else to release */ } }; try { - h = await buildKyselyFromUrl(urlForDatabase(target.url, name), "postgres"); + h = await connect(urlForDatabase(targetUrl, name)); } catch (err) { await drop(); throw err; @@ -72,16 +88,41 @@ export function shadowOpener( }; } -/** One line saying which comparison decided CHECK, index-expression and column-default equality. */ -export function describeExprCanonPath(path: "shadow" | "probe" | "normaliser"): string { +/** One run's scratch database: opened on first use, shared by every comparison, released by `close`. */ +export interface ShadowSession { + schema: ShadowSchemaSource; + /** Drop the database if this run created it. Never throws. */ + close(): Promise; +} + +export function shadowSession(url: string | undefined, targetUrl: string): ShadowSession { + const open = shadowOpener(url, targetUrl); + let opened: Promise | undefined; + const shared: ShadowOpener = async () => { + opened ??= open(); + const h = await opened; + return { db: h.db, close: async () => {} }; + }; + return { + schema: postgresShadowSchema(shared, { required: url !== undefined }), + close: async () => { + if (opened === undefined) return; + try { await (await opened).close(); } catch { /* it never opened */ } + }, + }; +} + +/** One line saying which comparison decided Postgres drift, and why a scratch database was not used. */ +export function describeExprCanonPath(path: ExprCanonPath, shadowUnavailable?: string): string { + const why = shadowUnavailable !== undefined ? ` The scratch database was not used: ${shadowUnavailable}.` : ""; switch (path) { case "shadow": - return "Postgres expressions compared against a scratch (shadow) database."; + return "Postgres drift compared against a scratch (shadow) database."; case "probe": return "Postgres expressions compared via the temp-object probe on the target database (no scratch database was " - + "available): it needs write privileges and takes brief locks on the target. Pass --shadow-db to avoid that."; + + `available): it needs write privileges and takes brief locks on the target. Pass --shadow-db to avoid that.${why}`; case "normaliser": return "Postgres expressions compared as text (no scratch database or probe was available); a re-spelled " - + "CHECK, index expression or default may read as drift."; + + `CHECK, index expression or default may read as drift.${why}`; } } diff --git a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap index 73334aacda..2192448bab 100644 --- a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap +++ b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap @@ -104,7 +104,6 @@ VERIFY FLAGS (ADR-0021 D2 — explicit subverbs; combine any; exit 1 on ANY drif Default: CREATE DATABASE on the --db server (dropped afterwards), else the temp-object probe on the target. Also SHADOW_DATABASE_URL or migrate.shadowDatabaseUrl in .metaobjects/config.json. - --no-shadow-db Never open a scratch database (use the probe on the target instead). --prompts Directory of provider-resolved template text (default: prompts) --dialect sqlite|postgres|d1 Optional override (auto-detected from --db URL scheme) --allow Accepted for parity with 'migrate'; does NOT affect the @@ -152,7 +151,6 @@ MIGRATE FLAGS: Default: CREATE DATABASE on the --db server (dropped afterwards), else the temp-object probe on the target. Also SHADOW_DATABASE_URL or migrate.shadowDatabaseUrl in .metaobjects/config.json. - --no-shadow-db Never open a scratch database (use the probe on the target instead). --dialect sqlite|postgres|d1 Optional override (auto-detected from URL scheme) --out-dir Migration directory (default: ./.metaobjects/migrations) --slug Required when changes are present (e.g., add-user-shipping) diff --git a/server/typescript/packages/cli/test/shadow-opener.test.ts b/server/typescript/packages/cli/test/shadow-opener.test.ts index 7a05f19ed1..4d5a61eb78 100644 --- a/server/typescript/packages/cli/test/shadow-opener.test.ts +++ b/server/typescript/packages/cli/test/shadow-opener.test.ts @@ -1,23 +1,23 @@ /** * Where the Postgres scratch database comes from: an explicit URL, else a database CREATEd on - * the target server and always dropped, else none. The live-server cases skip gracefully when - * MIGRATE_TS_PG_URL is not set. + * the target server and always dropped, else none. A run opens it once however many + * comparisons it makes, and an explicit one that cannot be used is an error. The live-server + * cases skip gracefully when MIGRATE_TS_PG_URL is not set. */ import { describe, expect, test } from "bun:test"; import { sql } from "kysely"; +import type { SchemaSnapshot } from "@metaobjectsdev/migrate-ts"; import { parseMigrateArgs, parseVerifyArgs } from "../src/lib/args.js"; import { buildKyselyFromUrl } from "../src/lib/kysely.js"; -import { describeExprCanonPath, shadowOpener, urlForDatabase } from "../src/lib/shadow.js"; +import { describeExprCanonPath, shadowOpener, shadowSession, urlForDatabase } from "../src/lib/shadow.js"; const PG_URL = process.env["MIGRATE_TS_PG_URL"]; describe("shadow flags", () => { - test("--shadow-db and --no-shadow-db parse on migrate and verify", () => { + test("--shadow-db parses on migrate and verify", () => { expect(parseMigrateArgs(["--shadow-db", "postgres://h/s"]).shadowDb).toBe("postgres://h/s"); - expect(parseMigrateArgs(["--no-shadow-db"]).noShadowDb).toBe(true); const v = parseVerifyArgs(["--db", "postgres://h/d", "--shadow-db", "postgres://h/s"]); expect(v.shadowDb).toBe("postgres://h/s"); - expect(parseVerifyArgs(["--db", "postgres://h/d", "--no-shadow-db"]).noShadowDb).toBe(true); }); }); @@ -26,13 +26,12 @@ describe("shadowOpener", () => { expect(urlForDatabase("postgres://u:p@h:5433/live?sslmode=require", "mo_shadow_ab")) .toBe("postgres://u:p@h:5433/mo_shadow_ab?sslmode=require"); }); - test("disabled opens nothing", () => { - expect(shadowOpener({ url: undefined, disabled: true }, { url: "postgres://h/d", handle: {} as never })).toBeUndefined(); - }); - test("each source names itself", () => { + test("each source names itself, and a fallback says why the scratch database was not used", () => { expect(describeExprCanonPath("shadow")).toContain("scratch"); expect(describeExprCanonPath("probe")).toContain("write privileges"); expect(describeExprCanonPath("normaliser")).toContain("as text"); + expect(describeExprCanonPath("probe", "could not be opened: permission denied to create database")) + .toContain("The scratch database was not used: could not be opened: permission denied to create database."); }); if (!PG_URL) { @@ -42,13 +41,13 @@ describe("shadowOpener", () => { const shadowDbs = async (target: Awaited>): Promise => (await sql<{ datname: string }>`SELECT datname FROM pg_database WHERE datname LIKE 'mo_shadow_%'`.execute(target.db)) .rows.map((r) => r.datname); + const EMPTY: SchemaSnapshot = { tables: [], views: [] }; test("without a URL it CREATEs a database on the target and drops it on close", async () => { const target = await buildKyselyFromUrl(PG_URL, "postgres"); try { const before = await shadowDbs(target); - const open = shadowOpener({ url: undefined, disabled: false }, { url: PG_URL, handle: target })!; - const handle = (await open())!; + const handle = await shadowOpener(undefined, PG_URL)(); expect((await shadowDbs(target)).length).toBe(before.length + 1); await sql`SELECT 1`.execute(handle.db); await handle.close(); @@ -61,8 +60,7 @@ describe("shadowOpener", () => { test("an explicit URL is opened as given and never dropped", async () => { const target = await buildKyselyFromUrl(PG_URL, "postgres"); try { - const open = shadowOpener({ url: PG_URL, disabled: false }, { url: PG_URL, handle: target })!; - const handle = (await open())!; + const handle = await shadowOpener(PG_URL, PG_URL)(); await handle.close(); // The explicit database is still there: the target itself answers. await sql`SELECT 1`.execute(target.db); @@ -70,4 +68,34 @@ describe("shadowOpener", () => { await target.close(); } }); + + test("a session creates one database for every comparison of the run, and drops it on close", async () => { + const target = await buildKyselyFromUrl(PG_URL, "postgres"); + try { + const before = await shadowDbs(target); + const session = shadowSession(undefined, PG_URL); + expect("catalog" in (await session.schema(EMPTY))).toBe(true); + expect("catalog" in (await session.schema(EMPTY))).toBe(true); + expect((await shadowDbs(target)).length).toBe(before.length + 1); + await session.close(); + expect(await shadowDbs(target)).toEqual(before); + } finally { + await target.close(); + } + }); + + test("an explicit URL that cannot be opened is an error naming it, without its password", async () => { + const target = await buildKyselyFromUrl(PG_URL, "postgres"); + try { + const bad = urlForDatabase(PG_URL, "mo_no_such_shadow_db"); + const session = shadowSession(bad, PG_URL); + const err = await session.schema(EMPTY).then(() => undefined, (e: unknown) => e as Error); + expect(err?.message).toMatch(/^shadow database: could not be opened: .*mo_no_such_shadow_db.*: .*does not exist/); + const password = new URL(PG_URL).password; + if (password !== "") expect(err?.message).not.toContain(password); + await session.close(); + } finally { + await target.close(); + } + }); }); diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index 1c3b9f45d4..c1cd4a1575 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -169,9 +169,10 @@ export async function diff( ? argsOrExpected : { expected: argsOrExpected, actual: actualMaybe!, ...(optsMaybe ?? {}) }; // Postgres, primary path: the expected side becomes the shadow database's own catalog. - const shadow = given.dialect === "postgres" && given.shadowSchema !== undefined + const shadowAnswer = given.dialect === "postgres" && given.shadowSchema !== undefined ? await given.shadowSchema(given.expected) : undefined; + const shadow = shadowAnswer !== undefined && "catalog" in shadowAnswer ? shadowAnswer.catalog : undefined; const args: DiffArgs = shadow !== undefined ? { ...given, expected: expectedFromShadow(shadow, given.expected) } : given; const changes: Change[] = []; @@ -273,7 +274,8 @@ export async function diff( if (shadow !== undefined) { canonProbe = shadowIdentityProbe; exprCanonPath = "shadow"; } else if (args.exprCanonProbe !== undefined) { canonProbe = args.exprCanonProbe; exprCanonPath = "probe"; } } - const canon = await probeExprCanon(canonProbe, args.dialect, changes, expectedTables, actualTables); + const probed = await probeExprCanon(canonProbe, args.dialect, changes, expectedTables, actualTables); + const canon: ExprCanon = shadow !== undefined ? { ...probed, catalog: true } : probed; const { renamed, predecessors } = diffRenamedTables(changes, expectedTables, actualTables, args.dialect, canon); // Pass 2: tables in both → compare columns/indexes/FKs @@ -292,7 +294,10 @@ export async function diff( const actualViewsInScope = args.actual.views.filter( (v) => inScope(v.schema) && !unmanaged.has(viewIdentity(v)), ); - await diffViews(expectedViewsInScope, actualViewsInScope, changes, args.dialect, args.viewReplaceProbe); + // On the shadow path a view's body is Postgres's deparse of the shadow's copy, compared + // with the deparse of the live one. + const catalogBodies = new Map((shadow?.views ?? []).flatMap((v) => (v.sql !== undefined ? [[viewIdentity(v), v.sql] as const] : []))); + await diffViews(expectedViewsInScope, actualViewsInScope, changes, args.dialect, args.viewReplaceProbe, catalogBodies); // Pass 2c: a change that disturbs the table under a view forces the view to be // dropped before and recreated after — postgres blocks ALTER on a column a view @@ -352,6 +357,7 @@ export async function diff( blocked: changes.filter((c) => c.status.state === "blocked"), hazards: changes.flatMap((c) => dataHazardOf(c) ?? []), ...(args.dialect === "postgres" ? { exprCanonPath } : {}), + ...(shadowAnswer !== undefined && "unavailable" in shadowAnswer ? { shadowUnavailable: shadowAnswer.unavailable } : {}), }; } @@ -485,6 +491,13 @@ function diffTablePair( canon: ExprCanon, ): void { diffTableColumns(expected, actual, changes, dialect, canon); + if (canon.catalog === true && expected.description !== actual.description) { + changes.push({ + kind: "change-comment", table: expected.name, ...schemaSpread(expected.schema), status: ALLOWED, + ...(actual.description !== undefined ? { from: actual.description } : {}), + ...(expected.description !== undefined ? { to: expected.description } : {}), + }); + } diffTableIndexes(expected, actual, changes, canon); diffTableForeignKeys(expected, actual, changes, dialect); // CHECK constraints on existing tables are evolved whenever a dialect is @@ -825,6 +838,25 @@ function diffTableColumns( }; changes.push(change); } + if (canon.catalog === true) { + // Both sides read `increment` off a GENERATED identity (attidentity) or a legacy serial's + // nextval default; a serial losing its sequence is the default change above, not this. + const from = ac.identity === "increment" ? "increment" : undefined; + const to = ec.identity === "increment" ? "increment" : undefined; + if (from !== to && !(from !== undefined && liveIsAutoSequenceDefault)) { + changes.push({ + kind: "change-column-identity", table, ...sx, column: name, status: ALLOWED, + ...(from !== undefined ? { from } : {}), ...(to !== undefined ? { to } : {}), + }); + } + if (ec.description !== ac.description) { + changes.push({ + kind: "change-comment", table, ...sx, column: name, status: ALLOWED, + ...(ac.description !== undefined ? { from: ac.description } : {}), + ...(ec.description !== undefined ? { to: ec.description } : {}), + }); + } + } } for (const [name, ac] of actualCols) { if (!expectedCols.has(name)) { @@ -1043,6 +1075,7 @@ async function diffViews( expected: ViewDescriptor[], actual: ViewDescriptor[], changes: Change[], dialect: Dialect | undefined, probe: ViewReplaceProbe | undefined, + catalogBodies: ReadonlyMap = new Map(), ): Promise { const exp = new Map(expected.map((v) => [viewIdentity(v), v] as const)); const act = new Map(actual.map((v) => [viewIdentity(v), v] as const)); @@ -1057,6 +1090,17 @@ async function diffViews( } if (dialect === "postgres") { + // Shadow path: the view applied to the scratch database, so Postgres's deparse of it + // is compared with its deparse of the live one, catalog to catalog. + const body = catalogBodies.get(id); + if (body !== undefined) { + if (a.sql === body) continue; + const firstDifference = a.sql !== undefined ? firstViewSqlDifference(body, a.sql) : undefined; + await pushViewUpdate(v, a, changes, probe, a.fingerprint === undefined, { + kind: "definition", compared: "text", ...(firstDifference !== undefined ? { firstDifference } : {}), + }); + continue; + } // No expected fingerprint (an unresolvable projection body) → we cannot prove a // change. Leave it alone rather than propose a spurious replace. if (v.fingerprint === undefined) continue; @@ -1133,13 +1177,15 @@ async function pushViewUpdate( // stamp the adopt-view gate (`unmanagedActual`) onto whichever change we emit so // status.ts fails closed on `allow.adoptView`, legal-replace and drop+create alike. unmanaged = false, + // Shadow path: the deparsed bodies were compared, and this says where they differ. + catalogReason?: ViewChangeReason, ): Promise { const sx = schemaSpread(expected.schema); const adopt = unmanaged ? { unmanagedActual: true as const } : {}; - // Postgres compares fingerprints, never text: an unstamped view cannot be compared at all. + // Off the shadow path Postgres compares fingerprints, never text: an unstamped view cannot be compared at all. const reason: ViewChangeReason = unmanaged ? { kind: "unfingerprinted" } - : { kind: "definition", compared: "fingerprint" }; + : catalogReason ?? { kind: "definition", compared: "fingerprint" }; const evidence = probe === undefined || planMayChangeWhatBodyReads(expected, actual, changes) ? undefined : await probe(expected, actual); diff --git a/server/typescript/packages/migrate-ts/src/diff/status.ts b/server/typescript/packages/migrate-ts/src/diff/status.ts index a545e6b61d..0e1b1712c5 100644 --- a/server/typescript/packages/migrate-ts/src/diff/status.ts +++ b/server/typescript/packages/migrate-ts/src/diff/status.ts @@ -161,6 +161,18 @@ function blockFor( return autoSequenceDropRefusal(c.table, c.column, c.from, c.to, allow); } + case "change-column-identity": + // Dropping an identity leaves the column with no generation mechanism: every insert + // that omits it starts failing, the same hazard as dropping an auto-sequence default. + if (c.to === "increment") return null; + return gate(allow, "dropIdentityDefault", `column "${c.table}"."${c.column}" is a live Postgres identity column ` + + `but its metadata declares no @generation. Dropping the identity is destructive (every insert that omits ` + + `the column starts failing). Declare @generation: increment on the identity to keep it, or pass ` + + `--allow drop-identity-default if removing it is intentional`); + + case "change-comment": + return null; + // Always-allowed kinds case "create-table": case "rename-table": diff --git a/server/typescript/packages/migrate-ts/src/emit/postgres.ts b/server/typescript/packages/migrate-ts/src/emit/postgres.ts index a0346aacc9..4f367f9344 100644 --- a/server/typescript/packages/migrate-ts/src/emit/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/emit/postgres.ts @@ -42,7 +42,9 @@ const STAGE_ORDER: Record = { "drop-index": 1.5, "add-column": 2, "drop-column": 2, "change-column-type": 2, "change-column-nullable": 2, "change-column-default": 2, + "change-column-identity": 2, "rename-column": 3, + "change-comment": 3.5, "add-index": 4, "add-fk": 5, "add-check": 5, @@ -244,6 +246,8 @@ function renderUp(c: Change, recreated: ReadonlySet = new Set()): string return c.to !== undefined ? `ALTER TABLE ${quoteQualified(c.table, c.schema)} ALTER COLUMN ${quote(c.column)} SET DEFAULT ${renderDefault(c.to)};` : `ALTER TABLE ${quoteQualified(c.table, c.schema)} ALTER COLUMN ${quote(c.column)} DROP DEFAULT;`; + case "change-column-identity": return renderIdentity(c.table, c.schema, c.column, c.to); + case "change-comment": return renderComment(c.table, c.schema, c.column, c.to); case "add-index": return renderCreateIndex(c.table, c.schema, c.index); // #285: an index OWNED by a UNIQUE / PRIMARY KEY / EXCLUDE constraint cannot be // dropped with DROP INDEX — Postgres refuses with "cannot drop index X because @@ -371,6 +375,8 @@ function renderDown(c: Change, recreated: ReadonlySet = new Set()): stri return c.from !== undefined ? `ALTER TABLE ${quoteQualified(c.table, c.schema)} ALTER COLUMN ${quote(c.column)} SET DEFAULT ${renderDefault(c.from)};` : `ALTER TABLE ${quoteQualified(c.table, c.schema)} ALTER COLUMN ${quote(c.column)} DROP DEFAULT;`; + case "change-column-identity": return renderIdentity(c.table, c.schema, c.column, c.from); + case "change-comment": return renderComment(c.table, c.schema, c.column, c.from); case "add-index": return `DROP INDEX ${quoteIndexQualified(c.index.name, c.schema)};`; case "drop-index": // #285: the up dropped a CONSTRAINT, so the down must add one back — recreating a @@ -449,6 +455,19 @@ function renderTableComments(t: TableDescriptor): string[] { return out; } +function renderIdentity(table: string, schema: string | undefined, column: string, to: "increment" | undefined): string { + const alter = `ALTER TABLE ${quoteQualified(table, schema)} ALTER COLUMN ${quote(column)}`; + return to === "increment" ? `${alter} ADD GENERATED BY DEFAULT AS IDENTITY;` : `${alter} DROP IDENTITY IF EXISTS;`; +} + +function renderComment(table: string, schema: string | undefined, column: string | undefined, to: string | undefined): string { + if (column !== undefined && to !== undefined) return columnCommentSql(table, schema, column, to); + const target = column !== undefined + ? `COLUMN ${quoteQualified(table, schema)}.${quote(column)}` + : `TABLE ${quoteQualified(table, schema)}`; + return `COMMENT ON ${target} IS ${to === undefined ? "NULL" : `'${pgEscape(to)}'`};`; +} + function columnCommentSql( table: string, schema: string | undefined, @@ -486,8 +505,10 @@ function renderColumn(c: ColumnDescriptor): string { if (c.identity === "increment") s += " GENERATED BY DEFAULT AS IDENTITY"; if (c.identity === "uuid") s += " DEFAULT gen_random_uuid()"; s += c.nullable ? "" : " NOT NULL"; - if (c.default !== undefined && c.identity !== "uuid") { - // For uuid identity we already set DEFAULT gen_random_uuid(); don't duplicate. + if (c.default !== undefined && c.identity === undefined) { + // For uuid identity we already set DEFAULT gen_random_uuid(); don't duplicate. An + // increment identity takes none: Postgres refuses a DEFAULT beside GENERATED, and the + // one a legacy serial column introspects with is the sequence the identity replaces. s += ` DEFAULT ${renderDefault(c.default)}`; } return s; diff --git a/server/typescript/packages/migrate-ts/src/emit/sqlite.ts b/server/typescript/packages/migrate-ts/src/emit/sqlite.ts index 4cafa87b52..afeeb6811e 100644 --- a/server/typescript/packages/migrate-ts/src/emit/sqlite.ts +++ b/server/typescript/packages/migrate-ts/src/emit/sqlite.ts @@ -46,6 +46,7 @@ const STAGE_ORDER: Record = { "drop-index": 1.5, "add-column": 2, "drop-column": 2, "change-column-type": 2, "change-column-nullable": 2, "change-column-default": 2, + "change-column-identity": 2, "change-comment": 2, "rename-column": 3, "add-index": 4, "add-fk": 5, @@ -408,6 +409,10 @@ function renderUpNative(c: Change): string { // (checks are create-time-only inline on SQLite, so a check change is a // recreate-triggering kind like the others). throw new Error(`renderUpNative: ${c.kind} should have been handled by recreate bundler`); + case "change-column-identity": + case "change-comment": + // Planned only on the Postgres shadow path; SQLite has neither GENERATED identity nor COMMENT. + throw new Error(`renderUpNative: ${c.kind} is a Postgres-only change`); // SQLite has no schema namespacing for views and no CREATE OR REPLACE VIEW; // a replace is DROP + CREATE. The view body lives in ViewDescriptor.sql. case "create-view": return renderCreateView(c.view); @@ -452,6 +457,10 @@ function renderDownNative(c: Change): string { // (checks are create-time-only inline on SQLite, so a check change is a // recreate-triggering kind like the others). throw new Error(`renderDownNative: ${c.kind} should have been handled by recreate bundler`); + case "change-column-identity": + case "change-comment": + // Planned only on the Postgres shadow path; SQLite has neither GENERATED identity nor COMMENT. + throw new Error(`renderDownNative: ${c.kind} is a Postgres-only change`); case "create-view": return `DROP VIEW IF EXISTS ${quote(c.view.name)};`; // The view as the DB held it (`restore`) is a valid restore payload: a view dropped // around a table rebuild must come back on rollback, or the schema is not what it was. diff --git a/server/typescript/packages/migrate-ts/src/errors.ts b/server/typescript/packages/migrate-ts/src/errors.ts index 9e0d8fb77d..06d51a947a 100644 --- a/server/typescript/packages/migrate-ts/src/errors.ts +++ b/server/typescript/packages/migrate-ts/src/errors.ts @@ -47,6 +47,7 @@ const ALLOW_OPTION_BY_KIND: Partial> = { // Postgres auto-sequence default with no declared @generation (see // diff/status.ts) — and allow.dropIdentityDefault is the flag that enables it. "change-column-default": "dropIdentityDefault", + "change-column-identity": "dropIdentityDefault", }; /** @@ -73,7 +74,10 @@ function changeLocator(c: Change): string { case "change-column-type": case "change-column-nullable": case "change-column-default": + case "change-column-identity": return `${c.table}.${"column" in c ? c.column : c.from}`; + case "change-comment": + return c.column !== undefined ? `${c.table}.${c.column}` : c.table; case "add-column": return `${c.table}.${c.column.name}`; case "drop-table": diff --git a/server/typescript/packages/migrate-ts/src/expr-canon.ts b/server/typescript/packages/migrate-ts/src/expr-canon.ts index 81f9aa1ead..a35db2e418 100644 --- a/server/typescript/packages/migrate-ts/src/expr-canon.ts +++ b/server/typescript/packages/migrate-ts/src/expr-canon.ts @@ -39,6 +39,11 @@ export interface ExprCanon { * through the introspector's own `parsePgDefault`), keyed by the expected column. */ readonly defaults: ReadonlyMap; + /** + * The expected side IS a catalog (the shadow path), so what only a catalog can say is + * compared too: a column's GENERATED identity and table/column comments. + */ + readonly catalog?: boolean; } export const NO_EXPR_CANON: ExprCanon = { checks: new Map(), indexes: new Map(), defaults: new Map() }; diff --git a/server/typescript/packages/migrate-ts/src/index.ts b/server/typescript/packages/migrate-ts/src/index.ts index 5255a85eb5..828f54c07a 100644 --- a/server/typescript/packages/migrate-ts/src/index.ts +++ b/server/typescript/packages/migrate-ts/src/index.ts @@ -99,7 +99,7 @@ export type { ExactViewColumn, ViewReplaceEvidence, ViewReplaceProbe } from "./v export { postgresExprCanonProbe } from "./expr-canon-probe.js"; export { postgresShadowSchema, canonProbeOverShadow } from "./shadow-canon.js"; export type { ShadowHandle, ShadowOpener } from "./shadow-canon.js"; -export type { ShadowSchemaSource } from "./shadow-expected.js"; +export type { ShadowCatalog, ShadowSchemaSource } from "./shadow-expected.js"; export type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; // `baseline --from-db`: record the generated text for an expression the probe proved equal. export { adoptGeneratedSpelling } from "./snapshot/adopt-generated-spelling.js"; diff --git a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts index bcb308072f..86bf8e1a66 100644 --- a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts @@ -51,6 +51,7 @@ export async function introspectPostgres(db: Kysely>): P for (const { schema, name } of tableRefs) { const columns = await readColumns(k, schema, name); const primaryKey = await readPrimaryKey(k, schema, name); + const description = await readPgCatalogColumnFacts(k, schema, name, columns); tables.push({ name, schema, @@ -59,6 +60,7 @@ export async function introspectPostgres(db: Kysely>): P foreignKeys: await readPgForeignKeys(k, schema, name), checks: await readPgChecks(k, schema, name), primaryKey, + ...(description !== undefined ? { description } : {}), }); } @@ -494,6 +496,44 @@ async function readColumns(k: Kysely, schema: string, tableName: string): P }); } +/** + * What only the catalog holds about a table's columns: a GENERATED identity + * (`pg_attribute.attidentity`, which information_schema's column_default leaves NULL) and + * the comments (`pg_description`). Sets `identity` and `description` on `columns` in place + * and returns the table comment. + */ +async function readPgCatalogColumnFacts( + k: RawKysely, schema: string, tableName: string, columns: ColumnDescriptor[], +): Promise { + // pg-mem gap: pg_attribute / col_description are not supported — the query throws and + // the columns stay as information_schema described them. + try { + const rows = await sql<{ column_name: string; attidentity: string; comment: string | null; table_comment: string | null }>` + SELECT a.attname AS column_name, + a.attidentity::text AS attidentity, + col_description(c.oid, a.attnum) AS comment, + obj_description(c.oid, 'pg_class') AS table_comment + FROM pg_attribute a + JOIN pg_class c ON c.oid = a.attrelid + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = ${schema} + AND c.relname = ${tableName} + AND a.attnum > 0 + AND NOT a.attisdropped + `.execute(k); + const byName = new Map(columns.map((c) => [c.name, c] as const)); + for (const r of rows.rows) { + const col = byName.get(r.column_name); + if (col === undefined) continue; + if (r.attidentity === "a" || r.attidentity === "d") col.identity = "increment"; + if (r.comment !== null) col.description = r.comment; + } + return rows.rows[0]?.table_comment ?? undefined; + } catch { + return undefined; + } +} + // Typed alias for raw Kysely — avoids per-call `as any` casts in the helpers below. // eslint-disable-next-line @typescript-eslint/no-explicit-any type RawKysely = Kysely; diff --git a/server/typescript/packages/migrate-ts/src/shadow-canon.ts b/server/typescript/packages/migrate-ts/src/shadow-canon.ts index 9e34b26fe8..e812fcdd8f 100644 --- a/server/typescript/packages/migrate-ts/src/shadow-canon.ts +++ b/server/typescript/packages/migrate-ts/src/shadow-canon.ts @@ -7,9 +7,14 @@ // // The DDL is the one `meta migrate` emits for an empty database, run inside ONE transaction // that is always rolled back, so nothing is left behind even in a shared shadow database. -// Where no shadow database can be opened, or the schema does not apply to it (an FK to a -// table outside the run's scope, a missing extension), the caller falls back to the -// temp-object probe (expr-canon-probe.ts), and from there to the string normaliser. +// Each view is created under its own savepoint: a view whose body reads an object only the +// live database has is left out of the shadow (and compared by its fingerprint instead) +// without costing the run its table comparison. +// +// When the shadow cannot be opened or the tables do not apply to it (an FK to a table outside +// the run's scope, a missing extension), a `required` shadow (one the user configured) is an +// error naming the cause; otherwise the source answers `unavailable` with that cause and the +// caller falls back to the temp-object probe (expr-canon-probe.ts), then the string normaliser. import type { Kysely } from "kysely"; import { sql } from "kysely"; @@ -18,7 +23,7 @@ import { emit } from "./emit/index.js"; import { introspectPostgres } from "./introspect/postgres.js"; import { qualifiedDbName } from "./qualified-name.js"; import type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; -import type { ShadowSchemaSource } from "./shadow-expected.js"; +import type { ShadowCatalog, ShadowSchemaSource } from "./shadow-expected.js"; import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor, SchemaSnapshot } from "./types.js"; /** A connection to a scratch Postgres database, and how to release it. */ @@ -28,41 +33,56 @@ export interface ShadowHandle { close(): Promise; } -/** Opens a shadow database, or resolves `undefined` when none is available. May throw; that reads as unavailable. */ -export type ShadowOpener = () => Promise; - +/** Opens a shadow database. A rejection means none could be opened; its message is the reason. */ +export type ShadowOpener = () => Promise; const ROLLBACK = new Error("shadow canon: always rolled back"); +const VIEW_SAVEPOINT = "mo_shadow_view"; + +const messageOf = (err: unknown): string => (err instanceof Error ? err.message : String(err)); /** - * The shadow catalog for a run: `expected` applied to a scratch database and introspected, or - * `undefined` when no shadow is available or the schema does not apply (the caller falls back). + * The shadow catalog for a run: `expected` applied to a scratch database and introspected. + * `required`: the shadow was configured explicitly, so a failure throws instead of falling back. */ -export function postgresShadowSchema(open: ShadowOpener): ShadowSchemaSource { +export function postgresShadowSchema(open: ShadowOpener, opts: { required?: boolean } = {}): ShadowSchemaSource { + const unavailable = (reason: string): ShadowCatalog => { + if (opts.required === true) throw new Error(`shadow database: ${reason}`); + return { unavailable: reason }; + }; return async (expected) => { - let handle: ShadowHandle | undefined; + let handle: ShadowHandle; try { handle = await open(); - } catch { - return undefined; + } catch (err) { + return unavailable(`could not be opened: ${messageOf(err)}`); } - if (handle === undefined) return undefined; try { const { changes } = await diff({ expected, actual: { tables: [], views: [] }, dialect: "postgres" }); - const { up } = emit(changes, { dialect: "postgres" }); + const views = changes.filter((c) => c.kind === "create-view"); + const { up } = emit(changes.filter((c) => c.kind !== "create-view"), { dialect: "postgres" }); let shadow: SchemaSnapshot | undefined; try { await handle.db.transaction().execute(async (trx) => { await sql.raw(up).execute(trx); + for (const view of views) { + await sql.raw(`SAVEPOINT ${VIEW_SAVEPOINT}`).execute(trx); + try { + await sql.raw(emit([view], { dialect: "postgres" }).up).execute(trx); + await sql.raw(`RELEASE SAVEPOINT ${VIEW_SAVEPOINT}`).execute(trx); + } catch { + await sql.raw(`ROLLBACK TO SAVEPOINT ${VIEW_SAVEPOINT}`).execute(trx); + } + } shadow = await introspectPostgres(trx as unknown as Kysely>); throw ROLLBACK; }); } catch (err) { - if (err !== ROLLBACK) return undefined; + if (err !== ROLLBACK) throw err; } - return shadow; - } catch { - return undefined; + return { catalog: shadow! }; + } catch (err) { + return unavailable(`the expected schema does not apply to it: ${messageOf(err)}`); } finally { await handle.close(); } diff --git a/server/typescript/packages/migrate-ts/src/shadow-expected.ts b/server/typescript/packages/migrate-ts/src/shadow-expected.ts index 18d199f33a..b08ad3f746 100644 --- a/server/typescript/packages/migrate-ts/src/shadow-expected.ts +++ b/server/typescript/packages/migrate-ts/src/shadow-expected.ts @@ -2,44 +2,29 @@ // // On the shadow path drift is the diff of introspect(shadow) against introspect(live): the // same introspector reads both, so every object in scope (tables, columns and types, -// nullability, defaults, CHECKs, PK/FK/unique constraints and their actions, indexes) is in -// Postgres's own spelling on both sides and no per-shape rule is needed. Model-only knowledge -// stays an INPUT to the same diff rather than a second comparison: -// - renames, @unmanaged and scope are `diff` arguments, applied as always; -// - a column's `identity` and a table's/column's `description` are carried over from the -// model, because the catalog cannot say which spelling of an auto-generated key the model -// asked for (a legacy `serial` column is deliberately not drift against an `increment` key); -// - views keep the model's own descriptor, since their drift check is a fingerprint the -// model computed, not a comparison of Postgres's deparsed body. +// nullability, defaults, identity, CHECKs, PK/FK/unique constraints and their actions, +// indexes, view bodies, comments) is in Postgres's own spelling on both sides and no +// per-shape rule is needed. Model-only knowledge stays an INPUT to the same diff rather than +// a second comparison: renames, @unmanaged and scope are `diff` arguments, applied as always. +// +// Views keep the model's descriptor as the thing a migration CREATES (its authored body and +// fingerprint stamp); whether a view differs is decided by comparing the shadow's deparsed +// body (`pg_get_viewdef`) with the live one, in `diff`. import { qualifiedDbName } from "./qualified-name.js"; import type { ExprCanon, ExprCanonProbe, IndexExprCanon } from "./expr-canon.js"; -import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor, SchemaSnapshot, TableDescriptor } from "./types.js"; +import type { CheckDescriptor, ColumnDefault, ColumnDescriptor, IndexDescriptor, SchemaSnapshot } from "./types.js"; -/** A shadow database's introspected catalog for `expected`'s run, or `undefined` when none is available. */ -export type ShadowSchemaSource = (expected: SchemaSnapshot) => Promise; +/** A shadow database's introspected catalog for a run, or why none could be had. */ +export type ShadowCatalog = { catalog: SchemaSnapshot } | { unavailable: string }; -/** - * `expected` re-read from `shadow`: each model table replaced by the shadow's table of the - * same name (a table the shadow lacks is kept as modelled), with the model-only fields laid back. - */ +/** Applies `expected` to a scratch database and reads it back (see shadow-canon.ts). */ +export type ShadowSchemaSource = (expected: SchemaSnapshot) => Promise; + +/** `expected` re-read from `shadow`: each model table replaced by the shadow's table of the same name. */ export function expectedFromShadow(shadow: SchemaSnapshot, expected: SchemaSnapshot): SchemaSnapshot { const byId = new Map(shadow.tables.map((t) => [qualifiedDbName(t), t] as const)); - const tables = expected.tables.map((model): TableDescriptor => { - const st = byId.get(qualifiedDbName(model)); - if (st === undefined) return model; - const modelCols = new Map(model.columns.map((c) => [c.name, c] as const)); - const columns = st.columns.map((c): ColumnDescriptor => { - const m = modelCols.get(c.name); - return { - ...c, - ...(m?.identity !== undefined ? { identity: m.identity } : {}), - ...(m?.description !== undefined ? { description: m.description } : {}), - }; - }); - return { ...st, columns, ...(model.description !== undefined ? { description: model.description } : {}) }; - }); - return { ...expected, tables }; + return { ...expected, tables: expected.tables.map((model) => byId.get(qualifiedDbName(model)) ?? model) }; } /** diff --git a/server/typescript/packages/migrate-ts/src/types.ts b/server/typescript/packages/migrate-ts/src/types.ts index e00b7e1ea0..b37a45a8b5 100644 --- a/server/typescript/packages/migrate-ts/src/types.ts +++ b/server/typescript/packages/migrate-ts/src/types.ts @@ -37,7 +37,8 @@ export interface TableDescriptor { * Human-readable description threaded from entity `@description`. * Postgres: emitted as `COMMENT ON TABLE … IS '…';` after CREATE TABLE. * SQLite: silently ignored (no native COMMENT support). - * Introspect side: not read back in v1 (no change-description variant yet). + * Introspect side: Postgres reads it back (`obj_description`); compared only on the + * shadow path, where both sides are a catalog (`change-comment`). */ description?: string; } @@ -58,7 +59,8 @@ export interface ColumnDescriptor { * Human-readable description threaded from field `@description`. * Postgres: emitted as `COMMENT ON COLUMN … IS '…';` after CREATE TABLE or ADD COLUMN. * SQLite: silently ignored (no native COMMENT support). - * Introspect side: not read back in v1 (no change-description variant yet). + * Introspect side: Postgres reads it back (`col_description`); compared only on the + * shadow path, where both sides are a catalog (`change-comment`). */ description?: string; } @@ -272,6 +274,12 @@ export type Change = from: boolean; to: boolean; status: ChangeStatus } | { kind: "change-column-default"; table: string; schema?: string; column: string; from?: ColumnDefault; to?: ColumnDefault; status: ChangeStatus } + /** Postgres, shadow path only: an `increment` identity (`GENERATED BY DEFAULT AS IDENTITY`) added or dropped. */ + | { kind: "change-column-identity"; table: string; schema?: string; column: string; + from?: "increment"; to?: "increment"; status: ChangeStatus } + /** Postgres, shadow path only: a table comment (no `column`) or column comment set, changed or cleared. */ + | { kind: "change-comment"; table: string; schema?: string; column?: string; + from?: string; to?: string; status: ChangeStatus } | { kind: "add-index"; table: string; schema?: string; index: IndexDescriptor; status: ChangeStatus } | { kind: "drop-index"; table: string; schema?: string; index: string; restore?: IndexDescriptor; @@ -528,6 +536,8 @@ export interface DiffResult { hazards: DataHazard[]; /** Postgres: which comparison decided CHECK, index-expression and column-default equality. */ exprCanonPath?: ExprCanonPath; + /** Postgres: why no scratch (shadow) database was used, when one was asked for and could not be. */ + shadowUnavailable?: string; } /** diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts index c146b2c128..099bde3ab4 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-rename-carry-canon.test.ts @@ -2,7 +2,9 @@ * A column rename carries the CHECK named after it only when Postgres says the rule is the * same. The pairing used to compare the author's text with the live text through the string * normaliser, which folds a literal-case change (`'OPEN'` → `'open'`) into a plain - * RENAME CONSTRAINT, so the stale values survived the migration. + * RENAME CONSTRAINT, so the stale values survived the migration. Both database paths are + * covered: the temp-object probe on the target, and the scratch (shadow) database, where the + * expected CHECK is Postgres's own spelling under the new column name. * * Skips gracefully when MIGRATE_TS_PG_URL is not set. */ @@ -13,9 +15,11 @@ import { Kysely, PostgresDialect, sql } from "kysely"; import { introspectPostgres } from "../../src/introspect/postgres.js"; import { diff } from "../../src/diff/index.js"; import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; +import { postgresShadowSchema } from "../../src/shadow-canon.js"; const PG_URL = process.env["MIGRATE_TS_PG_URL"]; const TABLE = "rename_carry_t"; +const SHADOW_DB = "mo_rename_carry_shadow_test"; describe("column rename carry (live PG)", () => { if (!PG_URL) { @@ -24,10 +28,18 @@ describe("column rename carry (live PG)", () => { } let pool: Pool; let db: Kysely>; + const shadow = postgresShadowSchema(async () => { + const u = new URL(PG_URL); + u.pathname = `/${SHADOW_DB}`; + const s = new Kysely>({ dialect: new PostgresDialect({ pool: new Pool({ connectionString: u.toString() }) }) }); + return { db: s, close: async () => { await s.destroy(); } }; + }, { required: true }); beforeAll(async () => { pool = new Pool({ connectionString: PG_URL }); db = new Kysely>({ dialect: new PostgresDialect({ pool }) }); + await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(db); + await sql.raw(`CREATE DATABASE ${SHADOW_DB}`).execute(db); await sql.raw(` DROP TABLE IF EXISTS ${TABLE}; CREATE TABLE ${TABLE} (id bigserial PRIMARY KEY, status text, @@ -35,10 +47,11 @@ describe("column rename carry (live PG)", () => { }); afterAll(async () => { await sql.raw(`DROP TABLE IF EXISTS ${TABLE}`).execute(db); + await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(db); await pool.end(); }); - const run = async (expression: string, probe: boolean) => { + const run = async (expression: string, path: "probe" | "shadow") => { const live = await introspectPostgres(db); const expected = structuredClone(live); const t = expected.tables.find((x) => x.name === TABLE)!; @@ -47,28 +60,33 @@ describe("column rename carry (live PG)", () => { return diff({ expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], renames: [{ kind: "column", table: TABLE, from: "status", to: "state" }], - ...(probe ? { exprCanonProbe: postgresExprCanonProbe(db) } : {}), + ...(path === "probe" ? { exprCanonProbe: postgresExprCanonProbe(db) } : { shadowSchema: shadow }), }); }; const kinds = (r: Awaited>) => r.changes.map((c) => c.kind).sort(); - test("the same rule under the new column name folds into the rename", async () => { - const r = await run(`"state" IN ('OPEN', 'CLOSED')`, true); - expect(kinds(r)).toEqual(["rename-column"]); - expect((r.changes[0] as { constraintRenames?: unknown }).constraintRenames).toEqual([ - { from: `${TABLE}_status_chk`, to: `${TABLE}_state_chk` }, - ]); - }); + for (const path of ["probe", "shadow"] as const) { + test(`${path}: the same rule under the new column name folds into the rename`, async () => { + const r = await run(`"state" IN ('OPEN', 'CLOSED')`, path); + expect(r.exprCanonPath).toBe(path); + expect(kinds(r)).toEqual(["rename-column"]); + expect((r.changes[0] as { constraintRenames?: unknown }).constraintRenames).toEqual([ + { from: `${TABLE}_status_chk`, to: `${TABLE}_state_chk` }, + ]); + }); - test("a rename plus a literal-case change is NOT folded: the stale values must be rewritten", async () => { - const r = await run(`"state" IN ('open', 'closed')`, true); - expect(kinds(r)).toEqual(["add-check", "drop-check", "rename-column"]); - }); + test(`${path}: a rename plus a literal-case change is NOT folded: the stale values must be rewritten`, async () => { + const r = await run(`"state" IN ('open', 'closed')`, path); + expect(r.exprCanonPath).toBe(path); + expect(kinds(r)).toEqual(["add-check", "drop-check", "rename-column"]); + }); - test("a rename plus a changed rule is not folded either", async () => { - const r = await run(`"state" IN ('OPEN')`, true); - expect(kinds(r)).toEqual(["add-check", "drop-check", "rename-column"]); - }); + test(`${path}: a rename plus a changed rule is not folded either`, async () => { + const r = await run(`"state" IN ('OPEN')`, path); + expect(r.exprCanonPath).toBe(path); + expect(kinds(r)).toEqual(["add-check", "drop-check", "rename-column"]); + }); + } test("the probe compiles an expected expression against the renamed columns", async () => { const live = (await introspectPostgres(db)).tables.find((t) => t.name === TABLE)!; diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts index da70cfbee1..1fd42e6c22 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-canon.test.ts @@ -15,7 +15,7 @@ import { introspectPostgres } from "../../src/introspect/postgres.js"; import { diff } from "../../src/diff/index.js"; import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; import { postgresShadowSchema, type ShadowOpener } from "../../src/shadow-canon.js"; -import type { SchemaSnapshot, TableDescriptor } from "../../src/types.js"; +import type { ChangeKind, SchemaSnapshot, TableDescriptor } from "../../src/types.js"; const PG_URL = process.env["MIGRATE_TS_PG_URL"]; const TABLE = "shadow_canon_t"; @@ -131,7 +131,7 @@ describe("shadow-database canon (live PG)", () => { }); test("a real change of each kind is still reported", async () => { - const cases: Array<[string, (t: TableDescriptor) => void, string[]]> = [ + const cases: Array<[string, (t: TableDescriptor) => void, ChangeKind[]]> = [ ["default value", (t) => { AUTHORED(t); t.columns.find((c) => c.name === "label")!.default = { kind: "literal", value: "b" }; }, ["change-column-default"]], ["default instant", (t) => { AUTHORED(t); t.columns.find((c) => c.name === "ts2")!.default = { kind: "literal", value: "2024-01-02" }; }, ["change-column-default"]], ["default expression", (t) => { AUTHORED(t); t.columns.find((c) => c.name === "soon")!.default = { kind: "expr", value: "now() + interval '2 days'" }; }, ["change-column-default"]], @@ -154,9 +154,28 @@ describe("shadow-database canon (live PG)", () => { shadowSchema: postgresShadowSchema(opener), exprCanonProbe: postgresExprCanonProbe(main.db), }); expect(r.exprCanonPath).toBe("probe"); + expect(r.shadowUnavailable).toContain("does not apply"); expect(r.changes.map((c) => c.kind)).toEqual(["add-fk"]); }); + test("an explicitly configured shadow that does not apply is an error naming the cause", async () => { + const { live, expected } = await declared(AUTHORED); + tableOf(expected).foreignKeys.push({ name: "sc_fk", columns: ["a"], refTable: "no_such_table", refColumns: ["id"], onDelete: "restrict" }); + const run = diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + shadowSchema: postgresShadowSchema(opener, { required: true }), exprCanonProbe: postgresExprCanonProbe(main.db), + }); + await expect(run).rejects.toThrow(/shadow database: the expected schema does not apply to it: .*no_such_table/); + }); + + test("a view that does not apply to the shadow is left out, and the tables are still compared there", async () => { + const { live, expected } = await declared(AUTHORED); + expected.views.push({ name: `${TABLE}_lost_v`, schema: "public", sql: "SELECT id FROM only_on_live_table", fingerprint: "f" }); + const r = await diff({ expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], shadowSchema: postgresShadowSchema(opener) }); + expect(r.exprCanonPath).toBe("shadow"); + expect(r.changes.map((c) => c.kind)).toEqual(["create-view"]); + }); + test("an opener that cannot open reads as unavailable", async () => { const { live, expected } = await declared(AUTHORED); const r = await diff({ @@ -164,5 +183,15 @@ describe("shadow-database canon (live PG)", () => { shadowSchema: postgresShadowSchema(async () => { throw new Error("no CREATE DATABASE"); }), }); expect(r.exprCanonPath).toBe("normaliser"); + expect(r.shadowUnavailable).toBe("could not be opened: no CREATE DATABASE"); + }); + + test("an explicitly configured shadow that cannot be opened is an error naming the cause", async () => { + const { live, expected } = await declared(AUTHORED); + const run = diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + shadowSchema: postgresShadowSchema(async () => { throw new Error("password authentication failed"); }, { required: true }), + }); + await expect(run).rejects.toThrow("shadow database: could not be opened: password authentication failed"); }); }); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts index fb17c05cdd..48ea0b6465 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts @@ -1,8 +1,10 @@ /** * The shadow path is a catalog-to-catalog diff, not a list of expression rules. Nothing in * this file has a special case anywhere in migrate-ts: an `int4` column against a model - * `integer`, and an FK `ON DELETE SET NULL`, read clean through the shadow path because - * Postgres (not a rule) says what they are, and each is still reported when it really differs. + * `integer`, an FK `ON DELETE SET NULL`, a GENERATED identity, table and column comments and + * a view body written differently from Postgres's deparse read clean through the shadow path + * because Postgres (not a rule) says what they are, and each is still reported when it + * really differs. The plan for those differences applies, and leaves nothing to report. * * Skips gracefully when MIGRATE_TS_PG_URL is not set. */ @@ -12,6 +14,7 @@ import { Pool } from "pg"; import { Kysely, PostgresDialect, sql } from "kysely"; import { introspectPostgres } from "../../src/introspect/postgres.js"; import { diff } from "../../src/diff/index.js"; +import { emit } from "../../src/emit/index.js"; import { postgresShadowSchema } from "../../src/shadow-canon.js"; import type { FkDescriptor, SchemaSnapshot } from "../../src/types.js"; @@ -42,28 +45,43 @@ describe("shadow path is schema-wide (live PG)", () => { await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); await sql.raw(`CREATE DATABASE ${SHADOW_DB}`).execute(main.db); await sql.raw(` - DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent; - CREATE TABLE sc2_parent (id int4 PRIMARY KEY, label character varying(20) NOT NULL); + DROP VIEW IF EXISTS sc2_v; DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent; + CREATE TABLE sc2_parent (id int4 GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, label character varying(20) NOT NULL); + COMMENT ON TABLE sc2_parent IS 'Parents'; + COMMENT ON COLUMN sc2_parent.label IS 'Shown name'; CREATE TABLE sc2_child ( id int4 PRIMARY KEY, parent_id integer, CONSTRAINT sc2_child_parent_fk FOREIGN KEY (parent_id) REFERENCES sc2_parent (id) ON DELETE SET NULL - )`).execute(main.db); + ); + CREATE VIEW sc2_v AS SELECT id, label FROM sc2_parent WHERE id > 0`).execute(main.db); }); afterAll(async () => { - await sql.raw(`DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent`).execute(main.db); + await sql.raw(`DROP VIEW IF EXISTS sc2_v; DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent`).execute(main.db); await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); await main.pool.end(); }); /** The model, written by hand (not read from the live database). */ - const model = (over?: { onDelete?: FkDescriptor["onDelete"]; parentIdBits?: 32 | 64; labelLength?: number }): SchemaSnapshot => ({ - views: [], + interface Over { + onDelete?: FkDescriptor["onDelete"]; parentIdBits?: 32 | 64; labelLength?: number; + identity?: boolean; tableComment?: string | null; labelComment?: string | null; viewWhere?: string; + } + const comment = (value: string | null | undefined, fallback: string): { description?: string } => { + const d = value === undefined ? fallback : value; + return d === null ? {} : { description: d }; + }; + const model = (over?: Over): SchemaSnapshot => ({ + views: [{ + name: "sc2_v", schema: "public", fingerprint: "sc2", + sql: `select "id", "label" from "sc2_parent" where ${over?.viewWhere ?? `"id" > 0`}`, + }], tables: [ { name: "sc2_parent", schema: "public", indexes: [], foreignKeys: [], checks: [], primaryKey: ["id"], + ...comment(over?.tableComment, "Parents"), columns: [ - { name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false }, - { name: "label", sqlType: { kind: "text", maxLength: over?.labelLength ?? 20 }, nullable: false }, + { name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false, ...(over?.identity === false ? {} : { identity: "increment" as const }) }, + { name: "label", sqlType: { kind: "text", maxLength: over?.labelLength ?? 20 }, nullable: false, ...comment(over?.labelComment, "Shown name") }, ], }, { @@ -85,16 +103,54 @@ describe("shadow path is schema-wide (live PG)", () => { expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], ...(withShadow ? { shadowSchema: shadow } : {}), }); - return { kinds: r.changes.filter((c) => "table" in c && String(c.table).startsWith("sc2_")).map((c) => c.kind).sort(), path: r.exprCanonPath }; + return { + kinds: r.changes.filter((c) => ("table" in c && String(c.table).startsWith("sc2_")) || "view" in c).map((c) => c.kind).sort(), + path: r.exprCanonPath, + changes: r.changes, + }; }; + const kindsOf = async (expected: SchemaSnapshot, withShadow = true) => (await run(expected, withShadow)).kinds; - test("int4 against integer, varchar spelling and an FK ON DELETE SET NULL read clean", async () => { - expect(await run(model(), true)).toEqual({ kinds: [], path: "shadow" }); + test("int4 against integer, varchar spelling, an FK action, identity, comments and a view body read clean", async () => { + const r = await run(model(), true); + expect({ kinds: r.kinds, path: r.path }).toEqual({ kinds: [], path: "shadow" }); }); test("each is still reported when it really differs", async () => { - expect((await run(model({ onDelete: "cascade" }), true)).kinds).toEqual(["add-fk", "drop-fk"]); - expect((await run(model({ parentIdBits: 64 }), true)).kinds).toEqual(["change-column-type"]); - expect((await run(model({ labelLength: 40 }), true)).kinds).toEqual(["change-column-type"]); + expect(await kindsOf(model({ onDelete: "cascade" }))).toEqual(["add-fk", "drop-fk"]); + expect(await kindsOf(model({ parentIdBits: 64 }))).toEqual(["change-column-type"]); + // sc2_v reads label, and Postgres refuses ALTER COLUMN TYPE under a view: it is recreated around it. + expect(await kindsOf(model({ labelLength: 40 }))).toEqual(["change-column-type", "create-view", "drop-view"]); + expect(await kindsOf(model({ identity: false }))).toEqual(["change-column-identity"]); + expect(await kindsOf(model({ tableComment: "All parents" }))).toEqual(["change-comment"]); + expect(await kindsOf(model({ labelComment: null }))).toEqual(["change-comment"]); + // The live view carries no MetaObjects stamp, so a changed body is an adoption (drop + create). + expect(await kindsOf(model({ viewWhere: `"id" > 1` }))).toEqual(["create-view", "drop-view"]); + }); + + test("off the shadow path identity and comments are not compared", async () => { + const kinds = await kindsOf(model({ identity: false, tableComment: null, labelComment: "x" }), false); + expect(kinds.filter((k) => k === "change-column-identity" || k === "change-comment")).toEqual([]); + }); + + test("the plan for a comment and identity difference applies, and leaves the schema clean", async () => { + const changed = model({ identity: false, tableComment: "All parents", labelComment: null }); + const live = await introspectPostgres(main.db); + const { changes } = await diff({ + expected: changed, actual: live, dialect: "postgres", scopeSchemas: ["public"], shadowSchema: shadow, + allow: { dropIdentityDefault: true }, + }); + expect(changes.map((c) => c.kind).sort()).toEqual(["change-column-identity", "change-comment", "change-comment"]); + const { up } = emit(changes.filter((c) => c.kind === "change-column-identity" || c.kind === "change-comment"), { dialect: "postgres" }); + const ROLLBACK = new Error("rollback"); + let after: string[] | undefined; + await main.db.transaction().execute(async (trx) => { + await sql.raw(up).execute(trx); + const live = await introspectPostgres(trx as unknown as Db); + const r = await diff({ expected: changed, actual: live, dialect: "postgres", scopeSchemas: ["public"], shadowSchema: shadow }); + after = r.changes.map((c) => c.kind); + throw ROLLBACK; + }).catch((err) => { if (err !== ROLLBACK) throw err; }); + expect(after).toEqual([]); }); }); From 320c7a57cb1083ccc544a6e7416a039b9b97b85f Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 23:36:26 -0400 Subject: [PATCH 5/8] no-mistakes(review): Gate comment clears, version snapshots, fix quoted-expression defaults --- CHANGELOG.md | 8 +- docs/features/migrations-and-drift.md | 7 +- server/typescript/packages/cli/README.md | 2 +- .../packages/cli/src/commands/migrate.ts | 19 ++- server/typescript/packages/cli/src/index.ts | 3 +- .../typescript/packages/cli/src/lib/allow.ts | 2 + .../typescript/packages/cli/src/lib/args.ts | 3 + .../cli/test/__snapshots__/cli.test.ts.snap | 3 +- .../integration/shadow-catalog-pg.test.ts | 138 ++++++++++++++++++ .../packages/migrate-ts/src/diff/index.ts | 4 +- .../packages/migrate-ts/src/diff/status.ts | 5 +- .../migrate-ts/src/introspect/postgres.ts | 21 +-- .../migrate-ts/src/snapshot/serialize.ts | 21 ++- .../packages/migrate-ts/src/types.ts | 17 ++- .../test/check/serialize-upgrade.test.ts | 17 ++- .../test/expected-schema-scope.test.ts | 4 +- .../integration/pg-shadow-schema-wide.test.ts | 62 +++++++- .../test/unit/diff-status-comment.test.ts | 39 +++++ .../unit/introspect-postgres-helpers.test.ts | 11 ++ server/typescript/packages/sdk/src/config.ts | 1 + 20 files changed, 357 insertions(+), 30 deletions(-) create mode 100644 server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts create mode 100644 server/typescript/packages/migrate-ts/test/unit/diff-status-comment.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b54a8dcde..8da6891037 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -318,7 +318,13 @@ here.** cannot be opened or applied is an error naming the cause. When a `CREATE`d one is not available, the rolled-back temp-object probe on the target answers (it needs write privileges and takes brief locks), and SQLite and offline plans keep the string normaliser. Every Postgres run says - which path ran and, on a fallback, why. The Postgres emitter no longer writes a `DEFAULT` beside + which path ran and, on a fallback, why. Clearing a live comment the metadata does not declare + needs the new `--allow drop-comment`; without it `meta migrate` keeps the comment, plans the + rest and says so, and `meta verify --db` still reports it. **Upgrade note:** the committed + snapshot moves to `formatVersion` 4, which records comments and GENERATED identity read off the + database; an older toolchain refuses a v4 snapshot. A v3 snapshot still loads, and the + committed-snapshot check treats its comments and identity as unknown rather than absent, so it + does not go red after the upgrade; re-run `meta migrate baseline --from-db` to record them. The Postgres emitter no longer writes a `DEFAULT` beside `GENERATED BY DEFAULT AS IDENTITY` (a snapshot of a legacy `serial` column carries both, and Postgres refuses the pair). See `docs/features/migrations-and-drift.md`. diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 6328a49995..0497e8f75b 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -394,7 +394,12 @@ Identity is read from `pg_attribute.attidentity` on both sides (a legacy `serial as an `increment` key, so it is not drift against one); a GENERATED identity added or dropped is a `change-column-identity`, and dropping one is gated behind `--allow drop-identity-default`. Table and column comments are read from `pg_description` on both sides, and a changed one is a -`change-comment` (`COMMENT ON … IS …`). A view's body is compared as Postgres's deparse +`change-comment` (`COMMENT ON … IS …`). Clearing a live comment the metadata does not declare +(somebody else may have written it) needs `--allow drop-comment`: without it `meta migrate` +leaves the comment in place, plans the rest and says so in a note, and `meta verify --db` still +reports it as drift. A committed snapshot written before `formatVersion` 4 never recorded comments +or a GENERATED identity, so the committed-snapshot check treats both as unknown rather than absent +and does not compare them; re-run `meta migrate baseline --from-db` to record them. A view's body is compared as Postgres's deparse (`pg_get_viewdef`) of the shadow's copy against the live one; each view is created under its own savepoint, so a view whose body reads something only the live database has is left out of the shadow and compared by its fingerprint instead, without costing the run its table comparison. diff --git a/server/typescript/packages/cli/README.md b/server/typescript/packages/cli/README.md index c199c67b78..cf0715839b 100644 --- a/server/typescript/packages/cli/README.md +++ b/server/typescript/packages/cli/README.md @@ -175,7 +175,7 @@ Flags: - `--dialect sqlite|postgres|d1` — auto-detected from URL scheme; use `d1` for Cloudflare D1 - `--out-dir ` (default `./.metaobjects/migrations`) - `--slug ` — required when changes are pending (e.g., `add-user-shipping`) -- `--allow ` — destructive-change permissions: `drop-column,drop-table,type-change,drop-index,drop-fk,drop-check,drop-view,drop-view-cascade,adopt-view,nullable-to-not-null,drop-identity-default` +- `--allow ` — destructive-change permissions: `drop-column,drop-table,type-change,drop-index,drop-fk,drop-check,drop-view,drop-view-cascade,adopt-view,nullable-to-not-null,drop-identity-default,drop-unmanaged,drop-comment` - `--on-ambiguous abort|rename|drop-add` (default `abort`) — non-interactive - `--rename-table [schema.]old=new` / `--rename-column [schema.]table.old=new` — declare a rename (repeatable), so the migration is `RENAME` instead of drop+add whatever the rename heuristic makes of the names. Refused if nothing matches. See [Renames and populated tables](../../../../docs/features/migrations-and-drift.md#renames-and-populated-tables). - `--dry-run` — print the SQL pair, write nothing (under `--format json|toon` the pair is the document's `sql` field, so stdout stays one parseable document) diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index e1da8f7ec5..072514aaed 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -122,7 +122,7 @@ MIGRATE FLAGS: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, adopt-view,nullable-to-not-null,drop-identity-default, - drop-unmanaged + drop-unmanaged,drop-comment drop-column DELETES the column and every value in it. If the column was RENAMED, use --rename-column instead: it keeps the data. @@ -891,6 +891,23 @@ export async function migrateCommand( throw err; } + // A live comment the metadata does not declare is cleared only with --allow drop-comment. + // Without it the change is reported and left out of the plan, rather than refusing the + // whole migration over somebody else's comment (`meta verify --db` still reports it). + const keptComments = diffResult.blocked.filter( + (c): c is Extract => c.kind === "change-comment"); + if (keptComments.length > 0) { + const kept = new Set(keptComments); + diffResult = { + ...diffResult, + changes: diffResult.changes.filter((c) => !kept.has(c)), + blocked: diffResult.blocked.filter((c) => !kept.has(c)), + }; + notes.push(...keptComments.map((c) => + `kept the comment on ${c.column !== undefined ? `${c.table}.${c.column}` : c.table}, which the metadata ` + + `does not declare; pass --allow drop-comment to clear it.`)); + } + changeCounts = summarizeChanges(diffResult.changes); hazardWarnings = warnDataHazards(diffResult.hazards); diff --git a/server/typescript/packages/cli/src/index.ts b/server/typescript/packages/cli/src/index.ts index 2d1adc1907..d83ef32c40 100644 --- a/server/typescript/packages/cli/src/index.ts +++ b/server/typescript/packages/cli/src/index.ts @@ -177,7 +177,8 @@ MIGRATE FLAGS: --allow Comma-separated destructive-change permissions: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, - adopt-view,nullable-to-not-null,drop-identity-default + adopt-view,nullable-to-not-null,drop-identity-default, + drop-comment drop-column DELETES the column's data; for a renamed column use --rename-column, which keeps it --on-ambiguous abort|rename|drop-add Default abort diff --git a/server/typescript/packages/cli/src/lib/allow.ts b/server/typescript/packages/cli/src/lib/allow.ts index b30f22f908..f1a7bc7c88 100644 --- a/server/typescript/packages/cli/src/lib/allow.ts +++ b/server/typescript/packages/cli/src/lib/allow.ts @@ -39,6 +39,8 @@ export const ALLOW_TOKEN_MAP: Record = { // migrate's generation-time provenance guard, not by diff()'s status pass — see // AllowOptions.dropUnmanaged for why it still belongs in that shape. "drop-unmanaged": "dropUnmanaged", + // Gates clearing a live table/column comment the metadata does not declare (shadow path only). + "drop-comment": "dropComment", }; /** Translate parsed `--allow` tokens into the migrate-ts `AllowOptions` shape. */ diff --git a/server/typescript/packages/cli/src/lib/args.ts b/server/typescript/packages/cli/src/lib/args.ts index 3f43b21544..0280813a89 100644 --- a/server/typescript/packages/cli/src/lib/args.ts +++ b/server/typescript/packages/cli/src/lib/args.ts @@ -332,6 +332,9 @@ export const ALLOW_TOKENS = [ // existed (#313). Unlike its neighbours this one is enforced by `migrate` itself // rather than by `diff()`'s status pass; see AllowOptions.dropUnmanaged. "drop-unmanaged", + // drop-comment permits clearing a live table or column comment the metadata does not + // declare (`COMMENT ON … IS NULL`); without it `migrate` leaves the comment and says so. + "drop-comment", ] as const; type AllowToken = (typeof ALLOW_TOKENS)[number]; diff --git a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap index 2192448bab..69e9cd685f 100644 --- a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap +++ b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap @@ -157,7 +157,8 @@ MIGRATE FLAGS: --allow Comma-separated destructive-change permissions: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, - adopt-view,nullable-to-not-null,drop-identity-default + adopt-view,nullable-to-not-null,drop-identity-default, + drop-comment drop-column DELETES the column's data; for a renamed column use --rename-column, which keeps it --on-ambiguous abort|rename|drop-add Default abort diff --git a/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts b/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts new file mode 100644 index 0000000000..d34b5f341a --- /dev/null +++ b/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts @@ -0,0 +1,138 @@ +/** + * The shadow path compares comments and GENERATED identity catalog to catalog. Two things that + * must hold around that, end to end through `meta verify --db` and `meta migrate`: + * + * - A committed snapshot written before formatVersion 4 never recorded comments or identity (a + * v3 `baseline --from-db` left them out), so the committed-snapshot check treats them as + * unknown and stays green; the same snapshot written as v4 says "none", and that difference + * from the database is reported. + * - A live comment the metadata does not declare is drift for `verify --db`, but `migrate` does + * not clear it without `--allow drop-comment`: it plans the rest and says it kept the comment. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ +import { describe, test, expect, beforeAll, afterAll } from "bun:test"; +import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Pool } from "pg"; +import { snapshotPath } from "@metaobjectsdev/migrate-ts"; +import { run } from "../../src/index.js"; + +const PG_URL = process.env.MIGRATE_TS_PG_URL; +const TABLE = "shadow_catalog_t"; + +const MODEL = JSON.stringify({ + "metadata.root": { + children: [{ + "object.entity": { + name: "ShadowCatalog", + "@description": "Catalog rows.", + children: [ + { "source.rdb": { name: "src", "@table": TABLE } }, + { "field.long": { name: "id" } }, + { "field.string": { name: "label", "@maxLength": 40, "@description": "Shown name." } }, + { "field.int": { name: "qty" } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }], + }, +}); + +interface SnapshotFile { + formatVersion: number; + snapshot: { tables: Array<{ name: string; description?: string; columns: Array<{ description?: string; identity?: string }> }> }; +} + +describe("shadow catalog: snapshot versions and undeclared comments (env-gated)", () => { + if (PG_URL === undefined || PG_URL.length === 0) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + let pool: Pool; + let root: string; + const snapFile = (): string => snapshotPath(join(root, ".metaobjects", "migrations"), "postgres"); + const pg = ["--cwd", "", "--db", PG_URL, "--dialect", "postgres"]; + const args = (...rest: string[]): string[] => [...rest, ...pg.map((a) => (a === "" ? root : a))]; + + /** Run the CLI, collecting everything it prints. */ + const captured = async (argv: string[]): Promise<{ exit: number; out: string }> => { + const lines: string[] = []; + const origLog = console.log; + const origErr = console.error; + const origWarn = console.warn; + const keep = (...a: unknown[]): void => { lines.push(a.map(String).join(" ")); }; + console.log = keep; console.error = keep; console.warn = keep; + try { + return { exit: await run(argv), out: lines.join("\n") }; + } finally { + console.log = origLog; console.error = origErr; console.warn = origWarn; + } + }; + + beforeAll(async () => { + pool = new Pool({ connectionString: PG_URL }); + await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); + root = mkdtempSync(join(tmpdir(), "shadow-catalog-pg-")); + mkdirSync(join(root, "metaobjects"), { recursive: true }); + writeFileSync(join(root, "metaobjects", "meta.catalog.json"), MODEL, "utf8"); + // Day 1: the table, with its comments and a GENERATED identity, and a v4 snapshot of it. + expect((await captured(args("migrate", "--from-db", "--slug", "init", "--apply"))).exit).toBe(0); + }); + afterAll(async () => { + await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); + await pool.end(); + rmSync(root, { recursive: true, force: true }); + }); + + test("the database the migration built is clean, against the metadata and the snapshot", async () => { + const live = await pool.query(`SELECT obj_description('${TABLE}'::regclass, 'pg_class') AS c`); + expect(live.rows[0].c).toBe("Catalog rows."); + expect((await captured(args("verify"))).exit).toBe(0); + }); + + test("a snapshot that never recorded identity and comments verifies clean as v3, and is a difference as v4", async () => { + const written = readFileSync(snapFile(), "utf8"); + const file = JSON.parse(written) as SnapshotFile; + expect(file.formatVersion).toBe(4); + // What a v3 `baseline --from-db` wrote for this very table: no comments, no identity. + for (const t of file.snapshot.tables) { + delete t.description; + for (const c of t.columns) { delete c.description; delete c.identity; } + } + try { + writeFileSync(snapFile(), JSON.stringify({ ...file, formatVersion: 3 }), "utf8"); + expect((await captured(args("verify"))).exit).toBe(0); + writeFileSync(snapFile(), JSON.stringify(file), "utf8"); + const v4 = await captured(args("verify")); + expect(v4.exit).toBe(1); + expect(v4.out).toContain("the committed schema snapshot disagrees"); + } finally { + writeFileSync(snapFile(), written, "utf8"); + } + }); + + test("an undeclared live comment is drift for verify, and migrate keeps it unless --allow drop-comment", async () => { + await pool.query(`COMMENT ON COLUMN ${TABLE}.qty IS 'set by a DBA'`); + try { + const verified = await captured(args("verify")); + expect(verified.exit).toBe(1); + expect(verified.out).toContain("comment"); + + const kept = await captured(args("migrate", "--from-db", "--dry-run")); + expect(kept.exit).toBe(0); + expect(kept.out).not.toContain("IS NULL"); + expect(kept.out).toContain(`kept the comment on ${TABLE}.qty`); + expect(kept.out).toContain("--allow drop-comment"); + + const cleared = await captured(args("migrate", "--from-db", "--dry-run", "--allow", "drop-comment")); + expect(cleared.exit).toBe(0); + expect(cleared.out).toContain("COMMENT ON COLUMN"); + expect(cleared.out).toContain("IS NULL;"); + expect(cleared.out).not.toContain("kept the comment"); + } finally { + await pool.query(`COMMENT ON COLUMN ${TABLE}.qty IS NULL`); + } + }); +}); diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index c1cd4a1575..d73e4b0c30 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -275,7 +275,9 @@ export async function diff( else if (args.exprCanonProbe !== undefined) { canonProbe = args.exprCanonProbe; exprCanonPath = "probe"; } } const probed = await probeExprCanon(canonProbe, args.dialect, changes, expectedTables, actualTables); - const canon: ExprCanon = shadow !== undefined ? { ...probed, catalog: true } : probed; + // A snapshot older than formatVersion 4 never recorded identity or comments: unknown, not absent. + const catalog = shadow !== undefined && given.expected.meta?.identityAndCommentsUnknown !== true; + const canon: ExprCanon = catalog ? { ...probed, catalog: true } : probed; const { renamed, predecessors } = diffRenamedTables(changes, expectedTables, actualTables, args.dialect, canon); // Pass 2: tables in both → compare columns/indexes/FKs diff --git a/server/typescript/packages/migrate-ts/src/diff/status.ts b/server/typescript/packages/migrate-ts/src/diff/status.ts index 0e1b1712c5..672bb42b17 100644 --- a/server/typescript/packages/migrate-ts/src/diff/status.ts +++ b/server/typescript/packages/migrate-ts/src/diff/status.ts @@ -171,7 +171,10 @@ function blockFor( + `--allow drop-identity-default if removing it is intentional`); case "change-comment": - return null; + if (c.from === undefined || c.to !== undefined) return null; + return gate(allow, "dropComment", `${c.column !== undefined ? `column "${c.table}"."${c.column}"` : `table "${c.table}"`} ` + + `has a comment its metadata does not declare. Clearing it cannot be undone. Declare it as the ` + + `description, or pass --allow drop-comment to clear it`); // Always-allowed kinds case "create-table": diff --git a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts index 86bf8e1a66..16008bfd43 100644 --- a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts @@ -229,19 +229,14 @@ export function parsePgDefault(raw: string | null | undefined): ColumnDefault | return { kind: "expr", value: raw }; } - // If it starts with a single-quote, it's a quoted literal (possibly with - // a trailing ::type cast that PG appends for type clarity). - if (raw.startsWith("'")) { - // Strip the cast suffix (e.g. `::boolean`, `::text`, `::integer`) - const withoutCast = raw.replace(/::[^']+$/, ""); - // Strip the surrounding single-quotes, then un-double the embedded `''` - // escapes PG stores verbatim (e.g. `'don''t'::text`) — without this the - // introspected value never equals the expected literal and the diff issues - // a bogus SET DEFAULT on every run. - const m = /^'(.*)'$/.exec(withoutCast); - const cleaned = m !== null ? m[1]!.replace(/''/g, "'") : withoutCast; - return { kind: "literal", value: cleaned }; - } + // ONE quoted literal, possibly with a trailing ::type cast that PG appends for type + // clarity, is a literal: the cast is stripped and the embedded `''` escapes PG stores + // verbatim (e.g. `'don''t'::text`) are un-doubled — without this the introspected value + // never equals the expected literal and the diff issues a bogus SET DEFAULT on every run. + // Anything more that starts with a quote (`'a'::text || 'b'::text`) is an expression. + const literal = /^'((?:[^']|'')*)'(?:::[\w ."]+(?:\(\d+(?:,\s*\d+)?\))?(?:\[\])*)*$/.exec(raw); + if (literal !== null) return { kind: "literal", value: literal[1]!.replace(/''/g, "'") }; + if (raw.startsWith("'")) return { kind: "expr", value: raw }; // Anything else that contains a :: cast is a complex expression // (e.g. NULL::text, ARRAY[]::text[]) diff --git a/server/typescript/packages/migrate-ts/src/snapshot/serialize.ts b/server/typescript/packages/migrate-ts/src/snapshot/serialize.ts index 1b94b2a121..74acfedd65 100644 --- a/server/typescript/packages/migrate-ts/src/snapshot/serialize.ts +++ b/server/typescript/packages/migrate-ts/src/snapshot/serialize.ts @@ -20,8 +20,14 @@ import type { SchemaSnapshot } from "../types.js"; * place is asked of the live database (view-replace.ts). It is no longer written, and * `parseSnapshot` drops it. The version does not move: a reader that still looks for * the field finds it absent, which it already treats as unknown. + * + * v4 — a snapshot records a table's and column's comment and a GENERATED identity as the + * database holds them: `baseline --from-db` now reads them back (`pg_description`, + * `attidentity`), where a v3 baseline left both out. Absence therefore means "none" only from + * v4 on; `parseSnapshot` marks an older snapshot `identityAndCommentsUnknown`, and the shadow + * comparison does not compare what it never recorded. */ -export const SNAPSHOT_FORMAT_VERSION = 3; +export const SNAPSHOT_FORMAT_VERSION = 4; interface SnapshotFile { formatVersion: number; @@ -50,10 +56,18 @@ function canonicalize(s: SchemaSnapshot): SchemaSnapshot { delete written.opaque; return written; }), - ...(s.meta ? { meta: s.meta } : {}), + ...metaWritten(s.meta), }; } +/** `meta` as written: the parse-time `identityAndCommentsUnknown` marker is not part of the file. */ +function metaWritten(meta: SchemaSnapshot["meta"]): Pick { + if (meta === undefined) return {}; + const written = { ...meta }; + delete written.identityAndCommentsUnknown; + return Object.keys(written).length > 0 ? { meta: written } : {}; +} + /** JSON.stringify with object keys sorted recursively (arrays left as-is). */ function stableStringify(value: unknown): string { return JSON.stringify( @@ -99,6 +113,9 @@ export function parseSnapshot(text: string): SchemaSnapshot { } } } + if (file.formatVersion < 4) { + file.snapshot.meta = { ...file.snapshot.meta, identityAndCommentsUnknown: true }; + } for (const v of file.snapshot.views ?? []) delete (v as { columns?: unknown }).columns; return file.snapshot; } diff --git a/server/typescript/packages/migrate-ts/src/types.ts b/server/typescript/packages/migrate-ts/src/types.ts index b37a45a8b5..b08fb361db 100644 --- a/server/typescript/packages/migrate-ts/src/types.ts +++ b/server/typescript/packages/migrate-ts/src/types.ts @@ -18,6 +18,12 @@ export interface SchemaSnapshot { export interface SnapshotMeta { sqliteVersion?: string; // e.g., "3.44.2"; only set for SQLite snapshots + /** + * Set by `parseSnapshot` on a snapshot written before formatVersion 4, which did not record + * comments or a GENERATED identity read off the database: there, an absent one is unknown + * rather than none, so the shadow comparison leaves both out. Never written to a file. + */ + identityAndCommentsUnknown?: true; } export interface TableDescriptor { @@ -277,7 +283,10 @@ export type Change = /** Postgres, shadow path only: an `increment` identity (`GENERATED BY DEFAULT AS IDENTITY`) added or dropped. */ | { kind: "change-column-identity"; table: string; schema?: string; column: string; from?: "increment"; to?: "increment"; status: ChangeStatus } - /** Postgres, shadow path only: a table comment (no `column`) or column comment set, changed or cleared. */ + /** + * Postgres, shadow path only: a table comment (no `column`) or column comment set, changed or + * cleared. Clearing one the model does not declare is gated behind `allow.dropComment`. + */ | { kind: "change-comment"; table: string; schema?: string; column?: string; from?: string; to?: string; status: ChangeStatus } | { kind: "add-index"; table: string; schema?: string; index: IndexDescriptor; status: ChangeStatus } @@ -487,6 +496,12 @@ export interface AllowOptions { * skips the default-diff for a live auto-sequence default entirely.) */ dropIdentityDefault?: boolean; + /** + * Permits clearing a live table or column comment the model does not declare + * (`COMMENT ON … IS NULL`). Somebody else (a DBA, another tool) may have written it, and + * nothing can restore it once cleared. + */ + dropComment?: boolean; /** * Permits dropping an object the COMMITTED SNAPSHOT never contained — i.e. one * this toolchain never managed. Without it, such a drop is refused at generation diff --git a/server/typescript/packages/migrate-ts/test/check/serialize-upgrade.test.ts b/server/typescript/packages/migrate-ts/test/check/serialize-upgrade.test.ts index 84c12dd6e6..2e24b1b504 100644 --- a/server/typescript/packages/migrate-ts/test/check/serialize-upgrade.test.ts +++ b/server/typescript/packages/migrate-ts/test/check/serialize-upgrade.test.ts @@ -16,8 +16,19 @@ describe("serialize with checks + snapshot format version", () => { // not understand them would silently fall back to comparing view TEXT against // Postgres's deparsed output — which never matches — and re-propose every view on // every migrate. So the version must be bumped, to hard-fail instead of misread. - test("format version is 3", () => { - expect(SNAPSHOT_FORMAT_VERSION).toBe(3); + // v4: comments and a GENERATED identity are recorded as the database holds them. + test("format version is 4", () => { + expect(SNAPSHOT_FORMAT_VERSION).toBe(4); + }); + + // Before v4 a `baseline --from-db` left comments and identity out, so in a v3 file an absent + // one says nothing; from v4 on it means none. The marker lives only in memory. + test("a v3 snapshot reads its identity and comments as unknown; a v4 one does not, and the marker is never written", () => { + const body = { tables: [], views: [] }; + const v3 = parseSnapshot(JSON.stringify({ formatVersion: 3, snapshot: body })); + expect(v3.meta?.identityAndCommentsUnknown).toBe(true); + expect(JSON.parse(serializeSnapshot(v3))).toEqual({ formatVersion: 4, snapshot: body }); + expect(parseSnapshot(JSON.stringify({ formatVersion: 4, snapshot: body })).meta).toBeUndefined(); }); test("checks round-trip and are order-stable", () => { @@ -60,7 +71,7 @@ describe("serialize with checks + snapshot format version", () => { const parsed = parseSnapshot(older); expect(parsed.views).toEqual([{ name: "v_orders", sql: "SELECT 1 AS n", fingerprint: "a".repeat(64) }]); expect(JSON.parse(serializeSnapshot(parsed))).toEqual({ - formatVersion: 3, + formatVersion: 4, snapshot: { tables: [], views: [{ name: "v_orders", sql: "SELECT 1 AS n", fingerprint: "a".repeat(64) }] }, }); }); diff --git a/server/typescript/packages/migrate-ts/test/expected-schema-scope.test.ts b/server/typescript/packages/migrate-ts/test/expected-schema-scope.test.ts index d2a3e0ca12..f1fac5969a 100644 --- a/server/typescript/packages/migrate-ts/test/expected-schema-scope.test.ts +++ b/server/typescript/packages/migrate-ts/test/expected-schema-scope.test.ts @@ -295,7 +295,7 @@ describe("an accepted scoped run keeps out-of-scope entries in the committed sna }); describe("provenance never reaches the committed snapshot", () => { - test("a view's declaring FQN is recorded but never serialized; formatVersion stays 3", async () => { + test("a view's declaring FQN is recorded but never serialized; formatVersion stays 4", async () => { const root = await loadBoth(); const view = { name: "v_jobs", sql: "SELECT id FROM jobs", dependsOn: ["jobs"] }; @@ -308,7 +308,7 @@ describe("provenance never reaches the committed snapshot", () => { // The provenance thread is invisible on disk: carrying it changes no bytes. expect(serializeSnapshot(withFqn.snapshot)).toBe(serializeSnapshot(withoutFqn)); expect(serializeSnapshot(withFqn.snapshot)).not.toContain("fqn"); - expect(SNAPSHOT_FORMAT_VERSION).toBe(3); + expect(SNAPSHOT_FORMAT_VERSION).toBe(4); // ...and it IS recorded, keyed like every other qualified name. expect(withFqn.provenance.get("public.v_jobs")).toBe("acme::platform::JobSummary"); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts index 48ea0b6465..06634efcd3 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-shadow-schema-wide.test.ts @@ -56,6 +56,7 @@ describe("shadow path is schema-wide (live PG)", () => { CREATE VIEW sc2_v AS SELECT id, label FROM sc2_parent WHERE id > 0`).execute(main.db); }); afterAll(async () => { + await sql.raw(`DROP SCHEMA IF EXISTS sc3 CASCADE`).execute(main.db); await sql.raw(`DROP VIEW IF EXISTS sc2_v; DROP TABLE IF EXISTS sc2_child; DROP TABLE IF EXISTS sc2_parent`).execute(main.db); await sql.raw(`DROP DATABASE IF EXISTS ${SHADOW_DB}`).execute(main.db); await main.pool.end(); @@ -138,7 +139,7 @@ describe("shadow path is schema-wide (live PG)", () => { const live = await introspectPostgres(main.db); const { changes } = await diff({ expected: changed, actual: live, dialect: "postgres", scopeSchemas: ["public"], shadowSchema: shadow, - allow: { dropIdentityDefault: true }, + allow: { dropIdentityDefault: true, dropComment: true }, }); expect(changes.map((c) => c.kind).sort()).toEqual(["change-column-identity", "change-comment", "change-comment"]); const { up } = emit(changes.filter((c) => c.kind === "change-column-identity" || c.kind === "change-comment"), { dialect: "postgres" }); @@ -153,4 +154,63 @@ describe("shadow path is schema-wide (live PG)", () => { }).catch((err) => { if (err !== ROLLBACK) throw err; }); expect(after).toEqual([]); }); + + // A new table on the shadow path is created from the shadow's catalog, so its CHECK, indexes, + // defaults, identity and comments are re-emitted in Postgres's spelling. Postgres is the oracle + // twice: it must accept the plan, and the table it built must read clean on the next diff. + test("a table created from the shadow catalog applies, and the next diff is clean", async () => { + const created: SchemaSnapshot = { + views: [], + tables: [ + { + name: "sc3_parent", schema: "sc3", indexes: [], foreignKeys: [], checks: [], primaryKey: ["id"], + description: "Parents", + columns: [ + { name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false, identity: "increment" }, + { name: "name", sqlType: { kind: "text", maxLength: 40 }, nullable: false, description: "Shown name" }, + ], + }, + { + name: "sc3_child", schema: "sc3", primaryKey: ["id"], + columns: [ + { name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false }, + { name: "parent_id", sqlType: { kind: "integer", bits: 32 }, nullable: true }, + { name: "qty", sqlType: { kind: "integer", bits: 32 }, nullable: false, default: { kind: "literal", value: "007" } }, + { name: "label", sqlType: { kind: "text" }, nullable: false, default: { kind: "expr", value: "'a'::text || 'b'" } }, + { name: "made", sqlType: { kind: "timestamp", withTimezone: true }, nullable: false, default: { kind: "expr", value: "NOW() + interval '1 day'" } }, + ], + checks: [{ name: "sc3_child_qty_chk", expression: `"qty" > 0 AND ("qty" < 100 OR "label" IN ('ab', 'big'))` }], + indexes: [ + { name: "sc3_child_label_idx", columns: [], unique: false, expr: `lower("label")` }, + { name: "sc3_child_big_idx", columns: ["qty"], unique: false, where: `"qty" > 10` }, + ], + foreignKeys: [{ name: "sc3_child_parent_fk", columns: ["parent_id"], refTable: "sc3_parent", refColumns: ["id"], onDelete: "cascade" }], + }, + ], + }; + const live = await introspectPostgres(main.db); + const plan = await diff({ expected: created, actual: live, dialect: "postgres", scopeSchemas: ["sc3"], shadowSchema: shadow }); + expect(plan.exprCanonPath).toBe("shadow"); + const { up } = emit(plan.changes, { dialect: "postgres" }); + const ROLLBACK = new Error("rollback"); + let after: string[] | undefined; + let row: { qty: number; label: string; tomorrow: boolean } | undefined; + await main.db.transaction().execute(async (trx) => { + await sql.raw(up).execute(trx); + await sql.raw(`INSERT INTO sc3.sc3_parent (name) VALUES ('p')`).execute(trx); + const inserted = await sql<{ qty: number; label: string; tomorrow: boolean }>` + INSERT INTO sc3.sc3_child (id, parent_id) SELECT 1, id FROM sc3.sc3_parent + RETURNING qty, label, made > now() AS tomorrow`.execute(trx); + row = inserted.rows[0]; + const r = await diff({ + expected: created, actual: await introspectPostgres(trx as unknown as Db), dialect: "postgres", + scopeSchemas: ["sc3"], shadowSchema: shadow, + }); + after = r.changes.map((c) => c.kind); + throw ROLLBACK; + }).catch((err) => { if (err !== ROLLBACK) throw err; }); + // The defaults Postgres evaluates are the authored ones: 007 is 7, and 'a' || 'b' is 'ab'. + expect(row).toEqual({ qty: 7, label: "ab", tomorrow: true }); + expect(after).toEqual([]); + }); }); diff --git a/server/typescript/packages/migrate-ts/test/unit/diff-status-comment.test.ts b/server/typescript/packages/migrate-ts/test/unit/diff-status-comment.test.ts new file mode 100644 index 0000000000..aa1cd5b477 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/unit/diff-status-comment.test.ts @@ -0,0 +1,39 @@ +/** + * A live comment the metadata does not declare may have been written by somebody else, and a + * `COMMENT ON … IS NULL` cannot be undone, so clearing one is gated behind allow.dropComment. + * Setting or changing a comment the metadata declares is not. + */ +import { describe, expect, test } from "bun:test"; +import { applyStatus } from "../../src/diff/status.js"; +import { BlockedChangesError } from "../../src/errors.js"; +import { emit } from "../../src/emit/index.js"; +import type { Change } from "../../src/types.js"; + +const comment = (from: string | undefined, to: string | undefined, column?: string): Change => ({ + kind: "change-comment", table: "orders", ...(column !== undefined ? { column } : {}), + ...(from !== undefined ? { from } : {}), ...(to !== undefined ? { to } : {}), + status: { state: "allowed" }, +}); + +describe("change-comment status", () => { + test("clearing an undeclared comment is blocked behind allow.dropComment", () => { + const changes = [comment("set by a DBA", undefined, "total")]; + applyStatus(changes, {}); + expect(changes[0]!.status).toMatchObject({ state: "blocked", blockedBy: "dropComment" }); + expect(changes[0]!.status.blockedReason).toContain("--allow drop-comment"); + expect(() => emit(changes, { dialect: "postgres" })).toThrow(BlockedChangesError); + }); + + test("allow.dropComment lets the clear through, as COMMENT ON … IS NULL", () => { + const changes = [comment("set by a DBA", undefined)]; + applyStatus(changes, { dropComment: true }); + expect(changes[0]!.status.state).toBe("allowed"); + expect(emit(changes, { dialect: "postgres" }).up).toBe(`COMMENT ON TABLE "orders" IS NULL;`); + }); + + test("setting or changing a declared comment is not gated", () => { + const changes = [comment(undefined, "Orders"), comment("Old", "New", "total")]; + applyStatus(changes, {}); + expect(changes.map((c) => c.status.state)).toEqual(["allowed", "allowed"]); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/unit/introspect-postgres-helpers.test.ts b/server/typescript/packages/migrate-ts/test/unit/introspect-postgres-helpers.test.ts index 5b4f12e5dd..324be0a7f7 100644 --- a/server/typescript/packages/migrate-ts/test/unit/introspect-postgres-helpers.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/introspect-postgres-helpers.test.ts @@ -113,4 +113,15 @@ describe("parsePgDefault", () => { test("a bare literal with no quotes and no cast is kind=literal", () => { expect(parsePgDefault("42")).toEqual({ kind: "literal", value: "42" }); }); + + // Postgres stores `DEFAULT 'a' || 'b'` as `('a'::text || 'b'::text)` and a bare `'a'::text || 'b'` + // as `'a'::text || 'b'::text`: an expression that merely starts with a quoted literal. + test("an expression that starts with a quoted literal is kind=expr, not the text between its first and last quote", () => { + expect(parsePgDefault("'a'::text || 'b'::text")).toEqual({ kind: "expr", value: "'a'::text || 'b'::text" }); + expect(parsePgDefault("'2024-01-01'::date + 1")).toEqual({ kind: "expr", value: "'2024-01-01'::date + 1" }); + expect(parsePgDefault("'don''t'::text")).toEqual({ kind: "literal", value: "don't" }); + expect(parsePgDefault("'x'::character varying")).toEqual({ kind: "literal", value: "x" }); + expect(parsePgDefault("'1.50'::numeric(10,2)")).toEqual({ kind: "literal", value: "1.50" }); + expect(parsePgDefault("'{}'::text[]")).toEqual({ kind: "literal", value: "{}" }); + }); }); diff --git a/server/typescript/packages/sdk/src/config.ts b/server/typescript/packages/sdk/src/config.ts index a39c274631..dd263d17f2 100644 --- a/server/typescript/packages/sdk/src/config.ts +++ b/server/typescript/packages/sdk/src/config.ts @@ -49,6 +49,7 @@ export const AllowTokenEnum = z.enum([ "nullable-to-not-null", "drop-identity-default", "drop-unmanaged", + "drop-comment", ]); // .strict(), like every other object in this schema: `.partial()` alone leaves From 4271a7506dcf1228cb374b165fa015fc67a5b62d Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 23:57:25 -0400 Subject: [PATCH 6/8] no-mistakes(test): Make plain migrate say it skips the database and refuse --shadow-db --- CHANGELOG.md | 4 +- docs/features/migrations-and-drift.md | 2 +- .../packages/cli/src/commands/migrate.ts | 15 +- .../shadow-default-drift-pg.test.ts | 131 ++++++++++++++++++ 4 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8da6891037..7494cde3f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -318,7 +318,9 @@ here.** cannot be opened or applied is an error naming the cause. When a `CREATE`d one is not available, the rolled-back temp-object probe on the target answers (it needs write privileges and takes brief locks), and SQLite and offline plans keep the string normaliser. Every Postgres run says - which path ran and, on a fallback, why. Clearing a live comment the metadata does not declare + which path ran and, on a fallback, why: a plain `meta migrate` (no `--from-db` or `--apply`) + says it compared the committed snapshot and read no database, and refuses `--shadow-db`, which + it would otherwise have ignored. Clearing a live comment the metadata does not declare needs the new `--allow drop-comment`; without it `meta migrate` keeps the comment, plans the rest and says so, and `meta verify --db` still reports it. **Upgrade note:** the committed snapshot moves to `formatVersion` 4, which records comments and GENERATED identity read off the diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 0497e8f75b..e0e5444312 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -411,7 +411,7 @@ the database. A real change of any of them is still reported. The scratch databa | Source | How | |---|---| -| An explicit URL | `--shadow-db ` on `verify` and `migrate`, else `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl` in `.metaobjects/config.json`. Used as given and never dropped; nothing is left in it because the schema is applied inside a rolled-back transaction. | +| An explicit URL | `--shadow-db ` on `verify` and on `migrate --from-db` / `--apply` (a plain `meta migrate` reads no database, says so, and refuses the flag), else `SHADOW_DATABASE_URL`, else `migrate.shadowDatabaseUrl` in `.metaobjects/config.json`. Used as given and never dropped; nothing is left in it because the schema is applied inside a rolled-back transaction. | | `CREATE DATABASE` | With none of the above, a database named `mo_shadow_` is created on the `--db` server when the role may, and **always dropped** afterwards, including on error. | A run opens its scratch database once, and every comparison it makes uses it: `meta verify --db` diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index 072514aaed..ae661808de 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -705,6 +705,13 @@ export async function migrateCommand( // when explicitly requested via --from-db, when --apply needs a connection, or // for --rollback (which runs hand-authored down.sql against the live DB). if (!config.fromDb && !config.apply && config.rollback === undefined) { + // The scratch database is one side of a comparison with a live database, which this + // path never reads; a --shadow-db it silently ignored would read as a clean comparison. + if (flags.shadowDb !== undefined) { + log.error(`migrate: --shadow-db needs --from-db (or --apply): without it migrate compares the metadata with the committed snapshot and reads no database`); + emitStructuredError("migrate: --shadow-db needs --from-db", "re-run with --from-db to compare the metadata with the database", fmt); + return 2; + } return await runOfflineGenerate(config, metaRoot, fmt, genRoot); } @@ -1562,6 +1569,12 @@ export async function runOfflineGenerate( logOutOfScope(plan.outOfScope, plan.importedOutOfScope ?? [], fmt); const offlineNotes = viewRecreateNotes(diffResult.changes); + // Which comparison ran (every Postgres run says so): this one never reads the database, so + // a change somebody made there by hand is not seen here. + if (config.dialect === "postgres") { + offlineNotes.unshift("compared the metadata with the committed snapshot; no database was read, so a change " + + "made directly in the database is not seen here (meta migrate --from-db or meta verify --db compares with it)."); + } const offlineResult = (extra: Partial): MigrateResultShape => ({ dialect: offlineDialect, displayUrl: "", @@ -1578,7 +1591,7 @@ export async function runOfflineGenerate( return 1; } if (diffResult.changes.length === 0) { - finishMigrate(fmt, [`migrate: no changes`], migrateResultToData(offlineResult({}))); + finishMigrate(fmt, [`migrate: no changes`, ...narratedNotes(offlineNotes)], migrateResultToData(offlineResult({}))); return 0; } const offlineWarnings = warnDataHazards(diffResult.hazards); diff --git a/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts b/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts new file mode 100644 index 0000000000..e986483b2e --- /dev/null +++ b/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts @@ -0,0 +1,131 @@ +/** + * Real column-default drift on a live Postgres database reaches the same verdict through + * `meta verify --db` and `meta migrate --from-db --dry-run`, both on the shadow path: a + * default somebody changed by hand (an integer 7 made 8, `now()` made `now() + 1 day`) is + * reported by verify and planned by migrate, while the defaults exactly as the metadata + * declares them read clean. The oracle is the ALTER TABLE the test runs itself. + * + * An explicitly configured scratch database that cannot be reached is a hard error naming + * the cause in both commands, never a silent fall-back to another comparison. Plain + * `meta migrate --dry-run` (no --from-db) reads no database at all: it says so, and it + * refuses a --shadow-db it would otherwise have ignored. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ +import { describe, test, expect, beforeAll, afterAll } from "bun:test"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Pool } from "pg"; +import { run } from "../../src/index.js"; + +const PG_URL = process.env.MIGRATE_TS_PG_URL; +const TABLE = "shadow_default_drift_events"; +const UNREACHABLE_SHADOW = "postgresql://nobody:nothing@127.0.0.1:1/nope"; + +const MODEL = JSON.stringify({ + "metadata.root": { + children: [{ + "object.entity": { + name: "Event", + children: [ + { "source.rdb": { name: "src", "@table": TABLE } }, + { "field.long": { name: "id" } }, + { "field.int": { name: "seq", "@default": 7, "@required": true } }, + { "field.timestamp": { name: "startsAt", "@column": "starts_at", "@default": "NOW()", "@required": true } }, + { "identity.primary": { name: "pk", "@fields": ["id"] } }, + ], + }, + }], + }, +}); + +describe("shadow path: live default drift and an unreachable explicit shadow (env-gated)", () => { + if (PG_URL === undefined || PG_URL.length === 0) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + let pool: Pool; + let root: string; + const args = (...rest: string[]): string[] => [...rest, "--cwd", root, "--db", PG_URL, "--dialect", "postgres"]; + + /** Run the CLI, collecting everything it prints. */ + const captured = async (argv: string[]): Promise<{ exit: number; out: string }> => { + const lines: string[] = []; + const origLog = console.log; + const origErr = console.error; + const origWarn = console.warn; + const keep = (...a: unknown[]): void => { lines.push(a.map(String).join(" ")); }; + console.log = keep; console.error = keep; console.warn = keep; + try { + return { exit: await run(argv), out: lines.join("\n") }; + } finally { + console.log = origLog; console.error = origErr; console.warn = origWarn; + } + }; + + beforeAll(async () => { + pool = new Pool({ connectionString: PG_URL }); + await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); + root = mkdtempSync(join(tmpdir(), "shadow-default-drift-pg-")); + mkdirSync(join(root, "metaobjects"), { recursive: true }); + writeFileSync(join(root, "metaobjects", "meta.events.json"), MODEL, "utf8"); + expect((await captured(args("migrate", "--from-db", "--slug", "init", "--apply"))).exit).toBe(0); + }); + afterAll(async () => { + await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); + await pool.end(); + rmSync(root, { recursive: true, force: true }); + }); + + test("the defaults as declared read clean through both commands", async () => { + expect((await captured(args("verify"))).exit).toBe(0); + const planned = await captured(args("migrate", "--from-db", "--dry-run")); + expect(planned.exit).toBe(0); + expect(planned.out).not.toContain("SET DEFAULT"); + }); + + test("seq 8 vs 7 and starts_at now() + 1 day vs now() are drift for verify and planned by migrate", async () => { + await pool.query(`ALTER TABLE ${TABLE} ALTER COLUMN seq SET DEFAULT 8`); + await pool.query(`ALTER TABLE ${TABLE} ALTER COLUMN starts_at SET DEFAULT now() + interval '1 day'`); + try { + const verified = await captured(args("verify")); + expect(verified.exit).toBe(1); + expect(verified.out).toContain("seq"); + expect(verified.out).toContain("starts_at"); + + const planned = await captured(args("migrate", "--from-db", "--dry-run")); + expect(planned.exit).toBe(0); + expect(planned.out).toContain("compared against a scratch (shadow) database"); + expect(planned.out).toMatch(/ALTER COLUMN \\?"seq\\?" SET DEFAULT '?7'?;/); + expect(planned.out).toMatch(/ALTER COLUMN \\?"starts_at\\?" SET DEFAULT now\(\);/); + + // Without --from-db, migrate diffs the metadata against the committed snapshot and + // never reads the database: it plans nothing here, and says so rather than reading + // as a clean comparison with the database. + const offline = await captured(args("migrate", "--dry-run")); + expect(offline.exit).toBe(0); + expect(offline.out).toContain("no database was read"); + expect(offline.out).toContain("--from-db"); + } finally { + await pool.query(`ALTER TABLE ${TABLE} ALTER COLUMN seq SET DEFAULT 7`); + await pool.query(`ALTER TABLE ${TABLE} ALTER COLUMN starts_at SET DEFAULT now()`); + } + }); + + test("an explicit --shadow-db that cannot be reached is a hard error in migrate and verify", async () => { + // A run that reads no database refuses a --shadow-db it would otherwise ignore. + const offline = await captured(args("migrate", "--dry-run", "--shadow-db", UNREACHABLE_SHADOW)); + expect(offline.exit).toBe(2); + expect(offline.out).toContain("--shadow-db needs --from-db"); + for (const argv of [ + args("migrate", "--from-db", "--dry-run", "--shadow-db", UNREACHABLE_SHADOW), + args("verify", "--shadow-db", UNREACHABLE_SHADOW), + ]) { + const r = await captured(argv); + expect(r.exit).not.toBe(0); + expect(r.out).toContain("shadow database"); + expect(r.out).toContain("127.0.0.1:1"); + } + }); +}); From f6828fe79bf406be0052589abab1a472e98e412c Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Thu, 29 Oct 2026 01:15:31 -0400 Subject: [PATCH 7/8] no-mistakes(document): Docs updated for shadow-db drift path: CLI README, CHANGELOG 1.2.0 heading, drift table row. --- CHANGELOG.md | 2 +- docs/features/migrations-and-drift.md | 2 +- server/typescript/packages/cli/README.md | 1 + .../packages/cli/test/integration/shadow-catalog-pg.test.ts | 2 +- .../cli/test/integration/shadow-default-drift-pg.test.ts | 2 +- 5 files changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7494cde3f7..312a8d2822 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ promises the **software** surface, `metamodelVersion` promises the **metadata** `^1.0.0` accepts a MINOR — so **a release that moves `metamodelVersion` says so in its entry here.** -## [Unreleased] +## [1.2.0] — Unreleased ### Docs diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index e0e5444312..790692ea09 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -13,7 +13,7 @@ There are **7 drift sources**, and the toolchain has a guard for each. |---|---|---| | **Code-vs-DB** | Codegen — the generated SQL DDL is emitted from the same metadata as the entity / table code. | Build time | | **Code-vs-API-doc** | Cross-port codegen from the same metadata. | Build time | -| **DB-vs-metadata** | `meta verify --db` (TS CLI) — introspects the live DB and fails if it has drifted from metadata. Includes modeled projection **view bodies** (a changed `CREATE VIEW` is drift, reported with what differs: the first differing excerpt of the text on SQLite/D1, a fingerprint mismatch on Postgres). A view `meta migrate` drops and recreates only because the migration alters a table it reads, with its definition unchanged, is not drift and is not reported; a hand-authored *unmodeled* view is unmanaged and never flagged. A schema concern owned by the Node toolchain regardless of server language; on the JVM ports the runtime auto-create/validator path was removed (ADR-0015) and the `metaobjects:verify` Maven goal is not available. Cloudflare D1 has no client wire protocol, so it can't go through `--db`'s Kysely-driver introspection — use `meta verify --dialect d1 [--d1 ] [--remote]` instead (the same wrangler-shelled-out path `meta migrate --dialect d1` uses); `--remote` is required to check the *deployed* database, not the local `wrangler dev` shadow copy. Pointing `--db file:` at wrangler's local D1 state directory (`.wrangler/state/**/d1/**`) still runs, but only verifies that local copy — `verify` warns when it detects this. | CI on every PR | +| **DB-vs-metadata** | `meta verify --db` (TS CLI) — introspects the live DB and fails if it has drifted from metadata. Includes modeled projection **view bodies** (a changed `CREATE VIEW` is drift, reported with what differs: the first differing excerpt of the text on SQLite/D1, the deparsed body on Postgres's scratch-database path, a fingerprint mismatch otherwise). A view `meta migrate` drops and recreates only because the migration alters a table it reads, with its definition unchanged, is not drift and is not reported; a hand-authored *unmodeled* view is unmanaged and never flagged. A schema concern owned by the Node toolchain regardless of server language; on the JVM ports the runtime auto-create/validator path was removed (ADR-0015) and the `metaobjects:verify` Maven goal is not available. Cloudflare D1 has no client wire protocol, so it can't go through `--db`'s Kysely-driver introspection — use `meta verify --dialect d1 [--d1 ] [--remote]` instead (the same wrangler-shelled-out path `meta migrate --dialect d1` uses); `--remote` is required to check the *deployed* database, not the local `wrangler dev` shadow copy. Pointing `--db file:` at wrangler's local D1 state directory (`.wrangler/state/**/d1/**`) still runs, but only verifies that local copy — `verify` warns when it detects this. | CI on every PR | | **Migration-vs-metadata** | The Node `meta migrate` emits migrations FROM metadata diffs — they cannot drift from metadata by construction. Schema migrations for **every** port are owned by this Node toolchain (`@metaobjectsdev/cli migrate`, ADR-0015); the C# and Python migrate surfaces were removed. | Build time | | **Generated-edited** | `@generated` headers in emitted code + a committed hash manifest that refuses an edited file by name; in TypeScript only, a three-way merge also preserves hand-edits on the machine that generated the file. | Code review | | **Prompt-vs-payload** | FR-004 `Renderer.verify` parses `{{...}}` references in templates and checks each one exists on the payload VO. | Build time + runtime | diff --git a/server/typescript/packages/cli/README.md b/server/typescript/packages/cli/README.md index cf0715839b..629d6e6848 100644 --- a/server/typescript/packages/cli/README.md +++ b/server/typescript/packages/cli/README.md @@ -173,6 +173,7 @@ Flags: - `--db ` (required or via `$DATABASE_URL` or `.metaobjects/config.json`) - Supported schemes: `file:`, `libsql:`, `postgres:`, `postgresql:` - `--dialect sqlite|postgres|d1` — auto-detected from URL scheme; use `d1` for Cloudflare D1 +- `--shadow-db ` — Postgres scratch database the expected schema is applied to for `--from-db` / `--apply` drift (also `SHADOW_DATABASE_URL` or `migrate.shadowDatabaseUrl`). Read [How CHECK constraints and expression indexes are compared](../../../../docs/features/migrations-and-drift.md#how-check-constraints-and-expression-indexes-are-compared) for the three paths. - `--out-dir ` (default `./.metaobjects/migrations`) - `--slug ` — required when changes are pending (e.g., `add-user-shipping`) - `--allow ` — destructive-change permissions: `drop-column,drop-table,type-change,drop-index,drop-fk,drop-check,drop-view,drop-view-cascade,adopt-view,nullable-to-not-null,drop-identity-default,drop-unmanaged,drop-comment` diff --git a/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts b/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts index d34b5f341a..13f5226776 100644 --- a/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts +++ b/server/typescript/packages/cli/test/integration/shadow-catalog-pg.test.ts @@ -78,7 +78,7 @@ describe("shadow catalog: snapshot versions and undeclared comments (env-gated)" mkdirSync(join(root, "metaobjects"), { recursive: true }); writeFileSync(join(root, "metaobjects", "meta.catalog.json"), MODEL, "utf8"); // Day 1: the table, with its comments and a GENERATED identity, and a v4 snapshot of it. - expect((await captured(args("migrate", "--from-db", "--slug", "init", "--apply"))).exit).toBe(0); + expect((await captured(args("migrate", "--from-db", "--slug", "shadow-catalog", "--apply"))).exit).toBe(0); }); afterAll(async () => { await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); diff --git a/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts b/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts index e986483b2e..3b17479f97 100644 --- a/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts +++ b/server/typescript/packages/cli/test/integration/shadow-default-drift-pg.test.ts @@ -70,7 +70,7 @@ describe("shadow path: live default drift and an unreachable explicit shadow (en root = mkdtempSync(join(tmpdir(), "shadow-default-drift-pg-")); mkdirSync(join(root, "metaobjects"), { recursive: true }); writeFileSync(join(root, "metaobjects", "meta.events.json"), MODEL, "utf8"); - expect((await captured(args("migrate", "--from-db", "--slug", "init", "--apply"))).exit).toBe(0); + expect((await captured(args("migrate", "--from-db", "--slug", "shadow-default-drift", "--apply"))).exit).toBe(0); }); afterAll(async () => { await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); From 0abd08b74a8940752b69390dc8f4585e6afb5ddf Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Thu, 29 Oct 2026 01:17:11 -0400 Subject: [PATCH 8/8] no-mistakes(document): Restored [Unreleased] heading; added drop-unmanaged to migrate --allow help and snapshot --- CHANGELOG.md | 2 +- server/typescript/packages/cli/src/index.ts | 2 +- .../typescript/packages/cli/test/__snapshots__/cli.test.ts.snap | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 312a8d2822..7494cde3f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ promises the **software** surface, `metamodelVersion` promises the **metadata** `^1.0.0` accepts a MINOR — so **a release that moves `metamodelVersion` says so in its entry here.** -## [1.2.0] — Unreleased +## [Unreleased] ### Docs diff --git a/server/typescript/packages/cli/src/index.ts b/server/typescript/packages/cli/src/index.ts index d83ef32c40..8c4dba3bc5 100644 --- a/server/typescript/packages/cli/src/index.ts +++ b/server/typescript/packages/cli/src/index.ts @@ -178,7 +178,7 @@ MIGRATE FLAGS: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, adopt-view,nullable-to-not-null,drop-identity-default, - drop-comment + drop-comment,drop-unmanaged drop-column DELETES the column's data; for a renamed column use --rename-column, which keeps it --on-ambiguous abort|rename|drop-add Default abort diff --git a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap index 69e9cd685f..7e4af6bf54 100644 --- a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap +++ b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap @@ -158,7 +158,7 @@ MIGRATE FLAGS: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, adopt-view,nullable-to-not-null,drop-identity-default, - drop-comment + drop-comment,drop-unmanaged drop-column DELETES the column's data; for a renamed column use --rename-column, which keeps it --on-ambiguous abort|rename|drop-add Default abort