diff --git a/CHANGELOG.md b/CHANGELOG.md index 5142264b1..371c3b641 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,16 @@ here.** ### Fixed +- **`meta verify --db` no longer reports false drift on a regex CHECK over a varchar/text + column.** Postgres stores `"key" ~ '^[a-z]+$'` as `((key)::text ~ '^[a-z]+$'::text)`; the + literal's `::text` was already folded away but the column's was not, so a database `migrate` + had just built showed the same constraint as both added and dropped. The comparison now + drops a `(col)::text` cast before a regex-match operator (`~`, `~*`, `!~`, `!~*`) only when + the database column is `text`, `varchar` or `bpchar`; on any other column, `citext` included, + the cast changes the operator and still reads as drift, as does a changed pattern or any + other cast. Schema comparison is TypeScript-owned, so no other port + carries this comparison. + - **The api reference now follows a routes generator's `filter` as well as its `expose`.** `routesFile({ filter })` and `routesFileHono({ filter })` mount nothing for an entity the filter rejects, but the API pages still listed all five verbs for it. The pages now document the 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 33c3b1e8a..bf5c4214e 100644 --- a/server/typescript/packages/migrate-ts/src/check-expr-compare.ts +++ b/server/typescript/packages/migrate-ts/src/check-expr-compare.ts @@ -41,12 +41,17 @@ function unwrapBacktickIdentifiers(expr: string): string { .join(""); } +const NO_COLUMNS: ReadonlySet = new Set(); + /** * Canonical form: drop casts/brackets/parens, fold `= ANY (ARRAY[…])` back to `IN`, * lower-case, collapse whitespace, canonicalize cast target types, and collapse spacing * around commas, casts and SYMBOLIC operators (outside single-quoted literals). + * + * `textColumns` (lower-cased names of text-family columns) lets a `(col)::text` cast before + * a regex operator fold away; see isElidableCast. */ -export function normalizeCheckExpr(expr: string): string { +export function normalizeCheckExpr(expr: string, textColumns: ReadonlySet = NO_COLUMNS): string { const stripped = unwrapBacktickIdentifiers(expr) .toLowerCase() // Drop `::text` / `::"MyType"` type casts PG adds to literals. The lookbehind @@ -95,7 +100,7 @@ export function normalizeCheckExpr(expr: string): string { // meaningful, so two checks differing only there must stay distinct — else a real // pattern change reads as clean drift, which is this file's failure in the other // direction and the worse of the two. - return collapseSeparatorSpacingOutsideQuotes(folded); + return collapseSeparatorSpacingOutsideQuotes(folded, textColumns); } /** @@ -192,7 +197,17 @@ function readCastTarget(s: string, i: number): { type: string; consumed: number * cast on anything that is not a literal or a text-returning operator is a REAL cast and * survives, because erasing it would let a genuine change read as clean drift. */ -function isElidableCast(type: string, lhs: string): boolean { +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 + // `((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. + if (type === "text" && /^\s*!?~\*?(?![-+*\/<>=~!@#%^&|?])/.test(rest)) { + const column = /(?:^|[^\w.])([a-z_]\w*)$/.exec(lhs)?.[1]; + if (column !== undefined && textColumns.has(column)) return true; + } // `->>` / `#>>` yield text; a following `::text` cannot change anything. if (type === "text" && /(?:->>|#>>)\s*(?:'(?:[^']|'')*'|[\w."]+)$/.test(lhs)) return true; // A numeric literal cast to a numeric type. @@ -210,7 +225,7 @@ function isElidableCast(type: string, lhs: string): boolean { * distinct regex CHECKs would compare equal — a real change reading as clean drift, which * is this file's failure in the worse direction. */ -function collapseSeparatorSpacingOutsideQuotes(s: string): string { +function collapseSeparatorSpacingOutsideQuotes(s: string, textColumns: ReadonlySet): string { let out = ""; let i = 0; const n = s.length; @@ -246,7 +261,7 @@ function collapseSeparatorSpacingOutsideQuotes(s: string): string { const target = readCastTarget(s, i); i += target.consumed; const lhs = out.replace(/\s+$/, ""); - token = isElidableCast(target.type, lhs) ? "" : `::${target.type}`; + token = isElidableCast(target.type, lhs, s.slice(i), textColumns) ? "" : `::${target.type}`; out = lhs; // Whitespace AFTER the target type is load-bearing — `x::integer is not null` must // not become `x::integeris not null` — so the cast, unlike a comma or an operator, @@ -272,9 +287,13 @@ function collapseSeparatorSpacingOutsideQuotes(s: string): string { } /** True when two CHECK expressions are equivalent after normalization. */ -export function checkExprEquals(a: string | undefined, b: string | undefined): boolean { +export function checkExprEquals( + a: string | undefined, + b: string | undefined, + textColumns?: ReadonlySet, +): boolean { if (a === undefined || b === undefined) return false; - return normalizeCheckExpr(a) === normalizeCheckExpr(b); + return normalizeCheckExpr(a, textColumns) === normalizeCheckExpr(b, textColumns); } /** diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index db8bae9a4..3f6194a76 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -809,6 +809,8 @@ 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"]); + function diffTableChecks( expected: TableDescriptor, actual: TableDescriptor, @@ -829,12 +831,15 @@ 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()), + ); for (const ec of expected.checks) { const ac = actualByName.get(ec.name); if (ac) { consumed.add(ac); - if (!checkExprEquals(ec.expression, ac.expression)) { + if (!checkExprEquals(ec.expression, ac.expression, 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 }); diff --git a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts index 1f0d84358..97dafaf8f 100644 --- a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts @@ -475,6 +475,7 @@ async function readColumns(k: Kysely, schema: string, tableName: string): P name: r.column_name, sqlType, nullable: r.is_nullable === "YES", + udtName: r.udt_name.toLowerCase(), }; const def = parsePgDefault(r.column_default); diff --git a/server/typescript/packages/migrate-ts/src/types.ts b/server/typescript/packages/migrate-ts/src/types.ts index 4a808836e..8caf6e675 100644 --- a/server/typescript/packages/migrate-ts/src/types.ts +++ b/server/typescript/packages/migrate-ts/src/types.ts @@ -46,6 +46,12 @@ export interface ColumnDescriptor { name: string; sqlType: SqlType; nullable: boolean; + /** + * Postgres `udt_name` as introspected (`text`, `varchar`, `bpchar`, `citext`, …). Set by + * the Postgres introspector only; absent on the expected side and on SQLite. `sqlType` + * folds every unrecognised type into `text`, so this is the only place citext survives. + */ + udtName?: string; default?: ColumnDefault; identity?: "increment" | "uuid"; /** 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 2a50e074e..20c06021f 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 @@ -50,3 +50,23 @@ describe("diffTableChecks — postgres existing-table evolution", () => { expect(r.changes.some((c) => c.kind.endsWith("-check"))).toBe(false); }); }); + +describe("diffTableChecks — regex check on a text-family column", () => { + const generated = CHK("spaces_key_regex_chk", `"key" ~ '^[a-z0-9][a-z0-9-]*$'`); + const stored = CHK("spaces_key_regex_chk", `(((key)::text ~ '^[a-z0-9][a-z0-9-]*$'::text))`); + const withKey = (udtName: string | undefined, checks: CheckDescriptor[]): SchemaSnapshot => ({ + tables: [{ name: "spaces", columns: [{ name: "key", sqlType: { kind: "text" }, nullable: false, ...(udtName ? { udtName } : {}) }], + indexes: [], foreignKeys: [], checks, primaryKey: [] }], + views: [], + }); + const checkChanges = async (udtName: string | undefined) => { + 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) => { + expect(await checkChanges(udt)).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 24723d9d5..2829a176e 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 @@ -204,3 +204,25 @@ describe("stripCheckWrapper", () => { expect(stripCheckWrapper("qty >= 1")).toBe("qty >= 1"); }); }); + +describe("regex check on a varchar/text column", () => { + const generated = `"key" ~ '^[a-z0-9][a-z0-9-]*$'`; + const introspected = `(((key)::text ~ '^[a-z0-9][a-z0-9-]*$'::text))`; + const textColumns = new Set(["key"]); + test("Postgres's added column cast does not read as drift on a text-family column", () => { + expect(checkExprEquals(generated, introspected, textColumns)).toBe(true); + expect(checkExprEquals(generated, `(((key)::text !~* '^[a-z0-9][a-z0-9-]*$'::text))`, textColumns)).toBe(false); + }); + test("without a text-family column set the cast is kept", () => { + expect(checkExprEquals(generated, introspected)).toBe(false); + }); + test("a cast on a column outside the set (citext) reads as drift", () => { + expect(checkExprEquals(generated, introspected, new Set(["other"]))).toBe(false); + }); + test("a changed regex pattern still reads as drift", () => { + expect(checkExprEquals(generated, `(((key)::text ~ '^[a-z][a-z0-9-]*$'::text))`, textColumns)).toBe(false); + }); + test("a cast that is not before a regex operator survives", () => { + expect(checkExprEquals("qty > 0", "((qty)::text > '0'::text)", new Set(["qty"]))).toBe(false); + }); +}); 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 new file mode 100644 index 000000000..128707730 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-regex-check-cast.test.ts @@ -0,0 +1,98 @@ +/** + * A regex CHECK on a text-family column must not read as drift on Postgres. + * + * Postgres stores `"key" ~ '^[a-z0-9][a-z0-9-]*$'` as `(((key)::text ~ '…'::text))`. The + * oracle is the live engine: create the table from the DDL migrate emits for the authored + * check, read the constraint back through the real introspector, and diff the authored + * metadata against it. On a citext column the cast changes the operator (case-sensitive vs + * citext's case-insensitive `~`), so the same pair must still read as drift. + * + * 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 { diff } from "../../src/diff/index.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const PATTERN = "^[a-z0-9][a-z0-9-]*$"; + +const meta = 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.regex": { name: "re", "@pattern": PATTERN } }] } }, + { "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", () => {}); + 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("CREATE EXTENSION IF NOT EXISTS citext").execute(kysely); + }); + afterAll(async () => { + await sql.raw("DROP TABLE IF EXISTS spaces CASCADE").execute(kysely); + await pool.end(); + }); + + const checkChanges = async (colType: string, authoredCheck = `"key" ~ '${PATTERN}'`) => { + await sql.raw("DROP TABLE IF EXISTS spaces CASCADE").execute(kysely); + // The constraint is created from the AUTHORED spelling; PG stores its own rendering. + await sql + .raw(`CREATE TABLE spaces (id bigserial PRIMARY KEY, "key" ${colType} NOT NULL, + CONSTRAINT spaces_key_regex_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_regex_chk"); + const loaded = await new MetaDataLoader().load([new InMemoryStringSource(meta)]); + 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.each(["varchar(64)", "text", "char(8)"])("%s column: PG's rewrite is no drift", async (colType) => { + const r = await checkChanges(colType); + // The gate is not vacuous: PG really did rewrite the authored text. + expect(r.stored).toContain("::text"); + expect(r.kinds).toEqual([]); + }); + + test("citext column, uncast check: PG keeps citext's own operator and it is clean", async () => { + const r = await checkChanges("citext"); + expect(r.stored).toContain("::citext"); + expect(r.kinds).toEqual([]); + }); + + test("citext column: a check stored with an explicit ::text cast is a different (case-sensitive) check, so drift", async () => { + const r = await checkChanges("citext", `("key")::text ~ '${PATTERN}'`); + 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"]); + }); +});