fix(cascade): restrict master from every restricted Part; validate part_integrity#1484
Open
dimitri-yatsenko wants to merge 1 commit into
Open
fix(cascade): restrict master from every restricted Part; validate part_integrity#1484dimitri-yatsenko wants to merge 1 commit into
dimitri-yatsenko wants to merge 1 commit into
Conversation
…rt_integrity Two fixes + one semantics pin from the 2.3 post-release audit: 1. part_integrity='cascade' under-restricted the master when two Parts of the same master were restricted via different FK paths from the seed: the upward part->master walk was deduplicated per MASTER (first Part wins), so later Parts never contributed their master rows. Empirically, with Ext <- Master.PartA and Ext <- Master.PartB (part rows under masters 1 and 2), cascading from Ext restricted only master 1 — the delete then removed master 2's PartB row while master 2 survived, silently violating the compositional integrity 'cascade' promises. The dedup is now keyed by (part, master) PAIR: every restricted Part fires the walk once. The pair set is finite and global across passes, so the multi-pass loop still terminates; duplicate master restrictions from sibling re-fires are harmless under OR semantics. 2. Eager part_integrity validation: Diagram.cascade() accepted any string (a typo like 'casade' silently degraded to enforce/ignore behavior) and Table.drop() only compared == 'enforce' (drop(part_integrity='cascade') silently behaved as 'ignore' although cascade is delete-only). Both now raise ValueError, mirroring Table.delete(). 3. Pins shipped trace semantics at master-part boundaries: trace walks ancestor edges only and does NOT descend from an ancestor Master into its Parts (merge shape Parent -> Master.P -> Master -> Child: trace reaches Master; Master.P and Parent raise). Matches provenance.md; corrects the unimplemented down-collection described on discussion 1232; capability tracked as #1481. Tests: two-parts-same-master regression (fails on the old dedup), cascade/drop validation, merge-shape trace pin. Full cascade+trace suites 46/46 on MySQL and PostgreSQL; wider regression (cascading_delete, dependencies, unit) 333 passed.
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.
Second code fix from the 2.3 post-release audit (companion to #1480).
1.
part_integrity="cascade"under-restriction — CRITICAL (empirically confirmed, both backends)The upward part→master walk was deduplicated per master (
visited_masters, first Part wins). When two Parts of the same master were restricted via different FK paths from the seed, the second Part never contributed its master rows:Ext ← Master.PartA,Ext ← Master.PartB; PartA row (ext 1) under master 1, PartB row (ext 1) under master 2.Diagram.cascade(Ext & {"ext_id": 1}, part_integrity="cascade")restricted only master 1 — the delete then removed master 2's PartB row while master 2 survived, silently violating the compositional integrity"cascade"promises. (The default"enforce"catches this topology via its post-check; only"cascade"failed silently.)Fix: dedup by (part, master) pair — every restricted Part fires the walk once. Termination is preserved (finite pair set, global across passes); sibling re-fires append duplicate master restrictions, harmless under OR semantics. Distinct from the documented single-shortest-path limitation.
2. Eager
part_integrityvalidationDiagram.cascade()accepted any string — a typo ("casade") silently degraded to enforce/ignore behavior. Now raisesValueError(mirrorsTable.delete).Table.drop()only compared== "enforce"—drop(part_integrity="cascade")silently behaved as"ignore"although cascade is delete-only. Now raisesValueError.3. Merge-shape trace semantics pinned
New test
test_trace_stops_at_master_no_part_down_collection: trace walks ancestor edges only and does not descend from an ancestor Master into its Parts (Parent → Master.P → Master → Child: trace reaches Master;Master.P/Parentraise). Matches provenance.md §2; corrects the unimplemented down-collection described in discussion #1232 (correction posted there); the capability is tracked as #1481. The pin exists so a future "fix" toward that promise can't silently change semantics.Validation
master count == 1), passes on the fix (== 2, and end-to-end delete removes both compositional units, control master untouched).Docs: the audit-batch PR (datajoint/datajoint-docs#195) adds the cascade Limitations section and does not document the once-per-master behavior — because this PR fixes it.