Merge pull request #140 from cbcoutinho/feature/etag

fix(notes): Include ETags in responses to avoid accidently updates

Author: cbcoutinho

CLAUDE.md

@@ -102,13 +102,19 @@ Each Nextcloud app has a corresponding server module that:
 - **Important**: Integration tests run against live Docker containers. After making code changes to the MCP server, rebuild only the MCP container with `docker-compose up --build -d mcp` before running tests
 
 #### Testing Best Practices
-- **Always restart MCP server** after code changes with `docker-compose up --build -d mcp`
+- **MANDATORY: Always run tests after implementing features or fixing bugs**
+  - Run tests to completion before considering any task complete
+  - If tests require modifications to pass, ask for permission before proceeding
+  - Use `docker-compose up --build -d mcp` to rebuild MCP container after code changes
 - **Use existing fixtures** from `tests/conftest.py` to avoid duplicate setup work:
   - `nc_mcp_client` - MCP client session for tool/resource testing  
   - `nc_client` - Direct NextcloudClient for setup/cleanup operations
   - `temporary_note` - Creates and cleans up test notes automatically
   - `temporary_addressbook` - Creates and cleans up test address books
   - `temporary_contact` - Creates and cleans up test contacts
+- **Test specific functionality** after changes:
+  - For Notes changes: `uv run pytest tests/integration/test_mcp.py -k "notes" -v`
+  - For specific API changes: `uv run pytest tests/integration/test_notes_api.py -v`
 - **Avoid creating standalone test scripts** - use pytest with proper fixtures instead
 
 ### Configuration Files

nextcloud_mcp_server/models/__init__.py

@@ -3,8 +3,6 @@
 # Base models
 from .base import (
     BaseResponse,
-    ErrorResponse,
-    SuccessResponse,
     IdResponse,
     StatusResponse,
 )
@@ -82,8 +80,6 @@
 __all__ = [
     # Base models
     "BaseResponse",
-    "ErrorResponse",
-    "SuccessResponse",
     "IdResponse",
     "StatusResponse",
     # Notes models

nextcloud_mcp_server/models/base.py

@@ -1,7 +1,7 @@
 """Base Pydantic models for common response patterns."""
 
 from datetime import datetime, timezone
-from typing import Any, Dict, Optional, Union
+from typing import Optional, Union
 
 from pydantic import BaseModel, Field, field_serializer
 
@@ -35,24 +35,6 @@ def serialize_timestamp(self, timestamp: datetime) -> str:
         return iso_string
 
 
-class ErrorResponse(BaseResponse):
-    """Response model for error cases."""
-
-    success: bool = Field(default=False, description="Always False for error responses")
-    error: str = Field(description="Error message")
-    error_code: Optional[str] = Field(None, description="Optional error code")
-    details: Optional[Dict[str, Any]] = Field(
-        None, description="Additional error details"
-    )
-
-
-class SuccessResponse(BaseResponse):
-    """Generic success response."""
-
-    message: Optional[str] = Field(None, description="Optional success message")
-    data: Optional[Dict[str, Any]] = Field(None, description="Optional response data")
-
-
 class IdResponse(BaseResponse):
     """Response model for operations that return a new ID."""
 

nextcloud_mcp_server/models/notes.py

@@ -19,7 +19,7 @@ class Note(BaseModel):
     favorite: bool = Field(
         default=False, description="Whether note is marked as favorite"
     )
-    etag: Optional[str] = Field(None, description="ETag for versioning")
+    etag: str = Field(description="ETag for versioning")
     readonly: bool = Field(default=False, description="Whether note is read-only")
 
     @property
@@ -50,6 +50,7 @@ class CreateNoteResponse(IdResponse):
 
     title: str = Field(description="The created note title")
     category: str = Field(description="The created note category")
+    etag: str = Field(description="Current ETag for the created note")
 
 
 class UpdateNoteResponse(BaseResponse):
@@ -58,6 +59,7 @@ class UpdateNoteResponse(BaseResponse):
     id: int = Field(description="The updated note ID")
     title: str = Field(description="The updated note title")
     category: str = Field(description="The updated note category")
+    etag: str = Field(description="Current ETag for the updated note")
 
 
 class DeleteNoteResponse(StatusResponse):
@@ -72,6 +74,7 @@ class AppendContentResponse(BaseResponse):
     id: int = Field(description="The updated note ID")
     title: str = Field(description="The updated note title")
     category: str = Field(description="The updated note category")
+    etag: str = Field(description="Current ETag for the updated note")
 
 
 class SearchNotesResponse(BaseResponse):

nextcloud_mcp_server/server/notes.py

@@ -1,10 +1,11 @@
 import logging
 from httpx import HTTPStatusError
+from mcp.shared.exceptions import McpError
+from mcp.types import ErrorData
 
 from mcp.server.fastmcp import Context, FastMCP
 
 from nextcloud_mcp_server.client import NextcloudClient
-from nextcloud_mcp_server.models.base import ErrorResponse
 from nextcloud_mcp_server.models.notes import (
     Note,
     NotesSettings,
@@ -54,8 +55,6 @@ async def nc_notes_get_attachment(note_id: int, attachment_filename: str):
     @mcp.resource("nc://Notes/{note_id}")
     async def nc_get_note(note_id: int):
         """Get user note using note id"""
-        from mcp.shared.exceptions import McpError
-        from mcp.types import ErrorData
 
         ctx: Context = mcp.get_context()
         client: NextcloudClient = ctx.request_context.lifespan_context.client
@@ -80,7 +79,7 @@ async def nc_get_note(note_id: int):
     @mcp.tool()
     async def nc_notes_create_note(
         title: str, content: str, category: str, ctx: Context
-    ) -> CreateNoteResponse | ErrorResponse:
+    ) -> CreateNoteResponse:
         """Create a new note"""
         client: NextcloudClient = ctx.request_context.lifespan_context.client
         try:
@@ -91,22 +90,31 @@ async def nc_notes_create_note(
             )
             note = Note(**note_data)
             return CreateNoteResponse(
-                id=note.id, title=note.title, category=note.category
+                id=note.id, title=note.title, category=note.category, etag=note.etag
             )
         except HTTPStatusError as e:
             if e.response.status_code == 403:
-                return ErrorResponse(
-                    error="Access denied: insufficient permissions to create notes"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message="Access denied: insufficient permissions to create notes",
+                    )
                 )
             elif e.response.status_code == 413:
