Skip to content

fix(migrations): keep revision tables aligned with their models - #548

Open
smartive-nicolai[bot] wants to merge 3 commits into
mainfrom
fix/revision-table-uniformity
Open

fix(migrations): keep revision tables aligned with their models#548
smartive-nicolai[bot] wants to merge 3 commits into
mainfrom
fix/revision-table-uniformity

Conversation

@smartive-nicolai

Copy link
Copy Markdown
Contributor

<Model>Revision tables can drift away from their model definitions in ways gqm generate-migration never reports. Two independent gaps, both in src/migrations/generate.ts.

1. createdById ignored the model

createRevisionTable hardcoded:

writer.writeLine(`table.uuid('createdById').notNullable();`);

regardless of the model's creatable: { createdBy: { nonNull: false } } / updatable: { updatedBy: { nonNull: false } }. A revision's author is whoever performed the mutation — the creator on the first revision, the updater on every later one (createRevision writes createdById: ctx.user?.id) — so the column can only be non-null when both entity columns are. It now mirrors that.

2. The preamble was never reconciled on existing tables

For a revision table that already exists, the generator only diffed model fields (createRevisionFields). The fixed preamble — <model>Id, createdById, createdAt, and deleted / deleteRootType / deleteRootId — was never re-checked. Consequences:

  • revision tables created before the deleteRoot feature never received deleteRootType/deleteRootId, and never will;
  • a createdById nullability that contradicts the model is never corrected (revisionFieldNeedsSchemaAlter deliberately diffs with respectNullability: false, which is right for model fields — they are all nullable in revisions by design — but it means nothing covers the preamble).

syncRevisionPreamble now reconciles both directions, with reversible down steps. A missing deleted is backfilled from the entity table rather than defaulted, so existing revisions don't all claim the row was never deleted.

Verification

Unit tests (tests/unit/migration-revision-tables.spec.ts, 8 cases) cover both create-time and existing-table paths, including two no-op guards so an aligned schema still generates nothing.

Checked end-to-end against a real consumer schema with 56 revision tables:

released 29.3.0 this branch
gqm check-needs-migration on a drifted DB no migration needed migration needed
generated migration 41 tables get deleteRootType/deleteRootId, 5 get createdById relaxed to match their model
collateral changes none — no non-revision table touched
after applying check-needs-migration clean; migrate:rollback restores the previous state

The 5 tables whose createdById contradicted their model were exactly the ones whose revision table had been created by the generator under a nullable-createdBy model; every table created by a hand-written migration already matched.

npm run lint, npx jest tests/unit (13 suites / 151 tests) and npm run build are green. The checked-in setup migration is unchanged, since the test models all use creatable: true / updatable: true.

🤖 Generated with Claude Code

Two independent gaps let `<Model>Revision` tables drift away from the model
definitions without `generate-migration` ever noticing.

1. `createRevisionTable` hardcoded `createdById` as `notNullable()`, ignoring
   the model's `creatable: { createdBy: { nonNull: false } }` /
   `updatable: { updatedBy: { nonNull: false } }`. A revision's author is the
   creator on the first revision and the updater on every later one, so the
   column can only be non-null when both entity columns are — it now mirrors
   that instead of always being NOT NULL.

2. The generator only ever diffed model *fields* into an existing revision
   table (`createRevisionFields`), never the fixed preamble. Revision tables
   created before the deleteRoot feature therefore never received
   `deleteRootType`/`deleteRootId`, and a `createdById` nullability that
   contradicts the model was never corrected. `syncRevisionPreamble` now
   reconciles both, and backfills a missing `deleted` from the entity table
   rather than defaulting it, so existing revisions do not all claim the row
   was never deleted.

Verified against a real consumer schema (56 revision tables): released 29.3.0
reports "no migration needed" for a database where 41 tables are missing the
deleteRoot columns and 5 contradict their model's createdBy nullability; the
patched build detects it, generates exactly those changes and nothing else,
and converges after applying. Rollback restores the previous state.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@smartive-nicolai

Copy link
Copy Markdown
Contributor Author

One caveat worth a reviewer's eye, since it is the one case I could not exercise against a real schema.

The nullability sync goes both ways. Relaxing NOT NULL → nullable is always safe, and that is the only direction our own schema needed (5 tables). The opposite direction — a model with a non-null createdBy/updatedBy whose revision table is currently nullable — generates ALTER COLUMN "createdById" SET NOT NULL, which fails at apply time if that table already contains rows with a NULL author.

