From fc62be08e2fa2023c00192042082ef989b238ba4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sun, 26 Apr 2026 18:02:19 +0200 Subject: [PATCH] fix(notes): defensively unwrap list-shaped Notes responses (refs #730) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Notes app v5.0.0 has scenarios where the API returns a JSON list where the MCP server expects a single note object — notably the notes_api#fail catch-all returning [] for unmatched routes. Without a guard, callers hit a cryptic Pydantic "argument after ** must be a mapping, not list" from Note(**payload). Add a small _expect_note_object helper at the client layer: - dict → pass through (the healthy case) - single-element list → unwrap and warn (Notes v5.0.0 quirk) - empty list, multi-element list, non-dict → raise a diagnostic ValueError that names the operation and points at the likely root cause (URL prefix, unmatched route, wrong API version) Wire it into get_note / create_note / update so any list-shaped response fails clearly instead of cryptically. Six unit tests pin every branch of the helper. Note: The 405s the issue reports for update_note / append_content match Notes v5.0.0's documented routes (PUT /api/v1/notes/{id}) per upstream appinfo/routes.php. They are most likely a downstream effect of #732 (missing /index.php URL prefix on installs without Pretty URLs) — the fix in PR #733 should resolve those once it lands. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/client/notes.py | 42 ++++++++++++++++-- tests/unit/client/test_notes.py | 65 ++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 tests/unit/client/test_notes.py diff --git a/nextcloud_mcp_server/client/notes.py b/nextcloud_mcp_server/client/notes.py index 94588ad8..4bd53e3c 100644 --- a/nextcloud_mcp_server/client/notes.py +++ b/nextcloud_mcp_server/client/notes.py @@ -9,6 +9,42 @@ from .webdav import WebDAVClient logger = logging.getLogger(__name__) +def _expect_note_object(payload: Any, *, operation: str) -> Dict[str, Any]: + """Coerce a Notes API single-note response into a dict. + + Notes v5.0.0 has a catch-all route (``notes_api#fail``) that returns ``[]`` + as JSON for unmatched paths, and a few edge cases where the response is a + list-wrapped object instead of a bare object — see issue #730. Without this + guard, callers hit a cryptic Pydantic ``"argument after ** must be a mapping, + not list"`` from ``Note(**payload)``. + + Returns the dict unchanged. If the payload is a single-element list, returns + the inner dict. Anything else (empty list, list of multiple, non-dict) raises + a clear ``ValueError`` so the failure mode is obvious in logs. + """ + if isinstance(payload, dict): + return payload + if isinstance(payload, list): + if len(payload) == 1 and isinstance(payload[0], dict): + logger.warning( + "Notes API returned a single-element list for %s; unwrapping. " + "This is a Notes app v5.0.0 quirk — see #730.", + operation, + ) + return payload[0] + raise ValueError( + f"{operation}: Notes API returned a list-shaped payload " + f"({len(payload)} elements) where a single note object was expected. " + f"This typically means the request was routed to the catch-all " + f"notes_api#fail handler (e.g. unmatched URL or wrong API version). " + f"Verify the Notes app version and URL prefix (#732)." + ) + raise ValueError( + f"{operation}: Notes API returned an unexpected payload type " + f"({type(payload).__name__}) where a single note object was expected." + ) + + class NotesClient(BaseNextcloudClient): """Client for Nextcloud Notes app operations.""" @@ -79,7 +115,7 @@ class NotesClient(BaseNextcloudClient): response = await self._make_request( "GET", f"/apps/notes/api/v1/notes/{note_id}" ) - return response.json() + return _expect_note_object(response.json(), operation="get_note") async def create_note( self, @@ -99,7 +135,7 @@ class NotesClient(BaseNextcloudClient): response = await self._make_request( "POST", "/apps/notes/api/v1/notes", json=body ) - return response.json() + return _expect_note_object(response.json(), operation="create_note") async def update( self, @@ -146,7 +182,7 @@ class NotesClient(BaseNextcloudClient): logger.info( f"Update response for note {note_id}: Status {response.status_code}" ) - updated_note = response.json() + updated_note = _expect_note_object(response.json(), operation="update_note") # Check for category change and cleanup old attachment directory if needed if ( diff --git a/tests/unit/client/test_notes.py b/tests/unit/client/test_notes.py new file mode 100644 index 00000000..54784b91 --- /dev/null +++ b/tests/unit/client/test_notes.py @@ -0,0 +1,65 @@ +"""Unit tests for the NotesClient response-shape guard. + +Notes app v5.0.0 has cases where the API returns a list-shaped JSON payload +where the MCP server expects a single note object — see issue #730. The +``_expect_note_object`` helper coerces these and surfaces clear errors instead +of letting Pydantic raise the cryptic ``"argument after ** must be a mapping, +not list"``. +""" + +import pytest + +from nextcloud_mcp_server.client.notes import _expect_note_object + +pytestmark = pytest.mark.unit + + +def test_dict_payload_passes_through(): + """The healthy case: API returns a single note object — return it unchanged.""" + payload = {"id": 1, "title": "Test", "content": "body", "etag": "abc"} + assert _expect_note_object(payload, operation="create_note") is payload + + +def test_single_element_list_is_unwrapped(caplog): + """Notes v5.0.0 sometimes wraps a single note in a list; unwrap it and + log a warning so operators notice the upstream quirk. + """ + inner = {"id": 1, "title": "Test", "etag": "abc"} + with caplog.at_level("WARNING"): + result = _expect_note_object([inner], operation="create_note") + assert result is inner + assert any("single-element list" in r.message for r in caplog.records) + + +def test_empty_list_raises_clear_error(): + """notes_api#fail returns ``[]`` for unmatched routes (#730). Surface a + diagnostic error rather than letting Pydantic complain about ``** mapping``. + """ + with pytest.raises(ValueError) as exc: + _expect_note_object([], operation="create_note") + msg = str(exc.value) + assert "create_note" in msg + assert "list-shaped payload" in msg + assert "notes_api#fail" in msg + + +def test_multi_element_list_raises_clear_error(): + """Defensive: a list with multiple elements is also wrong shape; bail loudly.""" + with pytest.raises(ValueError) as exc: + _expect_note_object([{"id": 1}, {"id": 2}], operation="update_note") + assert "update_note" in str(exc.value) + + +def test_non_dict_non_list_raises_clear_error(): + """Defensive: a string / int / None payload is also unexpected.""" + with pytest.raises(ValueError) as exc: + _expect_note_object("not a note", operation="create_note") + assert "unexpected payload type" in str(exc.value) + assert "str" in str(exc.value) + + +def test_list_of_non_dict_raises_clear_error(): + """A list whose element isn't a dict still fails with a clear error.""" + with pytest.raises(ValueError) as exc: + _expect_note_object(["not a note"], operation="get_note") + assert "list-shaped payload" in str(exc.value)