From fcad7b074dceea3f8cb972c6f767aeb0d807d632 Mon Sep 17 00:00:00 2001
From: phernandez <phernandez@basicmachines.co>
Date: Sat, 21 Dec 2024 23:52:08 -0600
Subject: [PATCH] fix delete to use get by id

---
 src/basic_memory/api/routers/documents.py     | 10 +++----
 src/basic_memory/services/document_service.py | 29 +++++++++++++++++++
 tests/api/test_documents_router.py            | 26 +++++++++++------
 3 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/src/basic_memory/api/routers/documents.py b/src/basic_memory/api/routers/documents.py
index 0f963f14..b2613176 100644
--- a/src/basic_memory/api/routers/documents.py
+++ b/src/basic_memory/api/routers/documents.py
@@ -100,18 +100,18 @@ async def update_document(
         raise HTTPException(status_code=400, detail=str(e))
 
 
-@router.delete("/{path:path}", status_code=204)
+@router.delete("/{id:int}", status_code=204)
 async def delete_document(
-    path: str,
+    id: int,
     service: DocumentServiceDep,
 ) -> None:
-    """Delete a document."""
+    """Delete a document by ID."""
     try:
-        await service.delete_document(path)
+        await service.delete_document_by_id(id)
     except DocumentNotFoundError:
         raise HTTPException(
             status_code=404,
-            detail=f"Document not found: {path}"
+            detail=f"Document not found: {id}"
         )
     except DocumentWriteError as e:
         raise HTTPException(status_code=400, detail=str(e))
diff --git a/src/basic_memory/services/document_service.py b/src/basic_memory/services/document_service.py
index 43e95813..94a9f844 100644
--- a/src/basic_memory/services/document_service.py
+++ b/src/basic_memory/services/document_service.py
@@ -266,6 +266,35 @@ class DocumentService(BaseService[DocumentRepository]):
         assert updated_document is not None, f"Could not update document {doc.id}"
         return updated_document
 
+    async def delete_document_by_id(self, id: int) -> None:
+        """
+        Delete a document by ID.
+        
+        Args:
+            id: Document ID
+            
+        Raises:
+            DocumentNotFoundError: If document doesn't exist
+            DocumentWriteError: If deletion fails
+        """
+        logger.debug(f"Deleting document with ID {id}")
+        
+        # Get document record first
+        query = select(Document).where(Document.id == id)
+        doc = await self.repository.find_one(query)
+        if not doc:
+            raise DocumentNotFoundError(f"Document not found: {id}")
+        
+        # Delete file first since it's source of truth
+        try:
+            file_path = Path(doc.path)
+            file_path.unlink(missing_ok=True)
+        except Exception as e:
+            raise DocumentWriteError(f"Failed to delete document {id}: {e}")
+            
+        # Delete database record
+        await self.repository.delete(doc.id)
+
     async def delete_document(self, path: str) -> None:
         """
         Delete a document.
diff --git a/tests/api/test_documents_router.py b/tests/api/test_documents_router.py
index eec018e2..2bfdbfb8 100644
--- a/tests/api/test_documents_router.py
+++ b/tests/api/test_documents_router.py
@@ -148,19 +148,27 @@ async def test_delete_document(client: AsyncClient, tmp_path: Path):
     assert create_response.status_code == 201
     created = create_response.json()
 
-    # Delete document
-    response = await client.delete(f"/documents/{test_doc['path']}")
+    # Delete document by ID
+    response = await client.delete(f"/documents/{created['id']}")
     assert response.status_code == 204
 
-    # Verify document is gone
+    # Verify document is gone from filesystem
     doc_path = Path(test_doc["path"])
     assert not doc_path.exists()
 
-    # Verify 404 on subsequent get
+    # Verify document is gone from DB (404 on get)
     get_response = await client.get(f"/documents/{created['id']}")
     assert get_response.status_code == 404
 
 
+@pytest.mark.asyncio
+async def test_delete_nonexistent_document(client: AsyncClient):
+    """Test deleting a document that doesn't exist."""
+    response = await client.delete("/documents/99999")
+    assert response.status_code == 404
+    assert "not found" in response.json()["detail"].lower()
+
+
 @pytest.mark.asyncio
 async def test_list_documents(client: AsyncClient, tmp_path: Path):
     """Test document listing endpoint."""
@@ -193,12 +201,12 @@ async def test_list_documents(client: AsyncClient, tmp_path: Path):
     # We should have both documents
     assert len(data) == 2
     
-    # Verify all paths are present
-    paths = {item["path"] for item in data}
-    expected_paths = {doc["path"] for doc in docs}
-    assert paths == expected_paths
+    # Verify all documents are present by ID
+    ids = {item["id"] for item in data}
+    expected_ids = {doc["id"] for doc in created_docs}
+    assert ids == expected_ids
 
     # Verify metadata was preserved
     for item in data:
-        matching_doc = next(d for d in docs if d["path"] == item["path"])
+        matching_doc = next(d for d in created_docs if d["id"] == item["id"])
         assert item["doc_metadata"] == matching_doc["doc_metadata"]
\ No newline at end of file