Merge pull request #1158 from CBroz1/master

Allow including master table in cascading delete

Author: ethho

CHANGELOG.md

@@ -11,9 +11,10 @@
 - Added - Missing tests for set_password - PR [#1106](https://github.com/datajoint/datajoint-python/pull/1106)
 - Changed - Returning success count after the .populate() call - PR [#1050](https://github.com/datajoint/datajoint-python/pull/1050)
 - Fixed - `Autopopulate.populate` excludes `reserved` jobs in addition to `ignore` and `error` jobs 
-- Fixed - Issue [#1159]((https://github.com/datajoint/datajoint-python/pull/1159)  (cascading delete)  - PR [#1160](https://github.com/datajoint/datajoint-python/pull/1160)
+- Fixed - Issue [#1159](https://github.com/datajoint/datajoint-python/pull/1159) (cascading delete) - PR [#1160](https://github.com/datajoint/datajoint-python/pull/1160)
 - Changed - Minimum Python version for Datajoint-Python is now 3.8 PR #1163
 - Fixed - `docker compose` commands in CI [#1164](https://github.com/datajoint/datajoint-python/pull/1164)
+- Changed - Default delete behavior now includes masters of part tables - PR [#1158](https://github.com/datajoint/datajoint-python/pull/1158)
 
 ### 0.14.1 -- Jun 02, 2023
 - Fixed - Fix altering a part table that uses the "master" keyword - PR [#991](https://github.com/datajoint/datajoint-python/pull/991)

datajoint/table.py

@@ -486,6 +486,7 @@ def delete(
         transaction: bool = True,
         safemode: Union[bool, None] = None,
         force_parts: bool = False,
+        force_masters: bool = False,
     ) -> int:
         """
         Deletes the contents of the table and its dependent tables, recursively.
@@ -497,6 +498,8 @@ def delete(
             safemode: If `True`, prohibit nested transactions and prompt to confirm. Default
                 is `dj.config['safemode']`.
             force_parts: Delete from parts even when not deleting from their masters.
+            force_masters: If `True`, include part/master pairs in the cascade.
+                Default is `False`.
 
         Returns:
             Number of deleted rows (excluding those from dependent tables).
@@ -507,6 +510,7 @@ def delete(
             DataJointError: Deleting a part table before its master.
         """
         deleted = set()
+        visited_masters = set()
 
         def cascade(table):
             """service function to perform cascading deletes recursively."""
@@ -566,7 +570,27 @@ def cascade(table):
                         )
                     else:
                         child &= table.proj()
-                    cascade(child)
+
+                    master_name = get_master(child.full_table_name)
+                    if (
+                        force_masters
+                        and master_name
+                        and master_name != table.full_table_name
+                        and master_name not in visited_masters
+                    ):
+                        master = FreeTable(table.connection, master_name)
+                        master._restriction_attributes = set()
+                        master._restriction = [
+                            make_condition(  # &= may cause in target tables in subquery
+                                master,
+                                (master.proj() & child.proj()).fetch(),
+                                master._restriction_attributes,
+                            )
+                        ]
+                        visited_masters.add(master_name)
+                        cascade(master)
+                    else:
+                        cascade(child)
                 else:
                     deleted.add(table.full_table_name)
                     logger.info(

docs/src/manipulation/delete.md

@@ -27,4 +27,5 @@ consequence of deleting the master table.
 
 To enforce this workflow, calling `delete` directly on a part table produces an error.
 In some cases, it may be necessary to override this behavior.
-To remove entities from a part table without calling `delete` master, use the argument `force=True`.
+To remove entities from a part table without calling `delete` master, use the argument `force_parts=True`.
+To include the corresponding entries in the master table, use the argument `force_masters=True`.

tests/conftest.py

@@ -339,6 +339,7 @@ def schema_simp(connection_test, prefix):
     schema(schema_simple.E)
     schema(schema_simple.F)
     schema(schema_simple.F)
+    schema(schema_simple.G)
     schema(schema_simple.DataA)
     schema(schema_simple.DataB)
     schema(schema_simple.Website)

tests/schema_simple.py

@@ -111,17 +111,46 @@ class F(dj.Part):
         -> B.C
         """
 
+    class G(dj.Part):
+        definition = """ # test secondary fk reference
+        -> E
+        id_g :int
+        ---
+        -> L
+        """
+
+    class H(dj.Part):
+        definition = """ # test no additional fk reference
+        -> E
+        id_h :int
+        """
+
+    class M(dj.Part):
+        definition = """ # test force_masters revisit
+        -> E
+        id_m :int
+        ---
+        -> E.H
+        """
+
     def make(self, key):
         random.seed(str(key))
-        self.insert1(dict(key, **random.choice(list(L().fetch("KEY")))))
-        sub = E.F()
-        references = list((B.C() & key).fetch("KEY"))
-        random.shuffle(references)
-        sub.insert(
+        l_contents = list(L().fetch("KEY"))
+        part_f, part_g, part_h, part_m = E.F(), E.G(), E.H(), E.M()
+        bc_references = list((B.C() & key).fetch("KEY"))
+        random.shuffle(bc_references)
+
+        self.insert1(dict(key, **random.choice(l_contents)))
+        part_f.insert(
             dict(key, id_f=i, **ref)
-            for i, ref in enumerate(references)
+            for i, ref in enumerate(bc_references)
             if random.getrandbits(1)
         )
+        g_inserts = [dict(key, id_g=i, **ref) for i, ref in enumerate(l_contents)]
+        part_g.insert(g_inserts)
+        h_inserts = [dict(key, id_h=i) for i in range(4)]
+        part_h.insert(h_inserts)
+        part_m.insert(dict(key, id_m=m, **random.choice(h_inserts)) for m in range(4))
 
 
 class F(dj.Manual):
@@ -132,6 +161,15 @@ class F(dj.Manual):
     """
 
 
+class G(dj.Computed):
+    definition = """ # test downstream of complex master/parts
+    -> E
+    """
+
+    def make(self, key):
+        self.insert1(key)
+
+
 class DataA(dj.Lookup):
     definition = """
     idx     : int

tests/test_cascading_delete.py

@@ -1,6 +1,6 @@
 import pytest
 import datajoint as dj
-from .schema_simple import A, B, D, E, L, Website, Profile
+from .schema_simple import A, B, D, E, G, L, Website, Profile
 from .schema import ComplexChild, ComplexParent
 
 
@@ -11,6 +11,7 @@ def schema_simp_pop(schema_simp):
     B().populate()
     D().populate()
     E().populate()
+    G().populate()
     yield schema_simp
 
 
@@ -96,7 +97,7 @@ def test_delete_complex_keys(schema_any):
         **{
             "child_id_{}".format(i + 1): (i + parent_key_count)
             for i in range(child_key_count)
-        }
+        },
     )
     assert len(ComplexParent & restriction) == 1, "Parent record missing"
     assert len(ComplexChild & restriction) == 1, "Child record missing"
@@ -110,11 +111,24 @@ def test_delete_master(schema_simp_pop):
     Profile().delete()
 
 
-def test_delete_parts(schema_simp_pop):
+def test_delete_parts_error(schema_simp_pop):
     """test issue #151"""
     with pytest.raises(dj.DataJointError):
         Profile().populate_random()
-        Website().delete()
+        Website().delete(force_masters=False)
+
+
+def test_delete_parts(schema_simp_pop):
+    """test issue #151"""
+    Profile().populate_random()
+    Website().delete(force_masters=True)
+
+
+def test_delete_parts_complex(schema_simp_pop):
+    """test issue #151 with complex master/part. PR #1158."""
+    prev_len = len(G())
+    (A() & "id_a=1").delete(force_masters=True)
+    assert prev_len - len(G()) == 16, "Failed to delete parts"
 
 
 def test_drop_part(schema_simp_pop):

tests/test_erd.py

@@ -1,5 +1,5 @@
 import datajoint as dj
-from .schema_simple import LOCALS_SIMPLE, A, B, D, E, L, OutfitLaunch
+from .schema_simple import LOCALS_SIMPLE, A, B, D, E, G, L, OutfitLaunch
 from .schema_advanced import *
 
 
@@ -20,7 +20,7 @@ def test_dependencies(schema_simp):
     assert set(D().parents(primary=True)) == set([A.full_table_name])
     assert set(D().parents(primary=False)) == set([L.full_table_name])
     assert set(deps.descendants(L.full_table_name)).issubset(
-        cls.full_table_name for cls in (L, D, E, E.F)
+        cls.full_table_name for cls in (L, D, E, E.F, E.G, E.H, E.M, G)
     )
 
 
@@ -38,10 +38,14 @@ def test_erd_algebra(schema_simp):
     erd3 = erd1 * erd2
     erd4 = (erd0 + E).add_parts() - B - E
     assert erd0.nodes_to_show == set(cls.full_table_name for cls in [B])
-    assert erd1.nodes_to_show == set(cls.full_table_name for cls in (B, B.C, E, E.F))
+    assert erd1.nodes_to_show == set(
+        cls.full_table_name for cls in (B, B.C, E, E.F, E.G, E.H, E.M, G)
+    )
     assert erd2.nodes_to_show == set(cls.full_table_name for cls in (A, B, D, E, L))
     assert erd3.nodes_to_show == set(cls.full_table_name for cls in (B, E))
-    assert erd4.nodes_to_show == set(cls.full_table_name for cls in (B.C, E.F))
+    assert erd4.nodes_to_show == set(
+        cls.full_table_name for cls in (B.C, E.F, E.G, E.H, E.M)
+    )
 
 
 def test_repr_svg(schema_adv):

tests/test_schema.py

@@ -184,7 +184,7 @@ def test_list_tables(schema_simp):
     """
     https://github.com/datajoint/datajoint-python/issues/838
     """
-    assert set(
+    expected = set(
         [
             "reserved_word",
             "#l",
@@ -194,6 +194,10 @@ def test_list_tables(schema_simp):
             "__b__c",
             "__e",
             "__e__f",
+            "__e__g",
+            "__e__h",
+            "__e__m",
+            "__g",
             "#outfit_launch",
             "#outfit_launch__outfit_piece",
             "#i_j",
@@ -207,7 +211,9 @@ def test_list_tables(schema_simp):
             "profile",
             "profile__website",
         ]
-    ) == set(schema_simp.list_tables())
+    )
+    actual = set(schema_simp.list_tables())
+    assert actual == expected, f"Missing from list_tables(): {expected - actual}"
 
 
 def test_schema_save_any(schema_any):