Skip to content

Commit 93b7dbf

Browse files
dmealingclaude
andcommitted
fix(migrate): --allow adopt-view no longer emits illegal CREATE OR REPLACE VIEW (#239)
The adopt-view branch (a Postgres DB view with no MetaObjects fingerprint — created before view stamping, or hand-written) pushed `replace-view` unconditionally, so a view that was BOTH being adopted AND structurally changed (column rename / reorder / mid-insert) in the same migration emitted `CREATE OR REPLACE VIEW`. Postgres rejects that at apply time (`cannot change name of view column …`), aborting the migration on any DB that already holds the prior view. Invisible until apply: offline migrate and a fresh DB (where CREATE OR REPLACE behaves as CREATE) never hit it. The adopt branch now routes through the same `viewReplaceIsLegal` decision the managed path makes (`pushViewUpdate(v, a, changes, /* unmanaged */ true)`): a legal append stays a non-destructive `replace-view`; a structural change is emitted as `drop-view` + `create-view` (the recreated view is re-stamped, so the next migrate converges). Adopting an unmanaged view still requires `allow.adoptView` in BOTH cases — the drop+ create carries the adopt-view gate (`unmanagedActual` on the drop-view), checked in status.ts BEFORE the recreate-pair auto-allow so it can't silently clobber hand-written SQL. SQLite/D1 unaffected (they always drop+create); the emit `orReplace` invariant (OR REPLACE only for a legal `replace-view`) is restored. TS-only (schema migrations are TS-owned, ADR-0015). Gated by unit + emit tests and a real-Postgres round-trip (hold prior unmanaged view → diff → emit → APPLY → re-diff==[]). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014xy8powhHYJ6gfFt9Ut8dL
1 parent 60e7d31 commit 93b7dbf

6 files changed

Lines changed: 244 additions & 8 deletions

File tree

CHANGELOG.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,30 @@ here. The format follows [Keep a Changelog](https://keepachangelog.com/), and
55
this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html)
66
(pre-1.0; MINOR bumps may introduce breaking changes with notice).
77

8+
## [Unreleased]
9+
10+
### Fixed — `--allow adopt-view` no longer emits an illegal `CREATE OR REPLACE VIEW` (#239)
11+
12+
**npm-only** (`migrate-ts` + `cli`; schema migrations are TS-owned, ADR-0015 — NuGet / PyPI /
13+
Maven have no migrate engine). `meta migrate --allow adopt-view` emitted `CREATE OR REPLACE
14+
VIEW` when adopting an **unmanaged** Postgres view (one with no MetaObjects fingerprint —
15+
created before view stamping, or hand-written) that was **also** structurally changed in the
16+
same migration (column rename / reorder / mid-list insert). Postgres rejects that DDL at apply
17+
time (`cannot change name of view column …`), aborting the migration on any DB that already
18+
holds the prior view. It was invisible until apply: offline `meta migrate` and a freshly
19+
provisioned DB (where `CREATE OR REPLACE` behaves as a plain `CREATE`) never hit it — it bites
20+
exactly once per project, on the first migration authored after upgrading into view
21+
fingerprinting.
22+
23+
The adopt-view branch now runs the **same** legal/illegal `CREATE OR REPLACE` decision the
24+
managed path already made (`viewReplaceIsLegal`): a legal append stays a non-destructive
25+
`replace-view`; a structural change is emitted as `drop-view` + `create-view` (the recreated
26+
view is re-stamped, so the next migrate converges). Adopting an unmanaged view still requires
27+
`allow.adoptView` in both cases — the drop+create path carries the adopt-view gate so the
28+
recreate-pair auto-allow can't silently clobber hand-written SQL. Gated by unit + emit tests
29+
and a real-Postgres round-trip (`hold prior unmanaged view → diff → emit → APPLY → re-diff ==
30+
[]`). Generated-output change — regenerate to pick it up; three-way merge preserves hand edits.
31+
832
## [0.20.3] — 2026-07-25
933

1034
**Coordinated release** — npm `0.20.3` · PyPI `0.19.6` · Maven Central `7.11.4` · NuGet

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

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -558,12 +558,14 @@ function diffViews(
558558
// existed, OR somebody's hand-written SQL sitting at a projection's name — and on
559559
// Postgres those are indistinguishable, because the deparser destroyed the text
560560
// evidence. Overwriting the second destroys work nothing can restore, so fail
561-
// closed: propose the replace but BLOCK it pending `allow.adoptView`.
561+
// closed: propose the adoption but BLOCK it pending `allow.adoptView`.
562+
//
563+
// #239: route through the SAME legal/illegal OR-REPLACE decision the managed
564+
// path makes — a structural change (rename/reorder/mid-insert) is not a legal
565+
// CREATE OR REPLACE and must be drop+create. `unmanaged: true` carries the
566+
// adopt-view gate onto whichever change kind we emit.
562567
if (a.fingerprint === undefined) {
563-
changes.push({
564-
kind: "replace-view", view: v, ...schemaSpread(v.schema),
565-
restore: a, unmanagedActual: true, status: ALLOWED,
566-
});
568+
pushViewUpdate(v, a, changes, /* unmanaged */ true);
567569
continue;
568570
}
569571

@@ -602,16 +604,23 @@ function diffViews(
602604
* view's columns come out in projection DECLARATION order, so a field APPENDED to a
603605
* projection lands last, which is exactly what Postgres's prefix rule permits.
604606
*/
605-
function pushViewUpdate(expected: ViewDescriptor, actual: ViewDescriptor, changes: Change[]): void {
607+
function pushViewUpdate(
608+
expected: ViewDescriptor, actual: ViewDescriptor, changes: Change[],
609+
// #239: when the ACTUAL view is unmanaged (no fingerprint), this is an ADOPTION —
610+
// stamp the adopt-view gate (`unmanagedActual`) onto whichever change we emit so
611+
// status.ts fails closed on `allow.adoptView`, legal-replace and drop+create alike.
612+
unmanaged = false,
613+
): void {
606614
const sx = schemaSpread(expected.schema);
615+
const adopt = unmanaged ? { unmanagedActual: true as const } : {};
607616
if (viewReplaceIsLegal(expected.columns, actual.columns)) {
608-
changes.push({ kind: "replace-view", view: expected, ...sx, restore: actual, status: ALLOWED });
617+
changes.push({ kind: "replace-view", view: expected, ...sx, restore: actual, ...adopt, status: ALLOWED });
609618
return;
610619
}
611620
// The column list changed shape (removed / renamed / reordered / retyped), so
612621
// Postgres refuses OR REPLACE. The view must be dropped and rebuilt — which is
613622
// destructive to anything depending on it (annotateViewDropDependents makes that loud).
614-
changes.push({ kind: "drop-view", view: expected.name, ...sx, restore: actual, status: ALLOWED });
623+
changes.push({ kind: "drop-view", view: expected.name, ...sx, restore: actual, ...adopt, status: ALLOWED });
615624
changes.push({ kind: "create-view", view: expected, ...sx, status: ALLOWED });
616625
}
617626

server/typescript/packages/migrate-ts/src/diff/status.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,16 @@ function blockedReasonFor(
7070
+ `(${external.map((d) => `${d.schema}.${d.name}`).join(", ")}) `
7171
+ `— pass allow.dropViewCascade to destroy them, or migrate them off this view first`;
7272
}
73+
// #239: the drop half of ADOPTING an unmanaged view (no fingerprint) whose shape
74+
// changed too much for a legal CREATE OR REPLACE. It is paired with a create-view,
75+
// so the recreate-pair rule below would auto-allow it — but clobbering (possibly
76+
// hand-written) SQL needs explicit consent, exactly like the replace-view adopt
77+
// path. Gate on allow.adoptView BEFORE the recreate-pair auto-allow.
78+
if (c.unmanagedActual && !allow.adoptView) {
79+
return `existing view "${c.view}" carries no MetaObjects fingerprint — it is hand-written, `
80+
+ `or was created before view fingerprinting. Its shape changed, so adopting it means `
81+
+ `DROP + rebuild, which takes ownership and cannot be undone. Pass allow.adoptView to adopt it`;
82+
}
7383
// Identity, not bare name — see viewIdentity().
7484
if (recreatedViews.has(viewIdentity(c.view, c.schema))) return null; // recreate pair — view survives
7585
return allow.dropView ? null : "destructive: drop-view not allowed (pass allow.dropView)";

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,15 @@ export type Change =
243243
* do not manage — so this is blocked pending `allow.dropViewCascade`.
244244
*/
245245
dependents?: readonly DependentRelation[];
246+
/**
247+
* This drop is the drop half of adopting an UNMANAGED view (no fingerprint)
248+
* whose shape changed too much for a legal `CREATE OR REPLACE` (#239). It is
249+
* paired with a `create-view`, so the recreate-pair rule would normally
250+
* auto-allow it — but clobbering (possibly hand-written) SQL still needs
251+
* explicit consent, so status.ts gates it on `allow.adoptView`, exactly like
252+
* the `replace-view` adopt path.
253+
*/
254+
unmanagedActual?: boolean;
246255
}
247256
| {
248257
kind: "replace-view";
Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/**
2+
* PG round-trip — #239: adopting an UNMANAGED view (no fingerprint) whose shape
3+
* changed too much for a legal `CREATE OR REPLACE` must be emitted as DROP+CREATE,
4+
* and it must APPLY cleanly against a DB that already holds the prior view.
5+
*
6+
* The bug: the adopt-view branch pushed `replace-view` unconditionally, so a renamed
7+
* output column emitted `CREATE OR REPLACE VIEW` — which Postgres rejects at apply
8+
* ("cannot change name of view column"). Invisible until apply (offline migrate /
9+
* a fresh DB where CREATE OR REPLACE behaves as CREATE never hit it).
10+
*
11+
* Acceptance gate (per the migrate round-trip discipline):
12+
* hold the prior unmanaged view → diff (adoptView) → emit → APPLY → re-diff == [].
13+
*
14+
* Skips gracefully when MIGRATE_TS_PG_URL is not set.
15+
*/
16+
17+
import { test, expect, beforeAll, afterAll, describe } from "bun:test";
18+
import { Pool } from "pg";
19+
import { Kysely, PostgresDialect, sql } from "kysely";
20+
import { introspectPostgres } from "../../src/introspect/postgres.js";
21+
import { diff } from "../../src/diff/index.js";
22+
import { emit } from "../../src/emit/index.js";
23+
import { viewFingerprint } from "../../src/view-fingerprint.js";
24+
import type { SchemaSnapshot } from "../../src/types.js";
25+
26+
const PG_URL = process.env["MIGRATE_TS_PG_URL"];
27+
28+
async function applyRaw(k: Kysely<Record<string, unknown>>, sqlText: string): Promise<void> {
29+
for (const stmt of sqlText.split(";").map((s) => s.trim()).filter((s) => s.length > 0)) {
30+
await sql.raw(stmt).execute(k);
31+
}
32+
}
33+
34+
describe("PG adopt-view structural change round-trip (#239)", () => {
35+
if (!PG_URL) {
36+
test.skip("skipped — MIGRATE_TS_PG_URL not set", () => {});
37+
return;
38+
}
39+
40+
let kysely: Kysely<Record<string, unknown>>;
41+
let pool: Pool;
42+
43+
beforeAll(async () => {
44+
pool = new Pool({ connectionString: PG_URL });
45+
kysely = new Kysely<Record<string, unknown>>({ dialect: new PostgresDialect({ pool }) });
46+
await sql.raw(`DROP VIEW IF EXISTS v_foo CASCADE`).execute(kysely);
47+
await sql.raw(`DROP TABLE IF EXISTS t_239 CASCADE`).execute(kysely);
48+
await sql.raw(`CREATE TABLE t_239 (a text, b text)`).execute(kysely);
49+
// An UNMANAGED view: NO metaobjects fingerprint COMMENT (pre-fingerprint / hand-written).
50+
await sql.raw(`CREATE VIEW v_foo AS SELECT t_239.a AS a, t_239.b AS b FROM t_239`).execute(kysely);
51+
});
52+
53+
afterAll(async () => {
54+
await sql.raw(`DROP VIEW IF EXISTS v_foo CASCADE`).execute(kysely);
55+
await sql.raw(`DROP TABLE IF EXISTS t_239 CASCADE`).execute(kysely);
56+
await pool.end();
57+
});
58+
59+
test("renamed output column: adopt via DROP+CREATE, applies clean, re-diff empty", async () => {
60+
const actual = await introspectPostgres(kysely);
61+
// Metadata renames the second output column b → c (a structural change).
62+
const body = `SELECT t_239.a AS a, t_239.b AS c FROM t_239`;
63+
const expectedView = {
64+
name: "v_foo",
65+
sql: body,
66+
fingerprint: viewFingerprint(body),
67+
columns: [
68+
{ name: "a", sqlType: { kind: "text" as const } },
69+
{ name: "c", sqlType: { kind: "text" as const } },
70+
],
71+
dependsOn: ["t_239"],
72+
};
73+
const expected: SchemaSnapshot = { tables: actual.tables, views: [expectedView] };
74+
75+
const r = await diff(expected, actual, { dialect: "postgres", allow: { adoptView: true } });
76+
// The fix: a structural adoption is DROP+CREATE, never an illegal OR-REPLACE.
77+
expect(r.changes.find((c) => c.kind === "replace-view")).toBeUndefined();
78+
expect(r.changes.find((c) => c.kind === "drop-view")).toBeDefined();
79+
expect(r.blocked).toEqual([]);
80+
81+
const up = emit(r.changes, { dialect: "postgres" }).up;
82+
expect(up).not.toMatch(/CREATE OR REPLACE VIEW/i);
83+
84+
// Apply against the DB that ALREADY HOLDS the prior view — the bug threw HERE
85+
// with "cannot change name of view column b to c".
86+
await applyRaw(kysely, up);
87+
88+
// Converged: the view is now fingerprint-stamped, so a re-diff is empty.
89+
const actual2 = await introspectPostgres(kysely);
90+
const r2 = await diff({ tables: actual2.tables, views: [expectedView] }, actual2, { dialect: "postgres" });
91+
expect(r2.changes.filter((c) => c.kind.endsWith("-view"))).toEqual([]);
92+
});
93+
});

server/typescript/packages/migrate-ts/test/unit/diff.test.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { test, expect, describe } from "bun:test";
22
import { diff } from "../../src/diff/index.js";
3+
import { emit } from "../../src/emit/index.js";
34
import type { SchemaSnapshot, ColumnDescriptor } from "../../src/types.js";
45

56
const empty: SchemaSnapshot = { tables: [], views: [] };
@@ -137,3 +138,93 @@ describe("diff — view-body drift", () => {
137138
});
138139
});
139140
});
141+
142+
// ---------------------------------------------------------------------------
143+
// #239 — the Postgres adopt-view branch (DB view carries no fingerprint) must
144+
// make the SAME legal/illegal OR-REPLACE decision the managed path makes.
145+
// A structural change (rename / reorder / mid-insert) is NOT a legal
146+
// CREATE OR REPLACE, so it must be emitted as drop-view + create-view — and the
147+
// adoption of an unmanaged view still requires allow.adoptView (the recreate-pair
148+
// auto-allow must not wave through clobbering hand-written SQL).
149+
// ---------------------------------------------------------------------------
150+
describe("diff — Postgres adopt-view legality (#239)", () => {
151+
const T = { kind: "text" as const };
152+
// A view metadata (expected) side always carries a fingerprint.
153+
const expView = (cols: string[]) => ({
154+
name: "v_foo",
155+
sql: `SELECT ${cols.map((c) => `t.${c} AS ${c}`).join(", ")} FROM t`,
156+
fingerprint: "a".repeat(64), // raw sha256-hex; renderFingerprintMarker adds the prefix
157+
columns: cols.map((name) => ({ name, sqlType: T })),
158+
});
159+
// The DB (actual) side has NO fingerprint — pre-fingerprint or hand-written.
160+
const dbView = (cols: string[]) => ({
161+
name: "v_foo",
162+
sql: `SELECT ${cols.map((c) => `t.${c} AS ${c}`).join(", ")} FROM t`,
163+
columns: cols.map((name) => ({ name, sqlType: T })),
164+
});
165+
const opts = (adoptView = false) => ({ dialect: "postgres" as const, allow: { adoptView } });
166+
167+
test("STRUCTURAL change (renamed output column) → drop-view + create-view, NOT replace-view", async () => {
168+
// DB view outputs (a, b); metadata renames b→c → columns (a, c). Postgres cannot
169+
// OR-REPLACE a column rename ("cannot change name of view column").
170+
const r = await diff(
171+
{ tables: [], views: [expView(["a", "c"])] },
172+
{ tables: [], views: [dbView(["a", "b"])] },
173+
opts(true),
174+
);
175+
expect(r.changes.find((c) => c.kind === "replace-view")).toBeUndefined();
176+
expect(r.changes.find((c) => c.kind === "drop-view")).toMatchObject({ kind: "drop-view", view: "v_foo" });
177+
expect(r.changes.find((c) => c.kind === "create-view")).toMatchObject({
178+
kind: "create-view", view: { name: "v_foo" },
179+
});
180+
});
181+
182+
test("adopting an unmanaged view via drop+create STILL requires allow.adoptView", async () => {
183+
// Without allow.adoptView the drop must be BLOCKED — the recreate-pair auto-allow
184+
// must not silently clobber the (possibly hand-written) unmanaged view.
185+
const r = await diff(
186+
{ tables: [], views: [expView(["a", "c"])] },
187+
{ tables: [], views: [dbView(["a", "b"])] },
188+
opts(false),
189+
);
190+
const dropv = r.changes.find((c) => c.kind === "drop-view");
191+
expect(dropv?.status.state).toBe("blocked");
192+
// With allow.adoptView → allowed.
193+
const r2 = await diff(
194+
{ tables: [], views: [expView(["a", "c"])] },
195+
{ tables: [], views: [dbView(["a", "b"])] },
196+
opts(true),
197+
);
198+
expect(r2.changes.find((c) => c.kind === "drop-view")?.status.state).toBe("allowed");
199+
});
200+
201+
test("PURE APPEND (columns are a prefix) is still a legal replace-view (unmanagedActual)", async () => {
202+
// DB (a, b); metadata (a, b, c) — c appended. Legal OR-REPLACE → replace-view,
203+
// still gated on adoptView.
204+
const r = await diff(
205+
{ tables: [], views: [expView(["a", "b", "c"])] },
206+
{ tables: [], views: [dbView(["a", "b"])] },
207+
opts(true),
208+
);
209+
expect(r.changes.find((c) => c.kind === "replace-view")).toMatchObject({
210+
kind: "replace-view", view: { name: "v_foo" }, unmanagedActual: true,
211+
});
212+
expect(r.changes.find((c) => c.kind === "drop-view")).toBeUndefined();
213+
});
214+
215+
test("emitted up.sql for a structural adoption is DROP+CREATE (no illegal CREATE OR REPLACE)", async () => {
216+
// The bug's symptom: `CREATE OR REPLACE VIEW` for a rename fails at apply on a DB
217+
// that already holds the prior view. The fix emits DROP VIEW + CREATE VIEW + a
218+
// fingerprint COMMENT (so the next migrate converges).
219+
const r = await diff(
220+
{ tables: [], views: [expView(["a", "c"])] },
221+
{ tables: [], views: [dbView(["a", "b"])] },
222+
opts(true),
223+
);
224+
const up = emit(r.changes, { dialect: "postgres" }).up;
225+
expect(up).not.toMatch(/CREATE OR REPLACE VIEW/i);
226+
expect(up).toMatch(/DROP VIEW/i);
227+
expect(up).toMatch(/CREATE VIEW/i);
228+
expect(up).toMatch(/COMMENT ON VIEW .* IS 'metaobjects:/i);
229+
});
230+
});

0 commit comments

Comments
 (0)