Skip to content

Commit 006d06d

Browse files
fix(diagram): walk all FK paths part->master; scrub alias-node docstrings
Complete the MultiDiGraph migration (#1492) in the part->master upward walk. _propagate_part_to_master used nx.shortest_path, following a single FK chain and silently dropping any others; replace it with nx.all_simple_edge_paths so a Part reachable from its Master through multiple FK chains (or parallel FK edges) is restricted through every one, combined with OR. Remove the now-unused _edge_props helper and the stale single-FK-path limitation note. Scrub residual alias-node language from the upward-walk docstrings: aliased FKs are direct parallel edges in the MultiDiGraph, not transparent alias-node hops. Add a two-chain part-of-part cascade test exercising the all-paths walk on both MySQL and PostgreSQL.
1 parent afc350f commit 006d06d

2 files changed

Lines changed: 106 additions & 52 deletions

File tree

src/datajoint/diagram.py

Lines changed: 48 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -887,9 +887,9 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
887887
by walking the full path (intermediate Parts get restricted too) and
888888
renamed FKs via the upward rules.
889889
890-
Alias nodes (integer-named graph nodes inserted for aliased edges)
891-
are transparent — both half-edges carry the same `attr_map` props,
892-
so we read props from one and skip the alias node when walking.
890+
A renamed/aliased FK is a direct parallel edge in the MultiDiGraph
891+
(keyed by the child-side attr tuple), carrying its own ``attr_map``;
892+
the upward rules read that map directly — there is no alias-node hop.
893893
894894
After the walk, the master's restriction is **materialized** to a
895895
literal value tuple via ``to_arrays()``. This is required for
@@ -912,56 +912,64 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
912912
913913
Limitations
914914
-----------
915-
- **Single FK path**: ``nx.shortest_path`` returns *one* path from
916-
``master_name`` to ``part_node``. If a Part is reachable from its
917-
Master through multiple distinct FK chains (e.g. references two
918-
different intermediate Parts), restrictions through the
919-
non-shortest paths are not applied. This pattern is unusual; if a
920-
schema hits it, the user is responsible for restricting the
921-
additional paths explicitly via ``part_integrity="ignore"`` plus
922-
manual ``delete()`` calls.
915+
- **Multiple FK paths**: every simple FK path from ``master_name`` to
916+
``part_node`` is walked (``nx.all_simple_edge_paths`` also enumerates
917+
parallel FK edges between the same table pair). A Part reachable from
918+
its Master through several distinct FK chains contributes master rows
919+
through each, combined with OR — a master row is affected if *any*
920+
part-path taints it.
923921
- **Memory cost of materialization**: ``master_ft.proj().to_arrays()``
924922
pulls the matching master primary keys into Python memory. Cost is
925923
bounded by the count of *distinct* master rows referenced by the
926924
matching parts — typically small for surgical cascades, but can
927925
grow with bulk cascades on tables with many master rows. Cascade
928926
*preview* (``Diagram.cascade(...).counts()``) pays the same cost.
929927
"""
928+
# Enumerate EVERY simple FK path master → part. Because the graph is a
929+
# MultiDiGraph, `all_simple_edge_paths` also yields parallel FK edges
930+
# between the same table pair, each as its own (parent, child, key)
931+
# tuple — so a Part reachable through more than one FK chain is
932+
# restricted through all of them, not just the shortest (see #1492,
933+
# completing the multigraph migration). OR convergence (cascade
934+
# semantics) combines the chains: a master row is affected if any
935+
# part-path taints it.
930936
try:
931-
path = nx.shortest_path(self, master_name, part_node)
937+
edge_paths = list(nx.all_simple_edge_paths(self, master_name, part_node))
932938
except (nx.NetworkXNoPath, nx.NodeNotFound):
933939
return False
934-
935-
# The path is a sequence of real tables (no alias nodes exist anymore).
936-
real_path = list(path)
937-
if len(real_path) < 2 or real_path[-1] != part_node or real_path[0] != master_name:
940+
if not edge_paths:
938941
return False
939942

940-
# Walk real_path in reverse (child → parent direction). For each
941-
# adjacent (parent, child) pair, look up the FK edge props.
943+
# Walk each path child → parent (reverse of the master → part order) so
944+
# every parent accumulates from its already-restricted child. Dedup by
945+
# (parent, child, key) across overlapping paths: re-applying an edge is
946+
# wasteful and, for the non-idempotent proj rules, would double-append.
942947
any_propagated = False
943-
for i in range(len(real_path) - 1, 0, -1):
944-
child = real_path[i]
945-
parent = real_path[i - 1]
946-
edge_props = self._edge_props(parent, child)
947-
if edge_props is None:
948-
return any_propagated # Path broken (shouldn't happen if shortest_path succeeded)
949-
950-
attr_map = edge_props.get("attr_map", {})
951-
aliased = edge_props.get("aliased", False)
952-
child_ft = self._restricted_table(child)
953-
child_attrs = self._restriction_attrs.get(child, set())
954-
955-
self._apply_propagation_rule_upward(
956-
child_ft,
957-
child_attrs,
958-
parent,
959-
attr_map,
960-
aliased,
961-
mode,
962-
restrictions,
963-
)
964-
any_propagated = True
948+
walked_edges = set()
949+
for edge_path in edge_paths:
950+
for parent, child, ekey in reversed(edge_path):
951+
if (parent, child, ekey) in walked_edges:
952+
continue
953+
walked_edges.add((parent, child, ekey))
954+
edge_props = self.get_edge_data(parent, child, ekey)
955+
if edge_props is None:
956+
continue # Path broken (shouldn't happen for an enumerated edge)
957+
958+
attr_map = edge_props.get("attr_map", {})
959+
aliased = edge_props.get("aliased", False)
960+
child_ft = self._restricted_table(child)
961+
child_attrs = self._restriction_attrs.get(child, set())
962+
963+
self._apply_propagation_rule_upward(
964+
child_ft,
965+
child_attrs,
966+
parent,
967+
attr_map,
968+
aliased,
969+
mode,
970+
restrictions,
971+
)
972+
any_propagated = True
965973

966974
# Materialize the master's restriction so subsequent forward cascade
967975
# doesn't produce self-referential subqueries. Replace the master's
@@ -989,18 +997,6 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
989997

990998
return any_propagated
991999

992-
def _edge_props(self, parent, child):
993-
"""
994-
Return the FK edge properties for a direct ``parent → child`` foreign
995-
key, or ``None`` if there is no such edge. When multiple parallel FKs
996-
exist between the pair, the first one is returned (consistent with the
997-
single-FK-path limitation documented on ``_propagate_part_to_master``).
998-
"""
999-
data = self.get_edge_data(parent, child)
1000-
if not data:
1001-
return None
1002-
return next(iter(data.values()))
1003-
10041000
def counts(self):
10051001
"""
10061002
Return affected row counts per table without modifying data.

tests/integration/test_cascade_integrity.py

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,61 @@ class P(dj.Part):
155155
f"only master 1 (via the secondary-FK part row) must be restricted; got {counts} — "
156156
"a bare proj() on the U3 arm would have restricted both masters."
157157
)
158+
159+
160+
def test_part_to_master_walks_all_fk_paths(schema_by_backend):
161+
"""A Part reachable from its Master through more than one FK chain must be
162+
restricted through EVERY chain, not just the shortest (see #1492). `Master.P`
163+
references both its Master directly (the implicit `-> master` edge) AND a
164+
sibling Part `Master.Q`, so there are two simple FK paths Master -> Master.P
165+
(`Master -> Master.P` and `Master -> Master.Q -> Master.P`). An external
166+
`Ext` feeds a restriction into both parts, firing the part->master upward
167+
walk along both chains. The pre-#1492 `nx.shortest_path` walk followed a
168+
single chain; the all-paths walk (`nx.all_simple_edge_paths`) must exercise
169+
both branches without over- or under-restricting the master's part-group."""
170+
171+
@schema_by_backend
172+
class Ext(dj.Manual):
173+
definition = """
174+
ext_id : int32
175+
"""
176+
177+
@schema_by_backend
178+
class Master(dj.Manual):
179+
definition = """
180+
master_id : int32
181+
"""
182+
183+
class Q(dj.Part):
184+
definition = """
185+
-> master
186+
q_id : int32
187+
---
188+
-> Ext
189+
"""
190+
191+
class P(dj.Part):
192+
definition = """
193+
-> master
194+
p_id : int32
195+
---
196+
-> master.Q
197+
-> Ext
198+
"""
199+
200+
Ext.insert([(1,), (2,)])
201+
Master.insert([(1,), (2,)])
202+
# master 1: one Q and one P (P references that Q), both via ext 1.
203+
# master 2: one Q and one P, both via ext 2 (must stay untouched).
204+
Master.Q.insert([(1, 10, 1), (2, 20, 2)])
205+
Master.P.insert([(1, 100, 10, 1), (2, 200, 20, 2)])
206+
207+
# Seed taints ext 1 only. Forward cascade restricts the Q and P rows that
208+
# reference ext 1; the part->master walk then pulls master 1 up both chains.
209+
counts = dj.Diagram.cascade(Ext & {"ext_id": 1}, part_integrity="cascade").counts()
210+
211+
# Only master 1 is pulled in, with its whole part-group (its Q and its P);
212+
# master 2 and its parts are untouched.
213+
assert counts.get(Master.full_table_name, 0) == 1, f"only master 1 must be restricted; got {counts}"
214+
assert counts.get(Master.P.full_table_name, 0) == 1, f"master 1's P must be pulled in; got {counts}"
215+
assert counts.get(Master.Q.full_table_name, 0) == 1, f"master 1's Q must be pulled in; got {counts}"

0 commit comments

Comments
 (0)