I left it failing loudly rather than inventing a backfill, because there is no defensible default author to write. But it means a consumer upgrading across this change may need a one-off data fix before the generated migration applies. Happy to soften it if you would rather the generator skipped the tightening and warned instead.

For reference, the deleted backfill (the other path that touches data) was validated against Postgres with rows on both sides, including a revision whose entity no longer exists — the coalesce(..., false) is what keeps the subsequent SET NOT NULL from failing on an orphaned revision.

Per review: the cascade root belongs on the entity only. `createRevision`
never writes these two columns, nothing ever reads them from a revision
table, and the restore logic and permission checks both join the entity —
so on revisions they are a snapshot taken once at table-creation time and
never maintained.

`createRevisionTable` no longer emits them, the initial-populate batch no
longer copies them, and `syncRevisionPreamble` now drops them where an
older generator left them behind (reversible). `deleted` stays: unlike the
cascade root it is genuine per-revision state that `createRevision`
maintains on every write.

Setup migration regenerated accordingly — only the two revision tables
change, the entities keep their columns. The full suite including the
DB-backed delete/restore/inheritance api tests passes without them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@smartive-nicolai

Copy link
Copy Markdown
Contributor Author

Pushed 0f43bc0 acting on the review point: the cascade root is entity-only, so deleteRootType/deleteRootId come off revision tables rather than being added to them. That inverts the second half of this PR.

Why the evidence agrees:

  • createRevision never writes them (src/resolvers/mutations.ts:602-636) — only deleted is set per revision.
  • Nothing reads them from a revision table. The restore guard (mutations.ts:472-510), the cascade-restore query (:572) and the permission check (src/permissions/check.ts:279-282) all join the entity.
  • Their only contact with revisions was createRevisionTable and the initial-populate batch — i.e. a snapshot written once at table-creation time and never maintained since.

So createRevisionTable no longer emits them, the populate batch no longer copies them, and syncRevisionPreamble now drops them where an older generator left them behind (reversible in down). The checked-in setup migration is regenerated: only the two revision tables change, both entities keep their columns.

deleted stays. Unlike the cascade root it is real per-revision state — createRevision writes it on every mutation, and consumers read it back out of the history (e.g. finding the revision at which a row was restored). Removing it would need a runtime change and would lose that.

Re-verified

Full suite green against a real Postgres: 16 suites / 168 tests, including the DB-backed delete / restore / inheritance api tests — which still pass with the columns gone, which is the strongest evidence they were unused. Lint and build green.

Against the same consumer schema as before, the generated migration is now much smaller and points the other way:

before this commit now
deleteRoot columns added to 41 tables dropped from the 15 that had them
createdById realignments 5 5 (unchanged)
non-revision tables touched none none
migration size 458 lines 198 lines

Applied → check-needs-migration clean and 0 revision tables left with either column; rolled back → all 30 columns restored; re-applied → clean again.

The SET NOT NULL caveat from my previous comment still stands unchanged.

@smartive-nicolai

Copy link
Copy Markdown
Contributor Author

Reverted in 4cfa06edeleteRootType/deleteRootId on revision tables are intended after all, so the PR is back to adding them where they're missing. The tree is now byte-identical to e44a40e, i.e. this PR is exactly the original two fixes:

  1. createRevisionTable mirrors the model's createdBy/updatedBy nullability instead of hardcoding NOT NULL.
  2. syncRevisionPreamble reconciles the preamble of existing revision tables — adding the missing deleteRootType/deleteRootId, correcting createdById nullability, and backfilling a missing deleted from the entity table.

Done as a revert commit rather than a force-push so the intermediate reasoning stays readable — happy to squash on merge.

Re-verified after the revert: npm run lint and npm run build green, and the full suite against a real Postgres is 16 suites / 167 tests passing, including the DB-backed delete/restore/inheritance api tests. Regenerating against the same consumer schema reproduces the original result exactly — 41 tables gain deleteRootType/deleteRootId, 5 get createdById relaxed, no non-revision table touched; applying it leaves check-needs-migration clean with all 56 revision tables carrying both columns.

The SET NOT NULL caveat from my first comment is the one open question left for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants