diff --git a/server/typescript/packages/migrate-ts/src/diff/index.ts b/server/typescript/packages/migrate-ts/src/diff/index.ts index 823cf18c1..3d9b06a43 100644 --- a/server/typescript/packages/migrate-ts/src/diff/index.ts +++ b/server/typescript/packages/migrate-ts/src/diff/index.ts @@ -13,6 +13,7 @@ import { detectColumnRenames, detectTableRenames } from "./rename-heuristic.js"; import { viewSqlEquals } from "../view-sql-compare.js"; import { viewReplaceIsLegal } from "../view-column-types.js"; import { checkExprEquals, normalizeCheckExpr } from "../check-expr-compare.js"; +import { isPgAutoSequenceDefault } from "../pg-identity-default.js"; import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; export interface DiffArgs { @@ -390,8 +391,26 @@ function diffTableColumns( // metadata changes. On SQLite/D1 the false positive is destructive rather than // merely noisy: there is no ALTER COLUMN, so the recreate-and-copy path rebuilds // the WHOLE table. Identity-driven values are not ordinary defaults — don't diff - // them (`increment` gets this implicitly: an AUTOINCREMENT column has no DEFAULT). - if (ec.identity !== "uuid" && !columnDefaultsEqual(ec.default, ac.default)) { + // them. + // + // `increment` is NOT automatically the same case: SQLite AUTOINCREMENT and a + // modern Postgres `GENERATED ... AS IDENTITY` column genuinely carry no DEFAULT, + // but a legacy Postgres `serial`/`bigserial` column is historical sugar for + // `integer` + a sequence + a REAL `DEFAULT nextval(...)` clause — introspection + // correctly reads that back as a live default even though the expected side + // correctly declares none. Left diffed, that surfaced as + // `ALTER COLUMN … DROP DEFAULT` with no replacement generation mechanism — + // destructive against a live table, since every insert that doesn't supply the + // PK explicitly then starts failing. So an `increment` PK skips the default-diff + // ONLY when the live default is that exact auto-sequence shape + // (isPgAutoSequenceDefault, shared with the introspector that already recognizes + // it) — a genuinely wrong, non-sequence default on an increment PK still reports + // as drift. + const liveIsAutoSequenceDefault = + ac.default !== undefined && ac.default.kind === "expr" && isPgAutoSequenceDefault(ac.default.value); + const skipIdentityDefaultDiff = + ec.identity === "uuid" || (ec.identity === "increment" && liveIsAutoSequenceDefault); + if (!skipIdentityDefaultDiff && !columnDefaultsEqual(ec.default, ac.default)) { const change: Change = { kind: "change-column-default", table, ...sx, column: name, status: ALLOWED, diff --git a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts index 062150616..19cb63544 100644 --- a/server/typescript/packages/migrate-ts/src/introspect/postgres.ts +++ b/server/typescript/packages/migrate-ts/src/introspect/postgres.ts @@ -35,6 +35,7 @@ import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; import { parseFingerprintMarker } from "../view-fingerprint.js"; import { MIGRATIONS_TABLE } from "../apply/ledger.js"; import { stripCheckWrapper } from "../check-expr-compare.js"; +import { isPgAutoSequenceDefault } from "../pg-identity-default.js"; // --------------------------------------------------------------------------- // Public API @@ -460,10 +461,11 @@ async function readColumns(k: Kysely, schema: string, tableName: string): P if (def !== undefined) col.default = def; // Detect auto-increment (sequence) columns — real PG surfaces bigserial / - // serial as nextval(...) in column_default. We also check udt_name for + // serial as nextval(...) in column_default (isPgAutoSequenceDefault, shared + // with the diff layer's identity-default guard). We also check udt_name for // explicit serial type names as a belt-and-suspenders guard. const isSerial = - (r.column_default !== null && /^nextval\(/i.test(r.column_default)) || + isPgAutoSequenceDefault(r.column_default) || /^(?:bigserial|serial8|serial4|serial|smallserial|serial2)$/i.test(r.udt_name); if (isSerial) col.identity = "increment"; diff --git a/server/typescript/packages/migrate-ts/src/pg-identity-default.ts b/server/typescript/packages/migrate-ts/src/pg-identity-default.ts new file mode 100644 index 000000000..1429b80f9 --- /dev/null +++ b/server/typescript/packages/migrate-ts/src/pg-identity-default.ts @@ -0,0 +1,19 @@ +// src/pg-identity-default.ts +// +// Postgres "auto-sequence" column DEFAULT — the `nextval(''::regclass)` +// expression a legacy `serial` / `bigserial` / `smallserial` column carries. +// `serial` is historical sugar for `integer` + a sequence + a genuine DEFAULT +// clause — unlike SQLite AUTOINCREMENT or a modern Postgres +// `GENERATED ... AS IDENTITY` column, neither of which surfaces a DEFAULT at all. +// Recognizing this exact shape lets both sides of the pipeline treat a live +// `serial` column as already satisfying `identity: "increment"`, instead of +// reporting its physical default as drift. +// +// Shared between introspection (introspect/postgres.ts, which also cross-checks +// udt_name for the serial type family) and the diff layer (diff/index.ts, which +// only sees the already-parsed default expression and must recognize this same +// shape to avoid proposing a destructive `ALTER COLUMN … DROP DEFAULT` for a live +// `serial` PK with no replacement generation mechanism). +export function isPgAutoSequenceDefault(raw: string | null | undefined): boolean { + return raw !== null && raw !== undefined && /^nextval\(/i.test(raw); +} diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-serial-identity-adoption.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-serial-identity-adoption.test.ts new file mode 100644 index 000000000..466ff10b5 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-serial-identity-adoption.test.ts @@ -0,0 +1,154 @@ +/** + * Real-Postgres gate for the "adopting a legacy `serial` PK" bug. + * + * Adopting MetaObjects metadata onto an EXISTING Postgres table whose PK was + * created as `serial` (Drizzle's `serial().primaryKey()`, Prisma's + * `autoincrement()`, Rails, SQLAlchemy, or plain `id SERIAL PRIMARY KEY` — the + * single most common pre-adoption shape) made `meta migrate --from-db` propose: + * + * ALTER TABLE "work_item" ALTER COLUMN "id" DROP DEFAULT; + * + * with NO replacement generation mechanism emitted in the same migration. + * Applying it leaves `id` NOT NULL with nothing to populate it, so every insert + * that does not explicitly supply `id` starts failing — destructive against a + * live table, and it fires on the first thing a new adopter does. + * + * A unit-level diff assertion on hand-built snapshots (diff-serial-identity- + * default.test.ts) is not sufficient evidence that a REAL Postgres `serial` + * column introspects into the exact shape the guard recognizes — this test + * proves the whole pipeline against a live engine: create a real `SERIAL + * PRIMARY KEY` table → introspect → diff against metadata declaring + * `identity.primary @generation: increment` → the diff must NOT propose + * DROP DEFAULT for `id` → apply whatever it DOES emit → re-introspect and + * re-diff, which MUST come back empty (idempotence) → insert a row WITHOUT + * supplying `id` and confirm it succeeds and auto-populates (the actual + * value-semantics the bug broke). + * + * Gated on MIGRATE_TS_PG_URL like every other pg integration test here; skips + * cleanly when unset. Reported against an adopting project. + */ + +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; + +// Metadata for the table an adopter would write on day one: the existing live +// `id` is `SERIAL PRIMARY KEY` (below), modeled as `field.int` + +// `identity.primary @generation: increment` — no `@default` (identity +// generation is modeled via `identity`, never `default`). `notes` does NOT +// exist on the live table yet, so the diff has a genuine `add-column` to +// apply — proving the round-trip does real work, not just a trivial no-op. +const META = JSON.stringify({ + "metadata.root": { + package: "acme", + children: [ + { + "object.entity": { + name: "WorkItem", + children: [ + { "source.rdb": { "@table": "work_item" } }, + { "field.int": { name: "id" } }, + { "field.string": { name: "title", "@required": true } }, + { "field.string": { name: "notes" } }, + { "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 "work_item" CASCADE`).execute(k); +} + +async function loadRoot(json: string) { + return (await new MetaDataLoader().load([new InMemoryStringSource(json)])).root; +} + +async function applyRaw(sqlText: string): Promise { + for (const stmt of sqlText.split(";").map((s) => s.trim()).filter((s) => s.length > 0)) { + await sql.raw(stmt).execute(k); + } +} + +realDescribe("PG — adopting a legacy `serial` PK does not drop its default", () => { + test("diff has no DROP DEFAULT for id; apply → re-diff empty; id-less insert succeeds", async () => { + await cleanup(); + + // A pre-existing table exactly as Drizzle's `serial().primaryKey()` (or plain + // `id SERIAL PRIMARY KEY`) would have created it, BEFORE MetaObjects adoption. + await sql + .raw(`CREATE TABLE "work_item" ("id" SERIAL PRIMARY KEY, "title" text NOT NULL)`) + .execute(k); + + const root = await loadRoot(META); + const expected = buildExpectedSchema(root, { dialect: "postgres" }); + const actual = await introspectPostgres(k); + + // Confirm the premise: introspection reads the live column back as a real + // `expr` default carrying the sequence's nextval(...) — proving the guard is + // exercised against the true shape a live `serial` column produces, not a + // hand-typed stand-in. + const liveWorkItem = actual.tables.find((t) => t.name === "work_item"); + const liveId = liveWorkItem?.columns.find((c) => c.name === "id"); + expect(liveId?.identity).toBe("increment"); + expect(liveId?.default?.kind).toBe("expr"); + expect(liveId?.default?.value).toMatch(/^nextval\(/i); + + const result = await diff({ expected, actual, dialect: "postgres" }); + + // The bug: this used to contain `change-column-default` for `id`, which the + // postgres emitter renders as `ALTER COLUMN "id" DROP DEFAULT` with nothing to + // replace it — the assertion that would have caught it BEFORE the fix. + const idDefaultChange = result.changes.find( + (c) => c.kind === "change-column-default" && c.column === "id", + ); + expect(idDefaultChange).toBeUndefined(); + + // Real adoption work remains: metadata declares `notes`, which the live table + // doesn't have yet. + expect(result.changes.some((c) => c.kind === "add-column" && c.column.name === "notes")).toBe(true); + + // Apply whatever the diff DID emit. + const emitted = emit(result.changes, { dialect: "postgres" }); + await applyRaw(emitted.up); + + // Idempotence: re-introspect + re-diff must come back COMPLETELY empty. + const actualAfter = await introspectPostgres(k); + const resultAfter = await diff({ expected, actual: actualAfter, dialect: "postgres" }); + expect(resultAfter.changes).toEqual([]); + + // The value-semantics half of the gate — the thing the bug actually broke: + // an insert that does NOT supply `id` must succeed and auto-populate it. + const inserted = await sql<{ id: number; title: string }>` + INSERT INTO "work_item" ("title") VALUES (${"a task with no explicit id"}) + RETURNING "id", "title" + `.execute(k); + expect(inserted.rows).toHaveLength(1); + expect(inserted.rows[0]?.id).toBeGreaterThan(0); + expect(inserted.rows[0]?.title).toBe("a task with no explicit id"); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/unit/diff-serial-identity-default.test.ts b/server/typescript/packages/migrate-ts/test/unit/diff-serial-identity-default.test.ts new file mode 100644 index 000000000..5a596bff5 --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/unit/diff-serial-identity-default.test.ts @@ -0,0 +1,99 @@ +/** + * Adopting MetaObjects metadata onto an EXISTING Postgres table whose PK was + * created as `serial` (Drizzle's `serial().primaryKey()`, Prisma's + * `autoincrement()`, Rails, SQLAlchemy, or plain `id SERIAL PRIMARY KEY` — the + * single most common pre-adoption shape) made `meta migrate --from-db` propose: + * + * ALTER TABLE "work_item" ALTER COLUMN "id" DROP DEFAULT; + * + * with NO replacement generation mechanism emitted in the same migration. + * Applying it leaves `id` NOT NULL with nothing to populate it, so every insert + * that does not explicitly supply `id` starts failing — destructive against a + * live table, and it fires on the first thing a new adopter does. + * + * Root cause: the expected side (identity: "increment") deliberately declares + * no `@default` (identity generation is modeled via `identity`, not `default` — + * see the sibling uuid comment in diff/index.ts), but a live legacy Postgres + * `serial` column IS backed by a genuine `DEFAULT nextval(...)` clause — unlike + * SQLite AUTOINCREMENT or a modern `GENERATED ... AS IDENTITY` column, neither of + * which has any DEFAULT at all. The default-diff guard was blind to that + * distinction and reported the live `nextval(...)` default as drift. + * + * Reported against an adopting project. + */ +import { test, expect, describe } from "bun:test"; +import { diff } from "../../src/diff/index.js"; +import type { ColumnDescriptor, SchemaSnapshot } from "../../src/types.js"; + +function snap(col: ColumnDescriptor): SchemaSnapshot { + return { + tables: [{ name: "work_item", columns: [col], indexes: [], foreignKeys: [], primaryKey: ["id"], checks: [] }], + views: [], + }; +} + +const expectedIncrementPk: ColumnDescriptor = { + name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false, identity: "increment", +}; + +async function defaultChanges(expected: ColumnDescriptor, actual: ColumnDescriptor) { + const r = await diff(snap(expected), snap(actual), { dialect: "postgres" }); + return r.changes.filter((c) => c.kind === "change-column-default"); +} + +describe("diff — legacy Postgres serial PK default is not diffed as drift", () => { + test("live nextval(...) default on an increment PK: no change-column-default (no DROP DEFAULT)", async () => { + const actual: ColumnDescriptor = { + ...expectedIncrementPk, + default: { kind: "expr", value: "nextval('work_item_id_seq'::regclass)" }, + }; + const changes = await defaultChanges(expectedIncrementPk, actual); + expect(changes).toEqual([]); + }); + + test("an already-adopted serial-PK table is a total no-op (the real-world symptom)", async () => { + const actual: ColumnDescriptor = { + ...expectedIncrementPk, + default: { kind: "expr", value: "nextval('work_item_id_seq'::regclass)" }, + }; + const r = await diff(snap(expectedIncrementPk), snap(actual), { dialect: "postgres" }); + expect(r.changes).toEqual([]); + }); + + test("qualified sequence name (schema-qualified nextval target) is still recognized", async () => { + const actual: ColumnDescriptor = { + ...expectedIncrementPk, + default: { kind: "expr", value: "nextval('public.work_item_id_seq'::regclass)" }, + }; + const changes = await defaultChanges(expectedIncrementPk, actual); + expect(changes).toEqual([]); + }); + + test("regression guard: a genuinely wrong NON-sequence default on an increment PK is still reported", async () => { + const actual: ColumnDescriptor = { + ...expectedIncrementPk, + default: { kind: "literal", value: "0" }, + }; + const changes = await defaultChanges(expectedIncrementPk, actual); + expect(changes).toHaveLength(1); + expect(changes[0]).toMatchObject({ + kind: "change-column-default", + column: "id", + from: { kind: "literal", value: "0" }, + }); + }); + + test("regression guard: a wrong FUNCTION-CALL default (not nextval) on an increment PK is still reported", async () => { + const actual: ColumnDescriptor = { + ...expectedIncrementPk, + default: { kind: "expr", value: "some_other_function()" }, + }; + const changes = await defaultChanges(expectedIncrementPk, actual); + expect(changes).toHaveLength(1); + }); + + test("no-churn: increment PK with no live default at all (SQLite AUTOINCREMENT / modern IDENTITY shape) still no-ops", async () => { + const changes = await defaultChanges(expectedIncrementPk, expectedIncrementPk); + expect(changes).toEqual([]); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/unit/diff-uuid-identity-default.test.ts b/server/typescript/packages/migrate-ts/test/unit/diff-uuid-identity-default.test.ts index 33f790195..6b2ea2b06 100644 --- a/server/typescript/packages/migrate-ts/test/unit/diff-uuid-identity-default.test.ts +++ b/server/typescript/packages/migrate-ts/test/unit/diff-uuid-identity-default.test.ts @@ -53,7 +53,7 @@ describe("diff — uuid-identity PK default is not diffed", () => { expect(r.changes).toEqual([]); }); - test("regression guard: increment-identity PK still no-ops (it never had a DEFAULT)", async () => { + test("regression guard: increment-identity PK still no-ops (no live default at all)", async () => { const inc: ColumnDescriptor = { name: "id", sqlType: { kind: "integer", bits: 64 }, nullable: false, identity: "increment", };