From 15a517e3895c1adff72b3b1a0e3f8ec5b89142b9 Mon Sep 17 00:00:00 2001 From: Robert Helewka Date: Mon, 3 Aug 2026 13:11:41 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=BE=20fix(api):=20cascade=20collection?= =?UTF-8?q?=20and=20item=20deletes=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collection and Item DELETE — both the REST endpoints and the HTML views — called bare .delete(), leaking each collection's Items and every item's Chunks/Images/ImageEmbeddings. New delete_collection_cascade and delete_item_cascade in the shared library_delete service now back all four paths. No orphan-Concept GC in either (that invariant stays library-delete-only, matching the existing supersede path); the ingest supersede helper in tasks.py now delegates to delete_item_cascade instead of duplicating its Cypher. Co-Authored-By: Claude Fable 5 --- mnemosyne/library/api/views.py | 22 +++- mnemosyne/library/services/library_delete.py | 96 +++++++++++++++++ mnemosyne/library/tasks.py | 13 +-- mnemosyne/library/tests/test_library_api.py | 108 ++++++++++++++++--- mnemosyne/library/views.py | 12 ++- 5 files changed, 221 insertions(+), 30 deletions(-) diff --git a/mnemosyne/library/api/views.py b/mnemosyne/library/api/views.py index 03b3238..7dbe38b 100644 --- a/mnemosyne/library/api/views.py +++ b/mnemosyne/library/api/views.py @@ -18,7 +18,11 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from library.content_types import get_library_type_config -from library.services.library_delete import delete_library_cascade +from library.services.library_delete import ( + delete_collection_cascade, + delete_item_cascade, + delete_library_cascade, +) from mcp_server.drf_auth import request_token_label from .serializers import ( @@ -273,7 +277,13 @@ def collection_detail(request, uid): col.save() return Response(CollectionSerializer(col).data) - col.delete() + # DELETE — cascade Items/Chunks/Images too; a bare col.delete() leaks them. + result = delete_collection_cascade(col) + logger.info( + "Collection deleted via API collection_uid=%s name=%s items=%d caller=%s", + result["collection_uid"], result["name"], result["item_count"], + request.user.username, + ) return Response(status=status.HTTP_204_NO_CONTENT) @@ -354,7 +364,13 @@ def item_detail(request, uid): item.save() return Response(ItemSerializer(item).data) - item.delete() + # DELETE — cascade Chunks/Images/embeddings too; a bare item.delete() + # leaks them. + delete_item_cascade(item.uid) + logger.info( + "Item deleted via API item_uid=%s caller=%s", + item.uid, request.user.username, + ) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/mnemosyne/library/services/library_delete.py b/mnemosyne/library/services/library_delete.py index 403f928..a5394e2 100644 --- a/mnemosyne/library/services/library_delete.py +++ b/mnemosyne/library/services/library_delete.py @@ -106,3 +106,99 @@ def delete_library_cascade(lib) -> dict: "item_s3_keys": item_s3_keys, "orphans_deleted": orphans_deleted, } + + +def delete_collection_cascade(col) -> dict: + """Delete ``col`` and all content reachable and unique to it. + + Removes the Collection's Items with their Chunks, Images, and + ImageEmbeddings, then the Collection itself. No orphan-Concept GC — + that invariant belongs to library-level deletes only (see + :func:`delete_library_cascade`); Concepts orphaned here are collected + on the next library delete. + + :param col: A ``library.models.Collection`` node instance. + :returns: Dict with ``collection_uid``, ``name``, ``item_count``, and + ``item_s3_keys`` (list of ``(uid, s3_key)`` for async S3 cleanup). + """ + collection_uid = col.uid + collection_name = col.name + + s3_rows, _ = db.cypher_query( + "MATCH (col:Collection {uid: $uid})-[:CONTAINS]->(i:Item) " + "RETURN i.uid, i.s3_key", + {"uid": collection_uid}, + ) + item_s3_keys = [(r[0], r[1]) for r in s3_rows if r[1]] + + db.cypher_query( + """ + MATCH (col:Collection {uid: $uid})-[:CONTAINS]->(i:Item) + -[:HAS_CHUNK]->(c:Chunk) + DETACH DELETE c + """, + {"uid": collection_uid}, + ) + db.cypher_query( + """ + MATCH (col:Collection {uid: $uid})-[:CONTAINS]->(i:Item) + -[:HAS_IMAGE]->(img:Image) + OPTIONAL MATCH (img)-[:HAS_EMBEDDING]->(emb:ImageEmbedding) + DETACH DELETE img, emb + """, + {"uid": collection_uid}, + ) + db.cypher_query( + """ + MATCH (col:Collection {uid: $uid})-[:CONTAINS]->(i:Item) + DETACH DELETE i + """, + {"uid": collection_uid}, + ) + db.cypher_query( + "MATCH (col:Collection {uid: $uid}) DETACH DELETE col", + {"uid": collection_uid}, + ) + + logger.info( + "Collection cascade-deleted collection_uid=%s name=%s items=%d", + collection_uid, collection_name, len(item_s3_keys), + ) + + return { + "collection_uid": collection_uid, + "name": collection_name, + "item_count": len(item_s3_keys), + "item_s3_keys": item_s3_keys, + } + + +def delete_item_cascade(item_uid: str) -> dict: + """Delete the Item ``item_uid`` with its Chunks, Images, and embeddings. + + Keyed on the uid (not a node instance) so the ingest supersede path in + ``library.tasks`` can share it. No orphan-Concept GC — see + :func:`delete_collection_cascade`. + + :returns: Dict with ``item_uid`` and ``s3_key`` (empty string when the + item had no stored file) for async S3 cleanup. + """ + s3_rows, _ = db.cypher_query( + "MATCH (i:Item {uid: $uid}) RETURN i.s3_key", + {"uid": item_uid}, + ) + s3_key = (s3_rows[0][0] or "") if s3_rows else "" + + db.cypher_query( + """ + MATCH (i:Item {uid: $uid}) + OPTIONAL MATCH (i)-[:HAS_CHUNK]->(c:Chunk) + OPTIONAL MATCH (i)-[:HAS_IMAGE]->(img:Image) + OPTIONAL MATCH (img)-[:HAS_EMBEDDING]->(emb:ImageEmbedding) + DETACH DELETE c, img, emb, i + """, + {"uid": item_uid}, + ) + + logger.info("Item cascade-deleted item_uid=%s", item_uid) + return {"item_uid": item_uid, "s3_key": s3_key} diff --git a/mnemosyne/library/tasks.py b/mnemosyne/library/tasks.py index b70525a..1c6e3b3 100644 --- a/mnemosyne/library/tasks.py +++ b/mnemosyne/library/tasks.py @@ -503,16 +503,9 @@ def ingest_from_daedalus(self, job_id: str): def _delete_item_and_chunks(item_uid: str): """Delete an Item, its chunks, and its images. Concept GC is workspace-delete only.""" - db.cypher_query( - """ - MATCH (i:Item {uid: $uid}) - OPTIONAL MATCH (i)-[:HAS_CHUNK]->(c:Chunk) - OPTIONAL MATCH (i)-[:HAS_IMAGE]->(img:Image) - OPTIONAL MATCH (img)-[:HAS_EMBEDDING]->(emb:ImageEmbedding) - DETACH DELETE c, img, emb, i - """, - {"uid": item_uid}, - ) + from library.services.library_delete import delete_item_cascade + + delete_item_cascade(item_uid) def _resolve_or_create_default_collection(lib, collection_uid: str = ""): diff --git a/mnemosyne/library/tests/test_library_api.py b/mnemosyne/library/tests/test_library_api.py index 975f414..2e36bca 100644 --- a/mnemosyne/library/tests/test_library_api.py +++ b/mnemosyne/library/tests/test_library_api.py @@ -1,9 +1,9 @@ """Tests for the plain library REST endpoints beyond create. -Currently covers the DELETE cascade: ``DELETE /library/api/libraries/{uid}/`` -must go through ``delete_library_cascade`` (shared with the HTML and -workspace delete paths) — a bare ``lib.delete()`` leaks Collections, Items, -Chunks, and Images and skips orphan-Concept GC. Neo4j is stubbed via +Currently covers the DELETE cascades: library, collection, and item DELETE +endpoints must go through the shared ``library_delete`` service functions — +bare ``.delete()`` calls leak child nodes (Collections, Items, Chunks, +Images) and, for libraries, skip orphan-Concept GC. Neo4j is stubbed via ``sys.modules``, same style as ``test_managed_by.py``. """ @@ -19,6 +19,23 @@ from rest_framework.test import APIClient User = get_user_model() +def _fake_node_cls(instance): + """A neomodel-class stand-in whose ``nodes.get`` returns ``instance``. + + ``instance=None`` makes ``nodes.get`` raise the class's DoesNotExist. + """ + fake_nodes = MagicMock() + + class DoesNotExist(Exception): + pass + + if instance is None: + fake_nodes.get.side_effect = DoesNotExist() + else: + fake_nodes.get.return_value = instance + return SimpleNamespace(nodes=fake_nodes, DoesNotExist=DoesNotExist) + + class LibraryApiDeleteTests(TestCase): """DELETE on the plain library endpoint cascades.""" @@ -28,17 +45,7 @@ class LibraryApiDeleteTests(TestCase): self.client.force_authenticate(user=self.user) def _fake_models_module(self, lib): - fake_nodes = MagicMock() - if lib is None: - class DoesNotExist(Exception): - pass - - fake_library = SimpleNamespace(nodes=fake_nodes, DoesNotExist=DoesNotExist) - fake_nodes.get.side_effect = DoesNotExist() - else: - fake_library = SimpleNamespace(nodes=fake_nodes, DoesNotExist=Exception) - fake_nodes.get.return_value = lib - return SimpleNamespace(Library=fake_library) + return SimpleNamespace(Library=_fake_node_cls(lib)) def test_delete_uses_shared_cascade(self): lib = SimpleNamespace(uid="lib-1", name="Docs") @@ -68,3 +75,74 @@ class LibraryApiDeleteTests(TestCase): self.assertEqual(response.status_code, 404) mock_cascade.assert_not_called() + + +class CollectionApiDeleteTests(TestCase): + """DELETE on the collection endpoint cascades Items/Chunks/Images.""" + + def setUp(self): + self.user = User.objects.create_user(username="op", password="pw") + self.client = APIClient() + self.client.force_authenticate(user=self.user) + + def test_delete_uses_shared_cascade(self): + col = SimpleNamespace(uid="col-1", name="Default") + with patch.dict( + "sys.modules", + {"library.models": SimpleNamespace(Collection=_fake_node_cls(col))}, + ), patch( + "library.api.views.delete_collection_cascade", + return_value={ + "collection_uid": "col-1", + "name": "Default", + "item_count": 2, + "item_s3_keys": [], + }, + ) as mock_cascade: + response = self.client.delete("/library/api/collections/col-1/") + + self.assertEqual(response.status_code, 204) + mock_cascade.assert_called_once_with(col) + + def test_delete_missing_collection_returns_404(self): + with patch.dict( + "sys.modules", + {"library.models": SimpleNamespace(Collection=_fake_node_cls(None))}, + ), patch("library.api.views.delete_collection_cascade") as mock_cascade: + response = self.client.delete("/library/api/collections/nope/") + + self.assertEqual(response.status_code, 404) + mock_cascade.assert_not_called() + + +class ItemApiDeleteTests(TestCase): + """DELETE on the item endpoint cascades Chunks/Images/embeddings.""" + + def setUp(self): + self.user = User.objects.create_user(username="op", password="pw") + self.client = APIClient() + self.client.force_authenticate(user=self.user) + + def test_delete_uses_shared_cascade(self): + item = SimpleNamespace(uid="item-1", title="Doc") + with patch.dict( + "sys.modules", + {"library.models": SimpleNamespace(Item=_fake_node_cls(item))}, + ), patch( + "library.api.views.delete_item_cascade", + return_value={"item_uid": "item-1", "s3_key": ""}, + ) as mock_cascade: + response = self.client.delete("/library/api/items/item-1/") + + self.assertEqual(response.status_code, 204) + mock_cascade.assert_called_once_with("item-1") + + def test_delete_missing_item_returns_404(self): + with patch.dict( + "sys.modules", + {"library.models": SimpleNamespace(Item=_fake_node_cls(None))}, + ), patch("library.api.views.delete_item_cascade") as mock_cascade: + response = self.client.delete("/library/api/items/nope/") + + self.assertEqual(response.status_code, 404) + mock_cascade.assert_not_called() diff --git a/mnemosyne/library/views.py b/mnemosyne/library/views.py index 2138c23..1937447 100644 --- a/mnemosyne/library/views.py +++ b/mnemosyne/library/views.py @@ -460,7 +460,11 @@ def collection_delete(request, uid): if request.method == "POST": name = col.name - col.delete() + # Shared cascade so Items/Chunks/Images go too — a bare + # col.delete() would leak them. + from .services.library_delete import delete_collection_cascade + + delete_collection_cascade(col) messages.success(request, f'Collection "{name}" deleted.') return redirect("library:library-list") return render( @@ -648,7 +652,11 @@ def item_delete(request, uid): if request.method == "POST": title = item.title - item.delete() + # Shared cascade so Chunks/Images/embeddings go too — a bare + # item.delete() would leak them. + from .services.library_delete import delete_item_cascade + + delete_item_cascade(item.uid) messages.success(request, f'Item "{title}" deleted.') return redirect("library:library-list") return render(request, "library/item_confirm_delete.html", {"item": item})