Skip to content

Commit aa29d70

Browse files
authored
Merge pull request #262 from metaobjectsdev/fix/258-migrate-pk-detect-refuse
fix(migrate-ts): refuse a primary-key move instead of silently dropping the PK (#258)
2 parents 494ebbb + cccbc61 commit aa29d70

10 files changed

Lines changed: 402 additions & 7 deletions

File tree

CHANGELOG.md

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
77

88
## [Unreleased]
99

10-
Shared-enum cross-package hardening (**#246** + its sibling **#259**). When cut this releases
11-
as a coordinated PATCH — the loader change (#246) lands in all five ports, the Kotlin codegen
12-
changes (#246 Bug 1, #259) land on Maven Central; no metadata vocabulary changes, byte-identical
13-
output for any model that doesn't hit the specific cross-package/two-hop enum shapes below.
10+
Shared-enum cross-package hardening (**#246** + its sibling **#259**), plus an **npm-only**
11+
migrate-ts fix (**#258**). When cut this releases as a coordinated PATCH — the loader change
12+
(#246) lands in all five ports, the Kotlin codegen changes (#246 Bug 1, #259) land on Maven
13+
Central, and #258 lands on npm only (`migrate-ts` + `cli`; schema/migrate is TS-owned, ADR-0015);
14+
no metadata vocabulary changes, byte-identical output for any model that doesn't hit the specific
15+
cross-package/two-hop enum shapes below (and, for #258, any migration that isn't a primary-key
16+
move).
1417

1518
- **#246 — a `field.enum` may now be shared across packages, and a conflicting redeclaration is
1619
rejected instead of silently dropped.** Two independent fixes:
@@ -48,6 +51,35 @@ documented as out-of-scope in the design spec
4851
Kotlin `enumTypeName` collapse gaining the `isAbstract` leg the other ports already carry (so a
4952
root-level *concrete* enum extended with own `@values` gets a per-field enum on every port).
5053

54+
### Fixed — migrate refuses a primary-key move instead of silently dropping the PK (#258)
55+
56+
**npm-only** (`migrate-ts` + `cli`; PyPI / NuGet / Maven Central unchanged — schema migrations are
57+
TS-owned, ADR-0015). The diff/emit has no primary-key change kind, so adopting an existing database
58+
(`--from-db`) whose `PRIMARY KEY` differs from the metadata identity degraded **silently** into an
59+
add-column + drop-column: the old PK column and its constraint were dropped, the new column was
60+
never made PK, leaving the table with **no primary key**, so every foreign key referencing it
61+
failed at apply (`there is no unique constraint matching given keys for referenced table`). Only
62+
observable when adopting an existing DB whose PK disagrees with the metadata — a greenfield
63+
`create-table` carries its PK inline. Follow-on from #255, which is what let the apply clear the
64+
column drops and reach the FK stage where this surfaced.
65+
66+
Migration generation now detects the move and throws a new `PrimaryKeyChangeError` (naming the
67+
table and both PKs) instead of emitting the un-appliable SQL — detect-and-refuse, the #226#241 arc
68+
for D1 FK cascades being the precedent (auto-migrating the PK remains a follow-up). The check runs
69+
**after** rename detection, mapping live PK column names through any detected `rename-column` for
70+
the table, so a PK column that was merely renamed (the engine preserves the PK through `RENAME
71+
COLUMN`) is not mistaken for a move. It is gated by a `DiffArgs.refusePrimaryKeyChange` flag set
72+
only by the migration-generation paths (the online `meta migrate --db` diff call and the offline
73+
`planOffline`); the read-only `meta verify`/drift path does **not** set it, so `verify` keeps
74+
reporting PK drift rather than throwing. The CLI catches `PrimaryKeyChangeError` at both throw
75+
sites (online + offline, including the D1 path) and emits a structured error + exit 1.
76+
77+
Byte-identical for any migration that is not a primary-key move (the full `migrate-ts` suite passes
78+
unchanged). Gated by 5 unit tests (refuse on a move; no-refuse on an unchanged PK; no-refuse on a
79+
resolved PK-column rename; no-throw without the flag) plus a real-Postgres integration round-trip
80+
(gated on `MIGRATE_TS_PG_URL`) that reproduces the original failure — a live
81+
`user_profiles PK(user_id)` with a referencing FK — and asserts the refusal fires.
82+
5183
## [0.20.10] — 2026-08-02
5284

5385
**Coordinated PATCH** — npm `0.20.10` · PyPI `0.19.9` · NuGet `0.19.7` · Maven Central `7.11.7`.

docs/bugs/2026-08-02-no-primary-key-change-kind.md

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,22 @@ title: "migrate: no primary-key change kind, so moving a table's PK leaves it wi
55
labels: bug
66
---
77

8-
> **Filed as** https://github.com/metaobjectsdev/metaobjects/issues/258 (2026-08-02). Open. Follow-on from #255.
8+
> **Filed as** https://github.com/metaobjectsdev/metaobjects/issues/258 (2026-08-02). **RESOLVED
9+
> (detect-and-refuse).** Follow-on from #255.
10+
>
11+
> Migration generation now refuses a primary-key move instead of emitting un-appliable SQL — the
12+
> second of the two approaches proposed below, chosen deliberately over auto-migrating. It landed
13+
> in `272ee9d5` ("fix(#258): migrate refuses a primary-key move instead of silently dropping the
14+
> PK") and was extended to the D1 path in `737d3244`. The diff throws a new `PrimaryKeyChangeError`
15+
> (naming the table and both PKs) when an existing table's live `PRIMARY KEY` differs from the
16+
> metadata identity; the check runs after rename detection, so a PK column that was merely renamed
17+
> (the engine preserves the PK through `RENAME COLUMN`) is not mistaken for a move. It is gated by
18+
> a `DiffArgs.refusePrimaryKeyChange` flag that only the migration-generation paths set (the online
19+
> `meta migrate --db` diff call and the offline `planOffline`); the read-only `meta verify`/drift
20+
> path does **not** set it, so `verify` keeps reporting PK drift rather than throwing. Gated by 5
21+
> unit tests plus a real-Postgres round-trip on the genuine reproduction. Auto-migrating the PK —
22+
> the `add-primary-key`/`drop-primary-key` change kinds and staging proposed in "Suggested fix"
23+
> below — remains a follow-up. Kept as a written record of the failure mode.
924
1025
**Affected port(s):** TypeScript (diff + emit; shared migration engine, so all ports)
1126
**Package + version:** `@metaobjectsdev/cli` + `@metaobjectsdev/migrate-ts` 0.20.10

docs/features/migrations-and-drift.md

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,32 @@ cycle, rebuild the tables, then restore it) or break the cycle in your metadata.
9797
self-referencing table (a table whose own foreign key targets itself) is not a cycle
9898
in this sense and is handled by the cascade like any other rebuild.
9999

100+
#### A moved primary key (adoption-time refusal)
101+
102+
The diff/emit has no `add-primary-key` / `drop-primary-key` change kind, so an **existing**
103+
table whose live `PRIMARY KEY` differs from the metadata identity cannot be expressed as a
104+
migration. When adopting such a database (`--from-db`), `meta migrate` now **refuses at
105+
generation time** instead of emitting un-appliable SQL — detect-and-refuse, the same arc as
106+
[#226](https://github.com/metaobjectsdev/metaobjects/issues/226)[#241](https://github.com/metaobjectsdev/metaobjects/issues/241)
107+
for the D1 foreign-key rebuilds above. It throws a `PrimaryKeyChangeError` (naming the table
108+
and both PKs), the CLI catches it and exits 1
109+
([#258](https://github.com/metaobjectsdev/metaobjects/issues/258)).
110+
111+
Previously the move degraded **silently** into an add-column + drop-column: the old PK
112+
column and its constraint were dropped while the new column was never made primary key,
113+
leaving the table with no primary key, so every foreign key referencing it failed at apply
114+
(`there is no unique constraint matching given keys`). This surfaces only when **adopting**
115+
an existing database whose PK disagrees with the metadata — a greenfield `create-table`
116+
carries its primary key inline.
117+
118+
The check is engine-wide (`postgres` / `sqlite` / `d1` — the diff is shared) and runs
119+
**after** rename detection, mapping live PK column names through any detected
120+
`rename-column` change, so a primary-key column that was merely **renamed** (the engine
121+
preserves the PK through `RENAME COLUMN`) is not mistaken for a move. The read-only
122+
`meta verify` / drift path does not set the refusal flag, so `verify` keeps **reporting**
123+
primary-key drift rather than throwing. Auto-migrating the move (adding the
124+
`add-primary-key` / `drop-primary-key` change kinds) is a documented future follow-up.
125+
100126
### Java
101127

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

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

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
readSnapshot,
2626
writeSnapshot,
2727
BlockedChangesError,
28+
PrimaryKeyChangeError,
2829
renderD1,
2930
writeMigrationD1,
3031
introspectD1,
@@ -389,6 +390,10 @@ export async function migrateCommand(
389390
actual,
390391
dialect: kysely.dialect,
391392
allow: tokensToAllowOptions(config.allow),
393+
// #258 — adopting a live DB whose PRIMARY KEY differs from the metadata identity
394+
// has no expressible migration; refuse loudly instead of emitting SQL that drops
395+
// the constraint and breaks referencing FKs at apply.
396+
refusePrimaryKeyChange: true,
392397
// #208 §7 — declared-@unmanaged objects are external: exclude them from the
393398
// actual side so migrate proposes neither create nor drop for them.
394399
unmanagedNames: collectUnmanagedNames(metadata),
@@ -398,6 +403,13 @@ export async function migrateCommand(
398403
},
399404
});
400405
} catch (err) {
406+
// #258 — a primary-key move has no expressible migration; refuse loudly.
407+
if (err instanceof PrimaryKeyChangeError) {
408+
log.error(`migrate: ${err.message}`);
409+
emitStructuredError(`migrate: ${err.message}`, "align the primary key manually, or reconcile the metadata identity to match the live table", fmt);
410+
await kysely.close();
411+
return 1;
412+
}
401413
// diff() throws when onAmbiguous returns "abort" — surface as exit 1
402414
// with the collected ambiguity list.
403415
if ((err as Error).message.includes("aborted by onAmbiguous")) {
@@ -809,6 +821,12 @@ export async function runOfflineGenerate(
809821
},
810822
});
811823
} catch (err) {
824+
// #258 — a primary-key move has no expressible migration; refuse loudly.
825+
if (err instanceof PrimaryKeyChangeError) {
826+
log.error(`migrate: ${err.message}`);
827+
emitStructuredError(`migrate: ${err.message}`, "align the primary key manually, or reconcile the metadata identity to match the live table", fmt);
828+
return 1;
829+
}
812830
if ((err as Error).message.includes("aborted by onAmbiguous")) {
813831
log.error(`migrate: ambiguous rename/drop detected; re-run with --on-ambiguous rename|drop-add`);
814832
return 1;
@@ -912,7 +930,7 @@ async function runD1Migrate(
912930
config: ResolvedMigrateConfig,
913931
metaRoot: string,
914932
runner: WranglerRunner,
915-
_fmt: OutputFormat = "text",
933+
fmt: OutputFormat = "text",
916934
): Promise<number> {
917935
// 1. Resolve wrangler.toml + binding.
918936
const wranglerConfigPath = config.d1.wranglerConfigPath
@@ -1012,6 +1030,10 @@ async function runD1Migrate(
10121030
// @constraintName models churning and enum @values changes silent on D1.
10131031
dialect: "d1",
10141032
allow: tokensToAllowOptions(config.allow),
1033+
// #258 — adopting a live D1 DB whose PRIMARY KEY differs from the metadata identity
1034+
// has no expressible migration; refuse loudly instead of emitting SQL that drops
1035+
// the constraint and breaks referencing FKs at apply (same failure as the online path).
1036+
refusePrimaryKeyChange: true,
10151037
// #208 §7 — declared-@unmanaged objects are external (see the online path above).
10161038
unmanagedNames: collectUnmanagedNames(metadata),
10171039
onAmbiguous: async (a) => {
@@ -1020,6 +1042,12 @@ async function runD1Migrate(
10201042
},
10211043
});
10221044
} catch (err) {
1045+
// #258 — a primary-key move has no expressible migration; refuse loudly.
1046+
if (err instanceof PrimaryKeyChangeError) {
1047+
log.error(`migrate: ${err.message}`);
1048+
emitStructuredError(`migrate: ${err.message}`, "align the primary key manually, or reconcile the metadata identity to match the live table", fmt);
1049+
return 1;
1050+
}
10231051
if ((err as Error).message.includes("aborted by onAmbiguous")) {
10241052
const entries = ambiguousToEntries(collectedAmbiguous);
10251053
for (const e of entries) {

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
import type { SqlType } from "../sql-type.js";
99
import { sqlTypeEquals } from "../sql-type.js";
1010
import { applyStatus } from "./status.js";
11+
import { PrimaryKeyChangeError } from "../errors.js";
1112
import { detectColumnRenames, detectTableRenames } from "./rename-heuristic.js";
1213
import { viewSqlEquals } from "../view-sql-compare.js";
1314
import { viewReplaceIsLegal } from "../view-column-types.js";
@@ -58,6 +59,16 @@ export interface DiffArgs {
5859
unmanagedNames?: string[];
5960
/** Dialect; CHECK-constraint evolution on existing tables is emitted for postgres only. */
6061
dialect?: Dialect;
62+
/**
63+
* #258 — refuse (throw {@link PrimaryKeyChangeError}) when an existing table's live
64+
* PRIMARY KEY differs from the metadata identity. There is no primary-key change kind
65+
* in the emitter, so such a move would silently degrade into add-column + drop-column
66+
* and leave the table with no PK, breaking referencing FKs at apply time. Set by the
67+
* migration-generation path (snapshot/plan.ts); left unset by the read-only drift/verify
68+
* path so `meta verify` keeps reporting drift rather than throwing. Off by default —
69+
* existing callers are byte-identical.
70+
*/
71+
refusePrimaryKeyChange?: boolean;
6172
}
6273

6374
const ALLOWED: ChangeStatus = { state: "allowed" };
@@ -266,10 +277,51 @@ export async function diff(
266277
delete (c as Aug)._columns;
267278
}
268279

280+
// #258: refuse a primary-key MOVE at generation time. There is no primary-key change
281+
// kind, so a table whose live PK differs from the metadata identity would degrade into
282+
// an add-column + drop-column and lose the constraint (breaking referencing FKs at
283+
// apply). Runs after rename detection so a PK column that was merely RENAMED (PK
284+
// preserved by the engine) is not mistaken for a move. Gated by refusePrimaryKeyChange
285+
// so only migration generation refuses; the read-only drift/verify path is unchanged.
286+
if (args.refusePrimaryKeyChange === true) {
287+
for (const [id, expectedTable] of expectedTables) {
288+
const actualTable = actualTables.get(id);
289+
if (actualTable === undefined) continue; // create-table: PK is inline, not a move
290+
assertPrimaryKeyUnchanged(expectedTable, actualTable, changes);
291+
}
292+
}
293+
269294
applyStatus(changes, args.allow ?? {});
270295
return { changes, blocked: changes.filter((c) => c.status.state === "blocked") };
271296
}
272297

298+
/**
299+
* #258 — throw {@link PrimaryKeyChangeError} when a table's live PRIMARY KEY differs from
300+
* the metadata identity. Live PK column names are first mapped through any detected
301+
* `rename-column` for this table, so a renamed PK column (the engine preserves the PK
302+
* through a `RENAME COLUMN`) is not treated as a move. A genuine move — a PK column added
303+
* or dropped, or the key repointed to different columns — has no expressible migration and
304+
* is refused.
305+
*/
306+
function assertPrimaryKeyUnchanged(
307+
expected: TableDescriptor,
308+
actual: TableDescriptor,
309+
changes: Change[],
310+
): void {
311+
const wantId = tableIdentity(expected);
312+
const renamed = new Map<string, string>();
313+
for (const c of changes) {
314+
if (c.kind === "rename-column" && tableIdentity({ name: c.table, ...schemaSpread(c.schema) }) === wantId) {
315+
renamed.set(c.from, c.to);
316+
}
317+
}
318+
const livePk = actual.primaryKey.map((col) => renamed.get(col) ?? col);
319+
const wantPk = expected.primaryKey;
320+
const unchanged = livePk.length === wantPk.length && livePk.every((col, i) => col === wantPk[i]);
321+
if (unchanged) return;
322+
throw new PrimaryKeyChangeError(expected.name, actual.primaryKey, expected.primaryKey, expected.schema);
323+
}
324+
273325
function isDiffArgs(x: DiffArgs | SchemaSnapshot): x is DiffArgs {
274326
return "expected" in x && "actual" in x;
275327
}

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,37 @@ export class BlockedChangesError extends Error {
9393
this.enableHints = hints;
9494
}
9595
}
96+
97+
/**
98+
* #258 — a table whose live PRIMARY KEY differs from the metadata identity cannot be
99+
* migrated: the diff/emit has no primary-key change kind, so the difference degrades
100+
* silently into an add-column + drop-column (the old PK column is dropped, the new one
101+
* is never made PK), leaving the table with no primary key and breaking every foreign
102+
* key that references it at apply time. Migration generation detects the move and throws
103+
* this instead of emitting un-appliable SQL (detect-and-refuse; the #226→#241 arc for D1
104+
* FK cascades is the precedent). A pure column RENAME is NOT a key move — the engine
105+
* preserves the PK through a `RENAME COLUMN` — and does not trigger this.
106+
*/
107+
export class PrimaryKeyChangeError extends Error {
108+
override readonly name = "PrimaryKeyChangeError";
109+
readonly table: string;
110+
readonly livePrimaryKey: string[];
111+
readonly expectedPrimaryKey: string[];
112+
readonly schema?: string;
113+
114+
constructor(table: string, livePrimaryKey: string[], expectedPrimaryKey: string[], schema?: string) {
115+
const qualified = schema !== undefined ? `${schema}.${table}` : table;
116+
const fmt = (cols: string[]) => (cols.length > 0 ? `PRIMARY KEY (${cols.join(", ")})` : "no primary key");
117+
super(
118+
`primary key of "${qualified}" differs from the live database: live ${fmt(livePrimaryKey)} vs ` +
119+
`metadata ${fmt(expectedPrimaryKey)}. migrate cannot express a primary-key change (there is no ` +
120+
`add/drop-primary-key change kind), so this would silently drop the constraint and break every ` +
121+
`foreign key that references this table. Align the primary key manually — or reconcile the metadata ` +
122+
`identity to match the live table — before migrating.`,
123+
);
124+
this.table = table;
125+
this.livePrimaryKey = livePrimaryKey;
126+
this.expectedPrimaryKey = expectedPrimaryKey;
127+
if (schema !== undefined) this.schema = schema;
128+
}
129+
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ export { planOffline, baselineFromMetadata } from "./snapshot/plan.js";
3131
export type { PlanOfflineArgs, PlanOfflineResult } from "./snapshot/plan.js";
3232

3333
// Errors
34-
export { BlockedChangesError, SetNullNotNullableError } from "./errors.js";
34+
export { BlockedChangesError, SetNullNotNullableError, PrimaryKeyChangeError } from "./errors.js";
3535

3636
// SqlType helpers (rarely needed but useful for advanced consumers)
3737
export { isWidening, sqlTypeEquals } from "./sql-type.js";

server/typescript/packages/migrate-ts/src/snapshot/plan.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ export async function planOffline(args: PlanOfflineArgs): Promise<PlanOfflineRes
3838
expected: nextSnapshot,
3939
actual: args.snapshot,
4040
dialect: args.dialect,
41+
// #258 — migration generation refuses a primary-key MOVE (there is no primary-key
42+
// change kind to express it; it would otherwise silently drop the constraint). The
43+
// read-only verify/drift path does NOT set this, so `meta verify` still reports drift.
44+
refusePrimaryKeyChange: true,
4145
// #208 §7 — exclude declared-@unmanaged objects from the actual (snapshot) side too,
4246
// so the OFFLINE generate path never proposes DROP for an external table that a
4347
// `baseline --from-db` captured into the snapshot (parity with the online/verify paths).

0 commit comments

Comments
 (0)