Router-gated enforcement, cascade timestamp provenance, and an inverse revive trigger (2.11.0) - #54
Merged
Merged
Conversation
…shape alone Every operation this kit emits is PostgreSQL DDL, and RunSQL.database_forwards already asks router.allow_migrate(alias, app_label) before running one. A model a project's own router sends to another backend therefore earned triggers, rules and policies that backend's parser refuses -- reported against ClickHouse as DB::parseQueryAndMovePosition on a CREATE TRIGGER (#52). The generator now asks the same question one release earlier, narrowed by model, through the new leaf module guitars.routing. A model keeps its enforcement while any alias router.allow_migrate_model accepts is PostgreSQL, and is otherwise skipped with a note in _skip_note's voice. allow_migrate_model rather than db_for_write: that one answers where a query goes, returns a single alias, and falls back to default for the read-replica router that implements it and not allow_migrate. any rather than all: withholding a rule where it can apply leaves .delete() permanently deleting rows on PostgreSQL, which is worse than aborting a migration on the other alias. The gate covers all seven families, not only tenancy, and is one answer read by hard_delete() and sweepowned through introspection.owner_arms and owned_tenancy_refusals as well as by the generator. Three walks are deliberately left ungated -- scanning.live_tables, _live_names and _index_reverse_relations answer "is this name still held", and gating them would start retiring live rules. audittenancy and sweepowned now refuse a non-PostgreSQL connection; RetireEnforcement stands aside on one. Provably a no-op without a router: the predicate short-circuits on an empty DATABASE_ROUTERS before touching connections, so makeguitarmigrations still opens no database and generates byte-identical migrations. No generated SQL, header, [SQL:...] identity or frozen name changed. Closes #52. See ADR 0022. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… live child The reverse-FK cascade rule's action carried no AND _deleted_at IS NULL, alone among the families -- the owned rule, the self-cascade trigger, the MTI redirect rule and the own-table rule all have one. So archiving a parent overwrote the _deleted_at of a child the caller had already archived, silently. Reproduced on 2.10.0: a child archived at 2020-01-01 read 2026-09-22 after its parent went, while the same shape through the owned family kept its value. _deleted_at is the only record of when a row was archived, and _archives is ordered and filtered by it, so the loss is invisible until the column is compared against an external record. It also destroyed archive provenance, which is the blocking constraint on the inverse cascade in #51: after the overwrite a child cascaded away by its parent and one archived deliberately beforehand are indistinguishable, so reviving the parent would resurrect both. The action now also copies new._deleted_at rather than calling NOW(). NOW() is transaction_timestamp(), so parent and child agreed only when one transaction archived both through the kit's own ON DELETE rule. The copy makes one archive one instant on every path, and chains down a subtree with no second mechanism because each level's rule fires on the level above's UPDATE. Both changes are to the private template the generator renders. The frozen public CREATE_SOFT_DELETE_RELATED_OBJECTS_RULE and _VIA keep their old bodies: a migration generated before 1.1.0 reads them by name at migrate time, so editing them would give the fixed rule on a fresh database and the old one on an incrementally-migrated database, from an identical history. The moved [SQL:...] identity emits one CREATE OR REPLACE RULE per cascade relation -- replace = forward for this family, so no DROP and no window without a rule. tests/testapp gains 0057_auto_enforcement for it, and 0058_retirement_host, an empty migration giving the retirement-ordering fixtures a node after the newest create: a file that both creates and retires one key is not its own evidence, so the two cannot share one. Closes #53. See ADR 0023. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every cascade family was gated on the archive transition alone, so clearing a parent's _deleted_at left its children archived. The parent read healthy, its children were invisible to every live-manager query, and nothing raised or logged. Re-ingest is the motivating path: a webhook meets a soft-deleted row on the same natural key and has to revive it, and inserting beside it is impossible because the archived row still holds the unique key. A new private rule family, soft_delete_revive_*, is emitted from inside _cascade_operations' own loop against the same key and after the same refusals, so which relations carry a revive IS which carry a cascade -- the precedent the 2.6.0 owned sweep set. Its name sizes every variable segment, nothing predating it, while keeping the primary/VIA split so the name stays a pure function of the dedupe key. Not a predicate flip. A bare inverse would also resurrect a child the caller archived before the parent went, failing toward exposing data. The guard is the archive timestamp, AND _deleted_at = old._deleted_at, which the previous release made exact by stamping a cascaded child with its parent's own value. Verified against PostgreSQL before any wiring: it implies IS NOT NULL, chains level by level because each level's rule fires on the level above's UPDATE, and stays correct for a multi-row statement, whose action expands as a join. No statement-level sweep, unlike the owned family: ADR 0014's problem was a guard reading sibling liveness that the same statement mutates, and this predicate is per-pair, reading nothing the statement changes but the child's own column. Plain and VIA only. The owned family keeps a last-owner NOT EXISTS whose inverse asks about rows the reviving statement is changing; the self-referential trigger recurses a subtree its transition tables carry no record of, and still writes NOW(), so its descendants never carry the root's value. A retirement now drops both rules per key, each arm testing its own recorded map so an upgrading project never gets a DROP RULE for a revive nothing created, and each ordered against the migration that created its own rule (ADR 0021). Also pinned, from the issue's secondary note: objects.bulk_update against archived rows returns 0 and changes nothing, the live manager's filter constraining the WHERE as well as the SELECT. Manager semantics unchanged. Closes #51. See ADR 0024. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… checklist CLAUDE.md's cascade bullet ended at the 2.6.0 owned sweep, so the two behaviours this branch stack adds were absent from the checklist that exists to carry them. Folded in place rather than appended -- the file is at its 100-line cap. Also pins two things the review asked be proved rather than assumed: the digest guard is waived for an app retiring a key recorded in only one of the two rule families, and the upgrade path every consumer takes -- a project on the pre-2.12.0 cascade digest with no revive recorded -- emits plain CREATE OR REPLACE for both rules on the --adopt path as well as the plain one, so no forward DROP leaves a window without a cascade rule mid-migrate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… stack
Rule names. _revive_rule_name had two optional sized segments -- the schema and
the key the primary form does not carry -- so ('myapp.x', None) and
('myapp', 'x') named one rule on one owner table, and PostgreSQL keeps whichever
was created last. _owned_rule_name escapes this only because its key is
mandatory, so the segment count disambiguates. A literal `via` now splits the two
forms; a sized segment always opens with a digit, so nothing can be read as it.
Regenerated 0058 accordingly, which is free before release and impossible after.
The cycle graph. _rule_update_edges was left ungated on the ground that an extra
edge only adds refusals and so fails safe. The module's own comment already said
otherwise: an invented edge closes a cycle that cannot form and takes the
legitimate rule pointing back down with it. A routed-away table on a cycle would
have withheld a rule between two PostgreSQL tables. Gated at both ends, and
ADR 0022 now records the decision rather than omitting the walk.
The vendor note. The generator and tenancy discovery rendered two spellings of
one skip, differing by a trailing sentence, so the report's equality dedupe could
not collapse them and a tenanted model printed twice. One string per model now.
The test that claimed to prove the dedupe counted a longer string no printed line
could contain, so it read 1 whether the dedupe fired or was deleted outright.
Retirement reporting. A CascadeRetirementSite carries no family, so the
missing-edge note named the cascade rule for a revive site -- and the two resolve
to different creates, this repo's own shape. And the unretirable-key note named
only the cascade half to drop by hand, against the precedent the owned pair set:
the revive left live would revive children on a later un-archive, the exposing
direction. Both now name the halves the project actually recorded.
Parity. _append_cascade_revive recorded no re-adoption edge, and
_scoped_cascade_gap_notes neither consulted the router nor asked about the
inverse family -- so the shape every upgrading project is in, cascade recorded
and revive not, reported no gap for the rule a scoped run was failing to create.
Also: _claim_rule_name was passed the quoted owner table where the cascade path
passes the bare one, splitting the (table, rule_name) key space and printing
quotes inside the report; and tests/test_routing.py's command helper left
soft_delete_revive uncleared, so one assertion passed through the map it forgot.
Every fix carries a test that fails against the unfixed code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…und 1 The dead note. _unmapped_cascade_notes has been defined, tested directly and never printed since 2.9.0 -- while _retired_cascade_operations' own comment promised the key it withholds is "named in _unmapped_cascade_notes instead". A consumer who deletes a model and drops its app from LOCAL_APPS kept both rules live on a table no model claims, with --check green. Now wired to stdout, and a test asserts the wiring rather than calling the method directly, which is how it stayed invisible through two rounds of tests that passed. The edge gate. Round 1's two new tests both asserted on the wrong end of the edge: a cascade edge is (target, declaring_table), so routing the target away is what they measured, and the model gate had no test at all -- deleting it left the suite green. One test now isolates each gate. The gate itself was also incomplete: it asked about the target's column owner where the generator refuses on the related model, so an MTI child routed away while its ancestor stayed still contributed an invented edge, which could close a cycle and withhold a rule between two PostgreSQL tables. Two notes named the wrong family. The scoped-gap note fires on the shape every upgrading project is in -- cascade recorded, revive not -- and said the *cascade* rule was skipped, sending the reader to a rule their migrations already carry. The unretirable-key note opened "Cascade rule ... is recorded" while printing the revive's DROP. Both now name the halves they mean. Test hygiene, all three from the same class round 1 fixed in one place and left in others: the two clash builders cleared one rule family and counted operations from both; the upgrade test's DROP-free assertion is true by construction and now says so; and the self-cascade docstring rewrite dropped the sentence explaining its setup and described the weaker of its two assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ation gap The gate was over-applied, and the worst case is bricking rather than a missing rule. `Field.related_model` for a `ForeignKey(SomeProxy)` *is* the proxy -- `_index_reverse_relations` normalises that to `_meta.concrete_model` precisely because 2.9.1 lost a cascade rule to it -- so every routing gate added on a related model asked the router about a model the generator never asks about. A proxy routed away made `_rule_update_edges` drop a real cycle edge while the generator emitted both rules, and PostgreSQL then rejects every UPDATE to either table, a plain save() included, with --check green. `migrates_to_postgresql` now resolves the concrete model itself, centrally, as `is_mti_child` guards the proxy question for every caller rather than per site. `_rule_update_edges` also asked about a *third* model -- the target's column owner -- where the generator gates exactly two per relation, the one declaring the key and the one it points at. Dropped; the round-1 comment claiming `_build_operations` walks the column owner was simply wrong, it walks the model. The revive tests had no coverage of the rule's foreign-key correlation: every "archived independently" row used a different timestamp from the parent being revived, so stripping `"<fk>" = old."<pk>"` from the rule left all of them green. Verified by doing exactly that -- only the new test, which archives a bystander's parent at the *same* instant, fails. That is the exposing direction ADR 0024 is written against, and it was untested. Also: the loop-invariant router question moved out of `_declared_owning_fields`' comprehension to beside its existing early return, off `hard_delete()`'s per-field path; both vendor refusals now name `--database` instead of asserting there is nothing to audit, which is false for a project whose guitars models live on a non-default alias; and the two family labels agree with their verbs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… second rule Two ON UPDATE rules on one table double PostgreSQL's rewriter expansion per cascade level. Both are ON UPDATE TO <table> with no column list, so the rewriter expands both arms for any UPDATE and then rewrites each arm against the child's two rules: a chain of depth N costs 2^(N+1)-1 query trees. The revive arm inside a cascade expansion is provably dead -- new._deleted_at IS NULL is constant-false there -- but the rewriter builds it and the planner plans it. Measured at depth 6: 127 query trees and 93ms to plan `UPDATE t0 SET label='x'`, against 7 and 2ms with the cascade rules alone. It doubles per level and is paid on every write, a plain save() included, not only on deletes and revives. The suite could not see it -- testapp's deepest cascade chain is three. That is precisely the cost ADR 0018 converted the self-referential cascade to avoid, and the cost CLAUDE.md gives as the whole reason for the rule-cycle refusal, arriving by expense rather than by error. The family is now an AFTER UPDATE ... FOR EACH STATEMENT trigger over the transition tables, modelled on the owned sweep: 7 query trees and ~6ms at the same depth. Semantics are unchanged and verified against PostgreSQL before any wiring -- a cascaded child revives, a child archived earlier keeps its stamp, and a child of another parent archived at the same instant is spared by the key correlation. No refusal on a key rewrite, unlike the owned sweep: the same shape leaves children archived, which is the state they were already in. Consequences: the name spells the owner table too, a function being namespaced per schema where a trigger is per table; it is claimed on the name alone as the sweep is; a retirement drops the trigger with its function; a rename drops both under every prior spelling; the family records no cross-app object refs at all, CREATE TRIGGER naming only its own table; and RetireEnforcement's whole-table form gained the prefix, its column-scoped form reaching no trigger by design. Found by review round 4, which measured it. Escalated rather than auto-fixed -- the design was not in question, the unpriced cost was. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n guarded Review round 4 found that a routed-away MTI child can let retirement drop a live rule on the PostgreSQL alias: the cascade keys its foreign keys produce name the ancestor's table and the referrer's, neither of which is the routed-away one, so the positive-evidence test passes. Declined rather than fixed -- the shape cannot function at all, the child's table carrying a foreign key into the ancestor's across two servers -- and recorded so the next loop does not re-derive it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The conversion in d5aa8e6 was justified by the rule form's 2^depth planning cost and left its replacement's cost unstated -- the same omission one size smaller. Measured for a 200-row UPDATE over a depth-6 cascade chain: 4.8ms with the cascade rules alone, 17.9ms with the revive triggers, 67.0ms with revive rules. The trigger fires on every UPDATE to a table carrying one and runs a no-op join when nothing was revived, exactly as the owned sweep does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CREATE TRIGGER has no OR REPLACE, and the conversion carried neither a _REPLACE_ nor an _ADOPT_ form -- so both the stale-digest path and --adopt emitted a bare create over a live trigger, aborting migrate with "already exists" and taking the rule beside it down, the operation being atomic. Both siblings ship that pair; this family shipped neither. The next edit to the function body would have moved the [SQL:...] identity and reached it in every consuming project, and --adopt reached it on any project that already had the triggers -- the one failure adopt exists to prevent. tests/test_command.py pinned the broken shape as a "guard"; it now asserts the drop that --adopt must emit. The dollar-quoted body inherited no $$ refusal. Its two sibling trigger families each refuse a rendered identifier containing $$, because an identifier admits '$' and a db_table like 'a$$b' closes the quoting early and fails migrate with a bare syntax error. Added, with the same escalation over a live object. The name folds in two tables since the conversion, but both rename branches asked about the related table alone. Renaming the *parent* left the old trigger and function live beside the new pair, running the same predicate on every update to that table for good; renaming it and relaxing the key together emitted a bare DROP TRIGGER for a name nothing has. Now handled by a _revive_form mirroring _owned_sweep_form, over the cross product of both chains. _revive_updated_at read models_by_table without resolving a proxy, where _retired_cascade_column fifteen lines below guards exactly that -- so a proxy registered in an earlier app made a retirement's reverse rebuild the function without the _updated_at splice its forward had, with --check green. Also corrected three claims this loop had overstated: the revive name is *not* injective (two optional schema segments, inherited from _owned_sweep_name, caught by the clash report rather than proved away); RetireEnforcement's whole-table form takes the trigger but not the function, and its column form reaches neither; and the key-rewrite argument covered losing a pairing but not a permutation making a wrong one, which is the exposing direction and is undetectable here for the reason the owned sweep records. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One release rather than the three this was planned as. The review loop's fix commits spanned all three changes, so branches 1 and 2 were frozen at their pre-review state -- merging them would have shipped the router gate with the defect round 3 found, then fixed it two releases later. Also repairs two stale doc paragraphs the collapse surfaced: docs/soft-deletion.md and docs/migrations.md still described the revive family as a rule with its pre-conversion naming. Both edits were written during round 4 and silently lost -- a failed assertion aborted the script before it wrote the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI runs `pytest -n auto --cov=guitars` with fail_under = 100. I ran `pytest -q` throughout, so I ran half the gate and reported it green: the branch landed at 99.52%, twelve statements and ten branches short, every one of them code added this session. Covered: both vendor CommandErrors (audittenancy, sweepowned), the routing early return in _declared_owning_fields on hard_delete()'s path, RetireEnforcement standing aside on a non-PostgreSQL connection, the revive family's $$ refusal and its escalation over a live trigger, and _revive_updated_at's two early returns. The scan half of the revive retirement had no corpus to read -- no committed migration carries that header -- so it is fed one synthetic file through the real walk rather than by editing a fixture whose history never created the trigger to drop. That also closes the gap round 4 flagged and round 5 left open, and exercises the provenance dedupe a single corpus file never reaches. Also drops _record_retirement_edge's defaulted creates_by_key. Both callers pass it explicitly, so the default was unreachable -- and a default there would hand a caller that forgot it the other family's provenance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #52, #53, #51.
Planned as three releases; shipping as one. The review loop's fix commits spanned all three changes, so the first two branches were frozen at their pre-review state — merging them would have shipped the router gate carrying a defect this loop later proved could brick tables, then fixed it two releases on.
What's in it
#52 — enforcement is generated per database, not per model.
RunSQL.database_forwardsalready asksrouter.allow_migrate(alias, app_label, **hints), somigrateconsults the router about every operation this kit emits while the generator asked only about model shape. A model routed to ClickHouse got PostgreSQL triggers and policies, andmigrate --database <alias>died in that parser. New leaf moduleguitars.routingcloses the gap withallow_migrate_model(notdb_for_write, which a read-replica router answers wrongly) andanyover the aliases (withholding a rule where it can apply destroys rows). Provably a no-op without a router: the predicate short-circuits before touchingconnections, asserted by a test that makes every connection access raise.#53 — the cascade rule re-stamped. Found while planning #51 and filed separately. The reverse-FK cascade action carried no
AND _deleted_at IS NULL, alone among the families, so archiving a parent silently overwrote the_deleted_atof a child archived earlier. Reproduced: a child archived at2020-01-01read2026-09-22after its parent went. It now also copiesnew._deleted_atrather than callingNOW(), making one archive one instant on every path.#51 — reviving a parent revives the children that archive took. A statement-level
AFTER UPDATEtrigger paired with each cascade rule, matching on the archive timestamp #53 made exact. Not a predicate flip: a bare inverse would resurrect a child archived deliberately, which fails toward exposing data.Review loop — five rounds, 42 findings
Stopped when loop-introduced outnumbered pre-existing, which is the condition firing on its own terms.
Two findings changed the design. Round 4 measured that the revive family as a second
ON UPDATErule cost 2^depth query trees on every write, a plainsave()included — 127 trees and 93 ms to plan at depth 6 against 7 and 2 ms — because PostgreSQL's rewriter expands both rules and recurses. It is now a statement trigger, as ADR 0018 did for the self-referential cascade. Round 3 found that rounds 1–2 had over-applied the router gate and could drop a real cycle edge, bricking every table on it.The loop wrote a material part of this branch, and round 5's four hard
migratefailures were all in round 4's conversion — a redesign executed inside a review loop rather than planned. Reviewers were read-only throughout; every fix carries a test that fails against the unfixed code.Not examined
README.md,docs/adr/README.mdanddocs/tenancy.mdwere never cited by any round. The other prose files were discharged by the maintainer without a line-level read, on a summary I produced.docs/adr/0023was opened by nobody until the final pass.Decisions recorded
ADRs 0022, 0023 and 0024, including the costs this loop got wrong twice before pricing them: the rule form's planning blow-up, and the trigger's own standing cost (17.9 ms against 4.8 ms for a 200-row update at depth 6).
Upgrading
Run
makeguitarmigrationsandmigrate. Consumers get one enforcement migration per app;makemigrations --checkis red until they do. Timestamps already overwritten by the re-stamp bug are not recoverable.🤖 Generated with Claude Code