fix(migrate-ts): stop meta migrate dropping a legacy Postgres serial PK's default - #279
Merged
Merged
Conversation
…PK's default 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`) made `meta migrate --from-db` propose `ALTER COLUMN "id" DROP DEFAULT` with no replacement generation mechanism -- destructive against a live table, since every insert that doesn't supply id explicitly then starts failing. The default-diff guard at diff/index.ts skipped identity/default comparison only for `identity: "uuid"`, on the strength of a comment claiming an increment column never carries a DEFAULT. True for SQLite AUTOINCREMENT and modern GENERATED ... AS IDENTITY; false for legacy Postgres `serial`, which is sugar for integer + sequence + a genuine `DEFAULT nextval(...)` clause. The guard now also skips the default-diff for `identity: "increment"` when the live default matches that exact nextval(...) shape -- narrowly, via a predicate (isPgAutoSequenceDefault) shared with the introspector that already recognized it, so a genuinely wrong non-sequence default on an increment PK still reports as drift. Rewrote the false comment. Reported against an adopting project. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TqsuDye2SfXGf43vuoD3n
… arg
Three one-line fixes from review of the serial-identity default-diff fix:
- diff/index.ts: the replacement comment inverted its own causal clause
("Left undiffed, that surfaced as ... DROP DEFAULT" claims skipping the
diff produced the bug -- the exact inverse; the bug was the case being
left DIFFED). Fixed to "Left diffed".
- diff-uuid-identity-default.test.ts: the increment-PK regression-guard test
name asserted "it never had a DEFAULT", the same false doctrine this
change disproved. Renamed to "(no live default at all)".
- diff-serial-identity-default.test.ts: the new unit tests called diff()
with no `dialect` for a Postgres-only bug. This repo's standing rule is
that omitting `dialect` runs a different pipeline -- pass it always. Both
diff() call sites now pass `dialect: "postgres"`, matching the
integration test.
No production behavior change beyond the comment; the two test edits are
name/argument-only and do not change what either test asserts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015TqsuDye2SfXGf43vuoD3n
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopting metadata onto an existing Postgres table whose PK was created as legacy
serialmademeta migrate --from-dbpropose:with no replacement generation mechanism. Applying it leaves
idNOT NULLwith nothing to populate it, so every insert that doesn't supplyidstarts failing — against a live table, on the first thing a new adopter does.serialPKs are what Drizzle, Prisma, Rails and SQLAlchemy all produce, so this is the most common pre-adoption shape.It also violated the repo's own adoption doctrine — "adopting onto existing code? Metadata FOLLOWS the code" — by modernizing
serial→IDENTITYas a side effect of adoption.Root cause
Introspection is correct: it detects
nextval(...)and setsidentity = "increment", matching the expected side, so the identity dimension itself doesn't diff. The bug is the separate column-default comparison, guarded only againstuuidon the strength of a comment claiming "an AUTOINCREMENT column has no DEFAULT" — true for SQLite and modernGENERATED ... AS IDENTITY, false for Postgresserial, which is sugar forinteger+ sequence + a realDEFAULT nextval(...).The fix
An
incrementPK skips the default-diff only when the live default is that exact auto-sequence shape. A genuinely wrong, non-sequence default on an increment PK still reports as drift (two regression tests assert the change payload, not just a count). Detection lives in one sharedisPgAutoSequenceDefaulthelper used by both introspection and the diff, so the rule can't drift into two spellings. The false comment is corrected.No
serial→IDENTITYmodernization is introduced — if ever wanted it should be opt-in, never a side effect of adoption.Verification
Ran against a real Postgres (ephemeral container), not a skipped integration test: live
serialPK introspects with a genuinenextval(...)→ diff emits noDROP DEFAULT→ apply → re-introspect → re-diff empty → an insert omittingidsucceeds and auto-populates. The last two assertions are what prove the bug fixed rather than silenced.Unit suite 401/401 · package 688 pass / 19 skip / 0 fail · reviewed, one fix round, re-reviewed clean.
npm-only — schema is TypeScript-owned (ADR-0015).
Known residual (not addressed here)
The guard keys on the expected side, and identity is only set when
@generationis explicit — so an adopter declaringidentity.primarywithout@generation: incrementstill hits the destructive path. Widening to key on the actual side would swallow a deliberate@generationremoval, so it needs a design ruling rather than a blind patch.