Migrations: reversing documents 0005 or accounts 0021 fails as soon as the table holds rows #571

New issue

Closed

opened 2026-10-01 10:47:44 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:47:44 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the reverse paths of the data migrations and running two of them backwards with MigrationExecutor on SQLite over seeded rows.

Related to #130.

Checked again on 2026-10-01 against main at 0a5cad587: still present.

Two migrations declare a reverse that does not work on a live instance. documents 0005 promises "a reverse that carries back, so this is not a one-way door on somebody's live instance", but its reverse re-creates the old check constraint before it has carried anything back, so it fails on the first document copy. accounts 0021 declares its data step reversible with RunPython.noop, but undoing its RemoveField re-adds a unique token column filled with one default for every row, so it fails with two invitations. In both cases the rollback aborts (in a transaction, so nothing is lost) with an IntegrityError rather than a clear refusal.

Where:
- documents/migrations/0005_polymorphic_links.py: the docstring at lines 8-10; RunPython(carry_across, carry_back) at line 126 comes before RemoveConstraint(model_name="documentcopy", name="documents_copy_of_one_document") at lines 129-131.
- accounts/migrations/0021_invite_token_fingerprint.py: RunPython(fingerprint_the_tokens, migrations.RunPython.noop) at line 33 and RemoveField(model_name="invite", name="token") at lines 45-48.
What happens:
1. Operations are undone in reverse order. Undoing documents 0005 re-adds the rendered and upload columns empty, then re-creates documents_copy_of_one_document (exactly one of rendered and upload set, from 0002_document_copies.py), and only then runs carry_back. Every existing DocumentCopy has both columns empty at that moment, so migrate documents 0004 on an instance where any document was ever copied to a store fails with IntegrityError: CHECK constraint failed: documents_copy_of_one_document. With no copies it succeeds.
2. Undoing accounts 0021 first undoes the RemoveField, which re-adds token as it was in 0002_invite_profile.py: unique, with the callable default generate_invite_token. Django evaluates that default once for the whole table, so with two or more invitations migrate accounts 0020 fails with IntegrityError: UNIQUE constraint failed: new__accounts_invite.token. With one invitation it succeeds, but that invitation's link no longer opens, since only the fingerprint was kept.
Why it matters: an operator who rolls back after a bad upgrade, trusting the docstring, meets an integrity error halfway through instead of either a working rollback or an honest refusal, and has to work out by hand which of the two it is.
Fix:
- documents 0005: move the three RemoveConstraint operations (lines 129-137) ahead of the RunPython. Forward behaviour is unchanged, and in reverse the old constraints are then re-created only after carry_back has filled the columns. The comment at lines 50-51 also says a copy with neither link is "left for the removal below to take with it"; removing columns removes no rows, so it should say the AlterField to not-null at lines 143-157 would refuse such a row.
- accounts 0021: say the step is one-way. Pass reverse_code=None (the default for RunPython) so Django refuses the rollback with IrreversibleError before touching the table, and add a line to the docstring that the tokens cannot be recovered from their fingerprints.

Done when

A new tests/test_migration_reversal.py migrates to documents 0005_polymorphic_links, creates one UploadedDocument and one DocumentCopy through the historical models, migrates back to 0004_letter_language, and asserts the copy's upload_id points at the upload; it fails today with IntegrityError.
The same file asserts that migrating accounts from 0021_invite_token_fingerprint back to 0020_profile_keyboard_shortcuts with two invitations raises IrreversibleError; today it raises IntegrityError.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the reverse paths of the data migrations and running two of them backwards with `MigrationExecutor` on SQLite over seeded rows. Related to #130. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. Two migrations declare a reverse that does not work on a live instance. documents 0005 promises "a reverse that carries back, so this is not a one-way door on somebody's live instance", but its reverse re-creates the old check constraint before it has carried anything back, so it fails on the first document copy. accounts 0021 declares its data step reversible with `RunPython.noop`, but undoing its `RemoveField` re-adds a unique token column filled with one default for every row, so it fails with two invitations. In both cases the rollback aborts (in a transaction, so nothing is lost) with an `IntegrityError` rather than a clear refusal. - **Where:** - `documents/migrations/0005_polymorphic_links.py`: the docstring at lines 8-10; `RunPython(carry_across, carry_back)` at line 126 comes before `RemoveConstraint(model_name="documentcopy", name="documents_copy_of_one_document")` at lines 129-131. - `accounts/migrations/0021_invite_token_fingerprint.py`: `RunPython(fingerprint_the_tokens, migrations.RunPython.noop)` at line 33 and `RemoveField(model_name="invite", name="token")` at lines 45-48. - **What happens:** 1. Operations are undone in reverse order. Undoing documents 0005 re-adds the `rendered` and `upload` columns empty, then re-creates `documents_copy_of_one_document` (exactly one of `rendered` and `upload` set, from `0002_document_copies.py`), and only then runs `carry_back`. Every existing `DocumentCopy` has both columns empty at that moment, so `migrate documents 0004` on an instance where any document was ever copied to a store fails with `IntegrityError: CHECK constraint failed: documents_copy_of_one_document`. With no copies it succeeds. 2. Undoing accounts 0021 first undoes the `RemoveField`, which re-adds `token` as it was in `0002_invite_profile.py`: unique, with the callable default `generate_invite_token`. Django evaluates that default once for the whole table, so with two or more invitations `migrate accounts 0020` fails with `IntegrityError: UNIQUE constraint failed: new__accounts_invite.token`. With one invitation it succeeds, but that invitation's link no longer opens, since only the fingerprint was kept. - **Why it matters:** an operator who rolls back after a bad upgrade, trusting the docstring, meets an integrity error halfway through instead of either a working rollback or an honest refusal, and has to work out by hand which of the two it is. - **Fix:** - documents 0005: move the three `RemoveConstraint` operations (lines 129-137) ahead of the `RunPython`. Forward behaviour is unchanged, and in reverse the old constraints are then re-created only after `carry_back` has filled the columns. The comment at lines 50-51 also says a copy with neither link is "left for the removal below to take with it"; removing columns removes no rows, so it should say the `AlterField` to not-null at lines 143-157 would refuse such a row. - accounts 0021: say the step is one-way. Pass `reverse_code=None` (the default for `RunPython`) so Django refuses the rollback with `IrreversibleError` before touching the table, and add a line to the docstring that the tokens cannot be recovered from their fingerprints. ## Done when - A new `tests/test_migration_reversal.py` migrates to `documents 0005_polymorphic_links`, creates one `UploadedDocument` and one `DocumentCopy` through the historical models, migrates back to `0004_letter_language`, and asserts the copy's `upload_id` points at the upload; it fails today with `IntegrityError`. - The same file asserts that migrating `accounts` from `0021_invite_token_fingerprint` back to `0020_profile_keyboard_shortcuts` with two invitations raises `IrreversibleError`; today it raises `IntegrityError`.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:47:44 +00:00

tiagoagueda added the

tier

bug

labels

2026-10-01 10:47:44 +00:00

tiagoagueda referenced this issue from a commit

2026-10-04 01:59:03 +00:00

Make two migration reverses work or refuse plainly

tiagoagueda closed this issue

2026-10-04 01:59:03 +00:00