-                return ErrorResponse(error="Note content too large")
+                raise McpError(ErrorData(code=-1, message="Note content too large"))
             elif e.response.status_code == 409:
-                return ErrorResponse(
-                    error=f"A note with title '{title}' already exists in this category"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"A note with title '{title}' already exists in this category",
+                    )
                 )
             else:
-                return ErrorResponse(
-                    error=f"Failed to create note: server error ({e.response.status_code})"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Failed to create note: server error ({e.response.status_code})",
+                    )
                 )
 
     @mcp.tool()
@@ -117,8 +125,13 @@ async def nc_notes_update_note(
         content: str | None,
         category: str | None,
         ctx: Context,
-    ) -> UpdateNoteResponse | ErrorResponse:
-        """Update an existing note's title, content, or category"""
+    ) -> UpdateNoteResponse:
+        """Update an existing note's title, content, or category.
+
+        REQUIRED: etag parameter must be provided to prevent overwriting concurrent changes.
+        Get the current ETag by first retrieving the note using nc://Notes/{note_id} resource.
+        If the note has been modified by someone else since you retrieved it,
+        the update will fail with a 412 error."""
         logger.info("Updating note %s", note_id)
         client: NextcloudClient = ctx.request_context.lifespan_context.client
         try:
@@ -131,31 +144,44 @@ async def nc_notes_update_note(
             )
             note = Note(**note_data)
             return UpdateNoteResponse(
-                id=note.id, title=note.title, category=note.category
+                id=note.id, title=note.title, category=note.category, etag=note.etag
             )
         except HTTPStatusError as e:
             if e.response.status_code == 404:
-                return ErrorResponse(error=f"Note {note_id} not found")
+                raise McpError(ErrorData(code=-1, message=f"Note {note_id} not found"))
             elif e.response.status_code == 412:
-                return ErrorResponse(
-                    error=f"Note {note_id} has been modified by someone else. Please refresh and try again."
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Note {note_id} has been modified by someone else. Please refresh and try again.",
+                    )
                 )
             elif e.response.status_code == 403:
-                return ErrorResponse(
-                    error=f"Access denied: insufficient permissions to update note {note_id}"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Access denied: insufficient permissions to update note {note_id}",
+                    )
                 )
             elif e.response.status_code == 413:
-                return ErrorResponse(error="Updated note content is too large")
+                raise McpError(
+                    ErrorData(code=-1, message="Updated note content is too large")
+                )
             else:
