Skip to content

Commit 18e5649

Browse files
fix(cascade): restrict only the rows a cascade reaches
Three cases deleted rows outside the cascade: - `(T & dj.Top(n)).delete()` and `delete_quick()` ignored the Top and deleted every row matching the other conditions. A Top seed is now replaced by the primary keys it selects. - A parent restriction was copied to a child whenever the restricted attributes were in the child's primary key. Through a nullable foreign key that shares a primary-key column, the copy also deleted child rows whose foreign key is NULL; in a diamond it missed rows. The copy now applies only when the restricted attributes are among the foreign key's attributes and the foreign key lies in the child's primary key. Otherwise the child is restricted by the projected parent. - With part_integrity="cascade", the part-to-master lift copied the part's condition to the master, which also deleted masters with no matching part rows. The master is now restricted by the restricted part, so a master joins the cascade only if one of its part rows did. The walk's restrictions on intermediate parts are restored once the master is materialized; the master cascades back to all of its parts. The Diagram copy constructor now copies each node's restriction list instead of deep-copying projected query expressions, which hold a live connection.
1 parent a00b89f commit 18e5649

3 files changed

Lines changed: 269 additions & 14 deletions

File tree

‎src/datajoint/diagram.py‎

Lines changed: 68 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,20 @@ def stroke_rule(light_hex, dark_hex, opacity=None):
154154
return "<style>\n@media (prefers-color-scheme: dark) {\n" + body + "\n}\n</style>"
155155

156156

157+
def _seed_restriction(table_expr):
158+
"""
159+
Return a traversal seed's restriction and the attributes it names.
160+
161+
A ``dj.Top`` restriction is an ORDER BY ... LIMIT, not part of the WHERE
162+
clause, so a seed restricted by ``Top`` is replaced by the primary keys
163+
it selects. Otherwise the traversal would start from every row that
164+
matches the seed's other conditions.
165+
"""
166+
if table_expr._top is not None:
167+
return AndList([table_expr.keys()]), set(table_expr.primary_key)
168+
return AndList(table_expr.restriction), set(table_expr.restriction_attributes)
169+
170+
157171
class Diagram(nx.MultiDiGraph): # noqa: C901
158172
"""
159173
Schema diagram as a directed acyclic graph (DAG).
@@ -210,9 +224,12 @@ def __init__(self, source, context=None) -> None:
210224
self._expanded_nodes = set(source._expanded_nodes)
211225
self.context = source.context
212226
self._connection = source._connection
213-
self._cascade_restrictions = copy.deepcopy(source._cascade_restrictions)
214-
self._restrict_conditions = copy.deepcopy(source._restrict_conditions)
215-
self._restriction_attrs = copy.deepcopy(source._restriction_attrs)
227+
# Copy each node's restriction list, not its items: projected
228+
# restrictions are query expressions bound to a live connection,
229+
# which cannot be deep-copied.
230+
self._cascade_restrictions = {k: copy.copy(v) for k, v in source._cascade_restrictions.items()}
231+
self._restrict_conditions = {k: copy.copy(v) for k, v in source._restrict_conditions.items()}
232+
self._restriction_attrs = {k: set(v) for k, v in source._restriction_attrs.items()}
216233
super().__init__(source)
217234
return
218235

@@ -471,9 +488,9 @@ def cascade(cls, table_expr, part_integrity="enforce"):
471488
result._expanded_nodes = set(descendants)
472489

473490
# Seed restriction
474-
restriction = AndList(table_expr.restriction)
491+
restriction, restriction_attrs = _seed_restriction(table_expr)
475492
result._cascade_restrictions[node] = [restriction] if restriction else []
476-
result._restriction_attrs[node] = set(table_expr.restriction_attributes)
493+
result._restriction_attrs[node] = restriction_attrs
477494

478495
# Propagate downstream
479496
result._propagate_restrictions(node, mode="cascade", part_integrity=part_integrity)
@@ -557,9 +574,9 @@ def trace(cls, table_expr):
557574
result._expanded_nodes = set(ancestors)
558575

559576
# Seed restriction
560-
restriction = AndList(table_expr.restriction)
577+
restriction, restriction_attrs = _seed_restriction(table_expr)
561578
result._cascade_restrictions[node] = [restriction] if restriction else []
562-
result._restriction_attrs[node] = set(table_expr.restriction_attributes)
579+
result._restriction_attrs[node] = restriction_attrs
563580

564581
# Propagate upstream
565582
result._propagate_restrictions_upstream(node)
@@ -883,16 +900,22 @@ def _apply_propagation_rule(
883900
884901
Rules (from table.py restriction propagation):
885902
886-
1. Non-aliased AND parent restriction attrs ⊆ child PK:
903+
1. Non-aliased AND parent restriction attrs ⊆ FK attrs ⊆ child PK:
887904
Copy parent restriction directly.
888905
2. Aliased FK (attr_map renames columns):
889906
``parent.proj(**{fk: pk for fk, pk in attr_map.items()})``
890-
3. Non-aliased AND parent restriction attrs ⊄ child PK:
891-
``parent.proj()``
907+
3. Otherwise (non-aliased): ``parent.proj()``
908+
909+
Copying is exact only when the restriction names attributes that this
910+
foreign key carries and the foreign key lies in the child's primary
911+
key. A restriction on an attribute that the child inherits through
912+
another foreign key would select child rows by that attribute instead
913+
of by the parent rows they reference. A secondary foreign key may be
914+
NULL, and then the child row references no parent row at all.
892915
"""
893916
child_pk = self.nodes[child_node].get("primary_key", set())
894917

