Skip to content

Commit 5bf97e0

Browse files
authored
Merge pull request #282 from metaobjectsdev/fix/identity-no-generation-refuse
fix(migrate-ts): refuse dropping a live serial default when @generation is undeclared
2 parents e7c4828 + 7898ef8 commit 5bf97e0

14 files changed

Lines changed: 501 additions & 9 deletions

File tree

docs/features/migrations-and-drift.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,21 @@ preserves the PK through `RENAME COLUMN`) is not mistaken for a move. The read-o
123123
primary-key drift rather than throwing. Auto-migrating the move (adding the
124124
`add-primary-key` / `drop-primary-key` change kinds) is a documented future follow-up.
125125

126+
#### A legacy `serial` primary key (adoption-time refusal)
127+
128+
When migrating against a live Postgres database whose primary key is a legacy
129+
`serial` / `bigserial` column — one carrying a live `nextval(...)` default — and the
130+
metadata declares that `identity.primary` **without** `@generation`, the diff would
131+
otherwise emit `ALTER COLUMN … DROP DEFAULT`. That is destructive: every insert that
132+
omits the id starts failing. The missing `@generation` is genuinely ambiguous — it reads
133+
identically whether the author simply never declared it (and wants to keep
134+
auto-increment) or deliberately dropped it (to move the column onto app-assigned ids) —
135+
so `meta migrate` **refuses rather than guessing**, the same detect-and-refuse arc as the
136+
primary-key move above. Declare `@generation: increment` on the identity to keep the
137+
sequence, or pass `--allow drop-identity-default` if removing auto-increment is
138+
intentional. An identity that *does* declare `@generation: increment` never reaches this
139+
gate (its default diff is skipped), so only the undeclared case fires.
140+
126141
### Java
127142

128143
Schema migrations for Java projects are owned by the **TypeScript toolchain**

server/typescript/packages/cli/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ Flags:
143143
- `--dialect sqlite|postgres|d1` — auto-detected from URL scheme; use `d1` for Cloudflare D1
144144
- `--out-dir <path>` (default `./.metaobjects/migrations`)
145145
- `--slug <name>` — required when changes are pending (e.g., `add-user-shipping`)
146-
- `--allow <csv>` — destructive-change permissions: `drop-column,drop-table,type-change,drop-index,drop-fk,drop-check,drop-view,nullable-to-not-null`
146+
- `--allow <csv>` — 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`
147147
- `--on-ambiguous abort|rename|drop-add` (default `abort`) — non-interactive
148148
- `--dry-run` — print SQL pair to stdout, write nothing
149149
- `--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).

server/typescript/packages/cli/src/commands/migrate.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ MIGRATE FLAGS:
8181
--allow <csv> Comma-separated destructive-change permissions:
8282
drop-column,drop-table,type-change,drop-index,drop-fk,
8383
drop-check,drop-view,drop-view-cascade,
84-
adopt-view,nullable-to-not-null
84+
adopt-view,nullable-to-not-null,drop-identity-default
8585
--on-ambiguous abort|rename|drop-add
8686
How to handle ambiguous renames (default: abort)
8787
--from-db Introspect live DB instead of using the committed snapshot
@@ -204,6 +204,7 @@ function allowFlagFor(kind: string): string {
204204
case "drop-fk": return "drop-fk";
205205
case "change-column-type": return "type-change";
206206
case "change-column-nullable": return "nullable-to-not-null";
207+
case "change-column-default": return "drop-identity-default";
207208
default: return kind;
208209
}
209210
}

server/typescript/packages/cli/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ MIGRATE FLAGS:
7676
--allow <csv> Comma-separated destructive-change permissions:
7777
drop-column,drop-table,type-change,drop-index,drop-fk,
7878
drop-check,drop-view,drop-view-cascade,
79-
adopt-view,nullable-to-not-null
79+
adopt-view,nullable-to-not-null,drop-identity-default
8080
--on-ambiguous abort|rename|drop-add Default abort
8181
--d1 <binding> D1 binding name from wrangler.toml (only with --dialect d1)
8282
--remote Target remote D1 instead of local (only with --dialect d1)

