Uh oh!
There was an error while loading. Please reload this page.
fix(cascade): restrict master from every restricted Part; validate part_integrity - #1484
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.
…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.
MilagrosMarin
left a comment
There was a problem hiding this comment.
Thanks @dimitri-yatsenko — real integrity gap caught. The (part, master) pair-keyed dedup is minimal and precisely-targeted, and the repro in test_cascade_two_parts_same_master_both_restrict (Ext ← {PartA, PartB} sharing one Ext row across two different masters) hits exactly the topology the old visited_masters set silently dropped. Under OR semantics the extra walks can only add, never subtract, so no risk of over-restricting cases the old behavior got right.
The eager validation on Diagram.cascade correctly mirrors Table.delete, and rejecting "cascade" on Table.drop is the right call — better a loud error than the silent "ignore" degradation.
The trace-stops-at-master pin is well-reasoned: it locks the pure-ancestor-walk semantics against a future well-intentioned "fix" toward the down-collection that discussion #1232 described but was never implemented.
One minor: the _propagate_restrictions docstring doesn't mention the pair-keyed invariant — future maintainers will see the inline comment on visited_part_master_pairs, but a one-liner at the method-docstring level ("each restricted Part fires its own upward walk once") would help someone landing here without the commit context.
Approving.
Uh oh!
There was an error while loading. Please reload this page.
…-docstring docs(#1484): document pair-keyed part-to-master invariant in _propagate_restrictions
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.