Skip to content

Commit c346048

Browse files
dmealingclaude
andcommitted
fix(migrate-ts): the one @expr spelling a human writes was the one that drifted
`pg_get_expr` re-emits every operator SPACED and appends casts, so a live index authored as `(request_context->>'device_id')` reads back as `((request_context ->> 'device_id'::text))`. The comparator already canonicalized the casts and the redundant parens — measured across five spellings against a live engine, three reconciled and two did not — so the ONE surviving difference was the whitespace around the operator. And it was the one difference a person actually produces. So `verify --db` reported drift and `migrate` proposed DROP + CREATE against an index the database already held, declared exactly as it stood. Found by an adopter estate running the 1.0 RC: its own README said to retire a hand-written index adjunct "the moment #342 ships", and following that instruction produced a proposed DROP against a live production index. The fix collapses whitespace around SYMBOLIC operator runs, outside single-quoted literals, in the pass that already did exactly that for commas. Two properties make it safe rather than merely convenient: - Whitespace around a symbolic operator is not semantic in ANY valid SQL. Where two spellings differ only there, at most one parses: Postgres lexes a maximal run of operator characters as a single token, so `a =- 1` is not `a = -1` written differently, it is a syntax error. - Only SYMBOLIC operators. Collapsing around an alphabetic one turns `a AND b` into `aANDb`, so `AND`/`OR`/`IN`/`IS NULL` keep their separators — asserted. Derived from Postgres' operator CHARACTER SET rather than a list of operators, so `->>`, `!~*`, `@>`, `?|` and anything user-defined normalize by one rule with nothing to maintain. Multi-character operators survive as one token. The file's header claimed this canonicalizer was safe because "every check expression we emit is machine-derived … there is no arbitrary author SQL to mis-normalize". That stopped being true when `index.lookup`'s `@expr` and `@where` started using it — those ARE author-written SQL — so the header now justifies each rewrite on the stronger ground it needs. Three existing assertions pinned the canonical STRING and were updated after checking, individually, that the property each test exists to prove still holds: the `::` inside a regex literal is still preserved, the `any array` literal still does not fold to `IN`, parens/case/whitespace still normalize. Not a blanket flip. Gated against a REAL engine, per this package's doctrine. A unit test proves the strings converge; it cannot prove the string PG hands back is the one being converged on, which is the half that was wrong. The new integration test creates the index in Postgres from the authored spelling, ASSERTS that PG re-emits it spaced and cast (so a future PG that renders differently fails there rather than making the suite vacuous), diffs all five spellings to empty, and proves the gate can still convict a genuinely different key. Reverting the fix fails exactly the two cases it exists for; migrate-ts is 827 pass / 0 fail with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NTcEKXTQMYt84fAjuw5A2M
1 parent 9a9049e commit c346048

3 files changed

Lines changed: 279 additions & 18 deletions

File tree

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

Lines changed: 61 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,26 @@
22
//
33
// CHECK-expression comparison. Postgres rewrites a stored CHECK body so the raw
44
// text we generate and the introspected text differ textually but mean the same
5-
// thing. This reduces both to ONE canonical form for comparison. Reliable here
6-
// because every check expression we emit is machine-derived with a simple, known
7-
// shape (comparison / IN / length / regex) — there is no arbitrary author SQL to
8-
// mis-normalize. The rewrites PG applies (verified against a live server):
5+
// thing. This reduces both to ONE canonical form for comparison.
6+
//
7+
// It was written when every expression reaching it was machine-derived with a simple,
8+
// known shape (comparison / IN / length / regex), and the header said there was "no
9+
// arbitrary author SQL to mis-normalize". That stopped being true when `index.lookup`'s
10+
// `@expr` and `@where` began using the same canonicalizer: those are AUTHOR-written SQL.
11+
// Each rewrite below is therefore justified on the stronger ground it now needs — that
12+
// the difference it erases is not semantic in ANY valid SQL, not merely in the shapes we
13+
// emit. The rewrites PG applies (verified against a live server):
914
// - parenthesizes terms: `col >= 0 AND col <= 100` → `(col >= 0) AND (col <= 100)`
1015
// - rewrites IN-lists: `status IN ('A','B')` → `status = ANY (ARRAY['A'::text, 'B'::text])`
1116
// - appends type casts: string literals gain `::text`
1217
// All three are canonicalized below so an enum/range CHECK introspected from PG
1318
// compares equal to the one we generate (idempotency on the --from-db / verify paths).
1419

