From d3870dec3bfacf0c8ae71ad17febeca4397aaf48 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sat, 1 Aug 2026 21:59:30 -0400 Subject: [PATCH 1/3] fix(#255): migrate emits drop-fk/drop-check before drop-column so referenced-column drops apply Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr --- .../packages/migrate-ts/src/emit/postgres.ts | 20 +- .../packages/migrate-ts/src/emit/sqlite.ts | 18 +- .../pg-drop-fk-before-drop-column.test.ts | 202 ++++++++++++++++++ .../test/unit/emit-postgres.test.ts | 48 +++++ .../migrate-ts/test/unit/emit-sqlite.test.ts | 60 ++++++ 5 files changed, 341 insertions(+), 7 deletions(-) create mode 100644 server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts diff --git a/server/typescript/packages/migrate-ts/src/emit/postgres.ts b/server/typescript/packages/migrate-ts/src/emit/postgres.ts index 3f3086c7a..b1040b69b 100644 --- a/server/typescript/packages/migrate-ts/src/emit/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/emit/postgres.ts @@ -7,18 +7,28 @@ import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; import { renderFingerprintMarker, viewFingerprint } from "../view-fingerprint.js"; import { viewReplaceIsLegal } from "../view-column-types.js"; -// Stages run low → high. drop-view + drop-fk run BEFORE drop-table so a view -// that depends on a soon-to-be-dropped table is removed first. create-view -// runs AFTER add-fk so the view can reference the new schema in full. +// Stages run low → high. drop-view runs BEFORE drop-table so a view that +// depends on a soon-to-be-dropped table is removed first. create-view runs +// AFTER add-fk so the view can reference the new schema in full. +// +// #255: constraint DROPS and constraint ADDS share the same "constraint" kind +// but need OPPOSITE ordering relative to column mutation — a drop must run +// BEFORE the column change (the constraint must be gone before its column is +// dropped, or Postgres refuses `DROP COLUMN` with "other objects depend on +// it"), while an add must run AFTER (the column it references must already +// exist). One stage can't satisfy both, so drop-fk/drop-check are hoisted to +// stage 1 (alongside create-table, before any column mutation); add-fk/ +// add-check stay at stage 5. const STAGE_ORDER: Record = { "drop-view": 0, + "drop-fk": 1, "drop-check": 1, "create-table": 1, "add-column": 2, "drop-column": 2, "change-column-type": 2, "change-column-nullable": 2, "change-column-default": 2, "rename-column": 3, "rename-table": 3, "add-index": 4, "drop-index": 4, - "add-fk": 5, "drop-fk": 5, - "add-check": 5, "drop-check": 5, + "add-fk": 5, + "add-check": 5, "drop-table": 6, "create-view": 7, "replace-view": 7, }; diff --git a/server/typescript/packages/migrate-ts/src/emit/sqlite.ts b/server/typescript/packages/migrate-ts/src/emit/sqlite.ts index 0b2d607f9..c6b54158a 100644 --- a/server/typescript/packages/migrate-ts/src/emit/sqlite.ts +++ b/server/typescript/packages/migrate-ts/src/emit/sqlite.ts @@ -8,19 +8,33 @@ export interface CarryColumns { insertCols: string[]; selectCols: string[]; } // Stage ordering similar to PG; recreate-and-copy bundles get inserted // at their first triggering change's position in Task 23. +// +// #255: constraint DROPS and constraint ADDS share the same "constraint" kind +// but need OPPOSITE ordering relative to column mutation — a drop must run +// BEFORE the column change (the constraint must be gone before its column is +// dropped, or the DDL fails on the referenced-column dependency), while an add +// must run AFTER (the column it references must already exist). One stage +// can't satisfy both, so drop-fk/drop-check are hoisted to stage 1 (alongside +// create-table, before any column mutation); add-fk/add-check stay at stage 5. +// (drop-fk/drop-check are always recreate-triggering on SQLite — see +// RECREATE_TRIGGERING_KINDS below — so this mainly orders a drop-fk's +// table-recreate ahead of a native drop-column on a DIFFERENT table it once +// referenced; within the SAME table's recreate bundle, tableChanges order +// doesn't affect the emitted recipe.) const STAGE_ORDER: Record = { // drop-view runs FIRST (mirrors postgres): a view that depends on a table about // to be recreated-and-copied must be dropped before the DROP TABLE / RENAME, or // SQLite's rename re-parses the dependent view and can error mid-recreate. The // diff's Pass 2c injects exactly this drop(before)/create(after) pair. "drop-view": 0, + "drop-fk": 1, "drop-check": 1, "create-table": 1, "add-column": 2, "drop-column": 2, "change-column-type": 2, "change-column-nullable": 2, "change-column-default": 2, "rename-column": 3, "rename-table": 3, "add-index": 4, "drop-index": 4, - "add-fk": 5, "drop-fk": 5, - "add-check": 5, "drop-check": 5, + "add-fk": 5, + "add-check": 5, "drop-table": 6, // create-view / replace-view run LAST — after every table change the view reads. "create-view": 99, "replace-view": 99, diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts new file mode 100644 index 000000000..9a9d94ad3 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts @@ -0,0 +1,202 @@ +/** + * Real-Postgres gate for #255 — "a table whose column is referenced by + * another table's FK, where the metadata drops both the FK and the column." + * + * Root cause: the emitter's STAGE_ORDER put `drop-column` (stage 2) before + * `drop-fk` (stage 5), so the emitted UP SQL issued + * `ALTER TABLE "programs" DROP COLUMN "code"` while `weeks_program_code_fk` — + * another table's FK constraint referencing that column — still existed. + * Postgres refuses: "cannot drop column code of table programs because other + * objects depend on it." The fix hoists `drop-fk`/`drop-check` to a stage + * before column mutation; this test proves the reordered SQL actually applies + * against a real engine (a unit assertion on statement order alone is not + * sufficient evidence — see emit-postgres.test.ts for that half). + * + * A third, INCIDENTAL finding surfaced while building this scenario: the FK's + * target column must be unique (Postgres requirement), so dropping it also + * drops its backing `identity.secondary` index. Postgres auto-cascades that + * index away together with the column (same as it does for the table's own + * FK/CHECK constraints) — so a SEPARATE, explicit `DROP INDEX` for it is + * redundant and fails post-cascade ("index … does not exist") regardless of + * the #255 fix. That is a distinct latent ordering interaction between + * drop-column and drop-index — NOT part of #255's drop-fk/drop-check scope, + * unverified beyond this single observation, and deliberately NOT fixed here. + * This test isolates #255 by excluding that drop-index change from the + * applied change-set (still real DDL, real engine — just not conflating two + * bugs in one assertion). + * + * Gated on MIGRATE_TS_PG_URL, like every other pg integration test in this + * package. Skips cleanly when unset. + */ + +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"; +import { emit } from "../../src/emit/index.js"; + +const PG_URL = process.env["MIGRATE_TS_PG_URL"]; +const realDescribe = PG_URL ? describe : describe.skip; + +// V1: Program (id PK, code UNIQUE) + Week (id PK, programCode FK -> Program.code). +const V1 = JSON.stringify({ + "metadata.root": { + package: "acme", + children: [ + { + "object.entity": { + name: "Program", + children: [ + { "source.rdb": {} }, + { "field.long": { name: "id" } }, + { "field.string": { name: "code", "@required": true } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + { "identity.secondary": { name: "uniqueCode", "@fields": ["code"] } }, + ], + }, + }, + { + "object.entity": { + name: "Week", + children: [ + { "source.rdb": {} }, + { "field.long": { name: "id" } }, + { "field.string": { name: "programCode", "@required": true } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + { "identity.reference": { name: "ref_program", "@fields": ["programCode"], "@references": "Program.code" } }, + ], + }, + }, + ], + }, +}); + +// V2: Program drops `code` (+ its identity.secondary); Week drops the FK +// (identity.reference) but keeps its own `programCode` column (now a plain, +// un-constrained column) — the exact shape that produces a drop-fk + +// drop-column pair where the dropped column is the FK's REFERENCED side. +const V2 = JSON.stringify({ + "metadata.root": { + package: "acme", + children: [ + { + "object.entity": { + name: "Program", + children: [ + { "source.rdb": {} }, + { "field.long": { name: "id" } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + { + "object.entity": { + name: "Week", + children: [ + { "source.rdb": {} }, + { "field.long": { name: "id" } }, + { "field.string": { name: "programCode", "@required": true } }, + { "identity.primary": { name: "pk", "@fields": ["id"], "@generation": "increment" } }, + ], + }, + }, + ], + }, +}); + +let k: Kysely>; +let pool: Pool; + +if (PG_URL) { + beforeAll(() => { + pool = new Pool({ connectionString: PG_URL }); + k = new Kysely>({ dialect: new PostgresDialect({ pool }) }); + }); + afterAll(async () => { + await cleanup(); + await k.destroy(); + }); +} + +async function cleanup(): Promise { + await sql.raw(`DROP TABLE IF EXISTS "weeks" CASCADE`).execute(k); + await sql.raw(`DROP TABLE IF EXISTS "programs" CASCADE`).execute(k); +} + +async function applyRaw(sqlText: string): Promise { + for (const stmt of sqlText.split(";").map((s) => s.trim()).filter(Boolean)) { + await sql.raw(stmt).execute(k); + } +} + +async function loadRoot(json: string) { + return (await new MetaDataLoader().load([new InMemoryStringSource(json)])).root; +} + +realDescribe("PG #255 — drop-fk before drop-column applies cleanly", () => { + test("dropping an FK's referenced column + the FK itself: emitted SQL applies, re-diff empty", async () => { + await cleanup(); + + // Establish v1 (Program + Week, FK referencing Program.code) against a real, empty DB. + const v1Root = await loadRoot(V1); + const expected1 = buildExpectedSchema(v1Root, { dialect: "postgres" }); + const actual0 = await introspectPostgres(k); + const initial = await diff({ expected: expected1, actual: actual0, dialect: "postgres" }); + expect(initial.blocked).toEqual([]); + const emit1 = emit(initial.changes, { dialect: "postgres", expectedSchema: expected1 }); + await applyRaw(emit1.up); + + // Sanity: the FK is live and both columns exist. + await sql.raw(`INSERT INTO "programs" ("id", "code") VALUES (1, 'P1')`).execute(k); + await sql.raw(`INSERT INTO "weeks" ("id", "program_code") VALUES (1, 'P1')`).execute(k); + + // Evolve to v2: drop Program.code (the FK's referenced column) AND the + // FK itself, in one change-set. + const v2Root = await loadRoot(V2); + const expected2 = buildExpectedSchema(v2Root, { dialect: "postgres" }); + const actual1 = await introspectPostgres(k); + const evolve = await diff({ + expected: expected2, actual: actual1, dialect: "postgres", + allow: { dropColumn: true, dropFk: true, dropIndex: true }, + }); + expect(evolve.blocked).toEqual([]); + const kinds = evolve.changes.map((c) => c.kind).sort(); + expect(kinds).toEqual(["drop-column", "drop-fk", "drop-index"]); + + // Exclude drop-index — see file header. Postgres auto-cascades the + // identity.secondary's backing index away together with its column, so a + // separate explicit DROP INDEX for it is redundant (and fails + // post-cascade). That is an unrelated latent interaction, not #255's + // drop-fk/drop-check scope; excluding it here isolates the fix under + // test without silently asserting a fix for a bug this change doesn't + // touch. + const changesUnderTest = evolve.changes.filter((c) => c.kind !== "drop-index"); + const emit2 = emit(changesUnderTest, { dialect: "postgres", expectedSchema: expected2 }); + + // The fix under test: DROP CONSTRAINT must precede DROP COLUMN in the + // emitted UP SQL, or the applyRaw below fails against the real engine. + const idxDropConstraint = emit2.up.indexOf("DROP CONSTRAINT"); + const idxDropColumn = emit2.up.indexOf("DROP COLUMN"); + expect(idxDropConstraint).toBeGreaterThanOrEqual(0); + expect(idxDropColumn).toBeGreaterThanOrEqual(0); + expect(idxDropConstraint).toBeLessThan(idxDropColumn); + + // The real-engine gate: pre-fix, this throws + // `error: cannot drop column code of table programs because other + // objects depend on it`. Post-fix, it applies cleanly. + await applyRaw(emit2.up); + + // Idempotence: re-introspecting must show no drift against expected2 — + // proving the index (never explicitly dropped) is genuinely gone too, + // cascaded away by the column drop, not just skipped. + const followup = await diff({ expected: expected2, actual: await introspectPostgres(k), dialect: "postgres" }); + if (followup.changes.length > 0) { + console.error("ROUND-TRIP FAILURE — a second `meta migrate` would emit:"); + for (const c of followup.changes) console.error(" -", JSON.stringify(c)); + } + expect(followup.changes).toEqual([]); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts b/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts index 8f2b903f6..5486eaf41 100644 --- a/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts @@ -205,6 +205,54 @@ describe("renderPostgres — statement ordering", () => { expect(idxCreate).toBeLessThan(idxAdd); expect(idxAdd).toBeLessThan(idxDrop); }); + + // #255 — a DROP CONSTRAINT for an FK still referencing a column must run + // before that column's DROP COLUMN, or Postgres refuses at apply time with + // "cannot drop column … because other objects depend on it". + test("drop-fk runs before drop-column for the column it references (#255)", () => { + const changes: Change[] = [ + { kind: "drop-column", table: "weeks", column: "program_id", status: ALLOWED }, + { kind: "drop-fk", table: "weeks", fk: "weeks_program_id_fk", status: ALLOWED }, + ]; + const { up } = emit(changes, { dialect: "postgres" }); + const idxDropConstraint = up.indexOf("DROP CONSTRAINT"); + const idxDropColumn = up.indexOf("DROP COLUMN"); + expect(idxDropConstraint).toBeGreaterThanOrEqual(0); + expect(idxDropColumn).toBeGreaterThanOrEqual(0); + expect(idxDropConstraint).toBeLessThan(idxDropColumn); + }); + + // Same invariant for drop-check — a CHECK constraint on a column must be + // dropped before that column is dropped. + test("drop-check runs before drop-column for the column it constrains (#255)", () => { + const changes: Change[] = [ + { kind: "drop-column", table: "orders", column: "qty", status: ALLOWED }, + { kind: "drop-check", table: "orders", check: "orders_qty_chk", status: ALLOWED }, + ]; + const { up } = emit(changes, { dialect: "postgres" }); + const idxDropConstraint = up.indexOf("DROP CONSTRAINT"); + const idxDropColumn = up.indexOf("DROP COLUMN"); + expect(idxDropConstraint).toBeGreaterThanOrEqual(0); + expect(idxDropColumn).toBeGreaterThanOrEqual(0); + expect(idxDropConstraint).toBeLessThan(idxDropColumn); + }); + + // add-fk must still run AFTER column mutation (the referenced column must + // already exist) — only the DROP direction was hoisted. + test("add-fk still runs after add-column (adds are unaffected by #255)", () => { + const changes: Change[] = [ + { kind: "add-fk", table: "weeks", + fk: { name: "weeks_program_id_fk", columns: ["program_id"], refTable: "programs", refColumns: ["id"] }, + status: ALLOWED }, + { kind: "add-column", table: "weeks", column: { name: "program_id", sqlType: { kind: "integer", bits: 64 }, nullable: true }, status: ALLOWED }, + ]; + const { up } = emit(changes, { dialect: "postgres" }); + const idxAddColumn = up.indexOf("ADD COLUMN"); + const idxAddConstraint = up.indexOf("ADD CONSTRAINT"); + expect(idxAddColumn).toBeGreaterThanOrEqual(0); + expect(idxAddConstraint).toBeGreaterThanOrEqual(0); + expect(idxAddColumn).toBeLessThan(idxAddConstraint); + }); }); describe("renderPostgres — down statements", () => { diff --git a/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts b/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts index 231b9c5f4..0f95f3165 100644 --- a/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts @@ -285,4 +285,64 @@ describe("renderSqlite — add-fk / drop-fk via recreate", () => { expect(up).toContain('CREATE TABLE "__new_weeks"'); expect(up).not.toContain('REFERENCES'); }); + + // #255 — drop-fk + drop-column on the SAME table both fold into ONE + // recreate-and-copy bundle regardless of STAGE_ORDER (bundling is by table, + // not by kind order), so the correct outcome — recreated table missing both + // the FK and the dropped column — already held pre-fix here. This pins that + // invariant so a future bundling change can't silently regress it. + test("drop-fk + drop-column on the same table: recreated table has neither (#255)", () => { + const newCols: ColumnDescriptor[] = [ + { name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false, identity: "increment" }, + ]; + const expectedSchema: SchemaSnapshot = { + tables: [table("weeks", newCols, ["id"])], + views: [], + }; + const { up } = emit( + [ + { kind: "drop-column", table: "weeks", column: "program_id", status: ALLOWED }, + { kind: "drop-fk", table: "weeks", fk: "weeks_program_id_fk", status: ALLOWED }, + ], + { dialect: "sqlite", expectedSchema }, + ); + expect(up).toContain('CREATE TABLE "__new_weeks"'); + expect(up).not.toContain("REFERENCES"); + expect(up).not.toContain("program_id"); + }); + + // #255 — the STAGE_ORDER-observable case for SQLite: a drop-fk on table B + // (always recreate-triggering) must sequence BEFORE a native drop-column on + // an unrelated table A that the dropped FK used to reference. Pre-fix, + // drop-column (stage 2) sorted before drop-fk (stage 5), so table A's + // column drop ran while table B's schema still declared the FK against it. + test("drop-fk on one table runs before a native drop-column on another table it referenced (#255)", () => { + const newProgramsCols: ColumnDescriptor[] = [ + { name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false, identity: "increment" }, + ]; + const newWeeksCols: ColumnDescriptor[] = [ + { name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false, identity: "increment" }, + ]; + const expectedSchema: SchemaSnapshot = { + tables: [ + table("programs", newProgramsCols, ["id"]), + table("weeks", newWeeksCols, ["id"]), + ], + views: [], + }; + const { up } = emit( + [ + // drop-column on "programs" (native — not recreate-triggering on modern SQLite) + { kind: "drop-column", table: "programs", column: "code", status: ALLOWED }, + // drop-fk on "weeks" (always recreate-triggering) + { kind: "drop-fk", table: "weeks", fk: "weeks_program_code_fk", status: ALLOWED }, + ], + { dialect: "sqlite", expectedSchema }, + ); + const idxWeeksRecreate = up.indexOf('CREATE TABLE "__new_weeks"'); + const idxProgramsDropColumn = up.indexOf('ALTER TABLE "programs" DROP COLUMN "code"'); + expect(idxWeeksRecreate).toBeGreaterThanOrEqual(0); + expect(idxProgramsDropColumn).toBeGreaterThanOrEqual(0); + expect(idxWeeksRecreate).toBeLessThan(idxProgramsDropColumn); + }); }); From 255426648222897cdc8f488d1c25fa44b39c7260 Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sat, 1 Aug 2026 22:11:26 -0400 Subject: [PATCH 2/3] fix(#255): also emit drop-index before drop-column (same backing-index cascade class) Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr --- .../packages/migrate-ts/src/emit/postgres.ts | 27 +++-- .../packages/migrate-ts/src/emit/sqlite.ts | 28 +++-- .../pg-drop-fk-before-drop-column.test.ts | 102 +++++++++--------- .../test/unit/emit-postgres.test.ts | 54 ++++++++++ .../migrate-ts/test/unit/emit-sqlite.test.ts | 38 +++++++ 5 files changed, 185 insertions(+), 64 deletions(-) diff --git a/server/typescript/packages/migrate-ts/src/emit/postgres.ts b/server/typescript/packages/migrate-ts/src/emit/postgres.ts index b1040b69b..a4db63781 100644 --- a/server/typescript/packages/migrate-ts/src/emit/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/emit/postgres.ts @@ -11,22 +11,33 @@ import { viewReplaceIsLegal } from "../view-column-types.js"; // depends on a soon-to-be-dropped table is removed first. create-view runs // AFTER add-fk so the view can reference the new schema in full. // -// #255: constraint DROPS and constraint ADDS share the same "constraint" kind +// #255: a constraint/index DROP and its ADD counterpart share the same kind // but need OPPOSITE ordering relative to column mutation — a drop must run -// BEFORE the column change (the constraint must be gone before its column is -// dropped, or Postgres refuses `DROP COLUMN` with "other objects depend on -// it"), while an add must run AFTER (the column it references must already -// exist). One stage can't satisfy both, so drop-fk/drop-check are hoisted to -// stage 1 (alongside create-table, before any column mutation); add-fk/ -// add-check stay at stage 5. +// BEFORE the column change (the constraint/index must be gone before its +// column is dropped, or Postgres refuses `DROP COLUMN` with "other objects +// depend on it" — this applies just as much to an index backing a UNIQUE/FK +// target as it does to the FK/CHECK constraint itself), while an add must run +// AFTER (the column it references must already exist). One stage can't +// satisfy both, so ALL drops — drop-fk/drop-check/drop-index — are hoisted +// ahead of any column mutation; their ADD counterparts (add-fk/add-check/ +// add-index) stay at their later stages. +// +// Within that "drops" group, drop-fk/drop-check must ALSO run BEFORE +// drop-index: an FK constraint depends on the unique/PK index backing its +// target column (that's how Postgres enforces the target must be unique), so +// dropping the index first fails with the same "other objects depend on it" +// class of error, one level removed — "constraint … depends on index …". +// drop-index therefore gets its own stage (1.5) strictly between drop-fk/ +// drop-check/create-table (1) and column mutation (2). const STAGE_ORDER: Record = { "drop-view": 0, "drop-fk": 1, "drop-check": 1, "create-table": 1, + "drop-index": 1.5, "add-column": 2, "drop-column": 2, "change-column-type": 2, "change-column-nullable": 2, "change-column-default": 2, "rename-column": 3, "rename-table": 3, - "add-index": 4, "drop-index": 4, + "add-index": 4, "add-fk": 5, "add-check": 5, "drop-table": 6, diff --git a/server/typescript/packages/migrate-ts/src/emit/sqlite.ts b/server/typescript/packages/migrate-ts/src/emit/sqlite.ts index c6b54158a..958dba81f 100644 --- a/server/typescript/packages/migrate-ts/src/emit/sqlite.ts +++ b/server/typescript/packages/migrate-ts/src/emit/sqlite.ts @@ -9,18 +9,29 @@ export interface CarryColumns { insertCols: string[]; selectCols: string[]; } // Stage ordering similar to PG; recreate-and-copy bundles get inserted // at their first triggering change's position in Task 23. // -// #255: constraint DROPS and constraint ADDS share the same "constraint" kind +// #255: a constraint/index DROP and its ADD counterpart share the same kind // but need OPPOSITE ordering relative to column mutation — a drop must run -// BEFORE the column change (the constraint must be gone before its column is -// dropped, or the DDL fails on the referenced-column dependency), while an add -// must run AFTER (the column it references must already exist). One stage -// can't satisfy both, so drop-fk/drop-check are hoisted to stage 1 (alongside -// create-table, before any column mutation); add-fk/add-check stay at stage 5. +// BEFORE the column change (the constraint/index must be gone before its +// column is dropped, or the DDL fails on the referenced-column dependency — +// this applies just as much to an index backing a UNIQUE/FK target as it does +// to the FK/CHECK constraint itself), while an add must run AFTER (the column +// it references must already exist). One stage can't satisfy both, so ALL +// drops — drop-fk/drop-check/drop-index — are hoisted ahead of any column +// mutation; their ADD counterparts (add-fk/add-check/add-index) stay at their +// later stages. +// +// Within that "drops" group, drop-fk/drop-check must ALSO run BEFORE +// drop-index — mirrors postgres.ts: an FK depends on the unique/PK index +// backing its target column, so dropping the index first can fail on that +// dependency, one level removed. drop-index gets its own stage (1.5) strictly +// between drop-fk/drop-check/create-table (1) and column mutation (2). // (drop-fk/drop-check are always recreate-triggering on SQLite — see // RECREATE_TRIGGERING_KINDS below — so this mainly orders a drop-fk's // table-recreate ahead of a native drop-column on a DIFFERENT table it once // referenced; within the SAME table's recreate bundle, tableChanges order -// doesn't affect the emitted recipe.) +// doesn't affect the emitted recipe. drop-index is NOT recreate-triggering — +// it's a plain `DROP INDEX`, so this ordering directly controls emission +// order among native statements.) const STAGE_ORDER: Record = { // drop-view runs FIRST (mirrors postgres): a view that depends on a table about // to be recreated-and-copied must be dropped before the DROP TABLE / RENAME, or @@ -29,10 +40,11 @@ const STAGE_ORDER: Record = { "drop-view": 0, "drop-fk": 1, "drop-check": 1, "create-table": 1, + "drop-index": 1.5, "add-column": 2, "drop-column": 2, "change-column-type": 2, "change-column-nullable": 2, "change-column-default": 2, "rename-column": 3, "rename-table": 3, - "add-index": 4, "drop-index": 4, + "add-index": 4, "add-fk": 5, "add-check": 5, "drop-table": 6, diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts index 9a9d94ad3..d1a876c42 100644 --- a/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts +++ b/server/typescript/packages/migrate-ts/test/integration/pg-drop-fk-before-drop-column.test.ts @@ -3,27 +3,26 @@ * another table's FK, where the metadata drops both the FK and the column." * * Root cause: the emitter's STAGE_ORDER put `drop-column` (stage 2) before - * `drop-fk` (stage 5), so the emitted UP SQL issued - * `ALTER TABLE "programs" DROP COLUMN "code"` while `weeks_program_code_fk` — - * another table's FK constraint referencing that column — still existed. + * `drop-fk`/`drop-check`/`drop-index` (originally stage 5/5/4), so the + * emitted UP SQL issued `ALTER TABLE "programs" DROP COLUMN "code"` while + * `weeks_program_code_fk` — another table's FK constraint referencing that + * column — AND `uniqueCode` — the UNIQUE index backing that same column + * (Postgres requires an FK target to be unique) — both still existed. * Postgres refuses: "cannot drop column code of table programs because other - * objects depend on it." The fix hoists `drop-fk`/`drop-check` to a stage - * before column mutation; this test proves the reordered SQL actually applies - * against a real engine (a unit assertion on statement order alone is not - * sufficient evidence — see emit-postgres.test.ts for that half). - * - * A third, INCIDENTAL finding surfaced while building this scenario: the FK's - * target column must be unique (Postgres requirement), so dropping it also - * drops its backing `identity.secondary` index. Postgres auto-cascades that - * index away together with the column (same as it does for the table's own - * FK/CHECK constraints) — so a SEPARATE, explicit `DROP INDEX` for it is - * redundant and fails post-cascade ("index … does not exist") regardless of - * the #255 fix. That is a distinct latent ordering interaction between - * drop-column and drop-index — NOT part of #255's drop-fk/drop-check scope, - * unverified beyond this single observation, and deliberately NOT fixed here. - * This test isolates #255 by excluding that drop-index change from the - * applied change-set (still real DDL, real engine — just not conflating two - * bugs in one assertion). + * objects depend on it." The fix hoists ALL drops that can have an external + * dependent — `drop-fk` / `drop-check` / `drop-index` — ahead of column + * mutation, while their ADD counterparts (`add-fk` / `add-check` / + * `add-index`) stay at their later, post-column stage. Within that group, + * `drop-fk`/`drop-check` ALSO run before `drop-index`: the FK constraint + * itself depends on the unique index backing its target column, so dropping + * the index first fails one level removed ("cannot drop index … because + * other objects depend on it" / "constraint … depends on index …"). This + * test proves the reordered SQL actually applies against a real engine, for + * the COMBINED scenario (a column that is BOTH FK-referenced AND + * index-backed, with the metadata dropping the FK, the index, and the column + * all in one change-set) — a unit assertion on statement order alone is not + * sufficient evidence; see emit-postgres.test.ts / emit-sqlite.test.ts for + * that half. * * Gated on MIGRATE_TS_PG_URL, like every other pg integration test in this * package. Skips cleanly when unset. @@ -74,10 +73,11 @@ const V1 = JSON.stringify({ }, }); -// V2: Program drops `code` (+ its identity.secondary); Week drops the FK -// (identity.reference) but keeps its own `programCode` column (now a plain, -// un-constrained column) — the exact shape that produces a drop-fk + -// drop-column pair where the dropped column is the FK's REFERENCED side. +// V2: Program drops `code` (+ its identity.secondary index); Week drops the +// FK (identity.reference) but keeps its own `programCode` column (now a +// plain, un-constrained column) — the exact shape that produces a drop-fk + +// drop-index + drop-column TRIPLE where the dropped column is both the FK's +// REFERENCED side and the index's target. const V2 = JSON.stringify({ "metadata.root": { package: "acme", @@ -136,8 +136,8 @@ async function loadRoot(json: string) { return (await new MetaDataLoader().load([new InMemoryStringSource(json)])).root; } -realDescribe("PG #255 — drop-fk before drop-column applies cleanly", () => { - test("dropping an FK's referenced column + the FK itself: emitted SQL applies, re-diff empty", async () => { +realDescribe("PG #255 — drop-fk/drop-check/drop-index before drop-column applies cleanly", () => { + test("dropping an FK's referenced+indexed column, the FK, and the index together: emitted SQL applies, re-diff empty", async () => { await cleanup(); // Establish v1 (Program + Week, FK referencing Program.code) against a real, empty DB. @@ -153,8 +153,11 @@ realDescribe("PG #255 — drop-fk before drop-column applies cleanly", () => { await sql.raw(`INSERT INTO "programs" ("id", "code") VALUES (1, 'P1')`).execute(k); await sql.raw(`INSERT INTO "weeks" ("id", "program_code") VALUES (1, 'P1')`).execute(k); - // Evolve to v2: drop Program.code (the FK's referenced column) AND the - // FK itself, in one change-set. + // Evolve to v2: drop Program.code (the FK's referenced + indexed + // column), the FK, AND the backing unique index, all in one change-set — + // the COMBINED scenario. Pre-fix, this fails on either the FK dependency + // or the auto-cascaded index (whichever the (buggy) order hits first); + // post-fix, both drops run before the column drop and it all applies. const v2Root = await loadRoot(V2); const expected2 = buildExpectedSchema(v2Root, { dialect: "postgres" }); const actual1 = await introspectPostgres(k); @@ -166,32 +169,35 @@ realDescribe("PG #255 — drop-fk before drop-column applies cleanly", () => { const kinds = evolve.changes.map((c) => c.kind).sort(); expect(kinds).toEqual(["drop-column", "drop-fk", "drop-index"]); - // Exclude drop-index — see file header. Postgres auto-cascades the - // identity.secondary's backing index away together with its column, so a - // separate explicit DROP INDEX for it is redundant (and fails - // post-cascade). That is an unrelated latent interaction, not #255's - // drop-fk/drop-check scope; excluding it here isolates the fix under - // test without silently asserting a fix for a bug this change doesn't - // touch. - const changesUnderTest = evolve.changes.filter((c) => c.kind !== "drop-index"); - const emit2 = emit(changesUnderTest, { dialect: "postgres", expectedSchema: expected2 }); - - // The fix under test: DROP CONSTRAINT must precede DROP COLUMN in the - // emitted UP SQL, or the applyRaw below fails against the real engine. + const emit2 = emit(evolve.changes, { dialect: "postgres", expectedSchema: expected2 }); + + // The fix under test: DROP CONSTRAINT must precede DROP INDEX (the FK + // depends on the index backing its target column) and both must precede + // DROP COLUMN, in the emitted UP SQL — or the applyRaw below fails + // against the real engine. const idxDropConstraint = emit2.up.indexOf("DROP CONSTRAINT"); + const idxDropIndex = emit2.up.indexOf("DROP INDEX"); const idxDropColumn = emit2.up.indexOf("DROP COLUMN"); expect(idxDropConstraint).toBeGreaterThanOrEqual(0); + expect(idxDropIndex).toBeGreaterThanOrEqual(0); expect(idxDropColumn).toBeGreaterThanOrEqual(0); - expect(idxDropConstraint).toBeLessThan(idxDropColumn); - - // The real-engine gate: pre-fix, this throws - // `error: cannot drop column code of table programs because other - // objects depend on it`. Post-fix, it applies cleanly. + expect(idxDropConstraint).toBeLessThan(idxDropIndex); + expect(idxDropIndex).toBeLessThan(idxDropColumn); + + // The real-engine gate: pre-fix (drop-fk/drop-index tied at the same + // stage, both after drop-column, or drop-index ordered before drop-fk), + // this throws one of: + // `error: cannot drop column code of table programs because other + // objects depend on it` (FK still referencing the column) + // `error: index "uniqueCode" does not exist` (index auto-cascaded away + // by an earlier DROP COLUMN) + // `error: cannot drop index "uniqueCode" because other objects depend + // on it` / `constraint weeks_program_code_fk … depends on index …` + // (index dropped before the FK constraint that depends on it) + // Post-fix, it applies cleanly. await applyRaw(emit2.up); - // Idempotence: re-introspecting must show no drift against expected2 — - // proving the index (never explicitly dropped) is genuinely gone too, - // cascaded away by the column drop, not just skipped. + // Idempotence: re-introspecting must show no drift against expected2. const followup = await diff({ expected: expected2, actual: await introspectPostgres(k), dialect: "postgres" }); if (followup.changes.length > 0) { console.error("ROUND-TRIP FAILURE — a second `meta migrate` would emit:"); diff --git a/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts b/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts index 5486eaf41..cbe5b52ff 100644 --- a/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/emit-postgres.test.ts @@ -253,6 +253,60 @@ describe("renderPostgres — statement ordering", () => { expect(idxAddConstraint).toBeGreaterThanOrEqual(0); expect(idxAddColumn).toBeLessThan(idxAddConstraint); }); + + // #255 (generalized) — dropping a column that has a backing index (e.g. a + // UNIQUE index also targeted by another table's FK) auto-cascades that + // index away in Postgres, so a standalone DROP INDEX emitted AFTER the + // DROP COLUMN fails ("index … does not exist"). drop-index must run before + // drop-column, same as drop-fk/drop-check. + test("drop-index runs before drop-column for the column it indexes (#255)", () => { + const changes: Change[] = [ + { kind: "drop-column", table: "programs", column: "code", status: ALLOWED }, + { kind: "drop-index", table: "programs", index: "uniqueCode", status: ALLOWED }, + ]; + const { up } = emit(changes, { dialect: "postgres" }); + const idxDropIndex = up.indexOf("DROP INDEX"); + const idxDropColumn = up.indexOf("DROP COLUMN"); + expect(idxDropIndex).toBeGreaterThanOrEqual(0); + expect(idxDropColumn).toBeGreaterThanOrEqual(0); + expect(idxDropIndex).toBeLessThan(idxDropColumn); + }); + + // #255 (sub-ordering within the drops group) — an FK constraint depends on + // the unique/PK index backing its target column (that's how Postgres + // enforces the target must be unique), so dropping the index BEFORE the FK + // that depends on it fails: "cannot drop index … because other objects + // depend on it" / "constraint … depends on index …". drop-fk must run + // before drop-index, not just before drop-column. + test("drop-fk runs before drop-index that backs its target column (#255)", () => { + const changes: Change[] = [ + { kind: "drop-index", table: "programs", index: "uniqueCode", status: ALLOWED }, + { kind: "drop-fk", table: "weeks", fk: "weeks_program_code_fk", status: ALLOWED }, + ]; + const { up } = emit(changes, { dialect: "postgres" }); + const idxDropConstraint = up.indexOf("DROP CONSTRAINT"); + const idxDropIndex = up.indexOf("DROP INDEX"); + expect(idxDropConstraint).toBeGreaterThanOrEqual(0); + expect(idxDropIndex).toBeGreaterThanOrEqual(0); + expect(idxDropConstraint).toBeLessThan(idxDropIndex); + }); + + // add-index must still run AFTER column mutation (the indexed column must + // already exist) — only the DROP direction was hoisted. + test("add-index still runs after add-column (adds are unaffected by #255)", () => { + const changes: Change[] = [ + { kind: "add-index", table: "users", + index: { name: "users_phone_idx", columns: ["phone"], unique: false }, + status: ALLOWED }, + { kind: "add-column", table: "users", column: { name: "phone", sqlType: { kind: "text" }, nullable: true }, status: ALLOWED }, + ]; + const { up } = emit(changes, { dialect: "postgres" }); + const idxAddColumn = up.indexOf("ADD COLUMN"); + const idxCreateIndex = up.indexOf("CREATE INDEX"); + expect(idxAddColumn).toBeGreaterThanOrEqual(0); + expect(idxCreateIndex).toBeGreaterThanOrEqual(0); + expect(idxAddColumn).toBeLessThan(idxCreateIndex); + }); }); describe("renderPostgres — down statements", () => { diff --git a/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts b/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts index 0f95f3165..0a20d9b8f 100644 --- a/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/emit-sqlite.test.ts @@ -346,3 +346,41 @@ describe("renderSqlite — add-fk / drop-fk via recreate", () => { expect(idxWeeksRecreate).toBeLessThan(idxProgramsDropColumn); }); }); + +describe("renderSqlite — drop-index vs drop-column (#255 generalized)", () => { + // Unlike drop-fk/drop-check, drop-index is NOT recreate-triggering on + // SQLite — it's a plain native `DROP INDEX` — so, unlike the drop-fk case + // above, the SAME-table ordering IS directly observable here as two + // separate emitted statements. + test("drop-index runs before drop-column for the column it indexes, same table", () => { + const { up } = emit( + [ + { kind: "drop-column", table: "programs", column: "code", status: ALLOWED }, + { kind: "drop-index", table: "programs", index: "uniqueCode", status: ALLOWED }, + ], + { dialect: "sqlite" }, + ); + const idxDropIndex = up.indexOf('DROP INDEX "uniqueCode"'); + const idxDropColumn = up.indexOf('ALTER TABLE "programs" DROP COLUMN "code"'); + expect(idxDropIndex).toBeGreaterThanOrEqual(0); + expect(idxDropColumn).toBeGreaterThanOrEqual(0); + expect(idxDropIndex).toBeLessThan(idxDropColumn); + }); + + // add-index must still run AFTER column mutation — only the DROP direction + // was hoisted. + test("add-index still runs after add-column (adds are unaffected)", () => { + const { up } = emit( + [ + { kind: "add-index", table: "users", index: { name: "users_phone_idx", columns: ["phone"], unique: false }, status: ALLOWED }, + { kind: "add-column", table: "users", column: { name: "phone", sqlType: { kind: "text" }, nullable: true }, status: ALLOWED }, + ], + { dialect: "sqlite" }, + ); + const idxAddColumn = up.indexOf('ADD COLUMN "phone"'); + const idxCreateIndex = up.indexOf("CREATE INDEX"); + expect(idxAddColumn).toBeGreaterThanOrEqual(0); + expect(idxCreateIndex).toBeGreaterThanOrEqual(0); + expect(idxAddColumn).toBeLessThan(idxCreateIndex); + }); +}); From b8b5a935c80ba484e954e65309f5d9bcc62fb74b Mon Sep 17 00:00:00 2001 From: Doug Mealing Date: Sat, 1 Aug 2026 22:21:45 -0400 Subject: [PATCH 3/3] =?UTF-8?q?docs(#255):=20changelog=20=E2=80=94=20migra?= =?UTF-8?q?te=20drop-before-column=20ordering=20fix=20(npm-only)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01HLoJkFSyoticveo5ehMUAr --- CHANGELOG.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b89f4e30a..a3e3cc62e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,26 @@ existing `--allow drop-table` policy, so nothing drops without explicit opt-in. Previously-generated broken `*.queries.ts` / `*.routes.ts` files are **not** auto-pruned (`meta gen` never deletes existing files) — remove them by hand. +### Fixed — migrate emits constraint/index DROPs before DROP COLUMN so referenced-column drops apply (#255) + +**npm-only** (`migrate-ts`; PyPI / NuGet / Maven Central unchanged — schema migrations are +TS-owned, ADR-0015). The SQL emitter's stage ordering ran `DROP COLUMN` before `DROP CONSTRAINT` +(foreign key / check) and `DROP INDEX`, so dropping a column that a still-present foreign key +referenced — or that a still-present index backed — produced an **un-appliable** migration +(`cannot drop column … because other objects depend on it`). Both the Postgres and SQLite +emitters now hoist every constraint/index drop ahead of column mutation: `drop-fk`/`drop-check` +first, then `drop-index` (a foreign key depends on the unique/PK index backing its target, so the +FK must be dropped before that index), then the column ops; the matching adds +(`add-fk`/`add-check`/`add-index`) stay after column mutation (they reference columns that must +already exist). Reproduced against a real Postgres before fixing (emit → apply → introspect → +re-diff-empty), including the combined FK + backing-index + column-drop case. + +Byte-identical for any migration that contains none of `drop-fk` / `drop-check` / `drop-index`. A +migration that combines one of those drops with a column change now emits the same statements in +the corrected order — **re-review any committed-but-not-yet-applied migration file** that drops a +foreign key / check / index alongside a column, since its statement order changes (and will now +apply where it previously failed). + ## [0.20.9] — 2026-07-28 **npm-only** — `migrate-ts` + `codegen-ts` (schema migrations and projection-view codegen