Skip to content

Commit ce24a71

Browse files
committed
no-mistakes(review): Gate regex ::text cast elision on text-family columns
1 parent ea18f15 commit ce24a71

7 files changed

Lines changed: 75 additions & 25 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ here.**
2626
column.** Postgres stores `"key" ~ '^[a-z]+$'` as `((key)::text ~ '^[a-z]+$'::text)`; the
2727
literal's `::text` was already folded away but the column's was not, so a database `migrate`
2828
had just built showed the same constraint as both added and dropped. The comparison now
29-
drops a `::text` cast only when it sits directly before a regex-match operator (`~`, `~*`,
30-
`!~`, `!~*`), whatever the column type; a changed pattern or any other cast still reads as
31-
drift. Known gap: on a `citext` column the cast and uncast spellings differ in meaning
32-
(case-sensitive vs case-insensitive) but compare equal. Schema comparison is TypeScript-owned, so no other port
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
3333
carries this comparison.
3434

3535
- **The api reference now follows a routes generator's `filter` as well as its `expose`.**

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

Lines changed: 26 additions & 15 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,15 +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, rest: string): boolean {
196-
// `::text` directly before a regex-match operator (`~`, `~*`, `!~`, `!~*`). Those
197-
// operators are defined on text, so on a varchar/text column PG adds the cast itself
198-
// (`"key" ~ '…'` comes back `((key)::text ~ '…'::text)`). The comparator sees only the
199-
// expression string, never the column type, so this elides the cast for every column.
200-
// Known gap: on a `citext` column `col::text ~ 'A'` (case-sensitive) and `col ~ 'A'`
201-
// (citext's own case-insensitive operator) both normalize alike, so switching between
202-
// the two spellings reads as clean.
203-
if (type === "text" && /^\s*!?~\*?(?![-+*\/<>=~!@#%^&|?])/.test(rest)) return true;
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+
}
204211
// `->>` / `#>>` yield text; a following `::text` cannot change anything.
205212
if (type === "text" && /(?:->>|#>>)\s*(?:'(?:[^']|'')*'|[\w."]+)$/.test(lhs)) return true;
206213
// A numeric literal cast to a numeric type.
@@ -218,7 +225,7 @@ function isElidableCast(type: string, lhs: string, rest: string): boolean {
218225
* distinct regex CHECKs would compare equal — a real change reading as clean drift, which
219226
* is this file's failure in the worse direction.
220227
*/
221-
function collapseSeparatorSpacingOutsideQuotes(s: string): string {
228+
function collapseSeparatorSpacingOutsideQuotes(s: string, textColumns: ReadonlySet<string>): string {
222229
let out = "";
223230
let i = 0;
224231
const n = s.length;
@@ -254,7 +261,7 @@ function collapseSeparatorSpacingOutsideQuotes(s: string): string {
254261
const target = readCastTarget(s, i);
255262
i += target.consumed;
256263
const lhs = out.replace(/\s+$/, "");
257-
token = isElidableCast(target.type, lhs, s.slice(i)) ? "" : `::${target.type}`;
264+
token = isElidableCast(target.type, lhs, s.slice(i), textColumns) ? "" : `::${target.type}`;
258265
out = lhs;
259266
// Whitespace AFTER the target type is load-bearing — `x::integer is not null` must
260267
// not become `x::integeris not null` — so the cast, unlike a comma or an operator,
@@ -280,9 +287,13 @@ function collapseSeparatorSpacingOutsideQuotes(s: string): string {
280287
}
281288

282289
/** True when two CHECK expressions are equivalent after normalization. */
283-
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 {
284295
if (a === undefined || b === undefined) return false;
285-
return normalizeCheckExpr(a) === normalizeCheckExpr(b);
296+
return normalizeCheckExpr(a, textColumns) === normalizeCheckExpr(b, textColumns);
286297
}
287298

288299
/**

‎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: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -208,14 +208,21 @@ describe("stripCheckWrapper", () => {
208208
describe("regex check on a varchar/text column", () => {
209209
const generated = `"key" ~ '^[a-z0-9][a-z0-9-]*$'`;
210210
const introspected = `(((key)::text ~ '^[a-z0-9][a-z0-9-]*$'::text))`;
211-
test("Postgres's added column cast does not read as drift", () => {
212-
expect(checkExprEquals(generated, introspected)).toBe(true);
213-
expect(checkExprEquals(generated, `(((key)::text !~* '^[a-z0-9][a-z0-9-]*$'::text))`)).toBe(false);
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);
214221
});
215222
test("a changed regex pattern still reads as drift", () => {
216-
expect(checkExprEquals(generated, `(((key)::text ~ '^[a-z][a-z0-9-]*$'::text))`)).toBe(false);
223+
expect(checkExprEquals(generated, `(((key)::text ~ '^[a-z][a-z0-9-]*$'::text))`, textColumns)).toBe(false);
217224
});
218225
test("a cast that is not before a regex operator survives", () => {
219-
expect(checkExprEquals("qty > 0", "((qty)::text > '0'::text)")).toBe(false);
226+
expect(checkExprEquals("qty > 0", "((qty)::text > '0'::text)", new Set(["qty"]))).toBe(false);
220227
});
221228
});

0 commit comments

Comments
 (0)