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.
dimitri-yatsenko
added a commit
that referenced
this pull request
Jul 6, 2026
…erage gaps Locks the behaviors the post-2.3 audit verified, so regressions in either direction — a documented bypass silently closing, or a blocking path silently opening — become visible. test_strict_provenance_limits.py: len/bool and query-expression 'in' bypass (documented); CLASS-form 'in' RAISES (gated __iter__); restriction-by-table bypasses; join with undeclared operand RAISES (the one gated read path, previously untested); Aggregation __len__ bypasses while its fetch RAISES; delete_quick ungated (documented); INSERT...SELECT skips per-row key consistency (documented exception); self.upstream[SelfPart] raises while direct self.PartName read is allowed; context cleared after raising make(); nested push/pop token restore. test_cascade_integrity.py: enforce post-check raises 'before its master' AND rolls back; empty-materialization sentinel previews master at 0 and deletes cleanly; U3 upward arm with SECONDARY -> master FK restricts only the correct master (bare proj() would restrict both). Also corrects the provenance.py module docstring (audit item B6): context is pushed/popped inside _populate_one in whichever process/thread runs make() — the old across-threads/fork-inheritance rationale was inaccurate. Behavior unchanged. New files only — no conflicts with the open #1480/#1484 branches.
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.