server/typescript/packages/cli/src/lib/allow.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,12 @@
55
import type { AllowOptions, Change } from "@metaobjectsdev/migrate-ts";
66

77
// Map CLI allow tokens → migrate-ts AllowOptions field names.
8-
const ALLOW_TOKEN_MAP: Record<string, keyof AllowOptions> = {
8+
// Exported (not just module-local) so allow-tokens-pinned.test.ts can pin its
9+
// key set against ALLOW_TOKENS (args.ts). ALLOW_TOKENS is the *validator* —
10+
// this map is what actually *grants* the permission; a token present in
11+
// ALLOW_TOKENS but missing here would pass validation and silently grant
12+
// nothing, on a destructive operation.
13+
export const ALLOW_TOKEN_MAP: Record<string, keyof AllowOptions> = {
914
"drop-column": "dropColumn",
1015
"drop-table": "dropTable",
1116
"type-change": "typeChange",
@@ -23,6 +28,11 @@ const ALLOW_TOKEN_MAP: Record<string, keyof AllowOptions> = {
2328
// Gates overwriting an unfingerprinted (hand-written or pre-fingerprint) view.
2429
"adopt-view": "adoptView",
2530
"nullable-to-not-null": "nullableToNotNull",
31+
// Gates dropping a live Postgres auto-sequence default (a legacy `serial`/
32+
// `bigserial` PK's `nextval(...)`) when the metadata declares no
33+
// @generation at all — ambiguous between "never declared it" and
34+
// "deliberately removing auto-increment", so migrate refuses without it.
35+
"drop-identity-default": "dropIdentityDefault",
2636
};
2737

2838
/** Translate parsed `--allow` tokens into the migrate-ts `AllowOptions` shape. */

server/typescript/packages/cli/src/lib/args.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,12 @@ type Dialect = (typeof DIALECTS)[number];
165165
export const MIGRATE_FORMATS = ["default", "flyway"] as const;
166166
export type MigrateFormat = (typeof MIGRATE_FORMATS)[number];
167167

168-
const ALLOW_TOKENS = [
168+
// Exported (not just module-local) so allow-tokens-pinned.test.ts can pin it
169+
// against sdk's AllowTokenEnum (config.json's static migrate.allow validator)
170+
// — the two lists drifted silently before that test existed: sdk's enum was
171+
// missing 5 of these 11 tokens, so a token that worked fine on the CLI was
172+
// REJECTED when set in .metaobjects/config.json.
173+
export const ALLOW_TOKENS = [
169174
"drop-column",
170175
"drop-table",
171176
"type-change",
@@ -187,6 +192,11 @@ const ALLOW_TOKENS = [
187192
// toolchain needs this exactly once, to stamp its existing views.
188193
"adopt-view",
189194
"nullable-to-not-null",
195+
// drop-identity-default permits dropping a live Postgres auto-sequence
196+
// default (a legacy `serial`/`bigserial` PK's `nextval(...)`) when the
197+
// metadata declares no @generation at all — ambiguous between "never
198+
// declared it" and "deliberately removing auto-increment".
199+
"drop-identity-default",
190200
] as const;
191201
type AllowToken = (typeof ALLOW_TOKENS)[number];
192202

server/typescript/packages/cli/test/__snapshots__/cli.test.ts.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ MIGRATE FLAGS:
7070
--allow <csv> Comma-separated destructive-change permissions:
7171
drop-column,drop-table,type-change,drop-index,drop-fk,
7272
drop-check,drop-view,drop-view-cascade,
73-
adopt-view,nullable-to-not-null
73+
adopt-view,nullable-to-not-null,drop-identity-default
7474
--on-ambiguous abort|rename|drop-add Default abort
7575
--d1 <binding> D1 binding name from wrangler.toml (only with --dialect d1)
7676
--remote Target remote D1 instead of local (only with --dialect d1)
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
/**
2+
* Drift guard across the three token-bearing structures behind `--allow`:
3+
*
4+
* - `ALLOW_TOKENS` (`lib/args.ts`) — the CLI's authoritative list; what
5+
* actually VALIDATES `--allow <csv>`.
6+
* - `AllowTokenEnum` (`sdk`'s `config.ts`) — validates the STATIC
7+
* `migrate.allow` array in `.metaobjects/config.json`.
8+
* - `ALLOW_TOKEN_MAP` (`lib/allow.ts`) — what actually GRANTS the permission,
9+
* translating a validated token into the `AllowOptions` field `diff()`
10+
* reads.
11+
*
12+
* `ALLOW_TOKENS` and `AllowTokenEnum` silently drifted before this test
13+
* existed: sdk's enum had only 6 of the 11 real tokens, missing
14+
* `drop-check`, `drop-view`, `drop-view-cascade`, `adopt-view` and
15+
* `drop-identity-default`. A user who set any of those five in
16+
* `.metaobjects/config.json`'s `migrate.allow` got a schema rejection for a
17+
* flag the CLI itself accepted fine on the command line — `adopt-view` had
18+
* shipped since 0.20.4 and was affected the whole time.
19+
*
20+
* `ALLOW_TOKEN_MAP` is a distinct, worse failure mode if it drifts from
21+
* `ALLOW_TOKENS`: a token present in `ALLOW_TOKENS` (and `AllowTokenEnum`)
22+
* but missing from the map passes validation cleanly and then
23+
* `tokensToAllowOptions` silently grants NOTHING for it — the user believes
24+
* `--allow <token>` authorized a destructive drop; it didn't, and the diff
25+
* blocks it anyway with no indication the flag was ever a no-op. That is a
26+
* silent-failure mode on exactly the path this whole feature exists to
27+
* protect.
28+
*
29+
* Import ALL of these rather than hardcoding a fourth "expected" list here —
30+
* a hardcoded list would just be a fifth copy that can itself drift.
31+
*
32+
* Package-dependency direction: `cli` depends on `sdk` (`workspace:*`), not
33+
* the other way around, so this test can only live in `cli` — `sdk` cannot
34+
* import from `cli` without introducing a cycle. `sdk`'s `AllowTokenEnum`
35+
* itself carries a doc comment pointing back at this test as the drift guard,
36+
* since `sdk` has no test that can perform the comparison from its own side.
37+
*/
38+
import { test, expect, describe } from "bun:test";
39+
import { ALLOW_TOKENS } from "../../src/lib/args.js";
40+
import { ALLOW_TOKEN_MAP } from "../../src/lib/allow.js";
41+
import { AllowTokenEnum } from "@metaobjectsdev/sdk";
42+
43+
describe("--allow token lists stay pinned across packages", () => {
44+
test("sdk's AllowTokenEnum and the CLI's ALLOW_TOKENS validate the exact same token set", () => {
45+
const cliTokens = new Set<string>(ALLOW_TOKENS);
46+
const sdkTokens = new Set<string>(AllowTokenEnum.options);
47+
48+
const missingFromSdk = [...cliTokens].filter((t) => !sdkTokens.has(t));
49+
const missingFromCli = [...sdkTokens].filter((t) => !cliTokens.has(t));
50+
51+
expect(missingFromSdk).toEqual([]);
52+
expect(missingFromCli).toEqual([]);
53+
expect(sdkTokens.size).toBe(cliTokens.size);
54+
});
55+
56+
test("ALLOW_TOKEN_MAP grants a permission for every validated token, and nothing extra", () => {
57+
const cliTokens = new Set<string>(ALLOW_TOKENS);
58+
const mapKeys = new Set<string>(Object.keys(ALLOW_TOKEN_MAP));
59+
60+
const validatedButNotGranted = [...cliTokens].filter((t) => !mapKeys.has(t));
61+
const grantedButNotValidated = [...mapKeys].filter((t) => !cliTokens.has(t));
62+
63+
expect(validatedButNotGranted).toEqual([]);
64+
expect(grantedButNotValidated).toEqual([]);
65+
expect(mapKeys.size).toBe(cliTokens.size);
66+
});
67+
68+
test("ALLOW_TOKEN_MAP's AllowOptions fields are unique — no two tokens grant the same permission", () => {
69+
const fields = Object.values(ALLOW_TOKEN_MAP);
70+
const uniqueFields = new Set(fields);
71+
expect(uniqueFields.size).toBe(fields.length);
72+
});
73+
});

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

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { Change, AllowOptions } from "../types.js";
22
import { isWidening } from "../sql-type.js";
3+
import { isPgAutoSequenceDefault } from "../pg-identity-default.js";
34
import { DEFAULT_DB_SCHEMA_POSTGRES } from "@metaobjectsdev/metadata";
45

56
/**
@@ -111,12 +112,42 @@ function blockedReasonFor(
111112
if (c.from === false && c.to === true) return null;
112113
return allow.nullableToNotNull ? null : "nullable→notnull requires existing data to satisfy (pass allow.nullableToNotNull)";
113114

115+
case "change-column-default": {
116+
// Ordinary default changes (literal→literal, adding/removing a plain
117+
// literal default, etc.) stay allowed unconditionally — only ONE narrow
118+
// shape is gated here. `to === undefined` means the default is being
119+
// DROPPED outright (see the ColumnDefault comment in types.ts), and
120+
// when what's being dropped is a live Postgres auto-sequence default
121+
// (`nextval(...)`, the shape a legacy `serial`/`bigserial` PK carries —
122+
// isPgAutoSequenceDefault, shared with diff/index.ts and the
123+
// introspector), reaching this point means the expected side declared
124+
// NO identity at all: an `identity: "increment"` expected column never
125+
// gets here, because diff/index.ts's skipIdentityDefaultDiff already
126+
// suppressed the change for that exact live shape. So an undeclared
127+
// `@generation` is the ONLY way this branch fires — and that silence is
128+
// ambiguous (never-declared vs. deliberately-removed), so ask rather
129+
// than guess. Anything else about change-column-default — including
130+
// dropping a plain literal default — falls through to the unconditional
131+
// `return null` below.
132+
const droppingAutoSequence =
133+
c.to === undefined && c.from?.kind === "expr" && isPgAutoSequenceDefault(c.from.value);
134+
if (droppingAutoSequence && !allow.dropIdentityDefault) {
135+
return `column "${c.table}"."${c.column}" has a live Postgres auto-increment default `
136+
+ `(${c.from!.value}) but its metadata declares no @generation — this is ambiguous: it `
137+
+ `could mean @generation was never declared, or that auto-increment is being removed on `
138+
+ `purpose. Dropping the default is destructive (every insert that omits the column starts `
139+
+ `failing), so this refuses rather than guessing. Declare @generation: increment on the `
140+
+ `identity to keep the sequence, or pass --allow drop-identity-default if removing it is `
141+
+ `intentional`;
142+
}
143+
return null;
144+
}
145+
114146
// Always-allowed kinds
115147
case "create-table":
116148
case "rename-table":
117149
case "add-column":
118150
case "rename-column":
119-
case "change-column-default":
120151
case "add-index":
121152
case "add-fk":
122153
case "add-check":

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,21 @@ export interface AllowOptions {
315315
adoptView?: boolean;
316316
/** Existing data must satisfy NOT NULL; diff cannot verify this. */
317317
nullableToNotNull?: boolean;
318+
/**
319+
* Gates dropping a live Postgres auto-sequence DEFAULT (the `nextval(...)`
320+
* shape a legacy `serial`/`bigserial` column carries — see
321+
* pg-identity-default.ts) when the expected side declares NO identity at
322+
* all, i.e. `@generation` was never set. That silence is genuinely
323+
* ambiguous: it reads identically whether the author simply never got
324+
* around to declaring `@generation: increment`, or deliberately dropped it
325+
* to move the column off auto-increment (e.g. onto app-assigned ULIDs).
326+
* The diff cannot tell those apart, so it refuses instead of guessing —
327+
* this flag is how the author confirms the second reading and lets the
328+
* DROP DEFAULT through. (An expected side that DOES declare
329+
* `@generation: increment` never reaches this gate at all — diff/index.ts
330+
* skips the default-diff for a live auto-sequence default entirely.)
331+
*/
332+
dropIdentityDefault?: boolean;
318333
}
319334

320335
export type AmbiguousChange =

0 commit comments

Comments
 (0)