diff --git a/docs/features/migrations-and-drift.md b/docs/features/migrations-and-drift.md index 30998f26d..bcdd2f554 100644 --- a/docs/features/migrations-and-drift.md +++ b/docs/features/migrations-and-drift.md @@ -123,6 +123,21 @@ preserves the PK through `RENAME COLUMN`) is not mistaken for a move. The read-o primary-key drift rather than throwing. Auto-migrating the move (adding the `add-primary-key` / `drop-primary-key` change kinds) is a documented future follow-up. +#### A legacy `serial` primary key (adoption-time refusal) + +When migrating against a live Postgres database whose primary key is a legacy +`serial` / `bigserial` column — one carrying a live `nextval(...)` default — and the +metadata declares that `identity.primary` **without** `@generation`, the diff would +otherwise emit `ALTER COLUMN … DROP DEFAULT`. That is destructive: every insert that +omits the id starts failing. The missing `@generation` is genuinely ambiguous — it reads +identically whether the author simply never declared it (and wants to keep +auto-increment) or deliberately dropped it (to move the column onto app-assigned ids) — +so `meta migrate` **refuses rather than guessing**, the same detect-and-refuse arc as the +primary-key move above. Declare `@generation: increment` on the identity to keep the +sequence, or pass `--allow drop-identity-default` if removing auto-increment is +intentional. An identity that *does* declare `@generation: increment` never reaches this +gate (its default diff is skipped), so only the undeclared case fires. + ### Java Schema migrations for Java projects are owned by the **TypeScript toolchain** diff --git a/server/typescript/packages/cli/README.md b/server/typescript/packages/cli/README.md index 1a5da3f97..53123e326 100644 --- a/server/typescript/packages/cli/README.md +++ b/server/typescript/packages/cli/README.md @@ -143,7 +143,7 @@ Flags: - `--dialect sqlite|postgres|d1` — auto-detected from URL scheme; use `d1` for Cloudflare D1 - `--out-dir ` (default `./.metaobjects/migrations`) - `--slug ` — required when changes are pending (e.g., `add-user-shipping`) -- `--allow ` — destructive-change permissions: `drop-column,drop-table,type-change,drop-index,drop-fk,drop-check,drop-view,nullable-to-not-null` +- `--allow ` — destructive-change permissions: `drop-column,drop-table,type-change,drop-index,drop-fk,drop-check,drop-view,drop-view-cascade,adopt-view,nullable-to-not-null,drop-identity-default` - `--on-ambiguous abort|rename|drop-add` (default `abort`) — non-interactive - `--dry-run` — print SQL pair to stdout, write nothing - `--apply` — after writing migration files, immediately apply all pending migrations against the DB (runs `up.sql` for each unapplied entry, tracked in the migration ledger). Mutually exclusive with `--rollback`. Postgres and SQLite only (D1 uses `--apply` to invoke `wrangler d1 migrations apply` instead). diff --git a/server/typescript/packages/cli/src/commands/migrate.ts b/server/typescript/packages/cli/src/commands/migrate.ts index ba083e5bc..41c0a9dad 100644 --- a/server/typescript/packages/cli/src/commands/migrate.ts +++ b/server/typescript/packages/cli/src/commands/migrate.ts @@ -81,7 +81,7 @@ MIGRATE FLAGS: --allow Comma-separated destructive-change permissions: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, - adopt-view,nullable-to-not-null + adopt-view,nullable-to-not-null,drop-identity-default --on-ambiguous abort|rename|drop-add How to handle ambiguous renames (default: abort) --from-db Introspect live DB instead of using the committed snapshot @@ -204,6 +204,7 @@ function allowFlagFor(kind: string): string { case "drop-fk": return "drop-fk"; case "change-column-type": return "type-change"; case "change-column-nullable": return "nullable-to-not-null"; + case "change-column-default": return "drop-identity-default"; default: return kind; } } diff --git a/server/typescript/packages/cli/src/index.ts b/server/typescript/packages/cli/src/index.ts index 69fe9c7e3..dac469d0d 100644 --- a/server/typescript/packages/cli/src/index.ts +++ b/server/typescript/packages/cli/src/index.ts @@ -76,7 +76,7 @@ MIGRATE FLAGS: --allow Comma-separated destructive-change permissions: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, - adopt-view,nullable-to-not-null + adopt-view,nullable-to-not-null,drop-identity-default --on-ambiguous abort|rename|drop-add Default abort --d1 D1 binding name from wrangler.toml (only with --dialect d1) --remote Target remote D1 instead of local (only with --dialect d1) diff --git a/server/typescript/packages/cli/src/lib/allow.ts b/server/typescript/packages/cli/src/lib/allow.ts index 4bbe9764b..947606f0f 100644 --- a/server/typescript/packages/cli/src/lib/allow.ts +++ b/server/typescript/packages/cli/src/lib/allow.ts @@ -5,7 +5,12 @@ import type { AllowOptions, Change } from "@metaobjectsdev/migrate-ts"; // Map CLI allow tokens → migrate-ts AllowOptions field names. -const ALLOW_TOKEN_MAP: Record = { +// Exported (not just module-local) so allow-tokens-pinned.test.ts can pin its +// key set against ALLOW_TOKENS (args.ts). ALLOW_TOKENS is the *validator* — +// this map is what actually *grants* the permission; a token present in +// ALLOW_TOKENS but missing here would pass validation and silently grant +// nothing, on a destructive operation. +export const ALLOW_TOKEN_MAP: Record = { "drop-column": "dropColumn", "drop-table": "dropTable", "type-change": "typeChange", @@ -23,6 +28,11 @@ const ALLOW_TOKEN_MAP: Record = { // Gates overwriting an unfingerprinted (hand-written or pre-fingerprint) view. "adopt-view": "adoptView", "nullable-to-not-null": "nullableToNotNull", + // Gates dropping a live Postgres auto-sequence default (a legacy `serial`/ + // `bigserial` PK's `nextval(...)`) when the metadata declares no + // @generation at all — ambiguous between "never declared it" and + // "deliberately removing auto-increment", so migrate refuses without it. + "drop-identity-default": "dropIdentityDefault", }; /** Translate parsed `--allow` tokens into the migrate-ts `AllowOptions` shape. */ diff --git a/server/typescript/packages/cli/src/lib/args.ts b/server/typescript/packages/cli/src/lib/args.ts index 8448c4611..4c232a66c 100644 --- a/server/typescript/packages/cli/src/lib/args.ts +++ b/server/typescript/packages/cli/src/lib/args.ts @@ -165,7 +165,12 @@ type Dialect = (typeof DIALECTS)[number]; export const MIGRATE_FORMATS = ["default", "flyway"] as const; export type MigrateFormat = (typeof MIGRATE_FORMATS)[number]; -const ALLOW_TOKENS = [ +// Exported (not just module-local) so allow-tokens-pinned.test.ts can pin it +// against sdk's AllowTokenEnum (config.json's static migrate.allow validator) +// — the two lists drifted silently before that test existed: sdk's enum was +// missing 5 of these 11 tokens, so a token that worked fine on the CLI was +// REJECTED when set in .metaobjects/config.json. +export const ALLOW_TOKENS = [ "drop-column", "drop-table", "type-change", @@ -187,6 +192,11 @@ const ALLOW_TOKENS = [ // toolchain needs this exactly once, to stamp its existing views. "adopt-view", "nullable-to-not-null", + // drop-identity-default permits dropping a live Postgres auto-sequence + // default (a legacy `serial`/`bigserial` PK's `nextval(...)`) when the + // metadata declares no @generation at all — ambiguous between "never + // declared it" and "deliberately removing auto-increment". + "drop-identity-default", ] as const; type AllowToken = (typeof ALLOW_TOKENS)[number]; diff --git a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap index 2338ff591..6e6c6a66c 100644 --- a/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap +++ b/server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap @@ -70,7 +70,7 @@ MIGRATE FLAGS: --allow Comma-separated destructive-change permissions: drop-column,drop-table,type-change,drop-index,drop-fk, drop-check,drop-view,drop-view-cascade, - adopt-view,nullable-to-not-null + adopt-view,nullable-to-not-null,drop-identity-default --on-ambiguous abort|rename|drop-add Default abort --d1 D1 binding name from wrangler.toml (only with --dialect d1) --remote Target remote D1 instead of local (only with --dialect d1) diff --git a/server/typescript/packages/cli/test/unit/allow-tokens-pinned.test.ts b/server/typescript/packages/cli/test/unit/allow-tokens-pinned.test.ts new file mode 100644 index 000000000..3f71d36ef --- /dev/null +++ b/server/typescript/packages/cli/test/unit/allow-tokens-pinned.test.ts @@ -0,0 +1,73 @@ +/** + * Drift guard across the three token-bearing structures behind `--allow`: + * + * - `ALLOW_TOKENS` (`lib/args.ts`) — the CLI's authoritative list; what + * actually VALIDATES `--allow `. + * - `AllowTokenEnum` (`sdk`'s `config.ts`) — validates the STATIC + * `migrate.allow` array in `.metaobjects/config.json`. + * - `ALLOW_TOKEN_MAP` (`lib/allow.ts`) — what actually GRANTS the permission, + * translating a validated token into the `AllowOptions` field `diff()` + * reads. + * + * `ALLOW_TOKENS` and `AllowTokenEnum` silently drifted before this test + * existed: sdk's enum had only 6 of the 11 real tokens, missing + * `drop-check`, `drop-view`, `drop-view-cascade`, `adopt-view` and + * `drop-identity-default`. A user who set any of those five in + * `.metaobjects/config.json`'s `migrate.allow` got a schema rejection for a + * flag the CLI itself accepted fine on the command line — `adopt-view` had + * shipped since 0.20.4 and was affected the whole time. + * + * `ALLOW_TOKEN_MAP` is a distinct, worse failure mode if it drifts from + * `ALLOW_TOKENS`: a token present in `ALLOW_TOKENS` (and `AllowTokenEnum`) + * but missing from the map passes validation cleanly and then + * `tokensToAllowOptions` silently grants NOTHING for it — the user believes + * `--allow ` authorized a destructive drop; it didn't, and the diff + * blocks it anyway with no indication the flag was ever a no-op. That is a + * silent-failure mode on exactly the path this whole feature exists to + * protect. + * + * Import ALL of these rather than hardcoding a fourth "expected" list here — + * a hardcoded list would just be a fifth copy that can itself drift. + * + * Package-dependency direction: `cli` depends on `sdk` (`workspace:*`), not + * the other way around, so this test can only live in `cli` — `sdk` cannot + * import from `cli` without introducing a cycle. `sdk`'s `AllowTokenEnum` + * itself carries a doc comment pointing back at this test as the drift guard, + * since `sdk` has no test that can perform the comparison from its own side. + */ +import { test, expect, describe } from "bun:test"; +import { ALLOW_TOKENS } from "../../src/lib/args.js"; +import { ALLOW_TOKEN_MAP } from "../../src/lib/allow.js"; +import { AllowTokenEnum } from "@metaobjectsdev/sdk"; + +describe("--allow token lists stay pinned across packages", () => { + test("sdk's AllowTokenEnum and the CLI's ALLOW_TOKENS validate the exact same token set", () => { + const cliTokens = new Set(ALLOW_TOKENS); + const sdkTokens = new Set(AllowTokenEnum.options); + + const missingFromSdk = [...cliTokens].filter((t) => !sdkTokens.has(t)); + const missingFromCli = [...sdkTokens].filter((t) => !cliTokens.has(t)); + + expect(missingFromSdk).toEqual([]); + expect(missingFromCli).toEqual([]); + expect(sdkTokens.size).toBe(cliTokens.size); + }); + + test("ALLOW_TOKEN_MAP grants a permission for every validated token, and nothing extra", () => { + const cliTokens = new Set(ALLOW_TOKENS); + const mapKeys = new Set(Object.keys(ALLOW_TOKEN_MAP)); + + const validatedButNotGranted = [...cliTokens].filter((t) => !mapKeys.has(t)); + const grantedButNotValidated = [...mapKeys].filter((t) => !cliTokens.has(t)); + + expect(validatedButNotGranted).toEqual([]); + expect(grantedButNotValidated).toEqual([]); + expect(mapKeys.size).toBe(cliTokens.size); + }); + + test("ALLOW_TOKEN_MAP's AllowOptions fields are unique — no two tokens grant the same permission", () => { + const fields = Object.values(ALLOW_TOKEN_MAP); + const uniqueFields = new Set(fields); + expect(uniqueFields.size).toBe(fields.length); + }); +}); diff --git a/server/typescript/packages/migrate-ts/src/diff/status.ts b/server/typescript/packages/migrate-ts/src/diff/status.ts index 023197e53..fdafbd906 100644 --- a/server/typescript/packages/migrate-ts/src/diff/status.ts +++ b/server/typescript/packages/migrate-ts/src/diff/status.ts @@ -1,5 +1,6 @@ import type { Change, AllowOptions } from "../types.js"; import { isWidening } from "../sql-type.js"; +import { isPgAutoSequenceDefault } from "../pg-identity-default.js"; import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata"; /** @@ -111,12 +112,42 @@ function blockedReasonFor( if (c.from === false && c.to === true) return null; return allow.nullableToNotNull ? null : "nullable→notnull requires existing data to satisfy (pass allow.nullableToNotNull)"; + case "change-column-default": { + // Ordinary default changes (literal→literal, adding/removing a plain + // literal default, etc.) stay allowed unconditionally — only ONE narrow + // shape is gated here. `to === undefined` means the default is being + // DROPPED outright (see the ColumnDefault comment in types.ts), and + // when what's being dropped is a live Postgres auto-sequence default + // (`nextval(...)`, the shape a legacy `serial`/`bigserial` PK carries — + // isPgAutoSequenceDefault, shared with diff/index.ts and the + // introspector), reaching this point means the expected side declared + // NO identity at all: an `identity: "increment"` expected column never + // gets here, because diff/index.ts's skipIdentityDefaultDiff already + // suppressed the change for that exact live shape. So an undeclared + // `@generation` is the ONLY way this branch fires — and that silence is + // ambiguous (never-declared vs. deliberately-removed), so ask rather + // than guess. Anything else about change-column-default — including + // dropping a plain literal default — falls through to the unconditional + // `return null` below. + const droppingAutoSequence = + c.to === undefined && c.from?.kind === "expr" && isPgAutoSequenceDefault(c.from.value); + if (droppingAutoSequence && !allow.dropIdentityDefault) { + return `column "${c.table}"."${c.column}" has a live Postgres auto-increment default ` + + `(${c.from!.value}) but its metadata declares no @generation — this is ambiguous: it ` + + `could mean @generation was never declared, or that auto-increment is being removed on ` + + `purpose. Dropping the default is destructive (every insert that omits the column starts ` + + `failing), so this refuses rather than guessing. Declare @generation: increment on the ` + + `identity to keep the sequence, or pass --allow drop-identity-default if removing it is ` + + `intentional`; + } + return null; + } + // Always-allowed kinds case "create-table": case "rename-table": case "add-column": case "rename-column": - case "change-column-default": case "add-index": case "add-fk": case "add-check": diff --git a/server/typescript/packages/migrate-ts/src/types.ts b/server/typescript/packages/migrate-ts/src/types.ts index 7f15f762d..1c998401b 100644 --- a/server/typescript/packages/migrate-ts/src/types.ts +++ b/server/typescript/packages/migrate-ts/src/types.ts @@ -315,6 +315,21 @@ export interface AllowOptions { adoptView?: boolean; /** Existing data must satisfy NOT NULL; diff cannot verify this. */ nullableToNotNull?: boolean; + /** + * Gates dropping a live Postgres auto-sequence DEFAULT (the `nextval(...)` + * shape a legacy `serial`/`bigserial` column carries — see + * pg-identity-default.ts) when the expected side declares NO identity at + * all, i.e. `@generation` was never set. That silence is genuinely + * ambiguous: it reads identically whether the author simply never got + * around to declaring `@generation: increment`, or deliberately dropped it + * to move the column off auto-increment (e.g. onto app-assigned ULIDs). + * The diff cannot tell those apart, so it refuses instead of guessing — + * this flag is how the author confirms the second reading and lets the + * DROP DEFAULT through. (An expected side that DOES declare + * `@generation: increment` never reaches this gate at all — diff/index.ts + * skips the default-diff for a live auto-sequence default entirely.) + */ + dropIdentityDefault?: boolean; } export type AmbiguousChange = diff --git a/server/typescript/packages/migrate-ts/test/integration/pg-identity-no-generation-refuse.test.ts b/server/typescript/packages/migrate-ts/test/integration/pg-identity-no-generation-refuse.test.ts new file mode 100644 index 000000000..34d4c774c --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/integration/pg-identity-no-generation-refuse.test.ts @@ -0,0 +1,177 @@ +/** + * Real-Postgres gate for the "undeclared @generation against a live serial PK" + * gap left open by #279. + * + * #279 (see pg-serial-identity-adoption.test.ts) stopped `meta migrate` from + * proposing a destructive `ALTER COLUMN "id" DROP DEFAULT` against a live + * legacy `serial` PK — but ONLY when the metadata explicitly declares + * `identity.primary @generation: increment`. `buildExpectedSchema` sets + * `ColumnDescriptor.identity` exclusively from a declared `@generation` + * (expected-schema.ts) — there is no default — so an adopter who writes + * `identity.primary` with NO `@generation` against a live `serial` PK gets + * `ec.identity === undefined`, the #279 guard never engages, and the same + * destructive drop reaches the diff as an ordinary, ALLOWED + * `change-column-default`. + * + * We deliberately do NOT widen the #279 guard to key off the LIVE column + * instead ("if it's serial, never touch its default") — that would silently + * refuse a DELIBERATE migration off auto-increment (e.g. moving to + * app-assigned ULIDs, which drops `@generation` precisely because the author + * wants the sequence gone). "No @generation declared" is genuinely ambiguous + * between "never declared it" and "deliberately removing it", so instead of + * guessing, migrate refuses and asks — same shape as #258 (refuse a PK move + * rather than emit an un-appliable migration). + * + * A unit-level diff assertion on hand-built snapshots + * (diff-status-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` WITHOUT `@generation` → the change + * must be BLOCKED, naming both remedies → confirm `--allow + * drop-identity-default` (allow.dropIdentityDefault) lets the DROP DEFAULT + * through and it actually applies. + * + * Gated on MIGRATE_TS_PG_URL like every other pg integration test here; 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; + +// Same live pre-adoption shape as pg-serial-identity-adoption.test.ts, but the +// metadata here deliberately OMITS `@generation` — the ambiguous case. +const META_NO_GENERATION = JSON.stringify({ + "metadata.root": { + package: "acme", + children: [ + { + "object.entity": { + name: "Task", + children: [ + { "source.rdb": { "@table": "task" } }, + { "field.int": { name: "id" } }, + { "field.string": { name: "title", "@required": true } }, + { "identity.primary": { name: "pk", "@fields": ["id"] } }, + ], + }, + }, + ], + }, +}); + +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 "task" CASCADE`).execute(k); +} + +async function loadRoot(json: string) { + return (await new MetaDataLoader().load([new InMemoryStringSource(json)])).root; +} + +async function createLiveSerialTable(): Promise { + await cleanup(); + await sql + .raw(`CREATE TABLE "task" ("id" SERIAL PRIMARY KEY, "title" text NOT NULL)`) + .execute(k); +} + +realDescribe("PG — undeclared @generation against a live serial PK refuses, not guesses", () => { + test("diff blocks change-column-default on id, naming both remedies", async () => { + await createLiveSerialTable(); + + const root = await loadRoot(META_NO_GENERATION); + const expected = buildExpectedSchema(root, { dialect: "postgres" }); + const actual = await introspectPostgres(k); + + // Confirm the premise: the expected side declares NO identity at all (no + // @generation was written), while the live column really does carry an + // auto-sequence default — proving the ambiguous case is exercised against + // the true shape a live `serial` column produces. + const expectedId = expected.tables.find((t) => t.name === "task")?.columns.find((c) => c.name === "id"); + expect(expectedId?.identity).toBeUndefined(); + const liveId = actual.tables.find((t) => t.name === "task")?.columns.find((c) => c.name === "id"); + expect(liveId?.identity).toBe("increment"); + expect(liveId?.default?.value).toMatch(/^nextval\(/i); + + const result = await diff({ expected, actual, dialect: "postgres" }); + + const idDefaultChange = result.changes.find( + (c) => c.kind === "change-column-default" && c.column === "id", + ); + expect(idDefaultChange).toBeDefined(); + expect(idDefaultChange!.status.state).toBe("blocked"); + expect(result.blocked).toContain(idDefaultChange!); + + const reason = idDefaultChange!.status.blockedReason ?? ""; + expect(reason).toContain("task"); + expect(reason).toContain("id"); + expect(reason).toContain("@generation: increment"); + expect(reason).toContain("--allow drop-identity-default"); + + // emit() must refuse to hand back applicable SQL for a blocked diff. + expect(() => emit(result.changes, { dialect: "postgres" })).toThrow(); + }); + + test("allow.dropIdentityDefault lets the DROP DEFAULT through and it actually applies", async () => { + await createLiveSerialTable(); + + const root = await loadRoot(META_NO_GENERATION); + const expected = buildExpectedSchema(root, { dialect: "postgres" }); + const actual = await introspectPostgres(k); + + const result = await diff({ + expected, + actual, + dialect: "postgres", + allow: { dropIdentityDefault: true }, + }); + + const idDefaultChange = result.changes.find( + (c) => c.kind === "change-column-default" && c.column === "id", + ); + expect(idDefaultChange).toBeDefined(); + expect(idDefaultChange!.status.state).toBe("allowed"); + expect(result.blocked).toHaveLength(0); + + const emitted = emit(result.changes, { dialect: "postgres" }); + expect(emitted.up).toMatch(/ALTER COLUMN "id" DROP DEFAULT/i); + + for (const stmt of emitted.up.split(";").map((s) => s.trim()).filter((s) => s.length > 0)) { + await sql.raw(stmt).execute(k); + } + + const actualAfter = await introspectPostgres(k); + const liveIdAfter = actualAfter.tables.find((t) => t.name === "task")?.columns.find((c) => c.name === "id"); + expect(liveIdAfter?.default).toBeUndefined(); + + // An id-less insert now fails — proving the default really is gone (the + // exact real-world consequence the refusal exists to prevent unless + // deliberately chosen). + await expect( + sql`INSERT INTO "task" ("title") VALUES (${"no id supplied"})`.execute(k), + ).rejects.toThrow(); + }); +}); diff --git a/server/typescript/packages/migrate-ts/test/unit/diff-status-identity-default.test.ts b/server/typescript/packages/migrate-ts/test/unit/diff-status-identity-default.test.ts new file mode 100644 index 000000000..c47a899fd --- /dev/null +++ b/server/typescript/packages/migrate-ts/test/unit/diff-status-identity-default.test.ts @@ -0,0 +1,143 @@ +/** + * PR #279 stopped `meta migrate` from proposing a destructive + * `ALTER COLUMN "id" DROP DEFAULT` against a live Postgres `serial` PK — but + * ONLY when the metadata explicitly declares `identity.primary + * @generation: increment`. `buildExpectedSchema` sets `ColumnDescriptor.identity` + * exclusively from a declared `@generation` (expected-schema.ts) — there is no + * default — so an adopter who writes `identity.primary` with NO `@generation` + * against a live `serial` PK gets `ec.identity === undefined`, the #279 guard + * (diff/index.ts's `skipIdentityDefaultDiff`) never engages, and the same + * destructive drop reaches the diff as an ordinary, ALLOWED + * `change-column-default`. + * + * We deliberately do NOT widen the #279 guard to key off the LIVE column + * instead: "no @generation declared" is genuinely ambiguous between "never + * got around to declaring it" and "deliberately moving off auto-increment" + * (e.g. onto app-assigned ULIDs) — the diff cannot tell those apart, so + * instead of guessing it must ask. This is the same shape as #258 (refuse a + * PK move rather than emit an un-appliable migration). + * + * This suite covers the blocking case, the `--allow drop-identity-default` + * escape hatch that makes the deliberate-removal path still work, and three + * no-churn guards: the #279 explicit-`@generation` path stays a total no-op + * (not merely "allowed"), an ordinary (non-auto-sequence) default change + * stays allowed, and a `uuid`-identity PK is unaffected. + */ +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: [], + }; +} + +// No `identity` at all — the shape `buildExpectedSchema` produces when +// `identity.primary` is declared WITHOUT `@generation`. +const expectedNoGeneration: ColumnDescriptor = { + name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false, +}; + +const liveAutoSequence: ColumnDescriptor = { + name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false, identity: "increment", + default: { kind: "expr", value: "nextval('work_item_id_seq'::regclass)" }, +}; + +describe("diff — undeclared @generation against a live serial PK refuses, not guesses", () => { + test("RED: blocked, with a reason naming both remedies", async () => { + const r = await diff(snap(expectedNoGeneration), snap(liveAutoSequence), { dialect: "postgres" }); + const change = r.changes.find((c) => c.kind === "change-column-default"); + expect(change).toBeDefined(); + expect(change!.status.state).toBe("blocked"); + expect(r.blocked).toContain(change!); + + const reason = change!.status.blockedReason ?? ""; + expect(reason).toContain("work_item"); + expect(reason).toContain("id"); + // Remedy 1: keep the sequence. + expect(reason).toContain("@generation: increment"); + // Remedy 2: the allow-flag escape. + expect(reason).toContain("--allow drop-identity-default"); + }); + + test("RED: allow.dropIdentityDefault lets the deliberate removal through", async () => { + const r = await diff(snap(expectedNoGeneration), snap(liveAutoSequence), { + dialect: "postgres", + allow: { dropIdentityDefault: true }, + }); + const change = r.changes.find((c) => c.kind === "change-column-default"); + expect(change).toBeDefined(); + expect(change!.status.state).toBe("allowed"); + expect(r.blocked).toHaveLength(0); + }); + + test("no-churn (a): the #279 explicit @generation: increment path is a total no-op, not merely allowed", async () => { + const expectedIncrementPk: ColumnDescriptor = { + name: "id", sqlType: { kind: "integer", bits: 32 }, nullable: false, identity: "increment", + }; + const r = await diff(snap(expectedIncrementPk), snap(liveAutoSequence), { dialect: "postgres" }); + expect(r.changes).toEqual([]); + expect(r.blocked).toHaveLength(0); + }); + + test("no-churn (b): an ordinary default change ('0' -> '1') stays allowed", async () => { + const expected: ColumnDescriptor = { + name: "n", sqlType: { kind: "integer", bits: 32 }, nullable: false, + default: { kind: "literal", value: "1" }, + }; + const actual: ColumnDescriptor = { + name: "n", sqlType: { kind: "integer", bits: 32 }, nullable: false, + default: { kind: "literal", value: "0" }, + }; + const r = await diff(snap(expected), snap(actual), { dialect: "postgres" }); + const change = r.changes.find((c) => c.kind === "change-column-default"); + expect(change).toBeDefined(); + expect(change!.status.state).toBe("allowed"); + expect(r.blocked).toHaveLength(0); + }); + + test("no-churn (b): dropping a plain LITERAL default (not an auto-sequence) stays allowed", async () => { + const expected: ColumnDescriptor = { + name: "n", sqlType: { kind: "integer", bits: 32 }, nullable: false, + }; + const actual: ColumnDescriptor = { + name: "n", sqlType: { kind: "integer", bits: 32 }, nullable: false, + default: { kind: "literal", value: "0" }, + }; + const r = await diff(snap(expected), snap(actual), { dialect: "postgres" }); + const change = r.changes.find((c) => c.kind === "change-column-default"); + expect(change).toBeDefined(); + expect(change!.status.state).toBe("allowed"); + expect(r.blocked).toHaveLength(0); + }); + + test("no-churn (b): dropping a non-sequence EXPR default stays allowed (only nextval(...) is gated)", async () => { + const expected: ColumnDescriptor = { + name: "n", sqlType: { kind: "timestamp", withTimezone: false }, nullable: false, + }; + const actual: ColumnDescriptor = { + name: "n", sqlType: { kind: "timestamp", withTimezone: false }, nullable: false, + default: { kind: "expr", value: "now()" }, + }; + const r = await diff(snap(expected), snap(actual), { dialect: "postgres" }); + const change = r.changes.find((c) => c.kind === "change-column-default"); + expect(change).toBeDefined(); + expect(change!.status.state).toBe("allowed"); + expect(r.blocked).toHaveLength(0); + }); + + test("no-churn (c): a uuid-identity PK (declared @generation: uuid) is unaffected", async () => { + const expectedUuidPk: ColumnDescriptor = { + name: "id", sqlType: { kind: "uuid" }, nullable: false, identity: "uuid", + }; + const actual: ColumnDescriptor = { + ...expectedUuidPk, + default: { kind: "expr", value: "gen_random_uuid()" }, + }; + const r = await diff(snap(expectedUuidPk), snap(actual), { dialect: "postgres" }); + expect(r.changes).toEqual([]); + expect(r.blocked).toHaveLength(0); + }); +}); diff --git a/server/typescript/packages/sdk/src/config.ts b/server/typescript/packages/sdk/src/config.ts index fd8e8460a..eaf548c0c 100644 --- a/server/typescript/packages/sdk/src/config.ts +++ b/server/typescript/packages/sdk/src/config.ts @@ -6,13 +6,30 @@ const DialectEnum = z.enum(["sqlite", "postgres", "d1"]); const OnAmbiguousEnum = z.enum(["abort", "rename", "drop-add"]); -const AllowTokenEnum = z.enum([ +/** + * Kept in lockstep with the CLI's authoritative `ALLOW_TOKENS` + * (`cli/src/lib/args.ts`) by hand — `sdk` has no dependency on `cli` (the + * dependency runs the other way: `cli` depends on `sdk`), so this list + * cannot import that one. `cli`'s `allow-tokens-pinned.test.ts` is the + * drift guard: it imports BOTH `ALLOW_TOKENS` and this enum's `.options` + * and asserts they're the same set, so an out-of-sync edit here fails that + * test rather than silently rejecting a token the CLI itself accepts (as + * happened before `drop-check`/`drop-view`/`drop-view-cascade`/ + * `adopt-view`/`drop-identity-default` were added to `cli` without a + * matching update here). + */ +export const AllowTokenEnum = z.enum([ "drop-column", "drop-table", "type-change", "drop-index", "drop-fk", + "drop-check", + "drop-view", + "drop-view-cascade", + "adopt-view", "nullable-to-not-null", + "drop-identity-default", ]); const D1Block = z.object({ diff --git a/server/typescript/packages/sdk/src/index.ts b/server/typescript/packages/sdk/src/index.ts index c159f3592..3c87b14d9 100644 --- a/server/typescript/packages/sdk/src/index.ts +++ b/server/typescript/packages/sdk/src/index.ts @@ -26,7 +26,7 @@ export type { ListOptions } from "./storage/index.js"; export { recordPath, resolveMetaRoot } from "./paths.js"; // Config -export { ConfigSchema, DEFAULT_CONFIG, loadConfig, saveConfig } from "./config.js"; +export { ConfigSchema, DEFAULT_CONFIG, loadConfig, saveConfig, AllowTokenEnum } from "./config.js"; export type { Config } from "./config.js"; // Meta Forge metadata types + attribute name constants (registered into a