-                return ErrorResponse(
-                    error=f"Failed to update note {note_id}: server error ({e.response.status_code})"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Failed to update note {note_id}: server error ({e.response.status_code})",
+                    )
                 )
 
     @mcp.tool()
     async def nc_notes_append_content(
         note_id: int, content: str, ctx: Context
-    ) -> AppendContentResponse | ErrorResponse:
-        """Append content to an existing note with a clear separator. The tool automatically adds separators between existing and new content - do not include separators in your content."""
+    ) -> AppendContentResponse:
+        """Append content to an existing note. The tool adds a `\n---\n`
+        between the note and what will be appended."""
+
         logger.info("Appending content to note %s", note_id)
         client: NextcloudClient = ctx.request_context.lifespan_context.client
         try:
@@ -164,28 +190,35 @@ async def nc_notes_append_content(
             )
             note = Note(**note_data)
             return AppendContentResponse(
-                id=note.id, title=note.title, category=note.category
+                id=note.id, title=note.title, category=note.category, etag=note.etag
             )
         except HTTPStatusError as e:
             if e.response.status_code == 404:
-                return ErrorResponse(error=f"Note {note_id} not found")
+                raise McpError(ErrorData(code=-1, message=f"Note {note_id} not found"))
             elif e.response.status_code == 403:
-                return ErrorResponse(
-                    error=f"Access denied: insufficient permissions to modify note {note_id}"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Access denied: insufficient permissions to modify note {note_id}",
+                    )
                 )
             elif e.response.status_code == 413:
-                return ErrorResponse(
-                    error="Content to append would make the note too large"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message="Content to append would make the note too large",
+                    )
                 )
             else:
-                return ErrorResponse(
-                    error=f"Failed to append content to note {note_id}: server error ({e.response.status_code})"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Failed to append content to note {note_id}: server error ({e.response.status_code})",
+                    )
                 )
 
     @mcp.tool()
-    async def nc_notes_search_notes(
-        query: str, ctx: Context
-    ) -> SearchNotesResponse | ErrorResponse:
+    async def nc_notes_search_notes(query: str, ctx: Context) -> SearchNotesResponse:
         """Search notes by title or content, returning only id, title, and category."""
         client: NextcloudClient = ctx.request_context.lifespan_context.client
         try:
@@ -207,20 +240,26 @@ async def nc_notes_search_notes(
             )
         except HTTPStatusError as e:
             if e.response.status_code == 403:
-                return ErrorResponse(
-                    error="Access denied: insufficient permissions to search notes"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message="Access denied: insufficient permissions to search notes",
+                    )
                 )
             elif e.response.status_code == 400:
-                return ErrorResponse(error="Invalid search query format")
+                raise McpError(
+                    ErrorData(code=-1, message="Invalid search query format")
+                )
             else:
-                return ErrorResponse(
-                    error=f"Search failed: server error ({e.response.status_code})"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Search failed: server error ({e.response.status_code})",
+                    )
                 )
 
     @mcp.tool()
-    async def nc_notes_delete_note(
-        note_id: int, ctx: Context
-    ) -> DeleteNoteResponse | ErrorResponse:
+    async def nc_notes_delete_note(note_id: int, ctx: Context) -> DeleteNoteResponse:
         """Delete a note permanently"""
         logger.info("Deleting note %s", note_id)
         client: NextcloudClient = ctx.request_context.lifespan_context.client
@@ -233,12 +272,18 @@ async def nc_notes_delete_note(
             )
         except HTTPStatusError as e:
             if e.response.status_code == 404:
-                return ErrorResponse(error=f"Note {note_id} not found")
+                raise McpError(ErrorData(code=-1, message=f"Note {note_id} not found"))
             elif e.response.status_code == 403:
-                return ErrorResponse(
-                    error=f"Access denied: insufficient permissions to delete note {note_id}"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Access denied: insufficient permissions to delete note {note_id}",
+                    )
                 )
             else:
-                return ErrorResponse(
-                    error=f"Failed to delete note {note_id}: server error ({e.response.status_code})"
+                raise McpError(
+                    ErrorData(
+                        code=-1,
+                        message=f"Failed to delete note {note_id}: server error ({e.response.status_code})",
+                    )
                 )

nextcloud_mcp_server/server/webdav.py

@@ -43,11 +43,11 @@ async def nc_webdav_read_file(path: str, ctx: Context):
         Examples:
             # Read a text file
             result = await nc_webdav_read_file("Documents/readme.txt")
-            print(result['content'])  # Decoded text content
+            logger.info(result['content'])  # Decoded text content
 
             # Read a binary file
             result = await nc_webdav_read_file("Images/photo.jpg")
-            print(result['encoding'])  # 'base64'
+            logger.info(result['encoding'])  # 'base64'
         """
         client: NextcloudClient = ctx.request_context.lifespan_context.client
         content, content_type = await client.webdav.read_file(path)