[dimitri-yatsenko]

Closes #1589, closes #1590, closes #1591.

A cascade delete reached rows outside the cascade in three cases:

• dj.Top seed (delete() ignores a dj.Top restriction and deletes every matching row #1589). Diagram.cascade and Diagram.trace built the seed from the WHERE restriction only, and delete_quick used where_clause() only. A seed restricted by Top is now replaced by the primary keys it selects.
• Copy through a foreign key (Cascade delete removes child rows whose nullable foreign key references nothing #1590). A parent's restriction was copied to the child whenever the restricted attributes were in the child's primary key. Through a nullable foreign key that shares a primary-key column, that also selected child rows whose foreign key is NULL. In a diamond, where the child inherits the attribute through another foreign key, 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.
• Part-to-master lift (part_integrity="cascade" deletes masters that have no matching part rows #1591). With part_integrity="cascade", the lift copied the part's condition to the master, which selected 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.

Two follow-on changes:

• The part-to-master walk restricts intermediate parts only to carry the restriction up to the master. Those restrictions reference the descendant part, which is deleted first (and on MySQL they raise error 1093). They are now restored once the master is materialized; the master then cascades back to all of its parts.
• The Diagram copy constructor copies each node's restriction list instead of deep-copying it. Projected restrictions are query expressions bound to a live connection and cannot be deep-copied. This affected prune() and the diagram operators after restrict().

Not changed: Diagram.trace keeps its upward copy for now. self.upstream traces from a row that make() has not inserted yet and relies on it. The traversal redesign for 2.4 defines that case explicitly.

Tests: tests/integration/test_cascade_overdelete.py adds four tests on MySQL and PostgreSQL. All fail on v2.3.4 and pass here. The full suite passes locally (1166 passed, 14 skipped).

The same commit is cherry-picked onto maint/2.3 for a v2.3.5 patch release.

[lum-agilyti]

Read through the change against 1589, 1590 and 1591. Each fix seems to close its issue, and the narrower copy rule looks like it leaves the common primary-key chains on the same path as before. A few notes and questions:

• No CI checks appear to have run on this PR or on 1593. Could they be re-triggered before merge?
• Diagram.restrict() (diagram.py:778) still builds its seed from table_expr.restriction, so Diagram(schema).restrict(T & dj.Top(2)) looks like it would keep every row matching the other conditions. It isn't a delete, but it seems to be the same seed issue as 1589. Is it meant to be in scope here, or a follow-up?
• In _propagate_part_to_master, saved keeps self._restriction_attrs.get(node), which is the same set object the walk then extends in place via setdefault(...).update(...). So restoring the attributes seems to be a no-op whenever the intermediate part already had attributes. As far as I can tell the extra attributes only make the copy rules stricter, so the effect looks limited to an extra subquery. The code comment says they are restored, though. Is that intended?
• Two cases don't seem to be covered by the new tests:
  • Diagram.trace(T & dj.Top(n)), although the seed change also applies to trace().
  • A Top that selects no rows. The seed is then [[]] and the delete correctly removes nothing. If the seed were ever empty instead, [restriction] if restriction else [] looks like it would leave it unrestricted.
• Outside this diff, a question about trace(): its upward Rule 1 (diagram.py:983) still copies the child's condition to the parent whenever the restricted attributes are in the parent's primary key. The new copy_allowed docstring notes that copying "would also select parent rows that no child row references", and that seems to apply here too. For example, with a table keyed by subject_id that has a secondary FK to Surgery, self.upstream[Surgery] looks like it would return every surgery of that subject, not just the one the row references. I can see the copy may be deliberate, since inside make() the traced row hasn't been inserted yet. Is this intended?
• 1593 appears to match this PR line for line.

[MilagrosMarin]

Thanks for the thorough write-up @dimitri-yatsenko — the three root causes and the "restrict only what the cascade reaches" framing hold up when I read the diff. I haven't run the suite locally; these notes come from reading the code.

Requested change

✅ Add a regression test for an empty seed. _seed_restriction replaces a Top seed with AndList([table_expr.keys()]). When keys() returns [], this relies on AndList([[]]) being truthy and compiling to FALSE. From condition.py that looks right, but if it were ever treated as "no restriction" the cascade would delete every row, so it's worth pinning down. For example: (Item & dj.Top(2)).delete() on an empty table, with an assertion that the preview counts are 0 and nothing is deleted.

Merge ordering

#1525 (also in milestone v2.3.5) rewrites _propagate_part_to_master, and a trial merge of the two conflicts in diagram.py. Which should land first? Whichever goes second will need the saved/restored intermediate restrictions in this PR re-checked against the all-paths walk.

Questions

• table_expr.keys() pulls every selected key into Python, and the seed becomes a list of dicts that compiles to an OR of conditions. For a large Top (or, after #1611, a large subquery seed), could that exceed max_allowed_packet? Is there a practical bound you'd want to document or test?
• On head 18e5649 I don't see any workflow runs or check-runs through the API. Could you point me to where CI ran?

Everything else reads correct to me: the nullable-FK and diamond rule (parent_attrs <= set(attr_map) <= child_pk), the existential part→master lift, and the copy-constructor change. Let me know if anything needs adjusting!

[dimitri-yatsenko]

Thanks, @lum-agilyti and @MilagrosMarin. Pushed f14d68b here; the same commit is on #1593.

Done in this PR

• Empty Top seed: a new test, test_top_seed_that_selects_no_rows_deletes_nothing. Item & "item_id > 100" & dj.Top(2) previews 0 rows in both tables, and the delete leaves every row in place.
• trace with a Top seed: test_top_seed_in_trace checks that the trace starts from the two rows the Top selects.
• Restoring the walked parts' attributes: you were right that saved held the same set the walk extends in place, so the restore did nothing. It now saves a copy.

Left for 2.4
In 2.4, Diagram.expand replaces the traversal (spec in datajoint/datajoint-docs#249), so this PR stays limited to the three over-delete cases. The points below are covered there:

• Diagram.restrict() with a Top seed: restrict is deprecated in 2.4, and expand materializes Top seeds.
• trace's upward copy: it is deliberate in 2.3. self.upstream traces from the row make() is about to insert, and that row does not exist yet. In 2.4, crossing upward is always a restriction, never a copy, and self.upstream is built from the parents of that row.
• Large key lists and max_allowed_packet: in 2.4, keys are materialized only at delete time and applied in chunks. A large Top here carries the same cost it did before this PR.

Other questions

• CI: the workflow runs from early to mid October were deleted across the repository, and with them the checks of this PR and fix(cascade): restrict only the rows a cascade reaches (2.3) #1593. I reopened both, so the checks are running again.
• fix(diagram): walk all FK paths in the part→master cascade walk #1525 versus this PR: fix(diagram): walk all FK paths in the part→master cascade walk #1525 will be closed unmerged. Its multi-path walk is replaced in 2.4 by a one-step lift, and its test cases move to the shape catalog (test(diagram): reference model and properties for Diagram.expand #1621).

[MilagrosMarin]

Thanks @dimitri-yatsenko — re-read against f14d68b6. The three follow-ups land, and I traced the empty-seed mechanism this time rather than reasoning from the diff:

✅ keys() returning [] gives AndList([[]]), which is truthy (length 1), so the seed survives [restriction] if restriction else []; make_condition on the inner empty list falls to the OrList branch and returns False, so the seed compiles to a false condition rather than to "no restriction". test_top_seed_that_selects_no_rows_deletes_nothing pins that.
✅ test_top_seed_in_trace fails without the fix — the unpatched seed is AndList([]), which is falsy, so trace would count 5/5 instead of 2/2.
✅ saved now snapshots the attribute set rather than aliasing it.
✅ CI: 16 checks green here and on #1593. I'd noted the matrix names only MySQL images — it does cover PostgreSQL, since test-cov runs all of tests/ and the backend fixture is parameterized over both via testcontainers.
✅ #1593 carries byte-identical diagram.py, table.py and test_cascade_overdelete.py.

Three notes, none of them blocking, in descending order of how much they matter:

1. The early return inside the walk skips the restore. diagram.py:1099 returns between the saved snapshot (1082) and the restore loop (1145), so on that path the intermediate parts keep the walk's temporary restrictions — the ones the restore exists to remove. The branch is marked "shouldn't happen if shortest_path succeeded", so this is hygiene rather than a live bug, but a try: ... finally: around the walk would make the restore unconditional and cost nothing.

2. Is the attribute half of the restore observable? Before f14d68b6 it was a no-op and CI was green; after the fix it is a real restore and CI is still green, so no test distinguishes it. I traced the part-of-part shape in test_cascade_part_of_part_no_master_reference: because cascade restrictions OR-combine, the subsequent Master → PartA → PartB copy decisions come out the same either way. If there is a shape where it changes the result, a test would be worth having; if it is consistency only, saying so in the comment at 1083 is better than leaving it reading as load-bearing.

3. delete_quick() with a Top that selects nothing. The new empty-seed test goes through delete() and Diagram.cascade, not through the new table.py branch. There self.keys() is [], target & [] appends the Python bool False to the restriction, and where_clause renders WHERE (False) — correct on both backends, I checked. But it is the one place where a wrong answer deletes the whole table, and test_top_seed_deletes_only_the_top_rows already exercises delete_quick with a non-empty Top. One more line in the empty test would close it:

(Item & "item_id > 100" & dj.Top(2)).delete_quick()
assert len(Item()) == 3

On Diagram.restrict(): understood that expand replaces it in 2.4. For 2.3.5, restrict still seeds from table_expr.restriction (diagram.py:778), and for T & dj.Top(2) that list is empty, so the Top is silently dropped and the diagram shows every matching row. It is a preview, not a delete, so the blast radius is small — but _seed_restriction now exists, and either reusing it there or noting the limitation in the restrict() docstring would keep it from reading as the same bug left unfixed.

Approving — everything else reads correct to me, and none of the three notes needs to hold up the v2.3.5 patch. Take them here or fold them into the 2.4 traversal work, whichever fits better.

[lum-agilyti]

Thanks, f14d68b covers what I raised.

[dimitri-yatsenko]

Thanks, @MilagrosMarin and @lum-agilyti. Merging for v2.3.5. The notes go to the 2.4 traversal work, where Diagram.expand replaces this code (datajoint/datajoint-docs#249):

1. The early return before the restore: the part-to-master walk is deleted in 2.4, and the lift becomes one step through the part's owning attributes, with nothing to save or restore.
2. Whether restoring the attributes changes a result: for consistency only, as far as I can find; cascade restrictions combine with OR, as you traced. It disappears with the walk.
3. delete_quick() with a Top that selects nothing: thanks for checking that it renders WHERE (False) on both backends. The 2.4 delete tests (L3 in test(diagram): reference model and properties for Diagram.expand #1621's plan) run every seed kind through both delete() and delete_quick(), and include the empty Top.
4. Diagram.restrict() with Top: restrict is deprecated in 2.4, and expand materializes Top seeds. The 2.3.5 release notes will mention the limitation.