Skip to content

Commit 781d0bc

Browse files
authored
fix(migrate-ts): regex CHECK on a varchar/text column no longer reads as drift on Postgres (#463)
* fix(migrate-ts): regex CHECK on a varchar/text column no longer reads as drift on Postgres * no-mistakes(review): apply reviewer inline fixes * no-mistakes(review): Gate regex ::text cast elision on text-family columns * no-mistakes(document): No documentation changes needed
1 parent c0a91e6 commit 781d0bc

8 files changed

Lines changed: 189 additions & 8 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,16 @@ here.**
2222

2323
### Fixed
2424

25+
- **`meta verify --db` no longer reports false drift on a regex CHECK over a varchar/text
26+
column.** Postgres stores `"key" ~ '^[a-z]+$'` as `((key)::text ~ '^[a-z]+$'::text)`; the
27+
literal's `::text` was already folded away but the column's was not, so a database `migrate`
28+
had just built showed the same constraint as both added and dropped. The comparison now
29+
drops a `(col)::text` cast before a regex-match operator (`~`, `~*`, `!~`, `!~*`) only when
30+
the database column is `text`, `varchar` or `bpchar`; on any other column, `citext` included,
31+
the cast changes the operator and still reads as drift, as does a changed pattern or any
32+
other cast. Schema comparison is TypeScript-owned, so no other port
33+
carries this comparison.
34+
2535
- **The api reference now follows a routes generator's `filter` as well as its `expose`.**
2636
`routesFile({ filter })` and `routesFileHono({ filter })` mount nothing for an entity the filter
2737
rejects, but the API pages still listed all five verbs for it. The pages now document the

‎server/typescript/packages/migrate-ts/src/check-expr-compare.ts‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,17 @@ function unwrapBacktickIdentifiers(expr: string): string {
4141
.join("");
4242
}
4343

44+
const NO_COLUMNS: ReadonlySet<string> = new Set();
45+
4446
/**
4547
* Canonical form: drop casts/brackets/parens, fold `= ANY (ARRAY[…])` back to `IN`,
4648
* lower-case, collapse whitespace, canonicalize cast target types, and collapse spacing
4749
* around commas, casts and SYMBOLIC operators (outside single-quoted literals).
50+
*
51+
* `textColumns` (lower-cased names of text-family columns) lets a `(col)::text` cast before
52+
* a regex operator fold away; see isElidableCast.
4853
*/
49-
export function normalizeCheckExpr(expr: string): string {
54+
export function normalizeCheckExpr(expr: string, textColumns: ReadonlySet<string> = NO_COLUMNS): string {
5055
const stripped = unwrapBacktickIdentifiers(expr)
5156
.toLowerCase()
5257
// Drop `::text` / `::"MyType"` type casts PG adds to literals. The lookbehind
@@ -95,7 +100,7 @@ export function normalizeCheckExpr(expr: string): string {
95100
// meaningful, so two checks differing only there must stay distinct — else a real
96101
// pattern change reads as clean drift, which is this file's failure in the other
97102
// direction and the worse of the two.
98-
return collapseSeparatorSpacingOutsideQuotes(folded);
103+
return collapseSeparatorSpacingOutsideQuotes(folded, textColumns);
99104
}
100105

101106
/**
@@ -192,7 +197,17 @@ function readCastTarget(s: string, i: number): { type: string; consumed: number
192197
* cast on anything that is not a literal or a text-returning operator is a REAL cast and
193198
* survives, because erasing it would let a genuine change read as clean drift.
194199
*/
195-
function isElidableCast(type: string, lhs: string): boolean {
200+
function isElidableCast(type: string, lhs: string, rest: string, textColumns: ReadonlySet<string>): boolean {
201+
// `::text` on a COLUMN directly before a regex-match operator (`~`, `~*`, `!~`, `!~*`),
202+
// and only when the caller names that column as text-family (text, character varying,
203+
// character). On such a column PG adds the cast itself (`"key" ~ '…'` comes back
204+
// `((key)::text ~ '…'::text)`). On any other column, citext included, the cast changes
205+
// the operator (`col::text ~ 'A'` is case-sensitive, `col ~ 'A'` on citext is not), so it
206+
// survives. With no set supplied nothing is elided.
207+
if (type === "text" && /^\s*!?~\*?(?![-+*\/<>=~!@#%^&|?])/.test(rest)) {
208+
const column = /(?:^|[^\w.])([a-z_]\w*)$/.exec(lhs)?.[1];
209+
if (column !== undefined && textColumns.has(column)) return true;
210+
}
196211
// `->>` / `#>>` yield text; a following `::text` cannot change anything.
197212
if (type === "text" && /(?:->>|#>>)\s*(?:'(?:[^']|'')*'|[\w."]+)$/.test(lhs)) return true;
198213
// A numeric literal cast to a numeric type.
@@ -210,7 +225,7 @@ function isElidableCast(type: string, lhs: string): boolean {
210225
* distinct regex CHECKs would compare equal — a real change reading as clean drift, which
211226
* is this file's failure in the worse direction.
212227
*/
213-
function collapseSeparatorSpacingOutsideQuotes(s: string): string {
228+
function collapseSeparatorSpacingOutsideQuotes(s: string, textColumns: ReadonlySet<string>): string {
214229
let out = "";
215230
let i = 0;
216231
const n = s.length;
@@ -246,7 +261,7 @@ function collapseSeparatorSpacingOutsideQuotes(s: string): string {
246261
const target = readCastTarget(s, i);
247262
i += target.consumed;
248263
const lhs = out.replace(/\s+$/, "");
249-
token = isElidableCast(target.type, lhs) ? "" : `::${target.type}`;
264+
token = isElidableCast(target.type, lhs, s.slice(i), textColumns) ? "" : `::${target.type}`;
250265
out = lhs;
251266
// Whitespace AFTER the target type is load-bearing — `x::integer is not null` must
252267
// not become `x::integeris not null` — so the cast, unlike a comma or an operator,
@@ -272,9 +287,13 @@ function collapseSeparatorSpacingOutsideQuotes(s: string): string {
272287
}
273288

274289
/** True when two CHECK expressions are equivalent after normalization. */
275-
export function checkExprEquals(a: string | undefined, b: string | undefined): boolean {
290+
export function checkExprEquals(
291+
a: string | undefined,
292+
b: string | undefined,
293+
textColumns?: ReadonlySet<string>,
294+
): boolean {
276295
if (a === undefined || b === undefined) return false;
277-
return normalizeCheckExpr(a) === normalizeCheckExpr(b);
296+
return normalizeCheckExpr(a, textColumns) === normalizeCheckExpr(b, textColumns);
278297
}
279298

280299
/**

‎server/typescript/packages/migrate-ts/src/diff/index.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -809,6 +809,8 @@ function respelledSpread(expected: CheckDescriptor, actual: CheckDescriptor): {
809809
return checkExprRespelledNullSafe(expected.expression, actual.expression) ? { respelled: true } : {};
810810
}
811811

812+
const PG_TEXT_FAMILY_UDTS: ReadonlySet<string> = new Set(["text", "varchar", "bpchar"]);
813+
812814
function diffTableChecks(
813815
expected: TableDescriptor,
814816
actual: TableDescriptor,
@@ -829,12 +831,15 @@ function diffTableChecks(
829831
const exprFallback = dialect === "sqlite" || dialect === "d1";
830832
const actualByName = new Map(actual.checks.filter((c) => c.name !== "").map((c) => [c.name, c]));
831833
const consumed = new Set<CheckDescriptor>();
834+
const textColumns = new Set(
835+
actual.columns.filter((c) => c.udtName !== undefined && PG_TEXT_FAMILY_UDTS.has(c.udtName)).map((c) => c.name.toLowerCase()),
836+
);
832837

833838
for (const ec of expected.checks) {
834839
const ac = actualByName.get(ec.name);
835840
if (ac) {
836841
consumed.add(ac);
837-
if (!checkExprEquals(ec.expression, ac.expression)) {
842+
if (!checkExprEquals(ec.expression, ac.expression, textColumns)) {
838843
const respelled = respelledSpread(ec, ac);
839844
changes.push({ kind: "drop-check", table: expected.name, ...sx, check: ec.name, restore: ac, ...respelled, status: ALLOWED });
840845
changes.push({ kind: "add-check", table: expected.name, ...sx, check: ec, ...respelled, status: ALLOWED });

‎server/typescript/packages/migrate-ts/src/introspect/postgres.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,7 @@ async function readColumns(k: Kysely<any>, schema: string, tableName: string): P
475475
name: r.column_name,
476476
sqlType,
477477
nullable: r.is_nullable === "YES",
478+
udtName: r.udt_name.toLowerCase(),
478479
};
479480

480481
const def = parsePgDefault(r.column_default);

‎server/typescript/packages/migrate-ts/src/types.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,12 @@ export interface ColumnDescriptor {
4646
name: string;
4747
sqlType: SqlType;
4848
nullable: boolean;
49+
/**
50+
* Postgres `udt_name` as introspected (`text`, `varchar`, `bpchar`, `citext`, …). Set by
51+
* the Postgres introspector only; absent on the expected side and on SQLite. `sqlType`
52+
* folds every unrecognised type into `text`, so this is the only place citext survives.
53+
*/
54+
udtName?: string;
4955
default?: ColumnDefault;
5056
identity?: "increment" | "uuid";
5157
/**

‎server/typescript/packages/migrate-ts/test/check-evolution/diff-offline.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,3 +50,23 @@ describe("diffTableChecks — postgres existing-table evolution", () => {
5050
expect(r.changes.some((c) => c.kind.endsWith("-check"))).toBe(false);
5151
});
5252
});
53+
54+
describe("diffTableChecks — regex check on a text-family column", () => {
55+
const generated = CHK("spaces_key_regex_chk", `"key" ~ '^[a-z0-9][a-z0-9-]*$'`);
56+
const stored = CHK("spaces_key_regex_chk", `(((key)::text ~ '^[a-z0-9][a-z0-9-]*$'::text))`);
57+
const withKey = (udtName: string | undefined, checks: CheckDescriptor[]): SchemaSnapshot => ({
58+
tables: [{ name: "spaces", columns: [{ name: "key", sqlType: { kind: "text" }, nullable: false, ...(udtName ? { udtName } : {}) }],
59+
indexes: [], foreignKeys: [], checks, primaryKey: [] }],
60+
views: [],
61+
});
62+
const checkChanges = async (udtName: string | undefined) => {
63+
const r = await diff({ expected: withKey(undefined, [generated]), actual: withKey(udtName, [stored]), dialect: "postgres" });
64+
return r.changes.filter((c) => c.kind.endsWith("-check"));
65+
};
66+
test.each(["text", "varchar", "bpchar"])("%s column: Postgres's rewrite is no drift", async (udt) => {
67+
expect(await checkChanges(udt)).toEqual([]);
68+
});
69+
test("citext column: cast vs uncast is drift", async () => {
70+
expect((await checkChanges("citext")).map((c) => c.kind).sort()).toEqual(["add-check", "drop-check"]);
71+
});
72+
});

‎server/typescript/packages/migrate-ts/test/check-evolution/normalize.test.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,3 +204,25 @@ describe("stripCheckWrapper", () => {
204204
expect(stripCheckWrapper("qty >= 1")).toBe("qty >= 1");
205205
});
206206
});
207+
208+
describe("regex check on a varchar/text column", () => {
209+
const generated = `"key" ~ '^[a-z0-9][a-z0-9-]*$'`;
210+
const introspected = `(((key)::text ~ '^[a-z0-9][a-z0-9-]*$'::text))`;
211+
const textColumns = new Set(["key"]);
212+
test("Postgres's added column cast does not read as drift on a text-family column", () => {
213+
expect(checkExprEquals(generated, introspected, textColumns)).toBe(true);
214+
expect(checkExprEquals(generated, `(((key)::text !~* '^[a-z0-9][a-z0-9-]*$'::text))`, textColumns)).toBe(false);
215+
});
216+
test("without a text-family column set the cast is kept", () => {
217+
expect(checkExprEquals(generated, introspected)).toBe(false);
218+
});
219+
test("a cast on a column outside the set (citext) reads as drift", () => {
220+
expect(checkExprEquals(generated, introspected, new Set(["other"]))).toBe(false);
221+
});
222+
test("a changed regex pattern still reads as drift", () => {
223+
expect(checkExprEquals(generated, `(((key)::text ~ '^[a-z][a-z0-9-]*$'::text))`, textColumns)).toBe(false);
224+
});
225+
test("a cast that is not before a regex operator survives", () => {
226+
expect(checkExprEquals("qty > 0", "((qty)::text > '0'::text)", new Set(["qty"]))).toBe(false);
227+
});
228+
});
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/**
2+
* A regex CHECK on a text-family column must not read as drift on Postgres.
3+
*
4+
* Postgres stores `"key" ~ '^[a-z0-9][a-z0-9-]*$'` as `(((key)::text ~ '…'::text))`. The
5+
* oracle is the live engine: create the table from the DDL migrate emits for the authored
6+
* check, read the constraint back through the real introspector, and diff the authored
7+
* metadata against it. On a citext column the cast changes the operator (case-sensitive vs
8+
* citext's case-insensitive `~`), so the same pair must still read as drift.
9+
*
10+
* Skips gracefully when MIGRATE_TS_PG_URL is not set.
11+
*/
12+
13+
import { test, expect, beforeAll, afterAll, describe } from "bun:test";
14+
import { Pool } from "pg";
15+
import { Kysely, PostgresDialect, sql } from "kysely";
16+
import { MetaDataLoader, InMemoryStringSource } from "@metaobjectsdev/metadata";
17+
import { buildExpectedSchema } from "../../src/expected-schema.js";
18+
import { introspectPostgres } from "../../src/introspect/postgres.js";
19+
import { diff } from "../../src/diff/index.js";
20+
21+
const PG_URL = process.env["MIGRATE_TS_PG_URL"];
22+
const PATTERN = "^[a-z0-9][a-z0-9-]*$";
23+
24+
const meta = JSON.stringify({
25+
"metadata.root": {
26+
package: "acme",
27+
children: [
28+
{
29+
"object.entity": {
30+
name: "Space",
31+
children: [
32+
{ "source.rdb": { "@table": "spaces" } },
33+
{ "field.long": { name: "id" } },
34+
{ "field.string": { name: "key", "@column": "key", children: [{ "validator.regex": { name: "re", "@pattern": PATTERN } }] } },
35+
{ "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } },
36+
],
37+
},
38+
},
39+
],
40+
},
41+
});
42+
43+
describe("regex CHECK vs Postgres's stored rewrite (live PG)", () => {
44+
if (!PG_URL) {
45+
test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {});
46+
return;
47+
}
48+
let pool: Pool;
49+
let kysely: Kysely<Record<string, unknown>>;
50+
51+
beforeAll(async () => {
52+
pool = new Pool({ connectionString: PG_URL });
53+
kysely = new Kysely<Record<string, unknown>>({ dialect: new PostgresDialect({ pool }) });
54+
await sql.raw("CREATE EXTENSION IF NOT EXISTS citext").execute(kysely);
55+
});
56+
afterAll(async () => {
57+
await sql.raw("DROP TABLE IF EXISTS spaces CASCADE").execute(kysely);
58+
await pool.end();
59+
});
60+
61+
const checkChanges = async (colType: string, authoredCheck = `"key" ~ '${PATTERN}'`) => {
62+
await sql.raw("DROP TABLE IF EXISTS spaces CASCADE").execute(kysely);
63+
// The constraint is created from the AUTHORED spelling; PG stores its own rendering.
64+
await sql
65+
.raw(`CREATE TABLE spaces (id bigserial PRIMARY KEY, "key" ${colType} NOT NULL,
66+
CONSTRAINT spaces_key_regex_chk CHECK (${authoredCheck}))`)
67+
.execute(kysely);
68+
const actual = await introspectPostgres(kysely);
69+
const stored = actual.tables.find((t) => t.name === "spaces")?.checks.find((c) => c.name === "spaces_key_regex_chk");
70+
const loaded = await new MetaDataLoader().load([new InMemoryStringSource(meta)]);
71+
expect(loaded.errors).toEqual([]);
72+
const d = await diff({ expected: buildExpectedSchema(loaded.root, { dialect: "postgres" }), actual, dialect: "postgres" });
73+
return { stored: stored?.expression, kinds: d.changes.filter((c) => c.kind.endsWith("-check")).map((c) => c.kind).sort() };
74+
};
75+
76+
test.each(["varchar(64)", "text", "char(8)"])("%s column: PG's rewrite is no drift", async (colType) => {
77+
const r = await checkChanges(colType);
78+
// The gate is not vacuous: PG really did rewrite the authored text.
79+
expect(r.stored).toContain("::text");
80+
expect(r.kinds).toEqual([]);
81+
});
82+
83+
test("citext column, uncast check: PG keeps citext's own operator and it is clean", async () => {
84+
const r = await checkChanges("citext");
85+
expect(r.stored).toContain("::citext");
86+
expect(r.kinds).toEqual([]);
87+
});
88+
89+
test("citext column: a check stored with an explicit ::text cast is a different (case-sensitive) check, so drift", async () => {
90+
const r = await checkChanges("citext", `("key")::text ~ '${PATTERN}'`);
91+
expect(r.kinds).toEqual(["add-check", "drop-check"]);
92+
});
93+
94+
test("varchar column: a changed pattern still reads as drift", async () => {
95+
const r = await checkChanges("varchar(64)", `"key" ~ '^[a-z]+$'`);
96+
expect(r.kinds).toEqual(["add-check", "drop-check"]);
97+
});
98+
});

0 commit comments

Comments
 (0)