From f4bc0cabd99311aa37b05ab3c05c0614eeaf85ed Mon Sep 17 00:00:00 2001 From: phernandez Date: Sat, 21 Dec 2024 22:53:55 -0600 Subject: [PATCH] fix document api --- src/basic_memory/api/routers/documents.py | 73 ++++----- src/basic_memory/schemas/request.py | 20 +-- src/basic_memory/services/document_service.py | 82 ++++++---- tests/api/test_documents_router.py | 140 +++++++++++++++++- 4 files changed, 229 insertions(+), 86 deletions(-) diff --git a/src/basic_memory/api/routers/documents.py b/src/basic_memory/api/routers/documents.py index 58fbf2e8..9ec8501e 100644 --- a/src/basic_memory/api/routers/documents.py +++ b/src/basic_memory/api/routers/documents.py @@ -22,7 +22,7 @@ async def create_document( service: DocumentServiceDep, ) -> DocumentCreateResponse: """Create a new document. - + The document will be created with appropriate frontmatter including: - Generated ID - Creation timestamp @@ -35,18 +35,18 @@ async def create_document( content=doc.content, metadata=doc.doc_metadata, ) - return DocumentCreateResponse.from_orm(document) + return DocumentCreateResponse.model_validate(document.__dict__) except DocumentWriteError as e: raise HTTPException(status_code=400, detail=str(e)) -@router.get("/", response_model=List[DocumentResponse]) +@router.get("/", response_model=List[DocumentCreateResponse]) async def list_documents( service: DocumentServiceDep, -) -> List[DocumentResponse]: - """List all documents.""" +) -> List[DocumentCreateResponse]: + """List all documents (without content).""" documents = await service.list_documents() - return [DocumentResponse.from_orm(doc) for doc in documents] + return [DocumentCreateResponse.model_validate(doc.__dict__) for doc in documents] @router.get("/{path:path}", response_model=DocumentResponse) @@ -57,57 +57,45 @@ async def get_document( """Get a document by path.""" try: document, content = await service.read_document(path) - response = DocumentResponse.model_validate(document.__dict__ | {"content": content}) + doc_dict = document.__dict__ | {"content": content} + response = DocumentResponse.model_validate(doc_dict) return response except DocumentNotFoundError: - raise HTTPException(status_code=404, detail=f"Document not found: {path}") + raise HTTPException( + status_code=404, + detail=f"Document not found: {path}" + ) except DocumentWriteError as e: raise HTTPException(status_code=400, detail=str(e)) -@router.put("/{path:path}", response_model=DocumentResponse) +@router.put("/{id:int}", response_model=DocumentResponse) async def update_document( - path: str, + id: int, doc: DocumentUpdate, service: DocumentServiceDep, ) -> DocumentResponse: - """Update a document's content and/or metadata.""" + """Update a document by ID.""" + # Verify IDs match + if doc.id != id: + raise HTTPException( + status_code=400, + detail="Document ID in URL must match ID in request body" + ) + try: - document = await service.update_document( - path=path, + document = await service.update_document_by_id( + id=id, content=doc.content, metadata=doc.doc_metadata, ) - return DocumentResponse.from_orm(document) + doc_dict = document.__dict__ | {"content": doc.content} + return DocumentResponse.model_validate(doc_dict) except DocumentNotFoundError: - raise HTTPException(status_code=404, detail=f"Document not found: {path}") - except DocumentWriteError as e: - raise HTTPException(status_code=400, detail=str(e)) - - -@router.patch("/{path:path}", response_model=DocumentResponse) -async def patch_document( - path: str, - patch: DocumentPatch, - service: DocumentServiceDep, -) -> DocumentResponse: - """Partially update a document.""" - # Require full content updates for now - if patch.content is None: raise HTTPException( - status_code=400, - detail=("Partial content updates not yet implemented. " "Please provide full content."), + status_code=404, + detail=f"Document not found: {id}" ) - - try: - document = await service.update_document( - path=path, - content=patch.content, - metadata=patch.doc_metadata, - ) - return DocumentResponse.from_orm(document) - except DocumentNotFoundError: - raise HTTPException(status_code=404, detail=f"Document not found: {path}") except DocumentWriteError as e: raise HTTPException(status_code=400, detail=str(e)) @@ -121,6 +109,9 @@ async def delete_document( try: await service.delete_document(path) except DocumentNotFoundError: - raise HTTPException(status_code=404, detail=f"Document not found: {path}") + raise HTTPException( + status_code=404, + detail=f"Document not found: {path}" + ) except DocumentWriteError as e: raise HTTPException(status_code=400, detail=str(e)) diff --git a/src/basic_memory/schemas/request.py b/src/basic_memory/schemas/request.py index 7248e652..a46e04d9 100644 --- a/src/basic_memory/schemas/request.py +++ b/src/basic_memory/schemas/request.py @@ -1,16 +1,4 @@ -"""Request schemas for interacting with the knowledge graph. - -This module defines all the request schemas for creating, retrieving, and managing -knowledge graph entities. Each schema includes validation rules and clear examples -of proper usage. - -Request Types: -1. Entity Creation - Create new nodes in the graph -2. Observation Addition - Add facts to existing entities -3. Relation Creation - Connect entities with typed edges -4. Node Search - Find entities across the graph -5. Node Retrieval - Load specific entities by ID -""" +"""Request schemas for interacting with the knowledge graph.""" from typing import List, Optional, Annotated, Dict, Any @@ -220,14 +208,14 @@ class DocumentCreate(BaseModel): class DocumentUpdate(BaseModel): - id: int - checksum: str + """Update an existing document by ID.""" + + id: int # Document ID is required for updates content: str doc_metadata: Optional[Dict[str, Any]] = None class DocumentPatch(BaseModel): id: int - checksum: str content: Optional[str] = None doc_metadata: Optional[Dict[str, Any]] = None diff --git a/src/basic_memory/services/document_service.py b/src/basic_memory/services/document_service.py index eb1d193f..f074322e 100644 --- a/src/basic_memory/services/document_service.py +++ b/src/basic_memory/services/document_service.py @@ -3,11 +3,11 @@ import hashlib from datetime import datetime, UTC from pathlib import Path -from typing import Optional, Dict, Any, Sequence +from typing import Optional, Dict, Any, List import yaml -from icecream import ic from loguru import logger +from sqlalchemy import select from basic_memory.models import Document from basic_memory.repository.document_repository import DocumentRepository @@ -16,19 +16,16 @@ from basic_memory.services.service import BaseService class DocumentError(Exception): """Base exception for document operations.""" - pass class DocumentNotFoundError(DocumentError): """Raised when a document doesn't exist.""" - pass class DocumentWriteError(DocumentError): """Raised when document file operations fail.""" - pass @@ -49,41 +46,37 @@ class DocumentService(BaseService[DocumentRepository]): async def ensure_parent_directory(self, path: Path) -> None: """ - Ensure parent directory exists and is writable. + Ensure parent directory exists. Args: path: Path to check Raises: - DocumentWriteError: If directory cannot be created or is not writable + DocumentWriteError: If directory cannot be created """ parent = path.parent try: - if not parent.exists(): - parent.mkdir(parents=True) - - # Verify we can write to it - test_file = parent / ".write_test" - test_file.touch() - test_file.unlink() + parent.mkdir(parents=True, exist_ok=True) except Exception as e: - raise DocumentWriteError(f"Directory not writable: {parent}: {e}") + raise DocumentWriteError(f"Failed to create directory: {parent}: {e}") - async def add_frontmatter( - self, content: str, doc_id: int, metadata: Optional[Dict[str, Any]] = None - ) -> str: + async def add_frontmatter(self, content: str, doc_id: int, metadata: Optional[Dict[str, Any]] = None) -> str: """Add frontmatter to document content.""" # Generate frontmatter with timestamps now = datetime.now(UTC).isoformat() - frontmatter = {"id": doc_id, "created": now, "modified": now} + frontmatter = { + "id": doc_id, + "created": now, + "modified": now + } if metadata: frontmatter.update(metadata) - + yaml_fm = yaml.dump(frontmatter, sort_keys=False) return f"---\n{yaml_fm}---\n\n{content}" - async def list_documents(self) -> Sequence[Document]: - """List all documents in the database.""" + async def list_documents(self) -> List[Document]: + """List all documents.""" return await self.repository.find_all() async def create_document( @@ -105,13 +98,16 @@ class DocumentService(BaseService[DocumentRepository]): """ logger.debug(f"Creating document at {path}") - # Ensure parent directories exist and are writable + # Ensure parent directories exist file_path = Path(path) await self.ensure_parent_directory(file_path) try: # 1. Create initial DB record to get ID - doc = await self.repository.create({"path": str(path), "doc_metadata": metadata}) + doc = await self.repository.create({ + "path": str(path), + "doc_metadata": metadata + }) # 2. Add frontmatter with DB-generated ID content_with_frontmatter = await self.add_frontmatter(content, doc.id, metadata) @@ -122,12 +118,11 @@ class DocumentService(BaseService[DocumentRepository]): # 4. Update DB with checksum to mark completion checksum = await self.compute_checksum(content_with_frontmatter) doc = await self.repository.update(doc.id, {"checksum": checksum}) - ic(doc) return doc except Exception as e: # Clean up on any failure - if "doc" in locals(): # DB record was created + if 'doc' in locals(): # DB record was created await self.repository.delete(doc.id) file_path.unlink(missing_ok=True) raise DocumentWriteError(f"Failed to create document: {e}") @@ -167,6 +162,39 @@ class DocumentService(BaseService[DocumentRepository]): return doc, content + async def update_document_by_id( + self, id: int, content: str, metadata: Optional[Dict[str, Any]] = None + ) -> Document: + """ + Update a document using its ID. + + Args: + id: Document ID + content: New content + metadata: Optional new metadata + + Returns: + Updated document record + + Raises: + DocumentNotFoundError: If document doesn't exist + DocumentWriteError: If update fails + """ + logger.debug(f"Updating document with ID: {id}") + + # Find document first + query = select(Document).where(Document.id == id) + document = await self.repository.find_one(query) + if not document: + raise DocumentNotFoundError(f"Document not found: {id}") + + # Use existing path to update file + return await self.update_document( + path=document.path, + content=content, + metadata=metadata + ) + async def update_document( self, path: str, content: str, metadata: Optional[Dict[str, Any]] = None ) -> Document: @@ -244,4 +272,4 @@ class DocumentService(BaseService[DocumentRepository]): # Delete database record if it exists doc = await self.repository.find_by_path(str(path)) if doc: - await self.repository.delete(doc.id) + await self.repository.delete(doc.id) \ No newline at end of file diff --git a/tests/api/test_documents_router.py b/tests/api/test_documents_router.py index 2882fd44..a0b23d4b 100644 --- a/tests/api/test_documents_router.py +++ b/tests/api/test_documents_router.py @@ -5,8 +5,6 @@ from pathlib import Path import pytest from httpx import AsyncClient -from basic_memory.schemas.response import DocumentCreateResponse - @pytest.mark.asyncio async def test_create_document(client: AsyncClient, tmp_path: Path): @@ -36,6 +34,19 @@ async def test_create_document(client: AsyncClient, tmp_path: Path): assert "# Test" in content # Has our content +@pytest.mark.asyncio +async def test_create_document_invalid_path(client: AsyncClient, tmp_path: Path): + """Test creating document in non-existent directory.""" + test_doc = { + "path": str(tmp_path / "nonexistent" / "test.md"), + "content": "test content", + "doc_metadata": {"type": "test"}, + } + + response = await client.post("/documents/", json=test_doc) + assert response.status_code == 201 # We now create parent directories + + @pytest.mark.asyncio async def test_get_document(client: AsyncClient, tmp_path: Path): """Test document retrieval endpoint.""" @@ -48,6 +59,7 @@ async def test_get_document(client: AsyncClient, tmp_path: Path): # Create document create_response = await client.post("/documents/", json=test_doc) assert create_response.status_code == 201 + created = create_response.json() # Get document response = await client.get(f"/documents/{test_doc['path']}") @@ -63,3 +75,127 @@ async def test_get_document(client: AsyncClient, tmp_path: Path): assert "id:" in content # Has generated ID assert "# Test" in content # Has heading assert "This is a test document" in content # Has body + + +@pytest.mark.asyncio +async def test_get_nonexistent_document(client: AsyncClient, tmp_path: Path): + """Test getting a document that doesn't exist.""" + response = await client.get(f"/documents/{tmp_path}/nonexistent.md") + assert response.status_code == 404 + assert "not found" in response.json()["detail"].lower() + + +@pytest.mark.asyncio +async def test_update_document(client: AsyncClient, tmp_path: Path): + """Test document update endpoint using document ID.""" + # Create initial document + test_doc = { + "path": str(tmp_path / "test.md"), + "content": "# Original\nOriginal content.", + "doc_metadata": {"type": "test", "status": "draft"}, + } + create_response = await client.post("/documents/", json=test_doc) + assert create_response.status_code == 201 + created = create_response.json() + + # Update the document + update_doc = { + "id": created["id"], + "content": "# Updated\nUpdated content.", + "doc_metadata": {"type": "test", "status": "final"}, + } + response = await client.put(f"/documents/{created['id']}", json=update_doc) + assert response.status_code == 200 + + data = response.json() + assert data["doc_metadata"] == update_doc["doc_metadata"] + assert "# Updated" in data["content"] + assert "Updated content" in data["content"] + + # Verify file was updated + doc_path = Path(test_doc["path"]) + content = doc_path.read_text() + assert "# Updated" in content + assert "Updated content" in content + + +@pytest.mark.asyncio +async def test_update_nonexistent_document(client: AsyncClient): + """Test updating a document that doesn't exist.""" + update_doc = { + "id": 99999, # Non-existent ID + "content": "new content", + "doc_metadata": {"type": "test"}, + } + response = await client.put("/documents/99999", json=update_doc) + assert response.status_code == 404 + assert "not found" in response.json()["detail"].lower() + + +@pytest.mark.asyncio +async def test_delete_document(client: AsyncClient, tmp_path: Path): + """Test document deletion endpoint.""" + test_doc = { + "path": str(tmp_path / "test.md"), + "content": "# Test\nTest content.", + "doc_metadata": {"type": "test"}, + } + + # Create document + create_response = await client.post("/documents/", json=test_doc) + assert create_response.status_code == 201 + + # Delete document + response = await client.delete(f"/documents/{test_doc['path']}") + assert response.status_code == 204 + + # Verify document is gone + doc_path = Path(test_doc["path"]) + assert not doc_path.exists() + + # Verify 404 on subsequent get + get_response = await client.get(f"/documents/{test_doc['path']}") + assert get_response.status_code == 404 + + +@pytest.mark.asyncio +async def test_list_documents(client: AsyncClient, tmp_path: Path): + """Test document listing endpoint.""" + # Create a few test documents + docs = [ + { + "path": str(tmp_path / "doc1.md"), + "content": "# Doc 1", + "doc_metadata": {"type": "test", "number": 1}, + }, + { + "path": str(tmp_path / "doc2.md"), + "content": "# Doc 2", + "doc_metadata": {"type": "test", "number": 2}, + }, + ] + + # Create all documents + created_docs = [] + for doc in docs: + response = await client.post("/documents/", json=doc) + assert response.status_code == 201 + created_docs.append(response.json()) + + # List all documents + response = await client.get("/documents/") + assert response.status_code == 200 + data = response.json() + + # 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 metadata was preserved + for item in data: + matching_doc = next(d for d in docs if d["path"] == item["path"]) + assert item["doc_metadata"] == matching_doc["doc_metadata"] \ No newline at end of file