From f1639dbdec9377d6755341c82c0ea8f0be4c9d13 Mon Sep 17 00:00:00 2001 From: Robert Helewka Date: Mon, 3 Aug 2026 12:41:55 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=BE=20fix(api):=20cascade=20plain=20li?= =?UTF-8?q?brary=20delete;=20case-insensitive=20name=20conflict?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETE /library/api/libraries/{uid}/ called bare lib.delete(), orphaning Collections/Items/Chunks/Images and skipping Concept GC — the only delete path not using the shared cascade. It now delegates to delete_library_cascade like the HTML and workspace delete paths. Name-conflict checks on both create endpoints are now case-insensitive via find_library_by_name_ci (parameterised Cypher toLower comparison — not neomodel iexact, which embeds the value in a regex and breaks on names like "C++ Notes"). The Neo4j unique index is case-sensitive, so "amazon connect" previously coexisted silently with "Amazon Connect", confusing every name-matching client (Spelunker matches names case-insensitively). The 409 reports the existing spelling. The plain create also gains a UniqueProperty catch for the pre-check/save race, mirroring the workspace path. Co-Authored-By: Claude Fable 5 --- mnemosyne/library/api/views.py | 49 +++++++++++---- mnemosyne/library/api/workspaces.py | 24 ++++++- mnemosyne/library/models.py | 17 +++++ mnemosyne/library/tests/test_library_api.py | 70 +++++++++++++++++++++ mnemosyne/library/tests/test_managed_by.py | 60 +++++++++++++----- 5 files changed, 191 insertions(+), 29 deletions(-) create mode 100644 mnemosyne/library/tests/test_library_api.py diff --git a/mnemosyne/library/api/views.py b/mnemosyne/library/api/views.py index d8f4fc9..03b3238 100644 --- a/mnemosyne/library/api/views.py +++ b/mnemosyne/library/api/views.py @@ -10,6 +10,7 @@ import os from django.core.files.base import ContentFile from django.core.files.storage import default_storage +from neomodel.exceptions import UniqueProperty from rest_framework import status from rest_framework.decorators import api_view, parser_classes, permission_classes from rest_framework.parsers import FormParser, JSONParser, MultiPartParser @@ -17,6 +18,7 @@ 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 mcp_server.drf_auth import request_token_label from .serializers import ( @@ -51,7 +53,7 @@ def library_list_create(request): a per-library ``item_count``. Off by default because the count is a Cypher aggregate; on for the Daedalus-side registry poll. """ - from library.models import Library + from library.models import Library, find_library_by_name_ci if request.method == "GET": include_workspace = request.GET.get("include_workspace", "true").lower() != "false" @@ -85,13 +87,13 @@ def library_list_create(request): serializer.is_valid(raise_exception=True) data = serializer.validated_data - # Library.name is globally unique; reject collisions with a clean 409 - # (uid + managed_by included so the caller can say who owns the name) - # instead of letting the unique-index save raise a 500. - try: - existing = Library.nodes.get(name=data["name"]) - except Library.DoesNotExist: - existing = None + # Library names are unique. The Neo4j index is case-sensitive, but + # clients (Spelunker, humans) treat names case-insensitively, so the + # create-time check is case-insensitive too — otherwise "amazon connect" + # silently creates a near-duplicate of "Amazon Connect". Reject with a + # clean 409 (uid + managed_by included so the caller can say who owns + # the name) instead of letting the unique-index save raise a 500. + existing = find_library_by_name_ci(data["name"]) if existing is not None: logger.warning( "library_create name_conflict name=%s existing_uid=%s caller=%s", @@ -99,7 +101,7 @@ def library_list_create(request): ) return Response( { - "detail": f"A library named '{data['name']}' already exists.", + "detail": f"A library named '{existing.name}' already exists.", "code": "name_conflict", "uid": existing.uid, "managed_by": existing.managed_by_display or None, @@ -127,7 +129,22 @@ def library_list_create(request): data.get("llm_context_prompt") or defaults["llm_context_prompt"] ), ) - lib.save() + try: + lib.save() + except UniqueProperty: + # Race between the pre-check and save — an exact-name twin landed + # in between. Same 409 shape, minus the loser's uid/manager. + logger.warning( + "library_create name_conflict (save race) name=%s caller=%s", + data["name"], request.user.username, + ) + return Response( + { + "detail": f"A library named '{data['name']}' already exists.", + "code": "name_conflict", + }, + status=status.HTTP_409_CONFLICT, + ) return Response(LibrarySerializer(lib).data, status=status.HTTP_201_CREATED) @@ -165,8 +182,16 @@ def library_detail(request, uid): lib.save() return Response(LibrarySerializer(lib).data) - # DELETE - lib.delete() + # DELETE — use the shared cascade so child nodes (Collections/Items/ + # Chunks/Images) and orphan Concepts are removed too; a bare + # lib.delete() would leak them all. + result = delete_library_cascade(lib) + logger.info( + "Library deleted via API library_uid=%s name=%s items=%d " + "orphans_deleted=%d caller=%s", + result["library_uid"], result["name"], result["item_count"], + result["orphans_deleted"], request.user.username, + ) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/mnemosyne/library/api/workspaces.py b/mnemosyne/library/api/workspaces.py index 3de0afe..d8c83b7 100644 --- a/mnemosyne/library/api/workspaces.py +++ b/mnemosyne/library/api/workspaces.py @@ -67,7 +67,7 @@ def workspace_create(request): workspace (200) — not an error. The library_type is frozen at first create; subsequent calls are not allowed to change it. """ - from library.models import Library + from library.models import Library, find_library_by_name_ci serializer = WorkspaceCreateSerializer(data=request.data) serializer.is_valid(raise_exception=True) @@ -127,6 +127,28 @@ def workspace_create(request): status=status.HTTP_200_OK, ) + # New workspace: reject a name already taken by any other library, + # case-insensitively — the Neo4j index is case-sensitive, so without + # this check "amazon connect" would silently coexist with an existing + # "Amazon Connect" and confuse every name-matching client. + name_taken = find_library_by_name_ci(data["name"]) + if name_taken is not None: + logger.warning( + "workspace_create name_conflict workspace_id=%s name=%s " + "existing_uid=%s", + data["workspace_id"], data["name"], name_taken.uid, + ) + return Response( + { + "detail": ( + f"A library named '{name_taken.name}' already exists in " + "Mnemosyne." + ), + "code": "name_conflict", + }, + status=status.HTTP_409_CONFLICT, + ) + defaults = get_library_type_config(data["library_type"]) lib = Library( name=data["name"], diff --git a/mnemosyne/library/models.py b/mnemosyne/library/models.py index 06105bd..0f184e5 100644 --- a/mnemosyne/library/models.py +++ b/mnemosyne/library/models.py @@ -63,6 +63,23 @@ def infer_legacy_manager(workspace_id): return "Kairos" if workspace_id.startswith("kairos-mail-") else "Daedalus" +def find_library_by_name_ci(name): + """Case-insensitively find a Library by name, or None. + + Parameterised Cypher rather than neomodel's ``iexact``, which embeds + the value in a regex and so breaks on names containing regex + metacharacters (e.g. "C++ Notes"). + """ + from neomodel import db + + rows, _ = db.cypher_query( + "MATCH (l:Library) WHERE toLower(l.name) = toLower($name) " + "RETURN l LIMIT 1", + {"name": name}, + ) + return Library.inflate(rows[0][0]) if rows else None + + class Library(StructuredNode): """ Top-level container representing a content library. diff --git a/mnemosyne/library/tests/test_library_api.py b/mnemosyne/library/tests/test_library_api.py new file mode 100644 index 0000000..975f414 --- /dev/null +++ b/mnemosyne/library/tests/test_library_api.py @@ -0,0 +1,70 @@ +"""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 +``sys.modules``, same style as ``test_managed_by.py``. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from django.contrib.auth import get_user_model +from django.test import TestCase +from rest_framework.test import APIClient + +User = get_user_model() + + +class LibraryApiDeleteTests(TestCase): + """DELETE on the plain library endpoint cascades.""" + + def setUp(self): + self.user = User.objects.create_user(username="op", password="pw") + self.client = APIClient() + 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) + + def test_delete_uses_shared_cascade(self): + lib = SimpleNamespace(uid="lib-1", name="Docs") + cascade_result = { + "library_uid": "lib-1", + "name": "Docs", + "item_count": 3, + "item_s3_keys": [], + "orphans_deleted": 1, + } + with patch.dict( + "sys.modules", {"library.models": self._fake_models_module(lib)} + ), patch( + "library.api.views.delete_library_cascade", + return_value=cascade_result, + ) as mock_cascade: + response = self.client.delete("/library/api/libraries/lib-1/") + + self.assertEqual(response.status_code, 204) + mock_cascade.assert_called_once_with(lib) + + def test_delete_missing_library_returns_404(self): + with patch.dict( + "sys.modules", {"library.models": self._fake_models_module(None)} + ), patch("library.api.views.delete_library_cascade") as mock_cascade: + response = self.client.delete("/library/api/libraries/nope/") + + self.assertEqual(response.status_code, 404) + mock_cascade.assert_not_called() diff --git a/mnemosyne/library/tests/test_managed_by.py b/mnemosyne/library/tests/test_managed_by.py index fff1a67..496c482 100644 --- a/mnemosyne/library/tests/test_managed_by.py +++ b/mnemosyne/library/tests/test_managed_by.py @@ -70,8 +70,9 @@ class _FakeLibrary: class DoesNotExist(Exception): pass - existing = None # what nodes.get(name=...) returns + existing = None # what find_library_by_name_ci returns instances = [] # constructor kwargs, in order + save_raises = None # exception instance save() should raise, if any def __init__(self, **kwargs): type(self).instances.append(kwargs) @@ -81,16 +82,20 @@ class _FakeLibrary: self.created_at = None def save(self): + if type(self).save_raises is not None: + raise type(self).save_raises return self - class _Nodes: - @staticmethod - def get(**kwargs): - if _FakeLibrary.existing is None: - raise _FakeLibrary.DoesNotExist() - return _FakeLibrary.existing - nodes = _Nodes() +def _fake_find_ci(name): + _FakeLibrary.ci_queries.append(name) + return _FakeLibrary.existing + + +def _fake_models_module(): + return SimpleNamespace( + Library=_FakeLibrary, find_library_by_name_ci=_fake_find_ci + ) class LibraryCreateStampingTests(TestCase): @@ -101,16 +106,15 @@ class LibraryCreateStampingTests(TestCase): self.client = APIClient() _FakeLibrary.existing = None _FakeLibrary.instances = [] + _FakeLibrary.save_raises = None + _FakeLibrary.ci_queries = [] - def _post(self, token=None): + def _post(self, token=None, name="Docs"): self.client.force_authenticate(user=self.user, token=token) - with patch.dict( - "sys.modules", - {"library.models": SimpleNamespace(Library=_FakeLibrary)}, - ): + with patch.dict("sys.modules", {"library.models": _fake_models_module()}): return self.client.post( "/library/api/libraries/", - {"name": "Docs", "library_type": "technical"}, + {"name": name, "library_type": "technical"}, format="json", ) @@ -129,7 +133,7 @@ class LibraryCreateStampingTests(TestCase): def test_duplicate_name_returns_409_name_conflict(self): _FakeLibrary.existing = SimpleNamespace( - uid="lib-old", managed_by_display="Daedalus" + uid="lib-old", name="Docs", managed_by_display="Daedalus" ) response = self._post(token=UserToken(name="Spelunker")) @@ -141,15 +145,39 @@ class LibraryCreateStampingTests(TestCase): self.assertEqual(body["managed_by"], "Daedalus") self.assertEqual(_FakeLibrary.instances, []) + def test_duplicate_check_is_case_insensitive(self): + """A case-variant name 409s and reports the existing spelling.""" + _FakeLibrary.existing = SimpleNamespace( + uid="lib-old", name="Amazon Connect", managed_by_display="Spelunker" + ) + response = self._post(token=None, name="amazon connect") + + self.assertEqual(response.status_code, 409) + # The lookup received the posted name (case-folding happens in + # Cypher), and the response names the existing spelling. + self.assertEqual(_FakeLibrary.ci_queries, ["amazon connect"]) + self.assertIn("Amazon Connect", response.json()["detail"]) + self.assertEqual(_FakeLibrary.instances, []) + def test_duplicate_of_unmanaged_reports_null_manager(self): _FakeLibrary.existing = SimpleNamespace( - uid="lib-old", managed_by_display="" + uid="lib-old", name="Docs", managed_by_display="" ) response = self._post(token=None) self.assertEqual(response.status_code, 409) self.assertIsNone(response.json()["managed_by"]) + def test_save_race_returns_409_not_500(self): + """An exact-name twin landing between pre-check and save 409s.""" + from neomodel.exceptions import UniqueProperty + + _FakeLibrary.save_raises = UniqueProperty("name") + response = self._post(token=None) + + self.assertEqual(response.status_code, 409) + self.assertEqual(response.json()["code"], "name_conflict") + class WorkspaceStatusSerializerManagedByTests(TestCase): """The workspace status payload carries ``managed_by`` (nullable)."""