🐾 fix(api): cascade plain library delete; case-insensitive name conflict
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 <noreply@anthropic.com>
This commit is contained in:
@@ -10,6 +10,7 @@ import os
|
|||||||
|
|
||||||
from django.core.files.base import ContentFile
|
from django.core.files.base import ContentFile
|
||||||
from django.core.files.storage import default_storage
|
from django.core.files.storage import default_storage
|
||||||
|
from neomodel.exceptions import UniqueProperty
|
||||||
from rest_framework import status
|
from rest_framework import status
|
||||||
from rest_framework.decorators import api_view, parser_classes, permission_classes
|
from rest_framework.decorators import api_view, parser_classes, permission_classes
|
||||||
from rest_framework.parsers import FormParser, JSONParser, MultiPartParser
|
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 rest_framework.response import Response
|
||||||
|
|
||||||
from library.content_types import get_library_type_config
|
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 mcp_server.drf_auth import request_token_label
|
||||||
|
|
||||||
from .serializers import (
|
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
|
a per-library ``item_count``. Off by default because the count is a
|
||||||
Cypher aggregate; on for the Daedalus-side registry poll.
|
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":
|
if request.method == "GET":
|
||||||
include_workspace = request.GET.get("include_workspace", "true").lower() != "false"
|
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)
|
serializer.is_valid(raise_exception=True)
|
||||||
data = serializer.validated_data
|
data = serializer.validated_data
|
||||||
|
|
||||||
# Library.name is globally unique; reject collisions with a clean 409
|
# Library names are unique. The Neo4j index is case-sensitive, but
|
||||||
# (uid + managed_by included so the caller can say who owns the name)
|
# clients (Spelunker, humans) treat names case-insensitively, so the
|
||||||
# instead of letting the unique-index save raise a 500.
|
# create-time check is case-insensitive too — otherwise "amazon connect"
|
||||||
try:
|
# silently creates a near-duplicate of "Amazon Connect". Reject with a
|
||||||
existing = Library.nodes.get(name=data["name"])
|
# clean 409 (uid + managed_by included so the caller can say who owns
|
||||||
except Library.DoesNotExist:
|
# the name) instead of letting the unique-index save raise a 500.
|
||||||
existing = None
|
existing = find_library_by_name_ci(data["name"])
|
||||||
if existing is not None:
|
if existing is not None:
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"library_create name_conflict name=%s existing_uid=%s caller=%s",
|
"library_create name_conflict name=%s existing_uid=%s caller=%s",
|
||||||
@@ -99,7 +101,7 @@ def library_list_create(request):
|
|||||||
)
|
)
|
||||||
return Response(
|
return Response(
|
||||||
{
|
{
|
||||||
"detail": f"A library named '{data['name']}' already exists.",
|
"detail": f"A library named '{existing.name}' already exists.",
|
||||||
"code": "name_conflict",
|
"code": "name_conflict",
|
||||||
"uid": existing.uid,
|
"uid": existing.uid,
|
||||||
"managed_by": existing.managed_by_display or None,
|
"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"]
|
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)
|
return Response(LibrarySerializer(lib).data, status=status.HTTP_201_CREATED)
|
||||||
|
|
||||||
|
|
||||||
@@ -165,8 +182,16 @@ def library_detail(request, uid):
|
|||||||
lib.save()
|
lib.save()
|
||||||
return Response(LibrarySerializer(lib).data)
|
return Response(LibrarySerializer(lib).data)
|
||||||
|
|
||||||
# DELETE
|
# DELETE — use the shared cascade so child nodes (Collections/Items/
|
||||||
lib.delete()
|
# 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)
|
return Response(status=status.HTTP_204_NO_CONTENT)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -67,7 +67,7 @@ def workspace_create(request):
|
|||||||
workspace (200) — not an error. The library_type is frozen at first
|
workspace (200) — not an error. The library_type is frozen at first
|
||||||
create; subsequent calls are not allowed to change it.
|
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 = WorkspaceCreateSerializer(data=request.data)
|
||||||
serializer.is_valid(raise_exception=True)
|
serializer.is_valid(raise_exception=True)
|
||||||
@@ -127,6 +127,28 @@ def workspace_create(request):
|
|||||||
status=status.HTTP_200_OK,
|
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"])
|
defaults = get_library_type_config(data["library_type"])
|
||||||
lib = Library(
|
lib = Library(
|
||||||
name=data["name"],
|
name=data["name"],
|
||||||
|
|||||||
@@ -63,6 +63,23 @@ def infer_legacy_manager(workspace_id):
|
|||||||
return "Kairos" if workspace_id.startswith("kairos-mail-") else "Daedalus"
|
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):
|
class Library(StructuredNode):
|
||||||
"""
|
"""
|
||||||
Top-level container representing a content library.
|
Top-level container representing a content library.
|
||||||
|
|||||||
70
mnemosyne/library/tests/test_library_api.py
Normal file
70
mnemosyne/library/tests/test_library_api.py
Normal file
@@ -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()
|
||||||
@@ -70,8 +70,9 @@ class _FakeLibrary:
|
|||||||
class DoesNotExist(Exception):
|
class DoesNotExist(Exception):
|
||||||
pass
|
pass
|
||||||
|
|
||||||
existing = None # what nodes.get(name=...) returns
|
existing = None # what find_library_by_name_ci returns
|
||||||
instances = [] # constructor kwargs, in order
|
instances = [] # constructor kwargs, in order
|
||||||
|
save_raises = None # exception instance save() should raise, if any
|
||||||
|
|
||||||
def __init__(self, **kwargs):
|
def __init__(self, **kwargs):
|
||||||
type(self).instances.append(kwargs)
|
type(self).instances.append(kwargs)
|
||||||
@@ -81,16 +82,20 @@ class _FakeLibrary:
|
|||||||
self.created_at = None
|
self.created_at = None
|
||||||
|
|
||||||
def save(self):
|
def save(self):
|
||||||
|
if type(self).save_raises is not None:
|
||||||
|
raise type(self).save_raises
|
||||||
return self
|
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):
|
class LibraryCreateStampingTests(TestCase):
|
||||||
@@ -101,16 +106,15 @@ class LibraryCreateStampingTests(TestCase):
|
|||||||
self.client = APIClient()
|
self.client = APIClient()
|
||||||
_FakeLibrary.existing = None
|
_FakeLibrary.existing = None
|
||||||
_FakeLibrary.instances = []
|
_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)
|
self.client.force_authenticate(user=self.user, token=token)
|
||||||
with patch.dict(
|
with patch.dict("sys.modules", {"library.models": _fake_models_module()}):
|
||||||
"sys.modules",
|
|
||||||
{"library.models": SimpleNamespace(Library=_FakeLibrary)},
|
|
||||||
):
|
|
||||||
return self.client.post(
|
return self.client.post(
|
||||||
"/library/api/libraries/",
|
"/library/api/libraries/",
|
||||||
{"name": "Docs", "library_type": "technical"},
|
{"name": name, "library_type": "technical"},
|
||||||
format="json",
|
format="json",
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -129,7 +133,7 @@ class LibraryCreateStampingTests(TestCase):
|
|||||||
|
|
||||||
def test_duplicate_name_returns_409_name_conflict(self):
|
def test_duplicate_name_returns_409_name_conflict(self):
|
||||||
_FakeLibrary.existing = SimpleNamespace(
|
_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"))
|
response = self._post(token=UserToken(name="Spelunker"))
|
||||||
|
|
||||||
@@ -141,15 +145,39 @@ class LibraryCreateStampingTests(TestCase):
|
|||||||
self.assertEqual(body["managed_by"], "Daedalus")
|
self.assertEqual(body["managed_by"], "Daedalus")
|
||||||
self.assertEqual(_FakeLibrary.instances, [])
|
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):
|
def test_duplicate_of_unmanaged_reports_null_manager(self):
|
||||||
_FakeLibrary.existing = SimpleNamespace(
|
_FakeLibrary.existing = SimpleNamespace(
|
||||||
uid="lib-old", managed_by_display=""
|
uid="lib-old", name="Docs", managed_by_display=""
|
||||||
)
|
)
|
||||||
response = self._post(token=None)
|
response = self._post(token=None)
|
||||||
|
|
||||||
self.assertEqual(response.status_code, 409)
|
self.assertEqual(response.status_code, 409)
|
||||||
self.assertIsNone(response.json()["managed_by"])
|
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):
|
class WorkspaceStatusSerializerManagedByTests(TestCase):
|
||||||
"""The workspace status payload carries ``managed_by`` (nullable)."""
|
"""The workspace status payload carries ``managed_by`` (nullable)."""
|
||||||
|
|||||||
Reference in New Issue
Block a user