Merge pull request '🐾 fix(api): cascade collection and item deletes too' (#10) from fix/collection-item-cascade into main

Reviewed-on: #10

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)
 
 

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}

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 = ""):

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()

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})