diff --git a/CHANGELOG.md b/CHANGELOG.md index 68b5a5ae8..e8fd38075 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -192,6 +192,34 @@ here.** ### Changed +- **On Postgres, `meta verify --db` and `meta migrate` ask Postgres whether a CHECK or an index + expression is the one stored, instead of comparing rewritten text.** For each table the plan + creates a temporary table with the live columns 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-index key and partial-index predicate as a temporary index, reads back + `pg_get_constraintdef`, `pg_get_indexdef` and `pg_get_expr`, and compares them with the live + definitions as plain strings. Every rewrite Postgres makes, including ones nobody has hand-coded, + now compares equal, and the string normaliser stops growing Postgres-shaped special cases. It + also ends the normaliser's two verified false negatives: a precedence change (`(a OR b) AND c` + against `a OR (b AND c)`) and a regrouped `NOT` (`NOT (a AND b)` against `NOT a AND b`) now report + as drift, as does the same change in an index `@where`. A database that held such a change and + verified clean before now reports it once: run `meta migrate` to bring it back. + The string normaliser stays, and decides, on SQLite and D1, in the offline `meta migrate` + (which has no database), on a read-only connection or hot standby, for a role without the `TEMP` + privilege, and for an expression that does not compile against the live columns. A probe that + cannot answer never reports drift or fails the command. An expression holding a `;` anywhere, or + an index `@using` that is not a plain access-method name, is never sent to the database. A refusal + for one table (such as no `SELECT` on it) gives that table no answer and does not stop the other + tables being probed. `meta migrate baseline --from-db` now records the generated text, not + Postgres's spelling, for a CHECK or an index expression the live database proves equal to the + metadata's, so the offline plan after a baseline compares like with like; it is best effort and + writes the introspected snapshot as before if the metadata does not load. + `meta verify --db` also uses the probe for its committed-snapshot check. New exports from + `@metaobjectsdev/migrate-ts`: `postgresExprCanonProbe`, `adoptGeneratedSpelling` and the + `ExprCanonProbe` types, with a `DiffArgs.exprCanonProbe` option. Schema comparison is + TypeScript-owned, so no other port carries this comparison. MySQL, SQL Server and Oracle are not + migrate dialects, so they have no probe. + - **A bare `verify` runs the same default gate set in every port: codegen drift, prompt templates and the requirement ledger.** Until now a bare run covered one gate: `meta verify` and `dotnet meta verify` ran only templates, so "run `verify` in CI" did not catch generated @@ -273,10 +301,10 @@ here.** `meta migrate` to bring it back. Separately, `char(n)` is no longer treated as a text column for the `(col)::text` fold: its own regex operator sees the blank padding the cast trims, and Postgres never adds the cast there, so an explicit `(col)::text ~ '…'` on a `char(n)` column - now reads as drift. A precedence change (`(a OR b) AND c` against `a OR (b AND c)`) is still - not detected; that needs Postgres to canonicalise the expected side. A new Postgres test - migrates, applies and verifies every CHECK shape `migrate` generates. Schema comparison is - TypeScript-owned, so no other port carries this comparison. + now reads as drift. A precedence change (`(a OR b) AND c` against `a OR (b AND c)`) is not + detected by the string comparison; on Postgres Postgres now answers that (see "Changed", the + CHECK probe). A new Postgres test migrates, applies and verifies every CHECK shape `migrate` + generates. Schema comparison is TypeScript-owned, so no other port carries this comparison. ### Documentation diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 20e187339..aab55a8a0 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -370,6 +370,45 @@ opened by an older SQLite (Python 3.10's stdlib, Ubuntu 22.04's `sqlite3`). The behind `--allow drop-check` and raises no data hazard, because the rule is the same — and the re-diff is empty afterwards. Postgres keeps `IS [NOT] DISTINCT FROM`. +## How CHECK constraints and expression indexes are compared + +Postgres rewrites an expression before it stores it (`"k" IN ('x')` becomes `k = 'x'::text`, +`a IS NOT DISTINCT FROM b` becomes `NOT (a IS DISTINCT FROM b)`, an `IN` list becomes +`= 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 +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. +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: + +- SQLite and D1, which store the text as written; +- the default **offline** `meta migrate`, which diffs the committed snapshot and has no database; +- a read-only connection or a hot standby, and a role without the `TEMP` privilege; +- a table the role cannot read (no `SELECT` on it, or no `USAGE` on its schema): that table only, + the other tables are still asked; +- an expected expression that does not compile against the live columns (the column diff reports + that case); +- an expression containing a `;` anywhere, quoted or not, or an index `@using` that is not a plain + access-method name: neither is ever sent to the database. + +The normaliser cannot tell a precedence change from a bracket-only change (it deletes every +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. + +`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 +compares like with like and a baselined project starts clean. Anything it cannot prove keeps +Postgres's text. If the metadata does not load, the baseline is written as before. + ## Drift verify commands per port | Port | Command | What it does | diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index eb907fae5..5f0200ddd 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -49,6 +49,7 @@ import { type AmbiguousResolution, type Change, type DataHazard, + type Dialect, type D1Binding, type EmitResult, type D1Runner, @@ -56,6 +57,8 @@ import { type SchemaSnapshot, type TableDescriptor, postgresViewReplaceProbe, + postgresExprCanonProbe, + adoptGeneratedSpelling, } from "@metaobjectsdev/migrate-ts"; import { buildWranglerExecuteArgs, @@ -825,6 +828,9 @@ export async function migrateCommand( // Postgres decides whether a changed view can be replaced in place: the probe // creates the new definition as a temporary view in a transaction it rolls back. ...(kysely.dialect === "postgres" ? { viewReplaceProbe: postgresViewReplaceProbe(kysely.db) } : {}), + // ...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) } : {}), allow: tokensToAllowOptions(config.allow), renames: config.renames, // #258 — adopting a live DB whose PRIMARY KEY differs from the metadata identity @@ -1113,6 +1119,54 @@ export async function migrateCommand( } } +/** + * Load the project's metadata the way `baseline` does: `metaobjects.config.ts` is read ONCE, up + * front, for the consumer providers/libraries (mirroring the DB path and `meta gen`, so + * config-registered custom subtypes resolve too, #157), and the result is deliberately NOT + * scoped — `baseline` records a STARTING POINT. + * + * Throws what loading throws; a `RemovedNamingConfigError` from the config propagates unchanged. + */ +async function loadBaselineMetadata(metaRoot: string, genRoot: string) { + let baselineLoadOptions: ReturnType = {}; + try { + const cfg = await loadMetaobjectsConfig(genRoot); + baselineLoadOptions = loadMemoryOptionsFrom(cfg); + } catch (e) { + if (e instanceof RemovedNamingConfigError) throw e; + // config absent — no custom providers, no libraries, default snake_case + } + const collection = await resolveCollection(metaRoot); + return await loadMemory(collection.configDir, { + ...collectionLoadOptions(collection), + ...baselineLoadOptions, + }); +} + +/** + * Postgres `baseline --from-db`: where the live database proves a CHECK or index expression is + * exactly what the metadata would generate, record the GENERATED text instead of Postgres's + * spelling of it, so the offline plan compares like with like (see migrate-ts + * `adoptGeneratedSpelling`). Best effort and never fatal: baseline worked without metadata + * before, so metadata that will not load, or a database that cannot answer the probe, leaves + * the introspected snapshot exactly as it was. + */ +async function withGeneratedSpelling( + snapshot: SchemaSnapshot, + db: Parameters[0], + dialect: Dialect, + metaRoot: string, + genRoot: string, +): Promise { + try { + const metadata = await loadBaselineMetadata(metaRoot, genRoot); + const expected = baselineFromMetadata(metadata, dialect, buildProjectionViews(metadata, { dialect })); + return await adoptGeneratedSpelling(snapshot, expected, postgresExprCanonProbe(db)); + } catch { + return snapshot; + } +} + /** * `meta migrate baseline [--from-db]` — seed the committed reference snapshot. * `--from-metadata` (default) derives it from metadata; `--from-db` introspects @@ -1150,6 +1204,9 @@ export async function runBaseline( try { 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); + } } finally { await kysely.close(); } @@ -1181,21 +1238,6 @@ export async function runBaseline( `baseline — this emits NO CREATE TABLE. Use it only if your database already matches ` + `your metadata; for a new/empty database run \`${greenfieldCreateCmd(config.dialect)}\` instead.`, ); - let metadata; - // Load metaobjects.config.ts ONCE, up front, for the consumer providers/libraries — - // mirroring the DB path (and `meta gen`) so offline baseline resolves - // config-registered custom subtypes too (#157). - let baselineLoadOptions: ReturnType = {}; - try { - const cfg = await loadMetaobjectsConfig(genRoot); - baselineLoadOptions = loadMemoryOptionsFrom(cfg); - } catch (e) { - if (e instanceof RemovedNamingConfigError) { - log.error(e.message); - return 2; - } - // config absent — no custom providers, no libraries, default snake_case - } // `baseline` records a STARTING POINT, so it is deliberately NOT scoped: the // `--from-db` arm captures whatever the database holds (there is no provenance // for an introspected table), and an offline baseline that recorded less would @@ -1205,13 +1247,14 @@ export async function runBaseline( // it — which is what makes that true. Committing the narrowed schema instead // would delete the entry, and removing the scope later would then propose // CREATE TABLE for a table that exists. + let metadata; try { - const collection = await resolveCollection(metaRoot); - metadata = await loadMemory(collection.configDir, { - ...collectionLoadOptions(collection), - ...baselineLoadOptions, - }); + metadata = await loadBaselineMetadata(metaRoot, genRoot); } catch (err) { + if (err instanceof RemovedNamingConfigError) { + log.error(err.message); + return 2; + } reportLoadError(log, "migrate baseline: failed to load metadata", err); return 1; } diff --git a/server/typescript/packages/cli/src/commands/verify.ts b/server/typescript/packages/cli/src/commands/verify.ts index 7df968fb1..35f26d29d 100644 --- a/server/typescript/packages/cli/src/commands/verify.ts +++ b/server/typescript/packages/cli/src/commands/verify.ts @@ -85,7 +85,9 @@ import { type D1Binding, type D1Runner, type DriftResult, + type ExprCanonProbe, postgresViewReplaceProbe, + postgresExprCanonProbe, withoutFlywayHistory, } from "@metaobjectsdev/migrate-ts"; import { @@ -1347,6 +1349,8 @@ export async function verifyCommand( // Expected views from the single view-SQL source (codegen-ts), so view-body // drift is detected against the live DB. 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; let driftResult; let actual: SchemaSnapshot; try { @@ -1363,6 +1367,8 @@ export async function verifyCommand( views: expectedViews, // The same question `meta migrate` asks, so the report names the change it plans. ...(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 } : {}), ...(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. @@ -1376,7 +1382,7 @@ export async function verifyCommand( const snapshotDrift = driftResult.changes.length === 0 - ? await checkCommittedSnapshot(actual, kysely.dialect, kysely.displayUrl, driftResult, migrateConfig) + ? await checkCommittedSnapshot(actual, kysely.dialect, kysely.displayUrl, driftResult, migrateConfig, exprCanonProbe) : []; return reportSchemaDrift(driftResult, [...ledgerDrift, ...snapshotDrift], kysely.displayUrl); @@ -1503,6 +1509,9 @@ export async function verifyCommand( // Resolved by the caller and threaded through — see the `verify --db` branch: the same // run's flyway filter reads this config too, and `tryLoadConfig` does not cache. migrateConfig: ResolvedMigrateConfig, + // 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, ): 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) @@ -1541,6 +1550,7 @@ export async function verifyCommand( // Note `unmanagedNames` is NOT restated here: `scopedDiffInputs` above supplies it // MERGED with the out-of-scope set, and a second key would silently drop that half. dialect, + ...(exprCanonProbe !== undefined ? { exprCanonProbe } : {}), }); // A view recreated only around a table change matches the snapshot (D3) — not a difference. const changes = withoutViewRecreates(result.changes); diff --git a/server/typescript/packages/cli/test/integration/migrate-baseline-from-db-pg.test.ts b/server/typescript/packages/cli/test/integration/migrate-baseline-from-db-pg.test.ts new file mode 100644 index 000000000..61ad58301 --- /dev/null +++ b/server/typescript/packages/cli/test/integration/migrate-baseline-from-db-pg.test.ts @@ -0,0 +1,108 @@ +/** + * `meta migrate baseline --from-db` on Postgres records the GENERATED spelling of a CHECK the + * live database proves equal to the metadata's, instead of Postgres's own spelling of it. + * + * The offline plan has no database to ask, so it compares the snapshot's text with the text + * migrate generates through a string normaliser. Postgres rewrites a one-element + * `IN ('SPACE')` to `= 'SPACE'::text`; with the generated text in the snapshot the two sides + * are the same string and the offline plan starts clean. A baseline whose metadata will not + * load must still work, and keeps Postgres's text. + * + * Skips gracefully when MIGRATE_TS_PG_URL is not set. + */ +import { describe, test, expect, beforeAll, afterAll } from "bun:test"; +import { mkdtempSync, mkdirSync, rmSync, writeFileSync, existsSync, readdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Pool } from "pg"; +import { snapshotPath, readSnapshot } from "@metaobjectsdev/migrate-ts"; +import { run } from "../../src/index.js"; + +const PG_URL = process.env.MIGRATE_TS_PG_URL; +const TABLE = "canon_baseline_t"; + +const MODEL = JSON.stringify({ + "metadata.root": { + children: [{ + "object.entity": { + name: "CanonBaseline", + children: [ + { "source.rdb": { name: "src", "@table": TABLE } }, + { "field.long": { name: "id" } }, + { "field.enum": { name: "kind", "@values": ["SPACE"] } }, + { "field.int": { name: "qty", children: [{ "validator.numeric": { name: "range", "@min": 1, "@max": 100 } }] } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }], + }, +}); + +describe("meta migrate baseline --from-db — postgres spelling (env-gated)", () => { + if (PG_URL === undefined || PG_URL.length === 0) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + let pool: Pool; + beforeAll(async () => { + pool = new Pool({ connectionString: PG_URL }); + await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); + }); + afterAll(async () => { + await pool.query(`DROP TABLE IF EXISTS ${TABLE} CASCADE`); + await pool.end(); + }); + + const checksOf = async (root: string): Promise> => { + const snapshot = await readSnapshot(snapshotPath(join(root, ".metaobjects", "migrations"), "postgres")); + const table = snapshot!.tables.find((t) => t.name === TABLE)!; + return Object.fromEntries(table.checks.map((c) => [c.name, c.expression])); + }; + const migrationDirs = (root: string): string[] => + readdirSync(join(root, ".metaobjects", "migrations")).filter((e) => !e.startsWith(".")); + + test("the generated text lands in the snapshot, and the offline plan is then clean", async () => { + const root = mkdtempSync(join(tmpdir(), "baseline-pg-canon-")); + try { + mkdirSync(join(root, "metaobjects"), { recursive: true }); + writeFileSync(join(root, "metaobjects", "meta.canon.json"), MODEL, "utf8"); + + // Day 1: create the table. The snapshot this writes holds the GENERATED spelling. + expect(await run(["migrate", "--cwd", root, "--from-db", "--db", PG_URL, "--dialect", "postgres", "--slug", "init", "--apply"])).toBe(0); + const generated = await checksOf(root); + expect(Object.keys(generated).length).toBeGreaterThan(0); + const live = (await pool.query( + `SELECT conname, pg_get_constraintdef(oid) AS def FROM pg_constraint WHERE conrelid = '${TABLE}'::regclass AND contype = 'c'`, + )).rows as Array<{ conname: string; def: string }>; + // The gate is not vacuous: Postgres really does spell the one-value enum differently. + expect(live.find((r) => r.conname.includes("kind"))!.def).toContain("'SPACE'::text"); + expect(generated[live.find((r) => r.conname.includes("kind"))!.conname]).not.toContain("::text"); + + // Re-baseline from the live database: the snapshot must hold the generated text again. + rmSync(snapshotPath(join(root, ".metaobjects", "migrations"), "postgres")); + expect(await run(["migrate", "baseline", "--cwd", root, "--from-db", "--db", PG_URL, "--dialect", "postgres"])).toBe(0); + expect(await checksOf(root)).toEqual(generated); + + // The offline plan (no database) is clean: it writes no migration. + const before = migrationDirs(root); + expect(await run(["migrate", "--cwd", root, "--dialect", "postgres", "--slug", "noop"])).toBe(0); + expect(migrationDirs(root)).toEqual(before); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + + test("metadata that will not load does not fail the baseline, which keeps Postgres's text", async () => { + const root = mkdtempSync(join(tmpdir(), "baseline-pg-canon-bad-")); + try { + mkdirSync(join(root, "metaobjects"), { recursive: true }); + writeFileSync(join(root, "metaobjects", "meta.canon.json"), "{ not json", "utf8"); + expect(await run(["migrate", "baseline", "--cwd", root, "--from-db", "--db", PG_URL, "--dialect", "postgres"])).toBe(0); + expect(existsSync(snapshotPath(join(root, ".metaobjects", "migrations"), "postgres"))).toBe(true); + const checks = await checksOf(root); + expect(Object.values(checks).some((e) => e.includes("::text"))).toBe(true); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); 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 11c2b728e..e43a3a35c 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -1,8 +1,16 @@ // src/check-expr-compare.ts // -// CHECK-expression comparison. Postgres rewrites a stored CHECK body so the raw -// text we generate and the introspected text differ textually but mean the same -// thing. This reduces both to ONE canonical form for comparison. +// CHECK-expression comparison — the STRING normaliser. Postgres rewrites a stored CHECK so the +// raw text we generate and the introspected text differ textually but mean the same thing. This +// reduces both to ONE canonical form for comparison. +// +// It is the comparison of last resort. On a live Postgres the planner asks the server instead +// (expr-canon-probe.ts: the expected expression is added to a rolled-back scratch table and +// Postgres's own stored text is compared with the live one), and this module decides only where +// that cannot be asked: SQLite and D1 (which store text as written), the offline plan (no +// database), a read-only connection or a role without TEMP, and an expression that does not +// compile against the live columns. Do NOT add another Postgres-shaped rewrite here for a shape +// the live comparison already settles; the probe takes it. // // It was written when every expression reaching it was machine-derived with a simple, // known shape (comparison / IN / length / regex), and the header said there was "no @@ -17,15 +25,16 @@ // - parenthesizes a cast OPERAND: `x::integer` → `(x)::integer` // - respells a cast TARGET canonically: `x::int` → `(x)::integer` // All five are canonicalized below so an enum/range CHECK introspected from PG -// compares equal to the one we generate (idempotency on the --from-db / verify paths). +// compares equal to the one we generate (idempotency on the offline and fallback paths). // Three more are folded back by foldPgAnalyzerRewrites and COLUMN_TEXT_CAST: a // one-element `x IN (v)` stored as `x = v`, `a IS NOT DISTINCT FROM b` stored as // `NOT (a IS DISTINCT FROM b)`, and `(varchar_col)::text` stored for a text operator. // // Everything textual runs OUTSIDE single-quoted literals (mapOutsideLiterals). One thing is -// still NOT canonicalized: dropping every bracket erases precedence, so `(a OR b) AND c` and -// `a OR (b AND c)` compare equal. Fixing that takes the server's own canonical form, not more -// string rules. +// still NOT canonicalized, and cannot be by string rules: dropping every bracket erases +// precedence, so `(a OR b) AND c` and `a OR (b AND c)` compare equal HERE. On a live Postgres +// the probe tells them apart; on the fallback paths above they stay equal (a real change reads +// as clean, the worse direction, which is why the live comparison no longer uses this module). // // The last two are why a cast-bearing `@expr` used to drift FOREVER. The paren-strip // turns PG's added parens into a space, so `(x)::integer` reduced to `x ::integer` while diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index b72fabf52..fb9a383d8 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -13,7 +13,8 @@ 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 { checkExprEquals, checkExprRespelledNullSafe, normalizeCheckExpr, renameExprIdentifiers } from "../check-expr-compare.js"; +import { checkExprEquals, checkExprRespelledNullSafe, renameExprIdentifiers } from "../check-expr-compare.js"; +import { checkMatchesLive, indexExprMatchesLive, NO_EXPR_CANON, type ExprCanon, type ExprCanonProbe, type IndexExprCanon } from "../expr-canon.js"; import { isPgAutoSequenceDefault } from "../pg-identity-default.js"; import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; import { qualifiedDbName } from "../qualified-name.js"; @@ -79,6 +80,15 @@ export interface DiffArgs { * committed snapshot), or when it cannot answer, the plan is DROP + CREATE. */ viewReplaceProbe?: ViewReplaceProbe; + /** + * Postgres: asks the live database how it stores each expected CHECK and index expression + * (`postgresExprCanonProbe`), so those are compared as the plain strings Postgres produced + * instead of through the string normaliser. Without a probe (an offline plan against the + * committed snapshot, SQLite, D1), or for any expression the probe cannot answer (a + * read-only connection, no TEMP privilege, an expression that does not compile against + * the live columns), the normaliser decides exactly as it always has. + */ + exprCanonProbe?: ExprCanonProbe; /** * #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 @@ -244,14 +254,15 @@ 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 { renamed, predecessors } = diffRenamedTables(changes, expectedTables, actualTables, args.dialect); + const canon = await probeExprCanon(args.exprCanonProbe, args.dialect, changes, expectedTables, actualTables); + const { renamed, predecessors } = diffRenamedTables(changes, expectedTables, actualTables, args.dialect, canon); // Pass 2: tables in both → compare columns/indexes/FKs for (const [id, expectedTable] of expectedTables) { const actualTable = actualTables.get(id); if (!actualTable) continue; // An FK onto a renamed table follows the rename in the engine — compare it as such. - diffTablePair(expectedTable, withRenamedRefTables(actualTable, renamed), changes, args.dialect); + diffTablePair(expectedTable, withRenamedRefTables(actualTable, renamed), changes, args.dialect, canon); } // Pass 2b: views. Identity is (schema, name). How "changed" is decided is @@ -344,6 +355,7 @@ function diffRenamedTables( expectedTables: ReadonlyMap, actualTables: ReadonlyMap, dialect: Dialect | undefined, + canon: ExprCanon, ): { renamed: Map; predecessors: Map } { const renames = changes.filter( (c): c is Extract => c.kind === "rename-table", @@ -371,12 +383,60 @@ function diffRenamedTables( predecessors.set(newId, actual); const carried = withRenamedRefTables({ ...actual, name: r.to }, renamed); const tableChanges: Change[] = []; - diffTablePair(expected, carried, tableChanges, dialect); - changes.push(...(dialect === "postgres" ? foldNameOnlyChanges(r, tableChanges, textColumnsOf(actual)) : tableChanges)); + diffTablePair(expected, carried, tableChanges, dialect, canon); + changes.push(...(dialect === "postgres" ? foldNameOnlyChanges(r, tableChanges, textColumnsOf(actual), canon) : tableChanges)); } return { renamed, predecessors }; } +/** + * Postgres: ask the live database how it stores each expected CHECK and index expression, + * for every table that has a live counterpart (present on both sides, or carried through a + * rename), so the comparison below sees Postgres's own spelling on both sides. Only + * expressions whose text differs from the live one need asking: identical text is equal. + * Anything the probe cannot answer is absent from the result and keeps the string + * normaliser (see expr-canon.ts). + */ +async function probeExprCanon( + probe: ExprCanonProbe | undefined, + dialect: Dialect | undefined, + changes: readonly Change[], + expectedTables: ReadonlyMap, + actualTables: ReadonlyMap, +): Promise { + if (probe === undefined || dialect !== "postgres") return NO_EXPR_CANON; + const liveOf = new Map(); + for (const c of changes) { + if (c.kind !== "rename-table") continue; + const live = actualTables.get(tableIdentity({ name: c.from, ...schemaSpread(c.schema) })); + if (live !== undefined) liveOf.set(tableIdentity({ name: c.to, ...schemaSpread(c.schema) }), live); + } + const checks = new Map(); + const indexes = 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 + // comparison could not settle by identical text. + const unsettled: TableDescriptor = { + ...expected, + checks: expected.checks.filter((c) => liveChecks.get(c.name) !== c.expression), + indexes: expected.indexes.filter((ix) => { + const l = liveIndexes.get(ix.name); + return l === undefined || ix.expr !== l.expr || ix.where !== l.where; + }), + }; + if (unsettled.checks.length === 0 && unsettled.indexes.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); + } + return { checks, indexes }; +} + /** * The per-table diff tier — run by Pass 2 for tables present on both sides and by * Pass 1c for tables carried through a rename. One sequence on purpose: a resolved @@ -389,9 +449,10 @@ function diffTablePair( actual: TableDescriptor, changes: Change[], dialect: Dialect | undefined, + canon: ExprCanon, ): void { diffTableColumns(expected, actual, changes, dialect); - diffTableIndexes(expected, actual, changes); + diffTableIndexes(expected, actual, changes, canon); diffTableForeignKeys(expected, actual, changes, dialect); // CHECK constraints on existing tables are evolved whenever a dialect is // known (postgres: ALTER ADD/DROP CONSTRAINT; sqlite/d1: the emitter routes @@ -401,7 +462,7 @@ function diffTablePair( // no dialect (legacy positional callers) checks stay un-diffed — those // callers may hold hand-built snapshots with empty check lists, and // diffing them would re-propose every modeled check forever. - if (dialect !== undefined) diffTableChecks(expected, actual, changes, dialect); + if (dialect !== undefined) diffTableChecks(expected, actual, changes, dialect, canon); } /** `t` with each FK's `refTable` mapped through the table renames (refs share `t`'s schema). */ @@ -429,6 +490,7 @@ function foldNameOnlyChanges( r: Extract, tableChanges: Change[], textColumns: ReadonlySet, + canon: ExprCanon, ): Change[] { const constraintRenames: NameChange[] = []; const indexRenames: NameChange[] = []; @@ -443,12 +505,12 @@ function foldNameOnlyChanges( } else if (d.kind === "drop-check" && d.restore !== undefined) { const restore = d.restore; const a = tableChanges.find((c): c is Extract => - c.kind === "add-check" && !folded.has(c) && checkExprEquals(c.check.expression, restore.expression, textColumns)); + c.kind === "add-check" && !folded.has(c) && checkMatchesLive(canon, c.check, restore, textColumns)); if (a) { folded.add(d).add(a); constraintRenames.push({ from: d.check, to: a.check.name }); } } else if (d.kind === "drop-index" && d.restore !== undefined) { const restore = d.restore; const a = tableChanges.find((c): c is Extract => - c.kind === "add-index" && !folded.has(c) && indexEquals(c.index, restore)); + c.kind === "add-index" && !folded.has(c) && indexEquals(c.index, restore, canon)); if (a) { folded.add(d).add(a); // Renaming a UNIQUE constraint renames its backing index with it. @@ -719,6 +781,7 @@ function diffTableIndexes( expected: TableDescriptor, actual: TableDescriptor, changes: Change[], + canon: ExprCanon, ): void { const table = expected.name; const sx = schemaSpread(expected.schema); @@ -728,7 +791,7 @@ function diffTableIndexes( const a = actualIdx.get(name); if (!a) { changes.push({ kind: "add-index", table, ...sx, index: ix, status: ALLOWED }); - } else if (!indexEquals(ix, a)) { + } else if (!indexEquals(ix, a, canon)) { // Index shape changed: drop + add (atomic from caller's perspective). // restore = the ACTUAL shape so the down re-creates the original index. changes.push({ kind: "drop-index", table, ...sx, index: name, restore: a, status: ALLOWED }); @@ -842,7 +905,8 @@ function diffTableChecks( expected: TableDescriptor, actual: TableDescriptor, changes: Change[], - dialect?: Dialect, + dialect: Dialect | undefined, + canon: ExprCanon, ): void { const sx = schemaSpread(expected.schema); // SQLite/D1 stores no constraint identity for an inline `CHECK (…)`, so hand-written @@ -864,7 +928,7 @@ function diffTableChecks( const ac = actualByName.get(ec.name); if (ac) { consumed.add(ac); - if (!checkExprEquals(ec.expression, ac.expression, textColumns)) { + if (!checkMatchesLive(canon, ec, ac, textColumns)) { const respelled = respelledSpread(ec, ac); changes.push({ kind: "drop-check", table: expected.name, ...sx, check: ec.name, restore: ac, ...respelled, status: ALLOWED }); changes.push({ kind: "add-check", table: expected.name, ...sx, check: ec, ...respelled, status: ALLOWED }); @@ -1341,7 +1405,8 @@ function recreateReason( return proven ? { kind: "unchanged", tables } : undefined; } -function indexEquals(a: IndexDescriptor, b: IndexDescriptor): boolean { +function indexEquals(a: IndexDescriptor, b: IndexDescriptor, canon: ExprCanon = NO_EXPR_CANON): boolean { + const stored = canon.indexes.get(a); if (a.unique !== b.unique) return false; // Access method: absent = "btree" (the default). if ((a.using ?? "btree") !== (b.using ?? "btree")) return false; @@ -1349,7 +1414,7 @@ function indexEquals(a: IndexDescriptor, b: IndexDescriptor): boolean { // as predicates). Both-absent = plain index; one-absent = different. if ((a.expr === undefined) !== (b.expr === undefined)) return false; if (a.expr !== undefined && b.expr !== undefined) { - if (normalizeCheckExpr(a.expr) !== normalizeCheckExpr(b.expr)) return false; + if (!indexExprMatchesLive(stored?.expr, a.expr, b.expr)) return false; } if (a.columns.length !== b.columns.length) return false; if (!a.columns.every((c, i) => c === b.columns[i])) return false; @@ -1362,7 +1427,7 @@ function indexEquals(a: IndexDescriptor, b: IndexDescriptor): boolean { // the introspected `pg_get_expr` form. Both-absent = equal; one-absent = different. if ((a.where === undefined) !== (b.where === undefined)) return false; if (a.where !== undefined && b.where !== undefined) { - if (normalizeCheckExpr(a.where) !== normalizeCheckExpr(b.where)) return false; + if (!indexExprMatchesLive(stored?.where, a.where, b.where)) return false; } return true; } diff --git a/server/typescript/packages/migrate-ts/src/drift/drift.ts b/server/typescript/packages/migrate-ts/src/drift/drift.ts index ab9ce8a78..498f8cfc1 100644 --- a/server/typescript/packages/migrate-ts/src/drift/drift.ts +++ b/server/typescript/packages/migrate-ts/src/drift/drift.ts @@ -24,6 +24,8 @@ import { scopeExpectedSchema, scopedDiffInputs, type ObjectScopePredicate } from import type { AllowOptions, Dialect, DiffResult, SchemaSnapshot } from "../types.js"; 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"; export interface ComputeDriftOptions { /** @@ -50,6 +52,13 @@ export interface ComputeDriftOptions { * so the drift report names the change `meta migrate` would plan. */ viewReplaceProbe?: ViewReplaceProbe; + /** + * Postgres: the live-database probe that supplies Postgres's own spelling of each expected + * CHECK and index expression (passed through to `diff`). `computeDrift` supplies it from + * its connection; a caller of `computeDriftFromActual` passes its own, and without one + * the string normaliser decides. + */ + exprCanonProbe?: ExprCanonProbe; /** * Per-command scope (`migrate.scope`): objects whose declaring FQN this predicate * rejects are governed by somebody else. They leave the expected side AND are @@ -139,6 +148,7 @@ export async function computeDriftFromActual( allow: opts?.allow ?? {}, ...(opts?.ignoreTables !== undefined ? { ignoreTables: opts.ignoreTables } : {}), ...(opts?.viewReplaceProbe !== undefined ? { viewReplaceProbe: opts.viewReplaceProbe } : {}), + ...(opts?.exprCanonProbe !== undefined ? { exprCanonProbe: opts.exprCanonProbe } : {}), }); return { ...result, @@ -171,5 +181,8 @@ export async function computeDrift( ...(dialect === "postgres" && opts?.viewReplaceProbe === undefined ? { viewReplaceProbe: postgresViewReplaceProbe(db) } : {}), + ...(dialect === "postgres" && opts?.exprCanonProbe === undefined + ? { exprCanonProbe: postgresExprCanonProbe(db) } + : {}), }); } diff --git a/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts b/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts new file mode 100644 index 000000000..9871f3c3b --- /dev/null +++ b/server/typescript/packages/migrate-ts/src/expr-canon-probe.ts @@ -0,0 +1,123 @@ +// expr-canon-probe.ts — the Postgres implementation of the expression-canon probe (see +// expr-canon.ts for why the planner asks the database instead of predicting). +// +// For one live table it creates a TEMPORARY table with the same columns, adds each expected +// CHECK to it and each expected index expression / predicate as a temporary index, reads +// back what Postgres stored, and rolls everything back. Nothing persists, and nothing is +// ever written to the live table. + +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; +import { indexDefKeyExpr } 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 { quote, rolledBack } from "./view-replace-probe.js"; + +const TABLE = "mo_check_probe"; +const CONSTRAINT = "mo_check_probe_c"; +const INDEX = "mo_check_probe_i"; +const SAVEPOINT = "mo_check_probe_sp"; + +/** SQLSTATE that means no table will ever probe on this connection: the transaction is read-only. */ +const READ_ONLY_SQLSTATE = "25006"; + +const ACCESS_METHOD = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** + * True when `text` can be sent to the server as part of one DDL statement. The probe runs an + * author's expression over the simple query protocol, where a second statement would run too, + * and Postgres has more ways to quote than a scanner here can track (`E'..'`, dollar quotes, + * comments). So an expression holding a `;` anywhere is not probed; it falls back to the + * string comparison. + */ +function probeable(text: string | undefined): boolean { + return text === undefined || !text.includes(";"); +} + +/** Run `work` inside a savepoint it always rolls back, resolving `undefined` when it throws. */ +async function attempt( + q: Kysely>, + work: () => Promise, +): Promise { + await sql.raw(`SAVEPOINT ${SAVEPOINT}`).execute(q); + try { + return await work(); + } catch { + return undefined; + } finally { + await sql.raw(`ROLLBACK TO SAVEPOINT ${SAVEPOINT}`).execute(q); + await sql.raw(`RELEASE SAVEPOINT ${SAVEPOINT}`).execute(q); + } +} + +function indexKeys(ix: IndexDescriptor): string { + return ix.expr ?? ix.columns.map((c, i) => (ix.orders?.[i] === "desc" ? `${quote(c)} DESC` : quote(c))).join(", "); +} + +/** + * The probe for a live Postgres database. A table it cannot probe, or an expression that + * does not compile against that table's live columns, simply has no entry in the answer. + */ +export function postgresExprCanonProbe(db: Kysely>): ExprCanonProbe { + // 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) => { + if (unavailable) return undefined; + const checks = expected.checks.filter((c) => probeable(c.expression)); + const indexes = expected.indexes.filter( + (ix) => (ix.expr !== undefined || ix.where !== undefined) + && probeable(ix.expr) && probeable(ix.where) + && (ix.using === undefined || ACCESS_METHOD.test(ix.using)), + ); + if (checks.length === 0 && indexes.length === 0) return undefined; + const liveTable = `${quote(live.schema ?? DEFAULT_DB_SCHEMA_POSTGRES)}.${quote(live.name)}`; + const scratch = `pg_temp.${quote(TABLE)}`; + try { + return await rolledBack(db, async (q): Promise => { + try { + await sql.raw(`CREATE TEMPORARY TABLE ${quote(TABLE)} (LIKE ${liveTable})`).execute(q); + } catch (err) { + const code = (err as { code?: unknown }).code; + if (code === READ_ONLY_SQLSTATE) unavailable = true; + throw err; + } + const checkAnswers = new Map(); + for (const check of checks) { + const def = await attempt(q, async () => { + await sql.raw(`ALTER TABLE ${scratch} ADD CONSTRAINT ${quote(CONSTRAINT)} CHECK (${check.expression})`).execute(q); + const rows = await sql<{ def: string }>` + SELECT pg_get_constraintdef(oid) AS def FROM pg_constraint + WHERE conrelid = ${scratch}::regclass AND conname = ${CONSTRAINT} + `.execute(q); + return rows.rows[0]?.def; + }); + if (def !== undefined) checkAnswers.set(check, stripCheckWrapper(def)); + } + const indexAnswers = new Map(); + for (const ix of indexes) { + const using = ix.using && ix.using !== "btree" ? ` USING ${ix.using}` : ""; + const where = ix.where !== undefined ? ` WHERE (${ix.where})` : ""; + const stored = await attempt(q, async () => { + await sql.raw(`CREATE INDEX ${quote(INDEX)} ON ${scratch}${using} (${indexKeys(ix)})${where}`).execute(q); + const rows = await sql<{ def: string; predicate: string | null }>` + SELECT pg_get_indexdef(indexrelid) AS def, pg_get_expr(indpred, indrelid) AS predicate + FROM pg_index WHERE indexrelid = ${`pg_temp.${quote(INDEX)}`}::regclass + `.execute(q); + return rows.rows[0]; + }); + if (stored === undefined) continue; + const canon: IndexExprCanon = {}; + if (ix.expr !== undefined) canon.expr = indexDefKeyExpr(stored.def); + if (ix.where !== undefined && stored.predicate !== null) canon.where = stored.predicate; + indexAnswers.set(ix, canon); + } + return { checks: checkAnswers, indexes: indexAnswers }; + }); + } 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 new file mode 100644 index 000000000..a855f1f65 --- /dev/null +++ b/server/typescript/packages/migrate-ts/src/expr-canon.ts @@ -0,0 +1,73 @@ +// expr-canon.ts — Postgres's own spelling of an expected CHECK / index expression. +// +// check-expr-compare.ts compares two SQL strings by rewriting both toward one shape. That is +// the only option on SQLite (which stores text as written) and offline (no database), but on +// a live Postgres it is a guess at what the server will do to the text, and every guess is +// either a false drift (an analyser rewrite nobody has hand-coded yet) or a missed one (a +// rewrite that erases meaning: precedence, literal case). The server already knows the +// answer, so the planner asks it: it creates the expected expression on a rolled-back +// scratch table and reads back the definition Postgres stores (expr-canon-probe.ts). The two +// definitions are then equal as plain strings or they are not. +// +// The probe is an optimisation of the TRUTH, never a source of drift. Every case it cannot +// answer (read-only connection, no TEMP privilege, an expression that does not compile +// 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 { checkExprEquals, normalizeCheckExpr } from "./check-expr-compare.js"; + +/** Postgres's stored form of the expression parts of one expected index. */ +export interface IndexExprCanon { + /** `pg_get_indexdef`'s key list, for an expression index. */ + expr?: string; + /** `pg_get_expr(indpred, …)`, for a partial index. */ + where?: string; +} + +/** + * What the database said about a set of expected descriptors. Keyed by DESCRIPTOR IDENTITY: + * the diff hands the very objects it was given to every comparison, and an expression + * string alone cannot name a check (two tables repeat the same text). + */ +export interface ExprCanon { + readonly checks: ReadonlyMap; + readonly indexes: ReadonlyMap; +} + +export const NO_EXPR_CANON: ExprCanon = { checks: new Map(), indexes: new Map() }; + +/** + * Ask the database how it would store `expected`'s CHECK constraints and index expressions + * on the columns of `live`. Resolves `undefined` when the question could not be answered; it + * never throws and never leaves anything behind. An answer may cover only part of what was + * asked (one expression that does not compile is simply absent). + */ +export type ExprCanonProbe = ( + live: TableDescriptor, + expected: TableDescriptor, +) => Promise; + +/** + * Whether `expected` and the live `live` constraint are the same. With the database's + * answer the two are compared as the plain strings Postgres produced; without it, through + * the string normaliser. + */ +export function checkMatchesLive( + canon: ExprCanon, + expected: CheckDescriptor, + live: CheckDescriptor, + textColumns?: ReadonlySet, +): boolean { + const stored = canon.checks.get(expected); + return stored !== undefined ? stored === live.expression : checkExprEquals(expected.expression, live.expression, textColumns); +} + +/** The same decision for one expression part of an index (`expr` or `where`). */ +export function indexExprMatchesLive( + stored: string | undefined, + expected: string, + live: string, +): boolean { + return stored !== undefined ? stored === live : normalizeCheckExpr(expected) === normalizeCheckExpr(live); +} diff --git a/server/typescript/packages/migrate-ts/src/index.ts b/server/typescript/packages/migrate-ts/src/index.ts index 47f7c5464..e192e4398 100644 --- a/server/typescript/packages/migrate-ts/src/index.ts +++ b/server/typescript/packages/migrate-ts/src/index.ts @@ -94,6 +94,12 @@ export { isViewRecreateOnly, withoutViewRecreates } from "./view-change-reason.j // transaction that is always rolled back. Pass the probe to `diff` on a live plan. export { postgresViewReplaceProbe } from "./view-replace-probe.js"; export type { ExactViewColumn, ViewReplaceEvidence, ViewReplaceProbe } from "./view-replace.js"; +// 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 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"; // D1 dialect emitter + safety pass. // renderD1 is exported directly (unlike renderSqlite/renderPostgres) so diff --git a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts index 97dafaf8f..bcb308072 100644 --- a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts @@ -653,7 +653,7 @@ async function readPgIndexes(k: RawKysely, schema: string, table: string): Promi /** Extract the raw key-expression text from a pg index def — the balanced `(...)` * after `USING `, stripped of a trailing partial-index `WHERE`. */ -function indexDefKeyExpr(indexdef: string): string { +export function indexDefKeyExpr(indexdef: string): string { const open = indexdef.indexOf("(", indexdef.indexOf(" USING ")); if (open === -1) return indexdef; let depth = 0; 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 new file mode 100644 index 000000000..889414536 --- /dev/null +++ b/server/typescript/packages/migrate-ts/src/snapshot/adopt-generated-spelling.ts @@ -0,0 +1,50 @@ +// adopt-generated-spelling.ts — what `baseline --from-db` writes for a CHECK / index +// expression 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 +// text migrate generates through the string normaliser, which cannot know every rewrite Postgres +// makes. Where the live database has just PROVEN, by the expression-canon probe, that its stored +// expression is exactly what the metadata's would be stored as, the snapshot records the +// generated text instead, so the offline plan compares like with like and a baselined project +// starts clean. An expression the probe cannot answer, or finds different, keeps Postgres's text: +// recording the generated text there would hide a real difference. + +import type { ExprCanonProbe } from "../expr-canon.js"; +import { qualifiedDbName } from "../qualified-name.js"; +import type { CheckDescriptor, IndexDescriptor, SchemaSnapshot, TableDescriptor } from "../types.js"; + +export async function adoptGeneratedSpelling( + live: SchemaSnapshot, + expected: SchemaSnapshot, + probe: ExprCanonProbe, +): Promise { + const expectedById = new Map(expected.tables.map((t) => [qualifiedDbName(t), t] as const)); + const tables: TableDescriptor[] = []; + for (const table of live.tables) { + const want = expectedById.get(qualifiedDbName(table)); + tables.push(want === undefined ? table : await adoptTable(table, want, probe)); + } + return { ...live, tables }; +} + +async function adoptTable(live: TableDescriptor, expected: TableDescriptor, probe: ExprCanonProbe): Promise { + const answer = await probe(live, expected); + if (answer === undefined) return live; + 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 => { + const ec = expectedChecks.get(ac.name); + return ec !== undefined && answer.checks.get(ec) === ac.expression ? { ...ac, expression: ec.expression } : ac; + }); + const indexes = live.indexes.map((ai): IndexDescriptor => { + const ei = expectedIndexes.get(ai.name); + const stored = ei === undefined ? undefined : answer.indexes.get(ei); + if (ei === undefined || stored === undefined) return ai; + const adopted = { ...ai }; + if (ei.expr !== undefined && ai.expr !== undefined && stored.expr === ai.expr) adopted.expr = ei.expr; + if (ei.where !== undefined && ai.where !== undefined && stored.where === ai.where) adopted.where = ei.where; + return adopted; + }); + return { ...live, checks, indexes }; +} diff --git a/server/typescript/packages/migrate-ts/src/view-replace-probe.ts b/server/typescript/packages/migrate-ts/src/view-replace-probe.ts index d2a16d83f..29000b3e1 100644 --- a/server/typescript/packages/migrate-ts/src/view-replace-probe.ts +++ b/server/typescript/packages/migrate-ts/src/view-replace-probe.ts @@ -16,7 +16,7 @@ const ROLLBACK = Symbol("rollback"); * exactly as it was, still open and still usable even when `work` fails. Otherwise it * runs in a transaction of its own, which is rolled back. */ -async function rolledBack( +export async function rolledBack( db: Kysely>, work: (q: Kysely>) => Promise, ): Promise { @@ -41,7 +41,7 @@ async function rolledBack( return result!.value; } -const quote = (ident: string): string => `"${ident.replace(/"/g, '""')}"`; +export const quote = (ident: string): string => `"${ident.replace(/"/g, '""')}"`; /** * The probe for a live Postgres database. It creates the expected definition as a diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-check-shapes-roundtrip.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-check-shapes-roundtrip.test.ts index f376ef3b7..6e35b338e 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-check-shapes-roundtrip.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-check-shapes-roundtrip.test.ts @@ -20,7 +20,8 @@ import { Kysely, PostgresDialect, sql } from "kysely"; import { MetaDataLoader, InMemoryStringSource } from "@metaobjectsdev/metadata"; import { buildExpectedSchema } from "../../src/expected-schema.js"; import { introspectPostgres } from "../../src/introspect/postgres.js"; -import { computeDrift } from "../../src/drift/drift.js"; +import { computeDrift, computeDriftFromActual } from "../../src/drift/drift.js"; +import { postgresExprCanonProbe } from "../../src/expr-canon-probe.js"; import { diff } from "../../src/diff/index.js"; import { emit } from "../../src/emit/index.js"; @@ -173,5 +174,18 @@ describe("every generated CHECK shape round-trips clean on live Postgres", () => } expect(checkDrift).toEqual([]); expect(drift.changes).toEqual([]); + + // The probe is what answered, for every shape: Postgres's own stored text equals the live one. + const live = await introspectPostgres(kysely); + const liveTable = live.tables.find((t) => t.name === TABLE)!; + const answer = await postgresExprCanonProbe(kysely)(liveTable, expected.tables.find((t) => t.name === TABLE)!); + expect(answer?.checks.size).toBe(checks.length); + for (const [check, stored] of answer!.checks) { + expect(stored).toBe(liveTable.checks.find((c) => c.name === check.name)!.expression); + } + + // The fallback lane (SQLite-style comparison: no probe) reads the same shapes as clean too. + const unprobed = await computeDriftFromActual(live, "postgres", loaded.root); + expect(unprobed.changes.filter((c) => c.kind.endsWith("-check"))).toEqual([]); }); }); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-expr-canon-probe.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-expr-canon-probe.test.ts new file mode 100644 index 000000000..252ca7a0d --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-expr-canon-probe.test.ts @@ -0,0 +1,309 @@ +/** + * The expression-canon probe: Postgres, not a string rewrite, decides whether an expected + * CHECK / index expression is the one stored. + * + * The oracle is the live engine. A table is created from SQL, the expected side is the same + * table with one expression changed, and the pair is diffed with and without the probe. The + * string normaliser alone calls a precedence change and a NOT regrouping equal (it deletes + * every bracket), so those are the cases that only the probe reports. + * + * 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 { SchemaSnapshot, TableDescriptor } from "../../src/types.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const TABLE = "canon_probe_t"; + +type Db = Kysely>; + +describe("expression-canon probe (live PG)", () => { + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + let pool: Pool; + let db: Db; + + const connect = (options?: string): { pool: Pool; db: Db } => { + const p = new Pool({ connectionString: PG_URL, ...(options !== undefined ? { options } : {}) }); + return { pool: p, db: new Kysely>({ dialect: new PostgresDialect({ pool: p }) }) }; + }; + + beforeAll(async () => { + ({ pool, db } = connect()); + await sql.raw(`DROP TABLE IF EXISTS ${TABLE} CASCADE`).execute(db); + await sql + .raw(`CREATE TABLE ${TABLE} ( + id bigserial PRIMARY KEY, a int, b int, c int, status text, code varchar(16), + CONSTRAINT canon_prec_chk CHECK ((a = 1 OR b = 1) AND c = 1), + CONSTRAINT canon_not_chk CHECK (NOT (a = 1 AND b = 1)), + CONSTRAINT canon_status_chk CHECK (status IN ('open')), + CONSTRAINT canon_code_chk CHECK (code ~ '^[a-z]+$') + )`) + .execute(db); + await sql.raw(`CREATE INDEX canon_where_idx ON ${TABLE} (id) WHERE (a = 1 OR b = 1) AND c = 1`).execute(db); + await sql.raw(`CREATE INDEX canon_expr_idx ON ${TABLE} (lower(status))`).execute(db); + }); + afterAll(async () => { + await sql.raw(`DROP TABLE IF EXISTS ${TABLE} CASCADE`).execute(db); + await pool.end(); + }); + + /** The live table, and the same table declared with `checks` / `indexes` overridden. */ + const declared = async ( + over: { checks?: Record; indexes?: Record }, + ): Promise<{ live: SchemaSnapshot; expected: SchemaSnapshot }> => { + const live = await introspectPostgres(db); + const want = structuredClone(live); + const t = want.tables.find((x) => x.name === TABLE)!; + for (const [name, expression] of Object.entries(over.checks ?? {})) t.checks.find((c) => c.name === name)!.expression = expression; + for (const [name, parts] of Object.entries(over.indexes ?? {})) Object.assign(t.indexes.find((i) => i.name === name)!, parts); + return { live, expected: want }; + }; + const kinds = async (over: Parameters[0], probe: boolean, conn: Db = db): Promise => { + const { live, expected } = await declared(over); + const d = await diff({ + expected, actual: live, dialect: "postgres", scopeSchemas: ["public"], + ...(probe ? { exprCanonProbe: postgresExprCanonProbe(conn) } : {}), + }); + return d.changes.map((c) => c.kind).sort(); + }; + + describe("the verified false negatives are reported as drift", () => { + const precedence = { checks: { canon_prec_chk: "a = 1 OR (b = 1 AND c = 1)" } }; + const notGrouping = { checks: { canon_not_chk: "NOT a = 1 AND b = 1" } }; + const literalCase = { checks: { canon_status_chk: "status IN ('OPEN')" } }; + const regexCase = { checks: { canon_code_chk: "code ~ '^[A-Z]+$'" } }; + + test("precedence: (a OR b) AND c vs a OR (b AND c)", async () => { + expect(await kinds(precedence, true)).toEqual(["add-check", "drop-check"]); + }); + test("NOT grouping: NOT (a AND b) vs NOT a AND b", async () => { + expect(await kinds(notGrouping, true)).toEqual(["add-check", "drop-check"]); + }); + test("literal case: IN ('open') vs IN ('OPEN')", async () => { + expect(await kinds(literalCase, true)).toEqual(["add-check", "drop-check"]); + }); + test("regex literal case", async () => { + expect(await kinds(regexCase, true)).toEqual(["add-check", "drop-check"]); + }); + test("the string normaliser alone still calls precedence and NOT grouping equal (the reason for the probe)", async () => { + expect(await kinds(precedence, false)).toEqual([]); + expect(await kinds(notGrouping, false)).toEqual([]); + }); + }); + + describe("what Postgres stores identically is no drift, however it was written", () => { + test("authored spellings of the stored constraints", async () => { + expect(await kinds({ + checks: { + canon_prec_chk: "((a=1) OR (b=1)) AND (c=1)", + canon_not_chk: "NOT(a=1 AND b=1)", + canon_status_chk: `"status" = 'open'::text`, + canon_code_chk: `"code" ~ '^[a-z]+$'`, + }, + }, true)).toEqual([]); + }); + test("a one-element IN and an IS NOT DISTINCT FROM, which Postgres rewrites on analysis", async () => { + await sql.raw(`ALTER TABLE ${TABLE} ADD CONSTRAINT canon_nd_chk CHECK ((a IS NOT NULL) = (status IS NOT DISTINCT FROM 'p'))`).execute(db); + try { + const live2 = await introspectPostgres(db); + const want = structuredClone(live2); + want.tables.find((x) => x.name === TABLE)!.checks.find((c) => c.name === "canon_nd_chk")!.expression = + `("a" IS NOT NULL) = ("status" IS NOT DISTINCT FROM 'p')`; + const d = await diff({ expected: want, actual: live2, dialect: "postgres", scopeSchemas: ["public"], exprCanonProbe: postgresExprCanonProbe(db) }); + expect(d.changes).toEqual([]); + } finally { + await sql.raw(`ALTER TABLE ${TABLE} DROP CONSTRAINT canon_nd_chk`).execute(db); + } + }); + }); + + describe("index @expr / @where", () => { + test("a partial-index predicate with different precedence is drift; the same predicate re-written is not", async () => { + expect(await kinds({ indexes: { canon_where_idx: { where: "a = 1 OR (b = 1 AND c = 1)" } } }, true)) + .toEqual(["add-index", "drop-index"]); + expect(await kinds({ indexes: { canon_where_idx: { where: "((a=1) OR (b=1)) AND (c=1)" } } }, true)).toEqual([]); + // ...which the normaliser alone cannot tell apart. + expect(await kinds({ indexes: { canon_where_idx: { where: "a = 1 OR (b = 1 AND c = 1)" } } }, false)).toEqual([]); + }); + test("an expression key authored tight or loose is the same index; a different function is not", async () => { + expect(await kinds({ indexes: { canon_expr_idx: { expr: "lower(status)" } } }, true)).toEqual([]); + expect(await kinds({ indexes: { canon_expr_idx: { expr: "upper(status)" } } }, true)).toEqual(["add-index", "drop-index"]); + }); + }); + + describe("falls back to the string normaliser and never reports drift of its own", () => { + test("a read-only connection answers nothing, and the normaliser decides as before", async () => { + const ro = connect("-c default_transaction_read_only=on"); + try { + const { live, expected } = await declared({}); + const t = expected.tables.find((x) => x.name === TABLE)!; + expect(await postgresExprCanonProbe(ro.db)(live.tables.find((x) => x.name === TABLE)!, t)).toBeUndefined(); + // Equal text: clean. Changed text: drift. Same as without a probe. + expect(await kinds({ checks: { canon_status_chk: `"status" = 'open'::text` } }, true, ro.db)).toEqual([]); + expect(await kinds({ checks: { canon_status_chk: "status IN ('OPEN')" } }, true, ro.db)).toEqual(["add-check", "drop-check"]); + // The normaliser's own blind spot stays a blind spot on this path. + expect(await kinds({ checks: { canon_prec_chk: "a = 1 OR (b = 1 AND c = 1)" } }, true, ro.db)).toEqual([]); + } finally { + await ro.pool.end(); + } + }); + test("an expression that does not compile against the live columns has no answer", async () => { + const { live, expected } = await declared({ checks: { canon_prec_chk: "no_such_column = 1" } }); + const answer = await postgresExprCanonProbe(db)( + live.tables.find((x) => x.name === TABLE)!, expected.tables.find((x) => x.name === TABLE)!, + ); + const t = expected.tables.find((x) => x.name === TABLE)!; + expect(answer?.checks.has(t.checks.find((c) => c.name === "canon_prec_chk")!)).toBe(false); + expect(answer?.checks.has(t.checks.find((c) => c.name === "canon_not_chk")!)).toBe(true); + }); + test("an expression carrying a second statement is not run", async () => { + const { live, expected } = await declared({ checks: { canon_not_chk: `a = 1); DROP TABLE ${TABLE}; --` } }); + const t = expected.tables.find((x) => x.name === TABLE)!; + const answer = await postgresExprCanonProbe(db)(live.tables.find((x) => x.name === TABLE)!, t); + expect(answer?.checks.has(t.checks.find((c) => c.name === "canon_not_chk")!)).toBe(false); + const still = await sql<{ n: string }>`SELECT count(*)::text AS n FROM ${sql.table(TABLE)}`.execute(db); + expect(still.rows[0]!.n).toBe("0"); + }); + }); + + describe("hostile expressions are never run", () => { + const answered = async (over: Parameters[0]) => { + const { live, expected } = await declared(over); + const t = expected.tables.find((x) => x.name === TABLE)!; + const answer = await postgresExprCanonProbe(db)(live.tables.find((x) => x.name === TABLE)!, t); + const rows = await sql<{ n: string }>`SELECT count(*)::text AS n FROM ${sql.table(TABLE)}`.execute(db); + return { answer, t, survived: rows.rows[0]!.n === "0" }; + }; + test("an E-string escape cannot smuggle a statement past the guard", async () => { + const { answer, t, survived } = await answered({ + checks: { canon_not_chk: `a = E'\\''; COMMIT; DROP TABLE ${TABLE}; --'` }, + }); + expect(answer?.checks.has(t.checks.find((c) => c.name === "canon_not_chk")!)).toBe(false); + expect(survived).toBe(true); + }); + test("a dollar quote cannot smuggle a statement past the guard", async () => { + const { answer, t, survived } = await answered({ + checks: { canon_not_chk: `a = 1 AND $a$'$a$ <> ''; COMMIT; DROP TABLE ${TABLE}; --'` }, + }); + expect(answer?.checks.has(t.checks.find((c) => c.name === "canon_not_chk")!)).toBe(false); + expect(survived).toBe(true); + }); + test("an index @using that is not an access-method name is not run", async () => { + const { live, expected } = await declared({ indexes: { canon_expr_idx: { expr: "lower(status)" } } }); + const t = expected.tables.find((x) => x.name === TABLE)!; + const ix = t.indexes.find((i) => i.name === "canon_expr_idx")!; + ix.using = `btree (id); COMMIT; DROP TABLE ${TABLE}; CREATE INDEX x ON ${TABLE} USING btree`; + const answer = await postgresExprCanonProbe(db)(live.tables.find((x) => x.name === TABLE)!, t); + expect(answer?.indexes.has(ix)).toBe(false); + const rows = await sql<{ n: string }>`SELECT count(*)::text AS n FROM ${sql.table(TABLE)}`.execute(db); + expect(rows.rows[0]!.n).toBe("0"); + }); + }); + + describe("a refusal for one table does not switch the probe off for the others", () => { + const OTHER = "canon_probe_locked"; + const ROLE = "canon_probe_nosel"; + test("a role without SELECT on one table gets no answer there, and is still probed on the next", async () => { + await sql.raw(`DROP TABLE IF EXISTS ${OTHER}`).execute(db); + await sql.raw(`DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = '${ROLE}') THEN CREATE ROLE ${ROLE}; END IF; END $$`).execute(db); + await sql.raw(`CREATE TABLE ${OTHER} (id int, a int, CONSTRAINT canon_locked_chk CHECK (a > 0))`).execute(db); + await sql.raw(`GRANT USAGE ON SCHEMA public TO ${ROLE}`).execute(db); + await sql.raw(`GRANT SELECT ON ${TABLE} TO ${ROLE}`).execute(db); + const restricted = connect(); + try { + const live = await introspectPostgres(db); + const expected = structuredClone(live); + const lockedLive = live.tables.find((x) => x.name === OTHER)!; + const lockedWant = expected.tables.find((x) => x.name === OTHER)!; + const t = live.tables.find((x) => x.name === TABLE)!; + const tWant = expected.tables.find((x) => x.name === TABLE)!; + const asRole = async (work: (q: Db) => Promise) => { + await restricted.db.transaction().execute(async (trx) => { + await sql.raw(`SET LOCAL ROLE ${ROLE}`).execute(trx); + await work(trx); + }); + }; + await asRole(async (trx) => { + const p = postgresExprCanonProbe(trx); + expect(await p(lockedLive, lockedWant)).toBeUndefined(); + const next = await p(t, tWant); + expect(next?.checks.has(tWant.checks.find((c) => c.name === "canon_not_chk")!)).toBe(true); + }); + } finally { + await restricted.pool.end(); + await sql.raw(`DROP TABLE IF EXISTS ${OTHER}`).execute(db); + await sql.raw(`REVOKE ALL ON ${TABLE} FROM ${ROLE}`).execute(db).catch(() => {}); + await sql.raw(`REVOKE USAGE ON SCHEMA public FROM ${ROLE}`).execute(db).catch(() => {}); + await sql.raw(`DROP OWNED BY ${ROLE}`).execute(db).catch(() => {}); + await sql.raw(`DROP ROLE IF EXISTS ${ROLE}`).execute(db).catch(() => {}); + } + }); + }); + + describe("leaves nothing behind", () => { + test("inside the caller's own transaction the probe uses a savepoint and the transaction stays usable", async () => { + const { live, expected } = await declared({ checks: { canon_prec_chk: "no_such_column = 1" } }); + const lt = live.tables.find((x) => x.name === TABLE)!; + const et = expected.tables.find((x) => x.name === TABLE)!; + await db.transaction().execute(async (trx) => { + const answer = await postgresExprCanonProbe(trx)(lt, et); + expect(answer?.checks.size).toBe(3); // one expression failed to compile; the savepoint absorbed it + const rows = await sql<{ t: string | null; i: string | null }>` + SELECT to_regclass('pg_temp.mo_check_probe')::text AS t, to_regclass('pg_temp.mo_check_probe_i')::text AS i + `.execute(trx); + expect(rows.rows[0]).toEqual({ t: null, i: null }); + await sql`SELECT 1`.execute(trx); // not aborted + }); + }); + test("on its own the probe commits nothing", async () => { + const { live, expected } = await declared({}); + await postgresExprCanonProbe(db)(live.tables.find((x) => x.name === TABLE)!, expected.tables.find((x) => x.name === TABLE)!); + const rows = await sql<{ n: string }>` + SELECT count(*)::text AS n FROM pg_class WHERE relname IN ('mo_check_probe', 'mo_check_probe_i') + `.execute(db); + expect(rows.rows[0]!.n).toBe("0"); + }); + }); + + describe("adoptGeneratedSpelling (baseline --from-db)", () => { + test("records the generated text for a proven-equal expression, and Postgres's for the rest", async () => { + const { live, expected } = await declared({ + checks: { + canon_status_chk: `"status" IN ('open')`, // proven equal: generated text replaces Postgres's + canon_prec_chk: "a = 1 OR (b = 1 AND c = 1)", // a real difference: Postgres's text is kept + }, + indexes: { canon_where_idx: { where: "(a=1 OR b=1) AND c=1" } }, + }); + const out = await adoptGeneratedSpelling(live, expected, postgresExprCanonProbe(db)); + const t = (s: SchemaSnapshot): TableDescriptor => s.tables.find((x) => x.name === TABLE)!; + const check = (s: SchemaSnapshot, n: string) => t(s).checks.find((c) => c.name === n)!.expression; + expect(check(out, "canon_status_chk")).toBe(`"status" IN ('open')`); + expect(check(out, "canon_prec_chk")).toBe(check(live, "canon_prec_chk")); + expect(check(out, "canon_not_chk")).toBe(check(live, "canon_not_chk")); + expect(t(out).indexes.find((i) => i.name === "canon_where_idx")!.where).toBe("(a=1 OR b=1) AND c=1"); + // The offline comparison (no database) now agrees with the model. + const offline = await diff({ expected, actual: out, dialect: "postgres", scopeSchemas: ["public"] }); + expect(offline.changes.filter((c) => c.kind.endsWith("-check") && "check" in c && c.check === "canon_status_chk")).toEqual([]); + }); + test("without an answer the snapshot is untouched", async () => { + const ro = connect("-c default_transaction_read_only=on"); + try { + const { live, expected } = await declared({}); + const out = await adoptGeneratedSpelling(live, expected, postgresExprCanonProbe(ro.db)); + expect(out).toEqual(live); + } finally { + await ro.pool.end(); + } + }); + }); +});