15-
/** Canonical form: drop casts/brackets/parens, fold `= ANY (ARRAY[…])` back to `IN`, lower-case, collapse whitespace. */
20+
/**
21+
* Canonical form: drop casts/brackets/parens, fold `= ANY (ARRAY[…])` back to `IN`,
22+
* lower-case, collapse whitespace, and collapse spacing around commas and SYMBOLIC
23+
* operators (outside single-quoted literals).
24+
*/
1625
export function normalizeCheckExpr(expr: string): string {
1726
const stripped = expr
1827
.toLowerCase()
@@ -37,17 +46,48 @@ export function normalizeCheckExpr(expr: string): string {
3746
// PG stores `col IN (…)` as `col = ANY (ARRAY[…])`; after the bracket strip above
3847
// that reads `col = any array …`. Fold it back to the `col in …` form we emit.
3948
const folded = stripped.replace(/=\s*any\s+array/g, "in").replace(/\s+/g, " ").trim();
40-
// Normalize spacing AROUND COMMAS so a generated IN-list (`'a', 'b'`) compares equal
41-
// to a hand-written one (`'a','b'`) — the separator commas are never semantically
42-
// meaningful, and a hand SQLite migration omits the space. This is done OUTSIDE
43-
// single-quoted literals only: a comma inside a string/regex literal (`'a, b'`, or a
44-
// quantifier `'{1, 3}'`) IS meaningful, so two checks that differ only there must
45-
// stay distinct (else a real regex/predicate change reads as clean drift).
46-
return collapseCommaSpacingOutsideQuotes(folded);
49+
// Normalize spacing around COMMAS and SYMBOLIC OPERATORS, so the spelling a human
50+
// writes compares equal to the one Postgres hands back.
51+
//
52+
// Commas: a generated IN-list (`'a', 'b'`) must equal a hand-written one (`'a','b'`) —
53+
// a hand SQLite migration omits the space.
54+
//
55+
// Operators: `pg_get_expr` re-emits every operator SPACED, and a person authoring an
56+
// `index.lookup @expr` writes it tight. So `(request_context->>'device_id')` — the
57+
// natural spelling — normalized to `request_context->>'device_id'` while the live index
58+
// introspected to `request_context ->> 'device_id'`, and `verify --db` proposed DROP +
59+
// CREATE against an index the database already held, exactly as declared. Casts and
60+
// redundant parens were ALREADY canonicalized above, so the one difference left was the
61+
// spacing — and it was the one difference a human actually produces.
62+
//
63+
// Safe because whitespace around a symbolic operator is not semantic in ANY valid SQL:
64+
// where two spellings differ only there, at most one of them parses. Postgres lexes a
65+
// maximal run of operator characters as a SINGLE token, so `a =- 1` is not `a = -1`
66+
// spelled differently — it is a syntax error. Only SYMBOLIC operators are touched;
67+
// collapsing around an alphabetic one would turn `a AND b` into `aANDb`.
68+
//
69+
// Both are done OUTSIDE single-quoted literals: a comma or an operator character inside
70+
// a string/regex literal (`'a, b'`, a quantifier `'{1, 3}'`, a pattern `'a ->> b'`) IS
71+
// meaningful, so two checks differing only there must stay distinct — else a real
72+
// pattern change reads as clean drift, which is this file's failure in the other
73+
// direction and the worse of the two.
74+
return collapseSeparatorSpacingOutsideQuotes(folded);
4775
}
4876

49-
/** Collapse `\s*,\s*` → `,` OUTSIDE single-quoted literals (see normalizeCheckExpr). */
50-
function collapseCommaSpacingOutsideQuotes(s: string): string {
77+
/**
78+
* Postgres' operator characters. A maximal run of these is ONE operator token — which is
79+
* why `->>` and `!~*` survive intact rather than being split apart. Derived as the
80+
* character SET rather than a list of known operators, so a spelling this file has never
81+
* seen (`@>`, `?|`, a user-defined operator) normalizes by the same rule instead of
82+
* needing an edit here.
83+
*/
84+
const OPERATOR_CHARS = new Set([..."+-*/<>=~!@#%^&|?"]);
85+
86+
/**
87+
* Collapse whitespace around commas and symbolic-operator runs, OUTSIDE single-quoted
88+
* literals (see normalizeCheckExpr).
89+
*/
90+
function collapseSeparatorSpacingOutsideQuotes(s: string): string {
5191
let out = "";
5292
let i = 0;
5393
const n = s.length;
@@ -75,6 +115,13 @@ function collapseCommaSpacingOutsideQuotes(s: string): string {
75115
while (i < n && /\s/.test(s[i]!)) i++; // skip whitespace after it
76116
continue;
77117
}
118+
if (OPERATOR_CHARS.has(ch)) {
119+
out = out.replace(/\s+$/, ""); // drop whitespace already emitted before it
120+
// Copy the WHOLE run, so `->>` stays one token rather than three.
121+
while (i < n && OPERATOR_CHARS.has(s[i]!)) { out += s[i]!; i++; }
122+
while (i < n && /\s/.test(s[i]!)) i++; // skip whitespace after it
123+
continue;
124+
}
78125
out += ch;
79126
i++;
80127
}

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

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@ import { normalizeCheckExpr, checkExprEquals, stripCheckWrapper } from "../../sr
33

44
describe("normalizeCheckExpr", () => {
55
test("strips parens, collapses whitespace, lowercases", () => {
6-
expect(normalizeCheckExpr("(price >= 0) AND (price <= 100)")).toBe("price >= 0 and price <= 100");
7-
expect(normalizeCheckExpr("price >= 0 AND price <= 100")).toBe("price >= 0 and price <= 100");
6+
expect(normalizeCheckExpr("(price >= 0) AND (price <= 100)")).toBe("price>=0 and price<=100");
7+
expect(normalizeCheckExpr("price >= 0 AND price <= 100")).toBe("price>=0 and price<=100");
8+
// Spacing around a SYMBOLIC operator is canonicalized away; spacing around an
9+
// ALPHABETIC one (`and`) is load-bearing and must survive.
10+
expect(normalizeCheckExpr("price>=0 AND price<=100")).toBe("price>=0 and price<=100");
811
});
912
test("PG-rewritten form equals the generated form", () => {
1013
expect(checkExprEquals("(col >= 0) AND (col <= 100)", "col >= 0 AND col <= 100")).toBe(true);
@@ -31,7 +34,7 @@ describe("normalizeCheckExpr", () => {
3134
// the cast-strip must not corrupt a regex literal containing `::`, else two
3235
// distinct regex CHECKs would compare equal and a pattern change be missed.
3336
expect(checkExprEquals("slug ~ 'a::foo'", "slug ~ 'a::bar'")).toBe(false);
34-
expect(normalizeCheckExpr("slug ~ 'a::foo'")).toBe("slug ~ 'a::foo'");
37+
expect(normalizeCheckExpr("slug ~ 'a::foo'")).toBe("slug~'a::foo'");
3538
// but PG's post-literal `::text` cast (enum form) is still stripped
3639
expect(normalizeCheckExpr("'open'::text")).toBe("'open'");
3740
});
@@ -40,7 +43,7 @@ describe("normalizeCheckExpr", () => {
4043
// produces after bracket-stripping). A quoted literal that merely contains
4144
// the words must be left intact — a stray quote breaks the `=…any…array`
4245
// adjacency, so there is no `in` rewrite.
43-
expect(normalizeCheckExpr("note = 'pick any array item'")).toBe("note = 'pick any array item'");
46+
expect(normalizeCheckExpr("note = 'pick any array item'")).toBe("note='pick any array item'");
4447
expect(checkExprEquals(
4548
"note = 'pick any array item'",
4649
"note in 'pick', 'item'",
@@ -50,6 +53,45 @@ describe("normalizeCheckExpr", () => {
5053
expect(checkExprEquals("col >= 0", "col >= 5")).toBe(false);
5154
});
5255

56+
test("operator spacing is not semantic: the natural `@expr` spelling matches pg_get_expr", () => {
57+
// THE defect. An `index.lookup @expr` is AUTHOR-written SQL and a person writes it
58+
// tight; `pg_get_expr` hands it back spaced and cast. Casts and redundant parens were
59+
// already canonicalized, so the one surviving difference was the spacing — and
60+
// `verify --db` proposed DROP + CREATE against an index the database already held
61+
// exactly as declared. Measured on a live postgres:16 across five spellings.
62+
const introspected = "((request_context ->> 'device_id'::text))";
63+
for (const authored of [
64+
"(request_context->>'device_id')",
65+
"(request_context ->> 'device_id')",
66+
"((request_context ->> 'device_id'))",
67+
"((request_context ->> 'device_id'::text))",
68+
"((request_context->>'device_id'::text))",
69+
]) {
70+
expect(checkExprEquals(authored, introspected)).toBe(true);
71+
}
72+
// A multi-character operator survives as ONE token rather than being split apart.
73+
expect(normalizeCheckExpr("a #>> b")).toBe("a#>>b");
74+
expect(normalizeCheckExpr("a !~* b")).toBe("a!~*b");
75+
// An operator this file has never seen normalizes by the same rule — the set is
76+
// PG's operator CHARACTERS, not a list of operators anyone has to maintain.
77+
expect(checkExprEquals("tags @> '{a}'", "tags@>'{a}'")).toBe(true);
78+
});
79+
80+
test("...and collapsing spacing does not make different expressions equal", () => {
81+
// The dangerous direction. Whitespace around a symbolic operator is never semantic,
82+
// so nothing here may compare equal — if it did, a real predicate change would read
83+
// as clean drift.
84+
expect(checkExprEquals("a ->> 'x'", "a ->> 'y'")).toBe(false);
85+
expect(checkExprEquals("a ->> 'x'", "a -> 'x'")).toBe(false);
86+
expect(checkExprEquals("a >= b", "a > b")).toBe(false);
87+
expect(checkExprEquals("a <> b", "a = b")).toBe(false);
88+
// An operator character INSIDE a literal is data, not an operator.
89+
expect(checkExprEquals("code ~ '^a >> b$'", "code ~ '^a>>b$'")).toBe(false);
90+
// And an ALPHABETIC operator keeps its separators — `a and b` must never become `aandb`.
91+
expect(normalizeCheckExpr("a AND b")).toBe("a and b");
92+
expect(checkExprEquals("a AND b", "a OR b")).toBe(false);
93+
});
94+
5395
test("separator commas normalize, but a comma INSIDE a literal stays meaningful", () => {
5496
// Separator commas (outside quotes) collapse, so a generated IN-list matches a
5597
// hand-written one regardless of spacing...
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
/**
2+
* An `index.lookup @expr` written the way a person writes it must not read as drift.
3+
*
4+
* `pg_get_expr` re-emits every operator SPACED and appends casts:
5+
*
6+
* authored (request_context->>'device_id')
7+
* introspected ((request_context ->> 'device_id'::text))
8+
*
9+
* The comparator already canonicalized the casts and the redundant parens, so the ONE
10+
* surviving difference was the whitespace around the operator — and it was the one
11+
* difference a human actually produces. `verify --db` therefore reported drift, and
12+
* `migrate` proposed DROP + CREATE, against an index the database already held exactly
13+
* as declared. Found by an adopter estate running the 1.0 RC: its own README told it to
14+
* retire a hand-written index adjunct "the moment #342 ships", and following that
15+
* instruction produced a proposed DROP against a live production index.
16+
*
17+
* A unit test over the normalizer proves the strings converge. It cannot prove that the
18+
* string PG hands back is the one being converged ON — which is the half that was wrong.
19+
* So the gate is a REAL engine, per this package's doctrine:
20+
* 1. Create the index in Postgres from the AUTHORED spelling.
21+
* 2. Read back what PG actually stored (asserted, so a future PG that re-emits
22+
* differently fails HERE rather than silently making the test vacuous).
23+
* 3. Diff metadata carrying the natural tight spelling against it → MUST be empty.
24+
* 4. Prove the gate can still convict: a genuinely different expression → drift.
25+
*
26+
* Skips gracefully when MIGRATE_TS_PG_URL is not set (ts-slow CI provides a PG sidecar).
27+
*/
28+
29+
import { test, expect, beforeAll, afterAll, describe } from "bun:test";
30+
import { Pool } from "pg";
31+
import { Kysely, PostgresDialect, sql } from "kysely";
32+
import { MetaDataLoader, InMemoryStringSource } from "@metaobjectsdev/metadata";
33+
import { buildExpectedSchema } from "../../src/expected-schema.js";
34+
import { introspectPostgres } from "../../src/introspect/postgres.js";
35+
import { diff } from "../../src/diff/index.js";
36+
37+
const PG_URL = process.env["MIGRATE_TS_PG_URL"];
38+
39+
/** `expr` is the authored spelling under test. */
40+
const meta = (expr: string): string =>
41+
JSON.stringify({
42+
"metadata.root": {
43+
package: "acme",
44+
children: [
45+
{
46+
"object.entity": {
47+
name: "Notification",
48+
children: [
49+
{ "source.rdb": { "@table": "notification" } },
50+
{ "field.long": { name: "id" } },
51+
// An open/untyped JSON map: a `field.string` with a jsonb physical type, which
52+
// is what the loader's own error message prescribes for a bare object.
53+
{ "field.string": { name: "requestContext", "@column": "request_context", "@dbColumnType": "jsonb" } },
54+
{ "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } },
55+
{ "index.lookup": { name: "byDevice", "@expr": expr } },
56+
],
57+
},
58+
},
59+
],
60+
},
61+
});
62+
63+
describe("an expression index written the human way is not drift (PG)", () => {
64+
if (!PG_URL) {
65+
test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {});
66+
return;
67+
}
68+
69+
let kysely: Kysely<Record<string, unknown>>;
70+
let pool: Pool;
71+
72+
const reset = async (): Promise<void> => {
73+
await sql.raw("DROP TABLE IF EXISTS notification CASCADE").execute(kysely);
74+
await sql
75+
.raw(
76+
`CREATE TABLE notification (
77+
id bigserial PRIMARY KEY,
78+
request_context jsonb
79+
)`,
80+
)
81+
.execute(kysely);
82+
// Created from the AUTHORED spelling — tight, no cast. PG stores its own rendering.
83+
// Named as the metadata names it, so the diff compares EXPRESSIONS: an index-name
84+
// mismatch presents as drop+add too, and would mask the thing under test.
85+
await sql
86+
.raw(
87+
`CREATE INDEX "byDevice"
88+
ON notification ((request_context->>'device_id'))`,
89+
)
90+
.execute(kysely);
91+
};
92+
93+
beforeAll(async () => {
94+
pool = new Pool({ connectionString: PG_URL });
95+
kysely = new Kysely<Record<string, unknown>>({ dialect: new PostgresDialect({ pool }) });
96+
await reset();
97+
});
98+
99+
afterAll(async () => {
100+
await sql.raw("DROP TABLE IF EXISTS notification CASCADE").execute(kysely);
101+
await pool.end();
102+
});
103+
104+
test("PG really does re-emit the expression spaced and cast", async () => {
105+
// Asserted rather than assumed. If a future Postgres rendered the expression the way
106+
// it was authored, every case below would pass while proving nothing, and this is the
107+
// line that would tell us.
108+
const actual = await introspectPostgres(kysely);
109+
const ix = actual.tables
110+
.find((t) => t.name === "notification")
111+
?.indexes?.find((i) => i.name === "byDevice");
112+
expect(ix).toBeDefined();
113+
expect(ix?.expr).toBeDefined();
114+
expect(ix?.expr).toContain(" ->> "); // spaced — the difference that caused the drift
115+
expect(ix?.expr).toContain("::text"); // and cast
116+
});
117+
118+
test("REGRESSION: the natural tight spelling diffs EMPTY against the live index", async () => {
119+
const loaded = await new MetaDataLoader().load([
120+
new InMemoryStringSource(meta("(request_context->>'device_id')")),
121+
]);
122+
expect(loaded.errors).toEqual([]);
123+
124+
const actual = await introspectPostgres(kysely);
125+
const d = await diff({
126+
expected: buildExpectedSchema(loaded.root, { dialect: "postgres" }),
127+
actual,
128+
dialect: "postgres",
129+
allow: { dropIndex: true },
130+
});
131+
// Pre-fix this was drop-index + add-index against an index already correct.
132+
expect(d.changes.filter((c) => c.kind === "drop-index" || c.kind === "add-index")).toEqual([]);
133+
expect(d.changes).toEqual([]);
134+
});
135+
136+
test("...and every other spelling of the same expression agrees with it", async () => {
137+
const actual = await introspectPostgres(kysely);
138+
for (const authored of [
139+
"(request_context ->> 'device_id')",
140+
"((request_context ->> 'device_id'))",
141+
"((request_context ->> 'device_id'::text))",
142+
"((request_context->>'device_id'::text))",
143+
]) {
144+
const loaded = await new MetaDataLoader().load([new InMemoryStringSource(meta(authored))]);
145+
expect(loaded.errors).toEqual([]);
146+
const d = await diff({
147+
expected: buildExpectedSchema(loaded.root, { dialect: "postgres" }),
148+
actual,
149+
dialect: "postgres",
150+
allow: { dropIndex: true },
151+
});
152+
expect(`${authored}: ${JSON.stringify(d.changes)}`).toBe(`${authored}: []`);
153+
}
154+
});
155+
156+
test("the gate can still CONVICT: a genuinely different key is drift", async () => {
157+
// The other half. A comparator that erases whitespace must not erase the key.
158+
const loaded = await new MetaDataLoader().load([
159+
new InMemoryStringSource(meta("(request_context->>'session_id')")),
160+
]);
161+
expect(loaded.errors).toEqual([]);
162+
const actual = await introspectPostgres(kysely);
163+
const d = await diff({
164+
expected: buildExpectedSchema(loaded.root, { dialect: "postgres" }),
165+
actual,
166+
dialect: "postgres",
167+
allow: { dropIndex: true },
168+
});
169+
expect(d.changes.some((c) => c.kind === "drop-index")).toBe(true);
170+
expect(d.changes.some((c) => c.kind === "add-index")).toBe(true);
171+
});
172+
});

0 commit comments

Comments
 (0)