From 8d8e2c36fd59390d04f2a2e4150c12b8bebd243b Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 06:53:14 -0400 Subject: [PATCH 1/5] fix(migrate-ts): stop false CHECK drift on Postgres and compare literals as written A one-value enum, presentIff and validators on varchar(n) columns reported drift after migrate had just built them, because Postgres stores IN (x) as = x, IS NOT DISTINCT FROM as NOT (IS DISTINCT FROM) and casts varchar columns to text. The normaliser also lower-cased and bracket-stripped quoted literals, so an enum member or regex case change read as clean. char(n) is no longer a text-family column for the (col)::text fold. Adds a Postgres round-trip over every generated CHECK shape. --- CHANGELOG.md | 20 ++ .../migrate-ts/src/check-expr-compare.ts | 115 +++++++++--- .../packages/migrate-ts/src/diff/index.ts | 2 +- .../test/check-evolution/diff-offline.test.ts | 13 +- .../test/check-evolution/normalize.test.ts | 69 ++++++- .../test/integration/lifecycle-pg.test.ts | 8 +- .../pg-check-shapes-roundtrip.test.ts | 177 ++++++++++++++++++ .../integration/pg-regex-check-cast.test.ts | 14 ++ 8 files changed, 384 insertions(+), 34 deletions(-) create mode 100644 server/typescript/packages/migrate-ts/test/integration/pg-check-shapes-roundtrip.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 077baeb9b6..e265d9bffb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,26 @@ here.** on any failure, so the previous state is intact. `meta deps check` also loads the model once its hashes match and exits 1 with the load error when it does not load. +- **`meta verify --db` no longer reports false CHECK drift on Postgres for a one-value enum, a + `presentIff` validator, or a validator on a `varchar(n)` column; a changed enum-member case + or regex case no longer reads as clean.** Postgres stores `"k" IN ('x')` as `k = 'x'` and + `a IS NOT DISTINCT FROM b` as `NOT (a IS DISTINCT FROM b)`, and casts a `varchar` column to + text (`(region)::text`) in a comparison, a `length()` and a null-safe test. The comparison now + folds all three back, so a database `migrate` just built verifies clean. The same false drift + reached `meta migrate --from-db`, which proposed a drop-check and add-check on every run, and + the offline `migrate --dry-run` after a `baseline --from-db`. It has done so since 1.0.x. In + the other direction, the comparison lower-cased quoted literals and deleted every bracket, + quote and run of blanks inside them, so `IN ('OPEN')` against `IN ('open')` and `~ '^[A-Z]+$'` + against `~ '^[a-z]+$'` compared as equal. Literals are now compared as written. A check + stored in a different case than the model declares therefore shows as drift once: run + `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. + ### Documentation - **Two doc errors corrected against the code.** `docs/recipes/mysql.md` said Python ships a 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 bf5c4214e9..71b93232df 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -18,6 +18,14 @@ // - 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). +// Three more are folded back by foldPgAnalyzerRewrites and PAREN_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. // // 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 @@ -43,38 +51,90 @@ function unwrapBacktickIdentifiers(expr: string): string { const NO_COLUMNS: ReadonlySet = new Set(); +/** + * `(col)::text` — the form Postgres writes when it casts a `varchar` column to `text` to reach + * a text operator or function (`length((code)::text)`, `(a)::text < (b)::text`, + * `((region)::text IS DISTINCT FROM 'eu'::text)`). Matched BEFORE the brackets are stripped, + * while `(col)` can still be told from a call `f(col)`: the opening bracket must not follow a + * word character. It is dropped only for a column the caller names as text-family + * (`text` / `varchar`), where the cast changes nothing; on any other column it changes the + * operator (citext) or the value (`char(n)` trims its padding), so it stays and reads as drift. + */ +const PAREN_COLUMN_TEXT_CAST = /(? string): string { + return s + .split(/('(?:[^']|'')*')/) + .map((part, i) => (i % 2 === 1 ? part : fn(part, i))) + .join(""); +} + +/** + * Two rewrites Postgres makes while ANALYZING an expression, which no textual normalisation + * of its output can undo, so they are folded back to the spelling migrate emits: + * + * - a ONE-element `x IN (v)` is stored as `x = v` (`IN` only becomes `= ANY (ARRAY[…])` from + * two elements, which the `any array` fold above already handles). A literal list is told + * apart by the comma after its first element, so `in 'a', 'b'` is left alone. + * - `a IS NOT DISTINCT FROM b` is stored as `NOT (a IS DISTINCT FROM b)` — only + * `IS DISTINCT FROM` has a node of its own. The left operand is matched as a bare (dotted) + * identifier, which is every shape migrate generates; a more complex operand reads as drift + * rather than risking a fold that changes the meaning. + * + * Both fire on text that SQLite and D1 never store, and both run outside literals. They sit on + * the normalised string (brackets already gone), where each is a plain word pattern. + */ +function foldPgAnalyzerRewrites(s: string): string { + const parts = s.split(/('(?:[^']|'')*')/); + return parts + .map((part, i) => { + if (i % 2 === 1) return part; + let p = part.replace(/(? in 'v'` — the literal is the next part; a following comma means a list. + if (/(? = NO_COLUMNS): string { - const stripped = unwrapBacktickIdentifiers(expr) - .toLowerCase() - // Drop `::text` / `::"MyType"` type casts PG adds to literals. The lookbehind - // restricts the strip to a cast that immediately follows a CLOSING single - // quote (`'open'::text`), so a `::` appearing INSIDE a regex pattern literal - // (`slug ~ 'a::foo'`) is preserved — otherwise two distinct regex CHECKs would - // normalize equal and a pattern change would be silently missed. - .replace(/(?<=')::\s*"?\w+"?/g, "") - // Unwrap double-quoted IDENTIFIERS only (`"enumval"` → `enumval`). We emit a - // quoted column (`"enumVal" IN (...)`) so a mixed-case column is valid SQL; - // PG's introspected definition quotes mixed-case identifiers too. Unwrapping - // makes the generated and introspected forms compare equal, and keeps a - // legacy unquoted check matching its quoted re-emit. The pattern matches only - // a quote-pair wrapping a bare identifier (word chars), so a `"` INSIDE a - // single-quoted string/regex literal (`slug ~ 'a"b'`) is preserved — two - // distinct patterns must not normalize equal. - .replace(/"(\w+)"/g, "$1") - .replace(/[()[\]]/g, " ") // drop parens AND square brackets (ARRAY[…]) - .replace(/\s+/g, " ") - .trim(); - // PG stores `col IN (…)` as `col = ANY (ARRAY[…])`; after the bracket strip above - // that reads `col = any array …`. Fold it back to the `col in …` form we emit. - const folded = stripped.replace(/=\s*any\s+array/g, "in").replace(/\s+/g, " ").trim(); + // Every textual rewrite below runs OUTSIDE single-quoted literals. A literal is DATA: its + // case, its brackets, its `"quoted"` words and its runs of spaces all change what the + // constraint accepts, so two checks differing only inside a literal (an enum member's + // case, a regex's character class, a `(` in a pattern) must stay distinct. Reading them + // as equal is this file's failure in the worse direction: a real change reported clean. + const stripped = mapOutsideLiterals(unwrapBacktickIdentifiers(expr), (part, index) => + // Drop `::text` / `::"MyType"` type casts PG adds to literals. Only a cast that + // immediately follows a CLOSING single quote (`'open'::text`) is one, which is exactly + // the head of an outside part that follows a literal — so a `::` INSIDE a regex pattern + // literal (`slug ~ 'a::foo'`) is never seen here. + (index > 0 ? part.replace(/^::\s*"?\w+"?/, "") : part) + .toLowerCase() + // Unwrap double-quoted IDENTIFIERS only (`"enumval"` → `enumval`). We emit a + // quoted column (`"enumVal" IN (...)`) so a mixed-case column is valid SQL; + // PG's introspected definition quotes mixed-case identifiers too. Unwrapping + // makes the generated and introspected forms compare equal, and keeps a + // legacy unquoted check matching its quoted re-emit. + .replace(/"(\w+)"/g, "$1") + .replace(PAREN_COLUMN_TEXT_CAST, (whole, column: string) => (textColumns.has(column) ? column : whole)) + .replace(/[()[\]]/g, " ") // drop parens AND square brackets (ARRAY[…]) + .replace(/\s+/g, " ") + // PG stores `col IN (…)` as `col = ANY (ARRAY[…])`; after the bracket strip above + // that reads `col = any array …`. Fold it back to the `col in …` form we emit. + .replace(/=\s*any\s+array/g, "in"), + ).trim(); + const folded = foldPgAnalyzerRewrites(stripped); // Normalize spacing around COMMAS and SYMBOLIC OPERATORS, so the spelling a human // writes compares equal to the one Postgres hands back. // @@ -199,8 +259,9 @@ function readCastTarget(s: string, i: number): { type: string; consumed: number */ function isElidableCast(type: string, lhs: string, rest: string, textColumns: ReadonlySet): boolean { // `::text` on a COLUMN directly before a regex-match operator (`~`, `~*`, `!~`, `!~*`), - // and only when the caller names that column as text-family (text, character varying, - // character). On such a column PG adds the cast itself (`"key" ~ '…'` comes back + // and only when the caller names that column as text-family (text, character varying; + // NOT char(n), whose own operator sees the blank padding the cast trims). On such a + // column PG adds the cast itself (`"key" ~ '…'` comes back // `((key)::text ~ '…'::text)`). On any other column, citext included, the cast changes // the operator (`col::text ~ 'A'` is case-sensitive, `col ~ 'A'` on citext is not), so it // survives. With no set supplied nothing is elided. diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index 3f6194a76d..a073ef281c 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -809,7 +809,7 @@ function respelledSpread(expected: CheckDescriptor, actual: CheckDescriptor): { return checkExprRespelledNullSafe(expected.expression, actual.expression) ? { respelled: true } : {}; } -const PG_TEXT_FAMILY_UDTS: ReadonlySet = new Set(["text", "varchar", "bpchar"]); +const PG_TEXT_FAMILY_UDTS: ReadonlySet = new Set(["text", "varchar"]); function diffTableChecks( expected: TableDescriptor, diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts index 20c06021f8..c111c37bdc 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts @@ -63,9 +63,20 @@ describe("diffTableChecks — regex check on a text-family column", () => { const r = await diff({ expected: withKey(undefined, [generated]), actual: withKey(udtName, [stored]), dialect: "postgres" }); return r.changes.filter((c) => c.kind.endsWith("-check")); }; - test.each(["text", "varchar", "bpchar"])("%s column: Postgres's rewrite is no drift", async (udt) => { + test.each(["text", "varchar"])("%s column: Postgres's rewrite is no drift", async (udt) => { expect(await checkChanges(udt)).toEqual([]); }); + // char(n) is NOT in the family: `~(bpchar, text)` is bpcharregexeq, which sees the blank + // padding that `(col)::text` trims, so an explicit cast is a different constraint. Postgres + // never adds the cast on a bpchar column, so its own rewrite arrives uncast and stays clean. + test("bpchar column: an explicit ::text cast is drift against the uncast check", async () => { + expect((await checkChanges("bpchar")).map((c) => c.kind).sort()).toEqual(["add-check", "drop-check"]); + }); + test("bpchar column: the uncast form Postgres stores is clean", async () => { + const uncast = CHK("spaces_key_regex_chk", `(key ~ '^[a-z0-9][a-z0-9-]*$'::text)`); + const r = await diff({ expected: withKey(undefined, [generated]), actual: withKey("bpchar", [uncast]), dialect: "postgres" }); + expect(r.changes.filter((c) => c.kind.endsWith("-check"))).toEqual([]); + }); test("citext column: cast vs uncast is drift", async () => { expect((await checkChanges("citext")).map((c) => c.kind).sort()).toEqual(["add-check", "drop-check"]); }); diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts index 2829a176e4..a4dc86985d 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts @@ -27,7 +27,7 @@ describe("normalizeCheckExpr", () => { "status = ANY (ARRAY['OPEN'::text, 'CLOSED'::text])", )).toBe(true); expect(normalizeCheckExpr("status = ANY (ARRAY['OPEN'::text, 'CLOSED'::text])")) - .toBe("status in 'open','closed'"); + .toBe("status in 'OPEN','CLOSED'"); // different member sets remain distinct after the fold expect(checkExprEquals( "status IN ('OPEN', 'CLOSED')", @@ -226,3 +226,70 @@ describe("regex check on a varchar/text column", () => { expect(checkExprEquals("qty > 0", "((qty)::text > '0'::text)", new Set(["qty"]))).toBe(false); }); }); + +describe("Postgres analyzer rewrites that text normalisation cannot undo", () => { + test("a one-value IN equals the `=` Postgres stores", () => { + expect(checkExprEquals(`"resource_kind" IN ('space')`, `(resource_kind = 'space'::text)`)).toBe(true); + expect(checkExprEquals(`"level" IN (3)`, `(level = 3)`)).toBe(true); + // the folded form still tells members apart + expect(checkExprEquals(`"kind" IN ('space')`, `(kind = 'other'::text)`)).toBe(false); + expect(checkExprEquals(`"level" IN (3)`, `(level = 4)`)).toBe(false); + }); + test("a list is not folded, and neither is NOT IN", () => { + expect(normalizeCheckExpr(`k IN ('a', 'b')`)).toBe(`k in 'a','b'`); + expect(normalizeCheckExpr(`k IN ('a' , 'b')`)).toBe(`k in 'a','b'`); + expect(normalizeCheckExpr(`n IN (1, 2)`)).toBe(`n in 1,2`); + expect(normalizeCheckExpr(`n IN (1 , 2)`)).toBe(`n in 1,2`); + expect(normalizeCheckExpr(`k NOT IN ('a')`)).toBe(`k not in 'a'`); + expect(normalizeCheckExpr(`n NOT IN (1)`)).toBe(`n not in 1`); + expect(checkExprEquals(`k IN ('a')`, `k NOT IN ('a')`)).toBe(false); + }); + test("IS NOT DISTINCT FROM equals the NOT (… IS DISTINCT FROM …) Postgres stores", () => { + expect(checkExprEquals( + `("principal_id" IS NOT NULL) = ("kind" IS NOT DISTINCT FROM 'principal')`, + `((principal_id IS NOT NULL) = (NOT (kind IS DISTINCT FROM 'principal'::text)))`, + )).toBe(true); + expect(checkExprEquals( + `("used_at" IS NOT NULL) = ("flag" IS NOT DISTINCT FROM TRUE)`, + `((used_at IS NOT NULL) = (NOT (flag IS DISTINCT FROM true)))`, + )).toBe(true); + // IS DISTINCT FROM (requiredWhen) is kept as written by Postgres and must not fold + expect(checkExprEquals(`"k" IS DISTINCT FROM 'p'`, `NOT (k IS DISTINCT FROM 'p'::text)`)).toBe(false); + // `IS NOT NULL` is not a left operand + expect(normalizeCheckExpr(`NOT (x IS NOT NULL IS DISTINCT FROM y)`)).toBe(`not x is not null is distinct from y`); + }); + test("a varchar column's `(col)::text` cast is dropped wherever Postgres adds it", () => { + const cols = new Set(["region", "name_a", "name_b", "short_code"]); + expect(checkExprEquals(`"region" IS NOT DISTINCT FROM 'eu'`, `NOT ((region)::text IS DISTINCT FROM 'eu'::text)`, cols)).toBe(true); + expect(checkExprEquals(`("region" IS DISTINCT FROM 'us') OR (x IS NOT NULL)`, `(((region)::text IS DISTINCT FROM 'us'::text) OR (x IS NOT NULL))`, cols)).toBe(true); + expect(checkExprEquals(`"name_a" < "name_b"`, `((name_a)::text < (name_b)::text)`, cols)).toBe(true); + expect(checkExprEquals(`length("short_code") >= 2`, `(length((short_code)::text) >= 2)`, cols)).toBe(true); + }); + test("…but only for a named text-family column, and never a cast on a call", () => { + expect(checkExprEquals(`"name_a" < "name_b"`, `((name_a)::text < (name_b)::text)`)).toBe(false); + expect(checkExprEquals(`"name_a" < "name_b"`, `((name_a)::text < (name_b)::text)`, new Set(["other"]))).toBe(false); + // `length(code)::text` is a cast of the RESULT, not of the column + expect(checkExprEquals(`length("code") = '3'`, `(length(code))::text = '3'`, new Set(["code"]))).toBe(false); + }); +}); + +describe("a quoted literal is data: a change inside it is drift", () => { + test("an enum member's case", () => { + expect(checkExprEquals(`"status" IN ('OPEN', 'CLOSED')`, `"status" IN ('open', 'closed')`)).toBe(false); + expect(checkExprEquals(`"status" IN ('OPEN')`, `(status = 'open'::text)`)).toBe(false); + expect(checkExprEquals(`"status" IN ('OPEN', 'CLOSED')`, `status = ANY (ARRAY['open'::text, 'closed'::text])`)).toBe(false); + }); + test("a regex's case", () => { + expect(checkExprEquals(`"code" ~ '^[A-Z]+$'`, `"code" ~ '^[a-z]+$'`)).toBe(false); + }); + test("brackets, parens, double quotes and runs of blanks inside a literal", () => { + expect(checkExprEquals(`slug ~ '^(a|b)$'`, `slug ~ '^a|b$'`)).toBe(false); + expect(checkExprEquals(`slug ~ '^[ab]$'`, `slug ~ '^ab$'`)).toBe(false); + expect(checkExprEquals(`slug ~ 'a"b"c'`, `slug ~ 'abc'`)).toBe(false); + expect(checkExprEquals(`k = 'a b'`, `k = 'a b'`)).toBe(false); + }); + test("the same literal still matches under Postgres's respelling of everything around it", () => { + expect(checkExprEquals(`("Code" ~ '^(A|b)$') AND (length("Code") >= 2)`, `((("Code" ~ '^(A|b)$'::text)) AND (length("Code") >= 2))`)).toBe(true); + expect(checkExprEquals(`"k" IN ('It''s')`, `(k = 'It''s'::text)`)).toBe(true); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/integration/lifecycle-pg.test.ts b/server/typescript/packages/migrate-ts/test/integration/lifecycle-pg.test.ts index d27e14f67c..832550a18a 100644 --- a/server/typescript/packages/migrate-ts/test/integration/lifecycle-pg.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/lifecycle-pg.test.ts @@ -173,7 +173,7 @@ d("migrate-ts lifecycle against real Postgres", () => { // too, so a check that vanished cannot hide behind two that matched. expect(orders1.checks).toHaveLength(2); expect(hasCheck(orders1, "qty >= 1")).toBe(true); - expect(hasCheck(orders1, "status in 'open','closed'")).toBe(true); + expect(hasCheck(orders1, "status in 'OPEN','CLOSED'")).toBe(true); // Proves `hasCheck` can convict: if the comparator ever normalized everything to one // value, every assertion above would pass while proving nothing, and this is the line // that would say so. @@ -205,7 +205,7 @@ d("migrate-ts lifecycle against real Postgres", () => { expect(colNames(orders2)).toEqual(["customer_id", "email", "id", "note", "qty", "status"]); expect(orders2.foreignKeys.some((f) => f.refTable === "lc_customers")).toBe(true); // evolved checks - expect(hasCheck(orders2, "status in 'open','closed','cancelled'")).toBe(true); + expect(hasCheck(orders2, "status in 'OPEN','CLOSED','CANCELLED'")).toBe(true); expect(hasCheck(orders2, "qty >= 1 and qty <= 1000")).toBe(true); // The pre-evolution bound is GONE, not merely joined by a second check. expect(hasCheck(orders2, "qty >= 1")).toBe(false); @@ -224,7 +224,7 @@ d("migrate-ts lifecycle against real Postgres", () => { // constraint behind alongside the restored one still fails here. expect(ordersBack.checks).toHaveLength(2); expect(hasCheck(ordersBack, "qty >= 1")).toBe(true); - expect(hasCheck(ordersBack, "status in 'open','closed'")).toBe(true); + expect(hasCheck(ordersBack, "status in 'OPEN','CLOSED'")).toBe(true); // ---- idempotency: re-diff v1 against the live (rolled-back) DB ---------- const liveSnap = await introspectPostgres(db); @@ -346,7 +346,7 @@ d("migrate-ts lifecycle against real Postgres", () => { expect(posts.foreignKeys.some((f) => f.refTable === "gf_users")).toBe(true); expect(comments.foreignKeys.map((f) => f.refTable).sort()).toEqual(["gf_posts", "gf_users"]); // enum + numeric checks + unique indexes survived the round-trip - expect(hasCheck(gf.find((t) => t.name === "gf_users")!, "role in 'admin','editor','viewer'")).toBe(true); + expect(hasCheck(gf.find((t) => t.name === "gf_users")!, "role in 'ADMIN','EDITOR','VIEWER'")).toBe(true); expect(hasCheck(posts, "views >= 0")).toBe(true); expect(gf.find((t) => t.name === "gf_tags")!.indexes.some((i) => i.unique && i.columns.includes("name"))).toBe(true); 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 new file mode 100644 index 0000000000..f376ef3b71 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-check-shapes-roundtrip.test.ts @@ -0,0 +1,177 @@ +/** + * Every CHECK shape `meta migrate` generates must read as clean on Postgres once applied. + * + * The gate that was missing: migrate → apply → `verify --db` (computeDrift, the same path the + * CLI runs) must report NO drift. Postgres rewrites a stored CHECK (`IN ('x')` → `= 'x'`, + * `IS NOT DISTINCT FROM` → `NOT (… IS DISTINCT FROM …)`, `IN (a, b)` → `= ANY (ARRAY[…])`, + * casts, parentheses), so the oracle is the live engine: a shape that is not in this model is a + * shape nothing proves round-trips. One-value enums and `presentIff` drifted forever because no + * integration model declared either. + * + * Every `buildChecks` branch in expected-schema.ts is declared once below, each on the column + * type it really lands on. Add a new generated CHECK shape to the model, not to a new test. + * + * 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 { 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 { diff } from "../../src/diff/index.js"; +import { emit } from "../../src/emit/index.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const TABLE = "check_shapes"; + +const model = JSON.stringify({ + "metadata.root": { + package: "acme", + children: [ + { + "object.entity": { + name: "CheckShape", + children: [ + { "source.rdb": { "@table": TABLE } }, + { "field.long": { name: "id" } }, + // Enum membership: string-backed and int-backed, one value and several. + { "field.enum": { name: "oneValue", "@values": ["SPACE"] } }, + { "field.enum": { name: "twoValues", "@values": ["PRINCIPAL", "GROUP"] } }, + { "field.enum": { name: "mixedCase", "@values": ["Open", "CLOSED"] } }, + { "field.enum": { name: "oneInt", "@values": ["ONLY"], "@intValueMap": { ONLY: 3 } } }, + { "field.enum": { name: "manyInts", "@values": ["A", "B", "C"], "@intValueMap": { A: 0, B: 5, C: 9 } } }, + // Single-field validators. + { "field.int": { name: "qty", children: [{ "validator.numeric": { name: "range", "@min": 1, "@max": 100 } }] } }, + { "field.int": { name: "floor", children: [{ "validator.numeric": { name: "min", "@min": 0 } }] } }, + { "field.string": { name: "code", children: [{ "validator.length": { name: "len", "@min": 2, "@max": 8 } }] } }, + { "field.string": { name: "slug", "@maxLength": 64, children: [{ "validator.regex": { name: "re", "@pattern": "^[a-z0-9][a-z0-9-]*$" } }] } }, + { "field.string": { name: "tag", children: [{ "validator.regex": { name: "re", "@pattern": "^[A-Z]+'s$" } }] } }, + // comparison: every operator. The constraint name is keyed on the LEFT column, so + // each operator gets its own left column. + { "field.int": { name: "lo" } }, + { "field.int": { name: "hi" } }, + { "field.int": { name: "cmpGt" } }, + { "field.int": { name: "cmpGte" } }, + { "field.int": { name: "cmpLt" } }, + { "field.int": { name: "cmpLte" } }, + { "field.int": { name: "cmpNe" } }, + { "field.int": { name: "cmpEq" } }, + { "validator.comparison": { name: "gt", "@left": "cmpGt", "@op": "gt", "@right": "lo" } }, + { "validator.comparison": { name: "gte", "@left": "cmpGte", "@op": "gte", "@right": "lo" } }, + { "validator.comparison": { name: "lt", "@left": "cmpLt", "@op": "lt", "@right": "hi" } }, + { "validator.comparison": { name: "lte", "@left": "cmpLte", "@op": "lte", "@right": "hi" } }, + { "validator.comparison": { name: "ne", "@left": "cmpNe", "@op": "ne", "@right": "hi" } }, + { "validator.comparison": { name: "eq", "@left": "cmpEq", "@op": "eq", "@right": "hi" } }, + // requiredWhen / presentIff gated on a string, a boolean, an enum and a number. + { "field.string": { name: "kind" } }, + { "field.boolean": { name: "flag" } }, + { "field.int": { name: "level" } }, + // A varchar(n) gate: PG casts such a column to text inside the stored expression. + { "field.string": { name: "region", "@maxLength": 16 } }, + { "field.string": { name: "regionNote" } }, + { "field.string": { name: "regionAudit" } }, + { "field.string": { name: "shortCode", "@maxLength": 8, children: [{ "validator.length": { name: "len", "@min": 2, "@max": 8 } }] } }, + { "field.string": { name: "nameA", "@maxLength": 8 } }, + { "field.string": { name: "nameB", "@maxLength": 8 } }, + { "validator.comparison": { name: "names", "@left": "nameA", "@op": "lt", "@right": "nameB" } }, + { "validator.requiredWhen": { name: "rwVarchar", "@field": "regionAudit", "@when": "region", "@equals": "us" } }, + { "field.string": { name: "principalId" } }, + { "field.string": { name: "groupId" } }, + { "field.timestamp": { name: "usedAt" } }, + { "field.timestamp": { name: "resolvedAt" } }, + { "field.string": { name: "levelNote" } }, + { "field.string": { name: "enumNote" } }, + { "field.string": { name: "enumNote2" } }, + { "validator.presentIff": { name: "piString", "@field": "principalId", "@when": "kind", "@equals": "principal" } }, + { "validator.presentIff": { name: "piBool", "@field": "usedAt", "@when": "flag", "@equals": "true" } }, + { "validator.presentIff": { name: "piBoolFalse", "@field": "groupId", "@when": "flag", "@equals": "false" } }, + { "validator.presentIff": { name: "piVarchar", "@field": "regionNote", "@when": "region", "@equals": "eu" } }, + { "validator.presentIff": { name: "piNumber", "@field": "levelNote", "@when": "level", "@equals": "3" } }, + { "validator.presentIff": { name: "piEnum", "@field": "enumNote", "@when": "oneValue", "@equals": "SPACE" } }, + { "validator.requiredWhen": { name: "rwString", "@field": "resolvedAt", "@when": "kind", "@equals": "group" } }, + { "validator.requiredWhen": { name: "rwBool", "@field": "enumNote2", "@when": "flag", "@equals": "true" } }, + { "validator.requiredWhen": { name: "rwEnum", "@field": "tag", "@when": "twoValues", "@equals": "GROUP" } }, + { "validator.atLeastOne": { name: "any", "@fields": ["principalId", "groupId"] } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + ], + }, +}); + +describe("every generated CHECK shape round-trips clean on live Postgres", () => { + if (!PG_URL) { + test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); + return; + } + let pool: Pool; + let kysely: Kysely>; + + beforeAll(async () => { + pool = new Pool({ connectionString: PG_URL }); + kysely = new Kysely>({ dialect: new PostgresDialect({ pool }) }); + await sql.raw(`DROP TABLE IF EXISTS ${TABLE} CASCADE`).execute(kysely); + }); + afterAll(async () => { + await sql.raw(`DROP TABLE IF EXISTS ${TABLE} CASCADE`).execute(kysely); + await pool.end(); + }); + + test("migrate → apply → verify --db reports no CHECK drift for any generated shape", async () => { + const loaded = await new MetaDataLoader().load([new InMemoryStringSource(model)]); + expect(loaded.errors).toEqual([]); + + const expected = buildExpectedSchema(loaded.root, { dialect: "postgres" }); + const checks = expected.tables.find((t) => t.name === TABLE)!.checks; + // The gate is not vacuous: every shape this model names was really generated. + expect(checks.map((c) => c.name).sort()).toEqual([ + "check_shapes_cmp_eq_cmp_chk", + "check_shapes_cmp_gt_cmp_chk", + "check_shapes_cmp_gte_cmp_chk", + "check_shapes_cmp_lt_cmp_chk", + "check_shapes_cmp_lte_cmp_chk", + "check_shapes_cmp_ne_cmp_chk", + "check_shapes_code_length_chk", + "check_shapes_enum_note2_reqwhen_chk", + "check_shapes_enum_note_presentiff_chk", + "check_shapes_floor_numeric_chk", + "check_shapes_group_id_presentiff_chk", + "check_shapes_level_note_presentiff_chk", + "check_shapes_many_ints_chk", + "check_shapes_mixed_case_chk", + "check_shapes_name_a_cmp_chk", + "check_shapes_one_int_chk", + "check_shapes_one_value_chk", + "check_shapes_principal_id_atleastone_chk", + "check_shapes_principal_id_presentiff_chk", + "check_shapes_qty_numeric_chk", + "check_shapes_region_audit_reqwhen_chk", + "check_shapes_region_note_presentiff_chk", + "check_shapes_resolved_at_reqwhen_chk", + "check_shapes_short_code_length_chk", + "check_shapes_slug_regex_chk", + "check_shapes_tag_regex_chk", + "check_shapes_tag_reqwhen_chk", + "check_shapes_two_values_chk", + "check_shapes_used_at_presentiff_chk", + ]); + + const initial = await diff({ expected, actual: await introspectPostgres(kysely), dialect: "postgres" }); + expect(initial.blocked).toEqual([]); + const { up } = emit(initial.changes, { dialect: "postgres" }); + await sql.raw(up).execute(kysely); + + const drift = await computeDrift(kysely, "postgres", loaded.root); + const checkDrift = drift.changes.filter((c) => c.kind.endsWith("-check")); + if (checkDrift.length > 0) { + console.error("CHECK DRIFT after apply:"); + for (const c of checkDrift) console.error(` - ${c.kind}:`, JSON.stringify(c, null, 2)); + } + expect(checkDrift).toEqual([]); + expect(drift.changes).toEqual([]); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts index 1287077309..8dfe3b4c8c 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts @@ -91,6 +91,20 @@ describe("regex CHECK vs Postgres's stored rewrite (live PG)", () => { expect(r.kinds).toEqual(["add-check", "drop-check"]); }); + // char(n) has its own regex operator (bpcharregexeq), which sees the blank padding that + // `(col)::text` trims — so an explicit cast is a different check, not Postgres's rewrite. + // Postgres itself never adds the column cast on a bpchar column. + test("char(n) column: Postgres stores the uncast check without a column cast", async () => { + const r = await checkChanges("char(8)"); + expect(r.stored).not.toContain("(key)::text"); + expect(r.kinds).toEqual([]); + }); + test("char(n) column: a check stored with an explicit ::text cast is drift", async () => { + const r = await checkChanges("char(8)", `("key")::text ~ '${PATTERN}'`); + expect(r.stored).toContain("(key)::text"); + expect(r.kinds).toEqual(["add-check", "drop-check"]); + }); + test("varchar column: a changed pattern still reads as drift", async () => { const r = await checkChanges("varchar(64)", `"key" ~ '^[a-z]+$'`); expect(r.kinds).toEqual(["add-check", "drop-check"]); From 6ae1046f1566b8940492d46ff65ac1dc94a05287 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 06:59:16 -0400 Subject: [PATCH 2/5] no-mistakes(review): Fold IS NOT DISTINCT FROM safely; keep text cast in renames --- .../migrate-ts/src/check-expr-compare.ts | 88 +++++++++++++++++-- .../packages/migrate-ts/src/diff/index.ts | 37 ++++++-- .../test/check-evolution/diff-offline.test.ts | 33 +++++++ .../test/check-evolution/normalize.test.ts | 6 ++ 4 files changed, 147 insertions(+), 17 deletions(-) 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 71b93232df..2727f48f47 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -70,6 +70,80 @@ function mapOutsideLiterals(s: string, fn: (part: string, index: number) => stri .join(""); } +/** + * Index of the `)` closing the `(` at `open`, skipping single-quoted literals and + * double-quoted identifiers; -1 when unbalanced. + */ +function matchingParen(s: string, open: number): number { + let depth = 0; + for (let i = open; i < s.length; i++) { + const ch = s[i]!; + if (ch === "'" || ch === '"') { + i++; + while (i < s.length && !(s[i] === ch && s[i + 1] !== ch)) i += s[i] === ch ? 2 : 1; + } else if (ch === "(") depth++; + else if (ch === ")" && --depth === 0) return i; + } + return -1; +} + +/** `inner` with its depth-0 text (literals, quoted identifiers and bracketed groups blanked). */ +function topLevelText(inner: string): string { + let out = ""; + let depth = 0; + for (let i = 0; i < inner.length; i++) { + const ch = inner[i]!; + if (ch === "'" || ch === '"') { + i++; + while (i < inner.length && !(inner[i] === ch && inner[i + 1] !== ch)) i += inner[i] === ch ? 2 : 1; + out += " "; + } else if (ch === "(" || ch === "[") { depth++; out += " "; } + else if (ch === ")" || ch === "]") { depth--; out += " "; } + else out += depth === 0 ? ch : " "; + } + return out; +} + +/** + * `NOT (A IS DISTINCT FROM B)` → `(A IS NOT DISTINCT FROM B)`: Postgres has a node only for + * `IS DISTINCT FROM`, so it stores `a IS NOT DISTINCT FROM b` in the first spelling. Run on + * the RAW expression, while the brackets still show what `NOT` applies to: only `NOT` + * followed by one balanced bracketed operand that is exactly a single `IS DISTINCT FROM` + * comparison (no depth-0 `and` / `or` / `not`) is folded. `(NOT x) IS DISTINCT FROM y` + * has no such operand and stays as written, so the fold cannot invent an equality. + */ +function foldNotDistinctFrom(expr: string): string { + let out = ""; + let i = 0; + while (i < expr.length) { + const ch = expr[i]!; + if (ch === "'" || ch === '"') { + const start = i++; + while (i < expr.length && !(expr[i] === ch && expr[i + 1] !== ch)) i += expr[i] === ch ? 2 : 1; + i++; + out += expr.slice(start, i); + continue; + } + const not = /^not\s*\(/i.exec(expr.slice(i)); + if (not && !/[\w.]/.test(expr[i - 1] ?? " ")) { + const open = i + not[0].length - 1; + const close = matchingParen(expr, open); + if (close !== -1) { + const inner = expr.slice(open + 1, close); + const top = topLevelText(inner); + if (top.match(/\bis\s+distinct\s+from\b/gi)?.length === 1 && !/\b(?:and|or|not)\b/i.test(top)) { + out += `(${inner.replace(/\bis\s+distinct\s+from\b/i, "is not distinct from")})`; + i = close + 1; + continue; + } + } + } + out += ch; + i++; + } + return out; +} + /** * Two rewrites Postgres makes while ANALYZING an expression, which no textual normalisation * of its output can undo, so they are folded back to the spelling migrate emits: @@ -77,20 +151,18 @@ function mapOutsideLiterals(s: string, fn: (part: string, index: number) => stri * - a ONE-element `x IN (v)` is stored as `x = v` (`IN` only becomes `= ANY (ARRAY[…])` from * two elements, which the `any array` fold above already handles). A literal list is told * apart by the comma after its first element, so `in 'a', 'b'` is left alone. - * - `a IS NOT DISTINCT FROM b` is stored as `NOT (a IS DISTINCT FROM b)` — only - * `IS DISTINCT FROM` has a node of its own. The left operand is matched as a bare (dotted) - * identifier, which is every shape migrate generates; a more complex operand reads as drift - * rather than risking a fold that changes the meaning. + * - `a IS NOT DISTINCT FROM b` is stored as `NOT (a IS DISTINCT FROM b)`; see + * foldNotDistinctFrom, which runs earlier because it needs the brackets. * - * Both fire on text that SQLite and D1 never store, and both run outside literals. They sit on - * the normalised string (brackets already gone), where each is a plain word pattern. + * The IN fold fires on text that SQLite and D1 never store and runs outside literals. It sits on + * the normalised string (brackets already gone), where it is a plain word pattern. */ function foldPgAnalyzerRewrites(s: string): string { const parts = s.split(/('(?:[^']|'')*')/); return parts .map((part, i) => { if (i % 2 === 1) return part; - let p = part.replace(/(? in 'v'` — the literal is the next part; a following comma means a list. if (/(? + const stripped = mapOutsideLiterals(foldNotDistinctFrom(unwrapBacktickIdentifiers(expr)), (part, index) => // Drop `::text` / `::"MyType"` type casts PG adds to literals. Only a cast that // immediately follows a CLOSING single quote (`'open'::text`) is one, which is exactly // the head of an outside part that follows a literal — so a `::` INSIDE a regex pattern diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index a073ef281c..00e5fba0d1 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -283,7 +283,10 @@ export async function diff( // Pass 3: column renames (table renames were resolved in Pass 1c). await detectColumnRenames(changes, args.onAmbiguous, args.renames); // Pass 3b: a renamed column carries the constraints and indexes named after it. - carryThroughColumnRenames(changes, args.dialect); + carryThroughColumnRenames(changes, args.dialect, (id) => { + const live = predecessors.get(id) ?? actualTables.get(id); + return live === undefined ? NO_TEXT_COLUMNS : textColumnsOf(live); + }); // Strip the rename-detection side-channel fields before status assignment / return. for (const c of changes) { @@ -369,7 +372,7 @@ function diffRenamedTables( const carried = withRenamedRefTables({ ...actual, name: r.to }, renamed); const tableChanges: Change[] = []; diffTablePair(expected, carried, tableChanges, dialect); - changes.push(...(dialect === "postgres" ? foldNameOnlyChanges(r, tableChanges) : tableChanges)); + changes.push(...(dialect === "postgres" ? foldNameOnlyChanges(r, tableChanges, textColumnsOf(actual)) : tableChanges)); } return { renamed, predecessors }; } @@ -422,7 +425,11 @@ function withRenamedRefTables(t: TableDescriptor, renamed: ReadonlyMap, tableChanges: Change[]): Change[] { +function foldNameOnlyChanges( + r: Extract, + tableChanges: Change[], + textColumns: ReadonlySet, +): Change[] { const constraintRenames: NameChange[] = []; const indexRenames: NameChange[] = []; const folded = new Set(); @@ -436,7 +443,7 @@ function foldNameOnlyChanges(r: Extract, table } 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)); + c.kind === "add-check" && !folded.has(c) && checkExprEquals(c.check.expression, restore.expression, 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; @@ -474,7 +481,11 @@ function foldNameOnlyChanges(r: Extract, table * * Anything that is not EXACTLY the old rule under the new names stays a gated drop+add. */ -function carryThroughColumnRenames(changes: Change[], dialect: Dialect | undefined): void { +function carryThroughColumnRenames( + changes: Change[], + dialect: Dialect | undefined, + textColumnsOfTable: (tableId: string) => ReadonlySet, +): void { const renamesByTable = new Map>(); const renameChangeByTable = new Map>(); for (const c of changes) { @@ -503,8 +514,10 @@ function carryThroughColumnRenames(changes: Change[], dialect: Dialect | undefin const sameTable = (c: Change): boolean => !paired.has(c) && idOf(c) === table; if (d.kind === "drop-check" && d.restore !== undefined) { const want = renameExprIdentifiers(d.restore.expression, renames); + 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)); + c.kind === "add-check" && sameTable(c) && checkExprEquals(c.check.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) }; @@ -811,6 +824,14 @@ function respelledSpread(expected: CheckDescriptor, actual: CheckDescriptor): { const PG_TEXT_FAMILY_UDTS: ReadonlySet = new Set(["text", "varchar"]); +const NO_TEXT_COLUMNS: ReadonlySet = new Set(); + +function textColumnsOf(table: TableDescriptor): Set { + return new Set( + table.columns.filter((c) => c.udtName !== undefined && PG_TEXT_FAMILY_UDTS.has(c.udtName)).map((c) => c.name.toLowerCase()), + ); +} + function diffTableChecks( expected: TableDescriptor, actual: TableDescriptor, @@ -831,9 +852,7 @@ function diffTableChecks( const exprFallback = dialect === "sqlite" || dialect === "d1"; const actualByName = new Map(actual.checks.filter((c) => c.name !== "").map((c) => [c.name, c])); const consumed = new Set(); - const textColumns = new Set( - actual.columns.filter((c) => c.udtName !== undefined && PG_TEXT_FAMILY_UDTS.has(c.udtName)).map((c) => c.name.toLowerCase()), - ); + const textColumns = textColumnsOf(actual); for (const ec of expected.checks) { const ac = actualByName.get(ec.name); diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts index c111c37bdc..e4c4a2be00 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts @@ -81,3 +81,36 @@ describe("diffTableChecks — regex check on a text-family column", () => { expect((await checkChanges("citext")).map((c) => c.kind).sort()).toEqual(["add-check", "drop-check"]); }); }); + +describe("renames keep the (col)::text fold", () => { + const col = (name: string) => ({ name, sqlType: { kind: "text" as const }, nullable: false, udtName: "varchar" }); + const table = (name: string, column: string, check: CheckDescriptor): TableDescriptor => + ({ name, columns: [col(column)], indexes: [], foreignKeys: [], checks: [check], primaryKey: [] }); + const s = (t: TableDescriptor): SchemaSnapshot => ({ tables: [t], views: [] }); + + test("table rename: a varchar length check Postgres stores cast is folded into the rename", async () => { + const r = await diff({ + expected: s(table("members", "code", CHK("members_code_chk", `length("code") >= 2`))), + actual: s(table("users", "code", CHK("users_code_chk", `(length((code)::text) >= 2)`))), + dialect: "postgres", + renames: [{ kind: "table", from: "users", to: "members" }], + }); + expect(r.changes.filter((c) => c.kind.endsWith("-check"))).toEqual([]); + expect(r.changes).toEqual([expect.objectContaining({ + kind: "rename-table", constraintRenames: [{ from: "users_code_chk", to: "members_code_chk" }], + })]); + }); + + test("column rename: the check follows the renamed varchar column without drop+add", async () => { + const r = await diff({ + expected: s(table("users", "handle", CHK("users_handle_chk", `length("handle") >= 2`))), + actual: s(table("users", "code", CHK("users_code_chk", `(length((code)::text) >= 2)`))), + dialect: "postgres", + renames: [{ kind: "column", table: "users", from: "code", to: "handle" }], + }); + expect(r.changes.filter((c) => c.kind.endsWith("-check"))).toEqual([]); + expect(r.changes).toEqual([expect.objectContaining({ + kind: "rename-column", constraintRenames: [{ from: "users_code_chk", to: "users_handle_chk" }], + })]); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts index a4dc86985d..53f06337f5 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts @@ -255,6 +255,12 @@ describe("Postgres analyzer rewrites that text normalisation cannot undo", () => )).toBe(true); // IS DISTINCT FROM (requiredWhen) is kept as written by Postgres and must not fold expect(checkExprEquals(`"k" IS DISTINCT FROM 'p'`, `NOT (k IS DISTINCT FROM 'p'::text)`)).toBe(false); + // `(NOT x) IS DISTINCT FROM y` is not the negation of a comparison: the fold must not equate it + expect(checkExprEquals(`(NOT x) IS DISTINCT FROM y`, `x IS NOT DISTINCT FROM y`)).toBe(false); + expect(checkExprEquals(`(NOT x) IS DISTINCT FROM y`, `NOT (x IS DISTINCT FROM y)`)).toBe(false); + expect(checkExprEquals(`x IS NOT DISTINCT FROM y`, `NOT (x IS DISTINCT FROM y)`)).toBe(true); + // a negated group that is more than one comparison is not folded + expect(checkExprEquals(`NOT (a IS DISTINCT FROM b OR c)`, `a IS NOT DISTINCT FROM b OR c`)).toBe(false); // `IS NOT NULL` is not a left operand expect(normalizeCheckExpr(`NOT (x IS NOT NULL IS DISTINCT FROM y)`)).toBe(`not x is not null is distinct from y`); }); From ed2eb026e043d82055c65cac87eb2bfd330f0be3 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 07:01:36 -0400 Subject: [PATCH 3/5] no-mistakes(review): Rewrite only the depth-0 IS DISTINCT FROM operator --- .../migrate-ts/src/check-expr-compare.ts | 16 +++++++++++----- .../test/check-evolution/normalize.test.ts | 6 ++++++ 2 files changed, 17 insertions(+), 5 deletions(-) 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 2727f48f47..1f9f0904a2 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -87,16 +87,20 @@ function matchingParen(s: string, open: number): number { return -1; } -/** `inner` with its depth-0 text (literals, quoted identifiers and bracketed groups blanked). */ +/** + * `inner` with everything below depth 0 blanked (literals, quoted identifiers, bracketed + * groups). Same length as `inner`, so an offset found here is the same offset in `inner`. + */ function topLevelText(inner: string): string { let out = ""; let depth = 0; for (let i = 0; i < inner.length; i++) { const ch = inner[i]!; if (ch === "'" || ch === '"') { - i++; + const start = i++; while (i < inner.length && !(inner[i] === ch && inner[i + 1] !== ch)) i += inner[i] === ch ? 2 : 1; - out += " "; + i = Math.min(i, inner.length - 1); + out += " ".repeat(i - start + 1); } else if (ch === "(" || ch === "[") { depth++; out += " "; } else if (ch === ")" || ch === "]") { depth--; out += " "; } else out += depth === 0 ? ch : " "; @@ -131,8 +135,10 @@ function foldNotDistinctFrom(expr: string): string { if (close !== -1) { const inner = expr.slice(open + 1, close); const top = topLevelText(inner); - if (top.match(/\bis\s+distinct\s+from\b/gi)?.length === 1 && !/\b(?:and|or|not)\b/i.test(top)) { - out += `(${inner.replace(/\bis\s+distinct\s+from\b/i, "is not distinct from")})`; + const operators = [...top.matchAll(/\bis\s+distinct\s+from\b/gi)]; + if (operators.length === 1 && !/\b(?:and|or|not)\b/i.test(top)) { + const op = operators[0]!; + out += `(${inner.slice(0, op.index)}is not distinct from${inner.slice(op.index + op[0].length)})`; i = close + 1; continue; } diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts index 53f06337f5..6f8d7806e7 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts @@ -261,6 +261,12 @@ describe("Postgres analyzer rewrites that text normalisation cannot undo", () => expect(checkExprEquals(`x IS NOT DISTINCT FROM y`, `NOT (x IS DISTINCT FROM y)`)).toBe(true); // a negated group that is more than one comparison is not folded expect(checkExprEquals(`NOT (a IS DISTINCT FROM b OR c)`, `a IS NOT DISTINCT FROM b OR c`)).toBe(false); + // only the depth-0 operator is rewritten: a nested comparison that comes first is not the one negated + expect(checkExprEquals(`NOT ((a IS DISTINCT FROM b) IS DISTINCT FROM c)`, `(a IS NOT DISTINCT FROM b) IS DISTINCT FROM c`)).toBe(false); + expect(checkExprEquals(`NOT ((a IS DISTINCT FROM b) IS DISTINCT FROM c)`, `(a IS DISTINCT FROM b) IS NOT DISTINCT FROM c`)).toBe(true); + // the phrase inside a literal is data, never the operator + expect(checkExprEquals(`NOT ('x is distinct from y' IS DISTINCT FROM z)`, `'x is not distinct from y' IS DISTINCT FROM z`)).toBe(false); + expect(checkExprEquals(`NOT ('x is distinct from y' IS DISTINCT FROM z)`, `'x is distinct from y' IS NOT DISTINCT FROM z`)).toBe(true); // `IS NOT NULL` is not a left operand expect(normalizeCheckExpr(`NOT (x IS NOT NULL IS DISTINCT FROM y)`)).toBe(`not x is not null is distinct from y`); }); From 0061305876826df3600e82bf097703b94ce91df6 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 07:04:17 -0400 Subject: [PATCH 4/5] no-mistakes(review): Fold authored bare col::text on text columns --- .../migrate-ts/src/check-expr-compare.ts | 17 ++++++++++------- .../test/check-evolution/normalize.test.ts | 2 ++ 2 files changed, 12 insertions(+), 7 deletions(-) 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 1f9f0904a2..11c2b728e7 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -18,7 +18,7 @@ // - 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). -// Three more are folded back by foldPgAnalyzerRewrites and PAREN_COLUMN_TEXT_CAST: a +// 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. // @@ -54,13 +54,13 @@ const NO_COLUMNS: ReadonlySet = new Set(); /** * `(col)::text` — the form Postgres writes when it casts a `varchar` column to `text` to reach * a text operator or function (`length((code)::text)`, `(a)::text < (b)::text`, - * `((region)::text IS DISTINCT FROM 'eu'::text)`). Matched BEFORE the brackets are stripped, - * while `(col)` can still be told from a call `f(col)`: the opening bracket must not follow a - * word character. It is dropped only for a column the caller names as text-family + * `((region)::text IS DISTINCT FROM 'eu'::text)`) — and the bare `col::text` an author writes + * for the same cast. Matched BEFORE the brackets are stripped, while `(col)` can still be told + * from a call `f(col)`: the match must not follow a word character or a dot. It is dropped only for a column the caller names as text-family * (`text` / `varchar`), where the cast changes nothing; on any other column it changes the * operator (citext) or the value (`char(n)` trims its padding), so it stays and reads as drift. */ -const PAREN_COLUMN_TEXT_CAST = /(? string): string { @@ -184,7 +184,7 @@ function foldPgAnalyzerRewrites(s: string): string { * around commas, casts and SYMBOLIC operators (outside single-quoted literals). * * `textColumns` (lower-cased names of `text` / `varchar` columns) lets a `(col)::text` cast - * fold away; see PAREN_COLUMN_TEXT_CAST and isElidableCast. + * fold away; see COLUMN_TEXT_CAST and isElidableCast. */ export function normalizeCheckExpr(expr: string, textColumns: ReadonlySet = NO_COLUMNS): string { // Every textual rewrite below runs OUTSIDE single-quoted literals. A literal is DATA: its @@ -205,7 +205,10 @@ export function normalizeCheckExpr(expr: string, textColumns: ReadonlySet (textColumns.has(column) ? column : whole)) + .replace(COLUMN_TEXT_CAST, (whole, parenthesised: string | undefined, bare: string | undefined) => { + const column = parenthesised ?? bare!; + return textColumns.has(column) ? column : whole; + }) .replace(/[()[\]]/g, " ") // drop parens AND square brackets (ARRAY[…]) .replace(/\s+/g, " ") // PG stores `col IN (…)` as `col = ANY (ARRAY[…])`; after the bracket strip above diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts index 6f8d7806e7..7579f0e18a 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts @@ -272,6 +272,8 @@ describe("Postgres analyzer rewrites that text normalisation cannot undo", () => }); test("a varchar column's `(col)::text` cast is dropped wherever Postgres adds it", () => { const cols = new Set(["region", "name_a", "name_b", "short_code"]); + expect(checkExprEquals(`length("short_code"::text) >= 2`, `(length((short_code)::text) >= 2)`, cols)).toBe(true); + expect(checkExprEquals(`length(lower(short_code)::text) >= 2`, `(length((short_code)::text) >= 2)`, cols)).toBe(false); expect(checkExprEquals(`"region" IS NOT DISTINCT FROM 'eu'`, `NOT ((region)::text IS DISTINCT FROM 'eu'::text)`, cols)).toBe(true); expect(checkExprEquals(`("region" IS DISTINCT FROM 'us') OR (x IS NOT NULL)`, `(((region)::text IS DISTINCT FROM 'us'::text) OR (x IS NOT NULL))`, cols)).toBe(true); expect(checkExprEquals(`"name_a" < "name_b"`, `((name_a)::text < (name_b)::text)`, cols)).toBe(true); From 87eb3c2ebbc5dd06332392912a2f3607f8e3de78 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Wed, 28 Oct 2026 09:06:42 -0400 Subject: [PATCH 5/5] test(migrate-ts): pin that a stored length((col)::text) on char(n) is drift Covers the non-regex bpchar case on real Postgres and offline. --- .../test/check-evolution/diff-offline.test.ts | 11 ++++ .../integration/pg-regex-check-cast.test.ts | 51 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts b/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts index e4c4a2be00..934a942c97 100644 --- a/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts +++ b/server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts @@ -77,6 +77,17 @@ describe("diffTableChecks — regex check on a text-family column", () => { const r = await diff({ expected: withKey(undefined, [generated]), actual: withKey("bpchar", [uncast]), dialect: "postgres" }); expect(r.changes.filter((c) => c.kind.endsWith("-check"))).toEqual([]); }); + test("bpchar column: a stored length((col)::text) is drift against the uncast length()", async () => { + const expected = CHK("spaces_key_length_chk", `length("key") >= 2`); + const cast = CHK("spaces_key_length_chk", `((length((key)::text) >= 2))`); + const uncast = CHK("spaces_key_length_chk", `(length(key) >= 2)`); + const kinds = async (udt: string, actual: CheckDescriptor) => + (await diff({ expected: withKey(undefined, [expected]), actual: withKey(udt, [actual]), dialect: "postgres" })) + .changes.filter((c) => c.kind.endsWith("-check")).map((c) => c.kind).sort(); + expect(await kinds("bpchar", cast)).toEqual(["add-check", "drop-check"]); + expect(await kinds("bpchar", uncast)).toEqual([]); + expect(await kinds("varchar", cast)).toEqual([]); + }); test("citext column: cast vs uncast is drift", async () => { expect((await checkChanges("citext")).map((c) => c.kind).sort()).toEqual(["add-check", "drop-check"]); }); diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts index 8dfe3b4c8c..ee16013ea4 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts @@ -40,6 +40,25 @@ const meta = JSON.stringify({ }, }); +const lengthMeta = JSON.stringify({ + "metadata.root": { + package: "acme", + children: [ + { + "object.entity": { + name: "Space", + children: [ + { "source.rdb": { "@table": "spaces" } }, + { "field.long": { name: "id" } }, + { "field.string": { name: "key", "@column": "key", children: [{ "validator.length": { name: "len", "@min": 2 } }] } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + ], + }, +}); + describe("regex CHECK vs Postgres's stored rewrite (live PG)", () => { if (!PG_URL) { test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {}); @@ -105,6 +124,38 @@ describe("regex CHECK vs Postgres's stored rewrite (live PG)", () => { expect(r.kinds).toEqual(["add-check", "drop-check"]); }); + // The same holds off the regex operator: `length(bpchar)` is its own function, so a stored + // `length((key)::text)` is a different constraint from the uncast `length("key")`, while a + // varchar column's cast is Postgres's own rewrite. + const lengthChanges = async (colType: string, authoredCheck: string) => { + await sql.raw("DROP TABLE IF EXISTS spaces CASCADE").execute(kysely); + await sql + .raw(`CREATE TABLE spaces (id bigserial PRIMARY KEY, "key" ${colType} NOT NULL, + CONSTRAINT spaces_key_length_chk CHECK (${authoredCheck}))`) + .execute(kysely); + const actual = await introspectPostgres(kysely); + const stored = actual.tables.find((t) => t.name === "spaces")?.checks.find((c) => c.name === "spaces_key_length_chk"); + const loaded = await new MetaDataLoader().load([new InMemoryStringSource(lengthMeta)]); + expect(loaded.errors).toEqual([]); + const d = await diff({ expected: buildExpectedSchema(loaded.root, { dialect: "postgres" }), actual, dialect: "postgres" }); + return { stored: stored?.expression, kinds: d.changes.filter((c) => c.kind.endsWith("-check")).map((c) => c.kind).sort() }; + }; + test("char(n) column: Postgres stores length() uncast and it is clean", async () => { + const r = await lengthChanges("char(8)", `length("key") >= 2`); + expect(r.stored).not.toContain("::text"); + expect(r.kinds).toEqual([]); + }); + test("char(n) column: a stored length((col)::text) is drift against the uncast length()", async () => { + const r = await lengthChanges("char(8)", `length(("key")::text) >= 2`); + expect(r.stored).toContain("(key)::text"); + expect(r.kinds).toEqual(["add-check", "drop-check"]); + }); + test("varchar column: Postgres's length((col)::text) rewrite is no drift", async () => { + const r = await lengthChanges("varchar(8)", `length("key") >= 2`); + expect(r.stored).toContain("(key)::text"); + expect(r.kinds).toEqual([]); + }); + test("varchar column: a changed pattern still reads as drift", async () => { const r = await checkChanges("varchar(64)", `"key" ~ '^[a-z]+$'`); expect(r.kinds).toEqual(["add-check", "drop-check"]);