895-
if not aliased and parent_attrs and parent_attrs <= child_pk:
918+
if not aliased and parent_attrs and parent_attrs <= set(attr_map) <= child_pk:
896919
# Rule 1: copy parent restriction directly
897920
parent_restr = restrictions.get(
898921
parent_ft.full_table_name,
@@ -912,7 +935,7 @@ def _apply_propagation_rule(
912935
restrictions.setdefault(child_node, AndList()).append(child_item)
913936
child_attrs = set(attr_map.keys())
914937
else:
915-
# Rule 3: non-aliased, restriction attrs ⊄ child PK — project
938+
# Rule 3: non-aliased, not a pure copy — project
916939
child_item = parent_ft.proj()
917940
if mode == "cascade":
918941
restrictions.setdefault(child_node, []).append(child_item)
@@ -922,7 +945,9 @@ def _apply_propagation_rule(
922945

923946
self._restriction_attrs.setdefault(child_node, set()).update(child_attrs)
924947

925-
def _apply_propagation_rule_upward(self, child_ft, child_attrs, parent_node, attr_map, aliased, mode, restrictions):
948+
def _apply_propagation_rule_upward(
949+
self, child_ft, child_attrs, parent_node, attr_map, aliased, mode, restrictions, copy_allowed=True
950+
):
926951
"""
927952
Apply the symmetric (upward) propagation rule to a parent←child edge.
928953
@@ -947,10 +972,15 @@ def _apply_propagation_rule_upward(self, child_ft, child_attrs, parent_node, att
947972
``child.proj(*attr_map.keys())`` — project child onto its FK columns
948973
(which, being non-aliased, share names with parent's PK columns) so
949974
the subsequent restriction on the parent joins on the right columns.
975+
976+
``copy_allowed=False`` disables Rule 1, so the parent is restricted to
977+
the rows that the restricted child rows actually reference. Copying
978+
the child's condition would also select parent rows that no child row
979+
references.
950980
"""
951981
parent_pk = self.nodes[parent_node].get("primary_key", set())
952982

953-
if not aliased and child_attrs and child_attrs <= parent_pk:
983+
if copy_allowed and not aliased and child_attrs and child_attrs <= parent_pk:
954984
# Backward Rule 1: copy child restriction directly
955985
child_restr = restrictions.get(
956986
child_ft.full_table_name,
@@ -1044,6 +1074,17 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
10441074
if len(real_path) < 2 or real_path[-1] != part_node or real_path[0] != master_name:
10451075
return False
10461076

1077+
# The walk restricts the intermediate parts only to carry the
1078+
# restriction up to the master. Those restrictions reference the
1079+
# descendant part, which is deleted first, so they are restored once
1080+
# the master is materialized. The master then cascades back down to
1081+
# all of its parts.
1082+
intermediates = real_path[1:-1]
1083+
saved = {
1084+
node: (copy.copy(restrictions[node]) if node in restrictions else None, self._restriction_attrs.get(node))
1085+
for node in intermediates
1086+
}
1087+
10471088
# Walk real_path in reverse (child → parent direction). For each
10481089
# adjacent (parent, child) pair, look up the FK edge props.
10491090
any_propagated = False
@@ -1059,6 +1100,8 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
10591100
child_ft = self._restricted_table(child)
10601101
child_attrs = self._restriction_attrs.get(child, set())
10611102

1103+
# The lift is existential: a master joins the cascade only if one
1104+
# of its part rows did, so the part's condition is never copied.
10621105
self._apply_propagation_rule_upward(
10631106
child_ft,
10641107
child_attrs,
@@ -1067,6 +1110,7 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
10671110
aliased,
10681111
mode,
10691112
restrictions,
1113+
copy_allowed=False,
10701114
)
10711115
any_propagated = True
10721116

@@ -1094,6 +1138,16 @@ def _propagate_part_to_master(self, part_node, master_name, mode, restrictions):
10941138
restrictions[master_name] = [False]
10951139
self._restriction_attrs.setdefault(master_name, set())
10961140

1141+
for node, (restriction, attrs) in saved.items():
1142+
if restriction is None:
1143+
restrictions.pop(node, None)
1144+
else:
1145+
restrictions[node] = restriction
1146+
if attrs is None:
1147+
self._restriction_attrs.pop(node, None)
1148+
else:
1149+
self._restriction_attrs[node] = attrs
1150+
10971151
return any_propagated
10981152

10991153
def _edge_props(self, parent, child):

‎src/datajoint/table.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import collections
2+
import copy
23
import csv
34
import inspect
45
import itertools
@@ -1083,6 +1084,12 @@ def delete_quick(self, get_count=False):
10831084
Deletes the table without cascading and without user prompt.
10841085
If this table has populated dependent tables, this will fail.
10851086
"""
1087+
if self._top is not None:
1088+
# dj.Top is an ORDER BY ... LIMIT, not part of the WHERE clause:
1089+
# delete exactly the rows it selects.
1090+
target = copy.copy(self)
1091+
target._top = None
1092+
return (target & self.keys()).delete_quick(get_count=get_count)
10861093
query = "DELETE FROM " + self.full_table_name + self.where_clause()
10871094
cursor = self.connection.query(query)
10881095
# Use cursor.rowcount (DB-API 2.0 standard, works for both MySQL and PostgreSQL)
Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,194 @@
1+
"""
2+
Regression tests for cascade restrictions that reached rows they should not.
3+
4+
A restriction may be copied verbatim across a foreign key only when it names
5+
attributes the foreign key carries and the foreign key lies in the child's
6+
primary key. The part-to-master lift never copies: a master joins the cascade
7+
only if one of its part rows did. A ``Top`` seed restricts the seed to the keys
8+
it selects.
9+
"""
10+
11+
import pytest
12+
13+
import datajoint as dj
14+
15+
16+
@pytest.fixture(scope="function")
17+
def schema_by_backend(connection_by_backend, db_creds_by_backend, request):
18+
"""Create a fresh schema per test (mirrors test_cascade_integrity.py)."""
19+
backend = db_creds_by_backend["backend"]
20+
import time
21+
22+
test_id = str(int(time.time() * 1000))[-8:]
23+
schema_name = f"djtest_overdelete_{backend}_{test_id}"[:64]
24+
25+
if connection_by_backend.is_connected:
26+
try:
27+
connection_by_backend.query(
28+
f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(schema_name)}"
29+
)
30+
except Exception:
31+
pass
32+
33+
schema = dj.Schema(schema_name, connection=connection_by_backend)
34+
yield schema
35+
36+
if connection_by_backend.is_connected:
37+
try:
38+
connection_by_backend.query(
39+
f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(schema_name)}"
40+
)
41+
except Exception:
42+
pass
43+
44+
45+
def test_top_seed_deletes_only_the_top_rows(schema_by_backend):
46+
"""``(T & dj.Top(n)).delete()`` and ``delete_quick()`` delete n rows, not
47+
every row of T."""
48+
49+
@schema_by_backend
50+
class Item(dj.Manual):
51+
definition = """
52+
item_id : int32
53+
"""
54+
55+
@schema_by_backend
56+
class Child(dj.Manual):
57+
definition = """
58+
-> Item
59+
child_id : int32
60+
"""
61+
62+
Item.insert([(i,) for i in range(5)])
63+
Child.insert([(i, 0) for i in range(5)])
64+
65+
seed = Item & dj.Top(2, order_by="item_id")
66+
counts = dj.Diagram.cascade(seed).counts()
67+
assert counts[Item.full_table_name] == 2
68+
assert counts[Child.full_table_name] == 2
69+
70+
seed.delete(prompt=False)
71+
assert sorted(Item.to_arrays("item_id")) == [2, 3, 4]
72+
assert sorted(Child.to_arrays("item_id")) == [2, 3, 4]
73+
74+
(Child & dj.Top(1, order_by="item_id")).delete_quick()
75+
assert sorted(Child.to_arrays("item_id")) == [3, 4]
76+
77+
78+
def test_nullable_fk_sharing_a_primary_key_column(schema_by_backend):
79+
"""A child whose nullable foreign key shares a primary-key column with the
80+
parent keeps the rows whose foreign key is NULL: they reference nothing."""
81+
82+
@schema_by_backend
83+
class Subject(dj.Manual):
84+
definition = """
85+
subject_id : int32
86+
"""
87+
88+
@schema_by_backend
89+
class Surgery(dj.Manual):
90+
definition = """
91+
-> Subject
92+
surgery_id : int32
93+
"""
94+
95+
@schema_by_backend
96+
class Session(dj.Manual):
97+
definition = """
98+
-> Subject
99+
session_id : int32
100+
---
101+
-> [nullable] Surgery
102+
"""
103+
104+
Subject.insert([(1,)])
105+
Surgery.insert([(1, 1)])
106+
Session.insert(
107+
[
108+
{"subject_id": 1, "session_id": 1, "surgery_id": 1},
109+
{"subject_id": 1, "session_id": 2, "surgery_id": None},
110+
]
111+
)
112+
113+
seed = Surgery & {"subject_id": 1}
114+
assert dj.Diagram.cascade(seed).counts()[Session.full_table_name] == 1
115+
116+
seed.delete(prompt=False)
117+
assert len(Surgery()) == 0
118+
assert Session.to_dicts() == [{"subject_id": 1, "session_id": 2, "surgery_id": None}]
119+
120+
121+
def test_diamond_child_is_reached_through_its_secondary_fk(schema_by_backend):
122+
"""D references B directly (primary) and C (secondary). Deleting B=1 must
123+
delete the D rows that reference the deleted C rows, whatever D's own b_id."""
124+
125+
@schema_by_backend
126+
class B(dj.Manual):
127+
definition = """
128+
b_id : int32
129+
"""
130+
131+
@schema_by_backend
132+
class C(dj.Manual):
133+
definition = """
134+
c_id : int32
135+
---
136+
-> B
137+
"""
138+
139+
@schema_by_backend
140+
class D(dj.Manual):
141+
definition = """
142+
-> B
143+
d_id : int32
144+
---
145+
-> C
146+
"""
147+
148+
B.insert([(1,), (2,)])
149+
C.insert([{"c_id": 10, "b_id": 1}])
150+
D.insert([{"b_id": 2, "d_id": 1, "c_id": 10}])
151+
152+
seed = B & {"b_id": 1}
153+
assert dj.Diagram.cascade(seed).counts()[D.full_table_name] == 1
154+
155+
seed.delete(prompt=False)
156+
assert B.to_arrays("b_id").tolist() == [2]
157+
assert len(C()) == 0
158+
assert len(D()) == 0
159+
160+
161+
def test_part_cascade_lifts_only_masters_that_have_a_matching_part(schema_by_backend):
162+
"""With part_integrity="cascade", a part seed lifts to the masters of the
163+
matching part rows. A master that has no matching part row is untouched."""
164+
165+
@schema_by_backend
166+
class Master(dj.Manual):
167+
definition = """
168+
master_id : int32
169+
"""
170+
171+
class P(dj.Part):
172+
definition = """
173+
-> master
174+
p_id : int32
175+
"""
176+
177+
@schema_by_backend
178+
class Downstream(dj.Manual):
179+
definition = """
180+
-> Master
181+
d_id : int32
182+
"""
183+
184+
Master.insert([(1,), (2,)])
185+
Master.P.insert([(1, 1)]) # master 2 has no parts
186+
Downstream.insert([(1, 1), (2, 1)])
187+
188+
seed = Master.P & [{"master_id": 1}, {"master_id": 2}]
189+
assert dj.Diagram.cascade(seed, part_integrity="cascade").counts()[Master.full_table_name] == 1
190+
191+
seed.delete(prompt=False, part_integrity="cascade")
192+
assert Master.to_arrays("master_id").tolist() == [2]
193+
assert len(Master.P()) == 0
194+
assert Downstream.to_arrays("master_id").tolist() == [2]

0 commit comments

Comments
 (0)