From 0204af4980eb5ebfeaa32b1c54188ebd8d2d1ef0 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 19 Jun 2026 11:31:06 +0200 Subject: [PATCH] fix(jmap): correctness fixes + dedup from June 2026 review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is part of a series of commits done to fix code review issues listed in docs/design/FULL_CODE_REVIEW_2026-06.md. The fixes were AI-generated; prompts were on the style "fix the code review issues". §1.3 create_event() has guarded against this since the original implementation: if 'new-0' not in created: raise JMAPMethodError(...). create_task() was missing the same check in both sync and async clients, so a JMAP server returning an empty created dict (possible when the server silently ignores the create) raised a bare KeyError instead of the documented JMAPMethodError. §4.1 jscal_to_ical: override child VEVENT with no "start" patch key got DTSTART from the master start_str instead of the occurrence's own time (the override key). Common title-only changes relocated every override to the master's first occurrence, breaking override display entirely. Default is now the override_key itself. §4.2 jscal_to_ical: EXDATE and RECURRENCE-ID values were always emitted as naive floating DATE-TIMEs. Per RFC 5545 the value type must match DTSTART. A floating EXDATE on a TZID-anchored event does not match any instance, so excluded occurrences reappeared. Override keys are now parsed with the event timezone applied (TZID events) or converted to date objects (all-day events). §4.3 _utils.py _format_local_dt(): UTC datetimes produced a Z-suffixed string. RFC 8984 §1.4 defines LocalDateTime (required for recurrenceOverrides keys and recurrenceRules.until) as YYYY-MM-DDThh:mm:ss without any suffix. Z-suffixed override keys cannot match LocalDateTime occurrence keys on strict servers. Function now always returns a timezone-stripped representation. §4.4 ical_to_jscal and jscal_to_ical: STATUS was silently dropped in both conversion directions. STATUS:CANCELLED round-tripped as status:confirmed (JSCalendar default), making cancelled meetings appear active. Added mappings CONFIRMED↔confirmed, TENTATIVE↔tentative, CANCELLED↔cancelled in both directions. §4.5 RFC 8620 §3.3: absent keys in a PatchObject preserve the server value; only explicit null entries delete a property. update_event sent the full converted JSCalendar object as the patch, so properties the caller removed (LOCATION, VALARM, DESCRIPTION, etc.) were simply absent and silently persisted on the server after the update. After converting ical_str to a JSCalendar dict, set all optional top-level properties to null when they are absent from the result. The list is maintained in caldav/jmap/convert/_patch.py and applied identically in both the sync (client.py) and async (async_client.py) update_event methods. §4.6: JMAPCalendar.search() passed datetime args through isoformat(), producing +HH:MM or bare datetimes instead of the UTCDate format (...Z) JMAP requires. Added _to_utcdate() helper in calendar.py that converts to UTC and strips microseconds. §4.7: get_objects_by_sync_token() discarded newState from CalendarEvent/changes into _, forcing callers to do a separate get_sync_token() call (race window). Now returns a 4-tuple (added, modified, deleted, new_sync_token). Updated all callers in unit and integration tests. §5.1: Moves all response-parsing logic from JMAPClient and AsyncJMAPClient into static methods on _JMAPClientBase. Each sync/async public method is now a ~3-line wrapper: get session, dispatch _request(), delegate to the shared parser. async_client.py drops from 550 → 415 lines; the parsers live in one place so future fixes (like the §1.13 create_task KeyError and §4.5 update_event nulling that this branch already carries) no longer need to be duplicated. §5.5: Hold one persistent HTTP session per client instead of per request --- caldav/jmap/async_client.py | 253 +++-------- caldav/jmap/client.py | 489 ++++++++++++++-------- caldav/jmap/convert/_patch.py | 33 ++ caldav/jmap/convert/_utils.py | 13 +- caldav/jmap/convert/ical_to_jscal.py | 11 + caldav/jmap/convert/jscal_to_ical.py | 23 +- caldav/jmap/objects/calendar.py | 21 +- docs/design/FULL_CODE_REVIEW_2026-06.md | 22 +- tests/test_jmap_integration.py | 6 +- tests/test_jmap_unit.py | 534 +++++++++++++++++++----- 10 files changed, 927 insertions(+), 478 deletions(-) create mode 100644 caldav/jmap/convert/_patch.py diff --git a/caldav/jmap/async_client.py b/caldav/jmap/async_client.py index d87fa682..07f64e57 100644 --- a/caldav/jmap/async_client.py +++ b/caldav/jmap/async_client.py @@ -3,6 +3,9 @@ Mirrors JMAPClient with all public methods as coroutines. Uses niquests.AsyncSession for HTTP — niquests is a core dependency. + +All response-parsing logic lives in _JMAPClientBase (client.py); each method +here is a ~3-line async wrapper: get session, send request, delegate to parser. """ from __future__ import annotations @@ -12,16 +15,13 @@ from niquests import AsyncSession -from caldav.jmap._methods.calendar import build_calendar_get, parse_calendar_get +from caldav.jmap._methods.calendar import build_calendar_get from caldav.jmap._methods.event import ( build_event_changes, build_event_get, build_event_set_create, build_event_set_destroy, build_event_set_update, - parse_event_changes, - parse_event_get, - parse_event_set, ) from caldav.jmap._methods.task import ( build_task_get, @@ -29,8 +29,6 @@ build_task_set_create, build_task_set_destroy, build_task_set_update, - parse_task_list_get, - parse_task_set, ) from caldav.jmap.client import _DEFAULT_USING, _TASK_USING, _JMAPClientBase from caldav.jmap.convert import ical_to_jscal @@ -64,11 +62,23 @@ class AsyncJMAPClient(_JMAPClientBase): timeout: HTTP request timeout in seconds. """ + def _get_http_session(self) -> AsyncSession: + """Return the persistent async HTTP session, creating it on first call.""" + if self._http_session is None: + sess = AsyncSession() + sess.auth = self._auth + sess.headers.update({"Content-Type": "application/json", "Accept": "application/json"}) + self._http_session = sess + return self._http_session + async def __aenter__(self) -> AsyncJMAPClient: + self._get_http_session() return self async def __aexit__(self, exc_type, exc_val, exc_tb) -> None: - return None + if self._http_session is not None: + await self._http_session.close() + self._http_session = None async def _get_session(self) -> Session: """Return the cached Session, fetching it on first call.""" @@ -103,14 +113,11 @@ async def _request(self, method_calls: list[tuple], using: list[str] | None = No log.debug("JMAP POST to %s: %d method call(s)", session.api_url, len(method_calls)) - async with AsyncSession() as http: - response = await http.post( - session.api_url, - json=payload, - auth=self._auth, - headers={"Content-Type": "application/json", "Accept": "application/json"}, - timeout=self.timeout, - ) + response = await self._get_http_session().post( + session.api_url, + json=payload, + timeout=self.timeout, + ) if response.status_code in (401, 403): raise JMAPAuthError( @@ -142,18 +149,8 @@ async def get_calendars(self) -> list[JMAPCalendar]: List of :class:`~caldav.jmap.objects.calendar.JMAPCalendar` objects. """ session = await self._get_session() - call = build_calendar_get(session.account_id) - responses = await self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "Calendar/get": - calendars = parse_calendar_get(resp_args) - for cal in calendars: - cal._client = self - cal._is_async = True - return calendars - - return [] + responses = await self._request([build_calendar_get(session.account_id)]) + return self._parse_get_calendars(responses, self, True) async def create_event(self, calendar_id: str, ical_str: str) -> str: """Create a calendar event from an iCalendar string. @@ -172,20 +169,7 @@ async def create_event(self, calendar_id: str, ical_str: str) -> str: jscal = ical_to_jscal(ical_str, calendar_id=calendar_id) call = build_event_set_create(session.account_id, {"new-0": jscal}) responses = await self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/set": - created, _, _, not_created, _, _ = parse_event_set(resp_args) - if "new-0" in not_created: - self._raise_set_error(session, not_created["new-0"]) - if "new-0" not in created: - raise JMAPMethodError( - url=session.api_url, - reason="CalendarEvent/set response missing created entry for new-0", - ) - return created["new-0"]["id"] - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/set response") + return self._parse_create_event_response(responses, session.api_url) async def get_event(self, event_id: str) -> JMAPCalendarObject: """Fetch a calendar event as an iCalendar string. @@ -203,21 +187,8 @@ async def get_event(self, event_id: str) -> JMAPCalendarObject: JMAPMethodError: If the event is not found. """ session = await self._get_session() - call = build_event_get(session.account_id, ids=[event_id]) - responses = await self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/get": - items = parse_event_get(resp_args) - if not items: - raise JMAPMethodError( - url=session.api_url, - reason=f"Event not found: {event_id}", - error_type="notFound", - ) - return JMAPCalendarObject(data=items[0], parent=None) - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/get response") + responses = await self._request([build_event_get(session.account_id, ids=[event_id])]) + return self._parse_get_event_response(responses, session.api_url, event_id) async def update_event(self, event_id: str, ical_str: str) -> None: """Update a calendar event from an iCalendar string. @@ -230,19 +201,17 @@ async def update_event(self, event_id: str, ical_str: str) -> None: JMAPMethodError: If the server rejects the update. """ session = await self._get_session() - patch = ical_to_jscal(ical_str) - patch.pop("uid", None) # uid is server-immutable after creation; patch must omit it - call = build_event_set_update(session.account_id, {event_id: patch}) - responses = await self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/set": - _, _, _, _, not_updated, _ = parse_event_set(resp_args) - if event_id in not_updated: - self._raise_set_error(session, not_updated[event_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/set response") + patch, nulled = self._build_event_update_patch(ical_str) + while True: + responses = await self._request( + [build_event_set_update(session.account_id, {event_id: patch})] + ) + drop = self._unsupported_null_keys(responses, event_id, patch, nulled) + if not drop: + break + for key in drop: + patch.pop(key, None) + self._parse_update_event_response(responses, session.api_url, event_id) async def _search( self, @@ -255,15 +224,7 @@ async def _search( session = await self._get_session() calls = self._build_event_search_calls(session.account_id, calendar_id, start, end, text) responses = await self._request(calls) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/get": - return [ - JMAPCalendarObject(data=item, parent=parent) - for item in parse_event_get(resp_args) - ] - - return [] + return self._parse_search_response(responses, parent) async def search_events( self, @@ -302,16 +263,12 @@ async def get_sync_token(self) -> str: retrieve only what changed since this point. """ session = await self._get_session() - call = build_event_get(session.account_id, ids=[]) - responses = await self._request([call]) - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/get": - return resp_args.get("state", "") - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/get response") + responses = await self._request([build_event_get(session.account_id, ids=[])]) + return self._parse_get_sync_token_response(responses, session.api_url) async def get_objects_by_sync_token( self, sync_token: str - ) -> tuple[list[JMAPCalendarObject], list[JMAPCalendarObject], list[str]]: + ) -> tuple[list[JMAPCalendarObject], list[JMAPCalendarObject], list[str], str]: """Fetch events changed since a previous sync token. Calls ``CalendarEvent/changes`` to discover which events were created, @@ -325,53 +282,28 @@ async def get_objects_by_sync_token( or by a prior call to this method. Returns: - A 3-tuple ``(added, modified, deleted)``: + A 4-tuple ``(added, modified, deleted, new_sync_token)``: - ``added``: objects for newly created events (``parent`` is ``None``). - ``modified``: objects for updated events (``parent`` is ``None``). - ``deleted``: Event IDs that were destroyed. + - ``new_sync_token``: Pass to the next call to this method as ``sync_token``. Raises: JMAPMethodError: If the server reports ``hasMoreChanges: true``. """ session = await self._get_session() - changes_call = build_event_changes(session.account_id, sync_token) - responses = await self._request([changes_call]) - - created_ids: list[str] = [] - updated_ids: list[str] = [] - destroyed: list[str] = [] - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/changes": - _, _, has_more, created_ids, updated_ids, destroyed = parse_event_changes(resp_args) - if has_more: - raise JMAPMethodError( - url=session.api_url, - reason=( - "CalendarEvent/changes response was truncated by the server " - "(hasMoreChanges=true). Call get_sync_token() to obtain a " - "fresh baseline and re-sync." - ), - error_type="serverPartialFail", - ) - + responses = await self._request([build_event_changes(session.account_id, sync_token)]) + created_ids, updated_ids, destroyed, new_sync_token = self._parse_event_changes_response( + responses, session.api_url + ) fetch_ids = created_ids + updated_ids if not fetch_ids: - return [], [], destroyed - - get_call = build_event_get(session.account_id, ids=fetch_ids) - get_responses = await self._request([get_call]) - - events_by_id: dict[str, JMAPCalendarObject] = {} - for method_name, resp_args, _ in get_responses: - if method_name == "CalendarEvent/get": - for item in parse_event_get(resp_args): - events_by_id[item["id"]] = JMAPCalendarObject(data=item, parent=None) - - added = [events_by_id[i] for i in created_ids if i in events_by_id] - modified = [events_by_id[i] for i in updated_ids if i in events_by_id] - return added, modified, destroyed + return [], [], destroyed, new_sync_token + get_responses = await self._request([build_event_get(session.account_id, ids=fetch_ids)]) + return self._assemble_sync_token_result( + get_responses, created_ids, updated_ids, destroyed, new_sync_token + ) async def delete_event(self, event_id: str) -> None: """Delete a calendar event. @@ -383,17 +315,8 @@ async def delete_event(self, event_id: str) -> None: JMAPMethodError: If the server rejects the delete. """ session = await self._get_session() - call = build_event_set_destroy(session.account_id, [event_id]) - responses = await self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/set": - _, _, _, _, _, not_destroyed = parse_event_set(resp_args) - if event_id in not_destroyed: - self._raise_set_error(session, not_destroyed[event_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/set response") + responses = await self._request([build_event_set_destroy(session.account_id, [event_id])]) + self._parse_delete_event_response(responses, session.api_url, event_id) async def _get_object_by_uid( self, uid: str, calendar_id: str | None = None, parent: JMAPCalendar | None = None @@ -414,14 +337,10 @@ async def get_task_lists(self) -> list[dict]: List of raw JMAP TaskList dicts as returned by the server. """ session = await self._get_session() - call = build_task_list_get(session.account_id) - responses = await self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "TaskList/get": - return parse_task_list_get(resp_args) - - return [] + responses = await self._request( + [build_task_list_get(session.account_id)], using=_TASK_USING + ) + return self._parse_get_task_lists_response(responses) async def create_task(self, task_list_id: str, title: str, **kwargs) -> str: """Create a task in a task list. @@ -452,15 +371,7 @@ async def create_task(self, task_list_id: str, title: str, **kwargs) -> str: task_dict.update(kwargs) call = build_task_set_create(session.account_id, {"new-0": task_dict}) responses = await self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/set": - created, _, _, not_created, _, _ = parse_task_set(resp_args) - if "new-0" in not_created: - self._raise_set_error(session, not_created["new-0"]) - return created["new-0"]["id"] - - raise JMAPMethodError(url=session.api_url, reason="No Task/set response") + return self._parse_create_task_response(responses, session.api_url) async def get_task(self, task_id: str) -> dict: """Fetch a task by ID. @@ -475,21 +386,10 @@ async def get_task(self, task_id: str) -> dict: JMAPMethodError: If the task is not found. """ session = await self._get_session() - call = build_task_get(session.account_id, ids=[task_id]) - responses = await self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/get": - items = resp_args.get("list", []) - if not items: - raise JMAPMethodError( - url=session.api_url, - reason=f"Task not found: {task_id}", - error_type="notFound", - ) - return items[0] - - raise JMAPMethodError(url=session.api_url, reason="No Task/get response") + responses = await self._request( + [build_task_get(session.account_id, ids=[task_id])], using=_TASK_USING + ) + return self._parse_get_task_response(responses, session.api_url, task_id) async def update_task(self, task_id: str, patch: dict) -> None: """Update a task with a partial patch. @@ -504,15 +404,7 @@ async def update_task(self, task_id: str, patch: dict) -> None: session = await self._get_session() call = build_task_set_update(session.account_id, {task_id: patch}) responses = await self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/set": - _, _, _, _, not_updated, _ = parse_task_set(resp_args) - if task_id in not_updated: - self._raise_set_error(session, not_updated[task_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No Task/set response") + self._parse_update_task_response(responses, session.api_url, task_id) async def delete_task(self, task_id: str) -> None: """Delete a task. @@ -524,14 +416,7 @@ async def delete_task(self, task_id: str) -> None: JMAPMethodError: If the server rejects the delete. """ session = await self._get_session() - call = build_task_set_destroy(session.account_id, [task_id]) - responses = await self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/set": - _, _, _, _, _, not_destroyed = parse_task_set(resp_args) - if task_id in not_destroyed: - self._raise_set_error(session, not_destroyed[task_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No Task/set response") + responses = await self._request( + [build_task_set_destroy(session.account_id, [task_id])], using=_TASK_USING + ) + self._parse_delete_task_response(responses, session.api_url, task_id) diff --git a/caldav/jmap/client.py b/caldav/jmap/client.py index 59cd333b..ebbba237 100644 --- a/caldav/jmap/client.py +++ b/caldav/jmap/client.py @@ -43,6 +43,7 @@ ) from caldav.jmap.constants import CALENDAR_CAPABILITY, CORE_CAPABILITY, TASK_CAPABILITY from caldav.jmap.convert import ical_to_jscal +from caldav.jmap.convert._patch import _NULL_FOR_UPDATE from caldav.jmap.error import JMAPAuthError, JMAPMethodError from caldav.jmap.objects.calendar import JMAPCalendar from caldav.jmap.objects.calendar_object import JMAPCalendarObject @@ -70,6 +71,7 @@ def __init__( self.password = password self.timeout = timeout self._session_cache: Session | None = None + self._http_session = None if auth is not None: self._auth = auth @@ -118,9 +120,10 @@ def _build_auth(self, auth_type: str | None): reason=f"Unsupported auth_type {effective_type!r}. Use 'basic' or 'bearer'.", ) - def _raise_set_error(self, session: Session, err: dict) -> None: + @staticmethod + def _raise_set_error(api_url: str, err: dict) -> None: raise JMAPMethodError( - url=session.api_url, + url=api_url, reason=f"set failed: {err}", error_type=err.get("type", "serverError"), ) @@ -158,6 +161,256 @@ def _build_event_search_calls( ) return [query_call, get_call] + @staticmethod + def _build_event_update_patch(ical_str: str) -> tuple[dict, frozenset[str]]: + """Build a JSCalendar PatchObject for a ``CalendarEvent/set`` update. + + RFC 8620 merge semantics preserve properties absent from the patch, so + any optional property removed client-side must be explicitly nulled to + actually clear it server-side. Returns the patch together with the set + of keys that were null-injected purely for this cleanup (i.e. were not + present in the converted iCalendar) so the caller can drop them if the + server refuses to null a property it does not support. + """ + patch = ical_to_jscal(ical_str) + patch.pop("uid", None) # uid is server-immutable after creation; patch must omit it + nulled: set[str] = set() + for key in _NULL_FOR_UPDATE: + if key not in patch: + patch[key] = None + nulled.add(key) + return patch, frozenset(nulled) + + @staticmethod + def _unsupported_null_keys( + responses: list, event_id: str, patch: dict, nulled: frozenset[str] + ) -> set[str] | None: + """Detect an update that failed *only* because the server rejects + null-clearing of properties it does not support. + + Some servers (e.g. Stalwart for ``recurrenceRules``) reject a property + outright in ``CalendarEvent/set``, even when it is being set to ``null``. + Nulling such a property is harmless cleanup — it was absent from the new + iCalendar — so we report it as droppable, letting the caller retry the + update without it. + + Returns the set of droppable keys when the failure is exactly this case, + or ``None`` when the update succeeded or failed for a genuine reason (in + which case the caller proceeds to :meth:`_parse_update_event_response`, + which raises the real error). Some servers report only one offending + property per response, so the caller retries in a loop, dropping the + reported keys until the update succeeds or hits a genuine error; each + returned key is guaranteed still present in ``patch``, so the loop + strictly shrinks the patch and terminates. + """ + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/set": + _, _, _, _, not_updated, _ = parse_event_set(resp_args) + err = not_updated.get(event_id) + if not err or err.get("type") != "invalidProperties": + return None + props = set(err.get("properties") or []) + droppable = {p for p in props if p in nulled and p in patch and patch[p] is None} + # Only retry when every offending property is null-cleanup we can + # safely omit; if the client actually set one of them to a value, + # the rejection is genuine and must surface. + if props and props == droppable: + return droppable + return None + return None + + # --------------------------------------------------------------------------- + # Shared response parsers — pure synchronous; used by both sync and async + # clients. Each method takes the raw ``methodResponses`` list returned by + # ``_request()`` plus whatever extra context is needed to build the result + # or raise an informative error, and returns/raises exactly what the public + # method should return/raise. + # --------------------------------------------------------------------------- + + @staticmethod + def _parse_get_calendars(responses: list, client, is_async: bool) -> list[JMAPCalendar]: + for method_name, resp_args, _ in responses: + if method_name == "Calendar/get": + calendars = parse_calendar_get(resp_args) + for cal in calendars: + cal._client = client + cal._is_async = is_async + return calendars + return [] + + @staticmethod + def _parse_create_event_response(responses: list, api_url: str) -> str: + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/set": + created, _, _, not_created, _, _ = parse_event_set(resp_args) + if "new-0" in not_created: + _JMAPClientBase._raise_set_error(api_url, not_created["new-0"]) + if "new-0" not in created: + raise JMAPMethodError( + url=api_url, + reason="CalendarEvent/set response missing created entry for new-0", + ) + return created["new-0"]["id"] + raise JMAPMethodError(url=api_url, reason="No CalendarEvent/set response") + + @staticmethod + def _parse_get_event_response( + responses: list, api_url: str, event_id: str + ) -> JMAPCalendarObject: + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/get": + items = parse_event_get(resp_args) + if not items: + raise JMAPMethodError( + url=api_url, + reason=f"Event not found: {event_id}", + error_type="notFound", + ) + return JMAPCalendarObject(data=items[0], parent=None) + raise JMAPMethodError(url=api_url, reason="No CalendarEvent/get response") + + @staticmethod + def _parse_update_event_response(responses: list, api_url: str, event_id: str) -> None: + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/set": + _, _, _, _, not_updated, _ = parse_event_set(resp_args) + if event_id in not_updated: + _JMAPClientBase._raise_set_error(api_url, not_updated[event_id]) + return + raise JMAPMethodError(url=api_url, reason="No CalendarEvent/set response") + + @staticmethod + def _parse_search_response( + responses: list, parent: JMAPCalendar | None + ) -> list[JMAPCalendarObject]: + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/get": + return [ + JMAPCalendarObject(data=item, parent=parent) + for item in parse_event_get(resp_args) + ] + return [] + + @staticmethod + def _parse_get_sync_token_response(responses: list, api_url: str) -> str: + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/get": + return resp_args.get("state", "") + raise JMAPMethodError(url=api_url, reason="No CalendarEvent/get response") + + @staticmethod + def _parse_event_changes_response( + responses: list, api_url: str + ) -> tuple[list[str], list[str], list[str], str]: + """Parse a CalendarEvent/changes response. + + Returns ``(created_ids, updated_ids, destroyed_ids, new_sync_token)``. + Raises :class:`JMAPMethodError` when the server truncated the result. + """ + created_ids: list[str] = [] + updated_ids: list[str] = [] + destroyed: list[str] = [] + new_sync_token: str = "" + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/changes": + _, new_sync_token, has_more, created_ids, updated_ids, destroyed = ( + parse_event_changes(resp_args) + ) + if has_more: + raise JMAPMethodError( + url=api_url, + reason=( + "CalendarEvent/changes response was truncated by the server " + "(hasMoreChanges=true). Call get_sync_token() to obtain a " + "fresh baseline and re-sync." + ), + error_type="serverPartialFail", + ) + return created_ids, updated_ids, destroyed, new_sync_token + + @staticmethod + def _assemble_sync_token_result( + get_responses: list, + created_ids: list[str], + updated_ids: list[str], + destroyed: list[str], + new_sync_token: str, + ) -> tuple[list[JMAPCalendarObject], list[JMAPCalendarObject], list[str], str]: + events_by_id: dict[str, JMAPCalendarObject] = {} + for method_name, resp_args, _ in get_responses: + if method_name == "CalendarEvent/get": + for item in parse_event_get(resp_args): + events_by_id[item["id"]] = JMAPCalendarObject(data=item, parent=None) + added = [events_by_id[i] for i in created_ids if i in events_by_id] + modified = [events_by_id[i] for i in updated_ids if i in events_by_id] + return added, modified, destroyed, new_sync_token + + @staticmethod + def _parse_delete_event_response(responses: list, api_url: str, event_id: str) -> None: + for method_name, resp_args, _ in responses: + if method_name == "CalendarEvent/set": + _, _, _, _, _, not_destroyed = parse_event_set(resp_args) + if event_id in not_destroyed: + _JMAPClientBase._raise_set_error(api_url, not_destroyed[event_id]) + return + raise JMAPMethodError(url=api_url, reason="No CalendarEvent/set response") + + @staticmethod + def _parse_get_task_lists_response(responses: list) -> list[dict]: + for method_name, resp_args, _ in responses: + if method_name == "TaskList/get": + return parse_task_list_get(resp_args) + return [] + + @staticmethod + def _parse_create_task_response(responses: list, api_url: str) -> str: + for method_name, resp_args, _ in responses: + if method_name == "Task/set": + created, _, _, not_created, _, _ = parse_task_set(resp_args) + if "new-0" in not_created: + _JMAPClientBase._raise_set_error(api_url, not_created["new-0"]) + if "new-0" not in created: + raise JMAPMethodError( + url=api_url, + reason="Task/set response missing created entry for new-0", + ) + return created["new-0"]["id"] + raise JMAPMethodError(url=api_url, reason="No Task/set response") + + @staticmethod + def _parse_get_task_response(responses: list, api_url: str, task_id: str) -> dict: + for method_name, resp_args, _ in responses: + if method_name == "Task/get": + items = resp_args.get("list", []) + if not items: + raise JMAPMethodError( + url=api_url, + reason=f"Task not found: {task_id}", + error_type="notFound", + ) + return items[0] + raise JMAPMethodError(url=api_url, reason="No Task/get response") + + @staticmethod + def _parse_update_task_response(responses: list, api_url: str, task_id: str) -> None: + for method_name, resp_args, _ in responses: + if method_name == "Task/set": + _, _, _, _, not_updated, _ = parse_task_set(resp_args) + if task_id in not_updated: + _JMAPClientBase._raise_set_error(api_url, not_updated[task_id]) + return + raise JMAPMethodError(url=api_url, reason="No Task/set response") + + @staticmethod + def _parse_delete_task_response(responses: list, api_url: str, task_id: str) -> None: + for method_name, resp_args, _ in responses: + if method_name == "Task/set": + _, _, _, _, _, not_destroyed = parse_task_set(resp_args) + if task_id in not_destroyed: + _JMAPClientBase._raise_set_error(api_url, not_destroyed[task_id]) + return + raise JMAPMethodError(url=api_url, reason="No Task/set response") + class JMAPClient(_JMAPClientBase): """Synchronous JMAP client for calendar operations. @@ -179,11 +432,23 @@ class JMAPClient(_JMAPClientBase): timeout: HTTP request timeout in seconds. """ + def _get_http_session(self): + """Return the persistent HTTP session, creating it on first call.""" + if self._http_session is None: + sess = requests.Session() + sess.auth = self._auth + sess.headers.update({"Content-Type": "application/json", "Accept": "application/json"}) + self._http_session = sess + return self._http_session + def __enter__(self) -> JMAPClient: + self._get_http_session() return self def __exit__(self, exc_type, exc_val, exc_tb) -> None: - return None + if self._http_session is not None: + self._http_session.close() + self._http_session = None def _get_session(self) -> Session: """Return the cached Session, fetching it on first call.""" @@ -217,11 +482,9 @@ def _request(self, method_calls: list[tuple], using: list[str] | None = None) -> log.debug("JMAP POST to %s: %d method call(s)", session.api_url, len(method_calls)) - response = requests.post( + response = self._get_http_session().post( session.api_url, json=payload, - auth=self._auth, - headers={"Content-Type": "application/json", "Accept": "application/json"}, timeout=self.timeout, ) @@ -255,18 +518,8 @@ def get_calendars(self) -> list[JMAPCalendar]: List of :class:`~caldav.jmap.objects.calendar.JMAPCalendar` objects. """ session = self._get_session() - call = build_calendar_get(session.account_id) - responses = self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "Calendar/get": - calendars = parse_calendar_get(resp_args) - for cal in calendars: - cal._client = self - cal._is_async = False - return calendars - - return [] + responses = self._request([build_calendar_get(session.account_id)]) + return self._parse_get_calendars(responses, self, False) def create_event(self, calendar_id: str, ical_str: str) -> str: """Create a calendar event from an iCalendar string. @@ -285,20 +538,7 @@ def create_event(self, calendar_id: str, ical_str: str) -> str: jscal = ical_to_jscal(ical_str, calendar_id=calendar_id) call = build_event_set_create(session.account_id, {"new-0": jscal}) responses = self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/set": - created, _, _, not_created, _, _ = parse_event_set(resp_args) - if "new-0" in not_created: - self._raise_set_error(session, not_created["new-0"]) - if "new-0" not in created: - raise JMAPMethodError( - url=session.api_url, - reason="CalendarEvent/set response missing created entry for new-0", - ) - return created["new-0"]["id"] - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/set response") + return self._parse_create_event_response(responses, session.api_url) def get_event(self, event_id: str) -> JMAPCalendarObject: """Fetch a calendar event by JMAP event ID. @@ -316,21 +556,8 @@ def get_event(self, event_id: str) -> JMAPCalendarObject: JMAPMethodError: If the event is not found. """ session = self._get_session() - call = build_event_get(session.account_id, ids=[event_id]) - responses = self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/get": - items = parse_event_get(resp_args) - if not items: - raise JMAPMethodError( - url=session.api_url, - reason=f"Event not found: {event_id}", - error_type="notFound", - ) - return JMAPCalendarObject(data=items[0], parent=None) - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/get response") + responses = self._request([build_event_get(session.account_id, ids=[event_id])]) + return self._parse_get_event_response(responses, session.api_url, event_id) def update_event(self, event_id: str, ical_str: str) -> None: """Update a calendar event from an iCalendar string. @@ -343,19 +570,17 @@ def update_event(self, event_id: str, ical_str: str) -> None: JMAPMethodError: If the server rejects the update. """ session = self._get_session() - patch = ical_to_jscal(ical_str) - patch.pop("uid", None) # uid is server-immutable after creation; patch must omit it - call = build_event_set_update(session.account_id, {event_id: patch}) - responses = self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/set": - _, _, _, _, not_updated, _ = parse_event_set(resp_args) - if event_id in not_updated: - self._raise_set_error(session, not_updated[event_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/set response") + patch, nulled = self._build_event_update_patch(ical_str) + while True: + responses = self._request( + [build_event_set_update(session.account_id, {event_id: patch})] + ) + drop = self._unsupported_null_keys(responses, event_id, patch, nulled) + if not drop: + break + for key in drop: + patch.pop(key, None) + self._parse_update_event_response(responses, session.api_url, event_id) def _search( self, @@ -368,15 +593,7 @@ def _search( session = self._get_session() calls = self._build_event_search_calls(session.account_id, calendar_id, start, end, text) responses = self._request(calls) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/get": - return [ - JMAPCalendarObject(data=item, parent=parent) - for item in parse_event_get(resp_args) - ] - - return [] + return self._parse_search_response(responses, parent) def search_events( self, @@ -417,16 +634,12 @@ def get_sync_token(self) -> str: retrieve only what changed since this point. """ session = self._get_session() - call = build_event_get(session.account_id, ids=[]) - responses = self._request([call]) - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/get": - return resp_args.get("state", "") - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/get response") + responses = self._request([build_event_get(session.account_id, ids=[])]) + return self._parse_get_sync_token_response(responses, session.api_url) def get_objects_by_sync_token( self, sync_token: str - ) -> tuple[list[JMAPCalendarObject], list[JMAPCalendarObject], list[str]]: + ) -> tuple[list[JMAPCalendarObject], list[JMAPCalendarObject], list[str], str]: """Fetch events changed since a previous sync token. Calls ``CalendarEvent/changes`` to discover which events were created, @@ -440,53 +653,28 @@ def get_objects_by_sync_token( or by a prior call to this method. Returns: - A 3-tuple ``(added, modified, deleted)``: + A 4-tuple ``(added, modified, deleted, new_sync_token)``: - ``added``: objects for newly created events (``parent`` is ``None``). - ``modified``: objects for updated events (``parent`` is ``None``). - ``deleted``: Event IDs that were destroyed. + - ``new_sync_token``: Pass to the next call to this method as ``sync_token``. Raises: JMAPMethodError: If the server reports ``hasMoreChanges: true``. """ session = self._get_session() - changes_call = build_event_changes(session.account_id, sync_token) - responses = self._request([changes_call]) - - created_ids: list[str] = [] - updated_ids: list[str] = [] - destroyed: list[str] = [] - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/changes": - _, _, has_more, created_ids, updated_ids, destroyed = parse_event_changes(resp_args) - if has_more: - raise JMAPMethodError( - url=session.api_url, - reason=( - "CalendarEvent/changes response was truncated by the server " - "(hasMoreChanges=true). Call get_sync_token() to obtain a " - "fresh baseline and re-sync." - ), - error_type="serverPartialFail", - ) - + responses = self._request([build_event_changes(session.account_id, sync_token)]) + created_ids, updated_ids, destroyed, new_sync_token = self._parse_event_changes_response( + responses, session.api_url + ) fetch_ids = created_ids + updated_ids if not fetch_ids: - return [], [], destroyed - - get_call = build_event_get(session.account_id, ids=fetch_ids) - get_responses = self._request([get_call]) - - events_by_id: dict[str, JMAPCalendarObject] = {} - for method_name, resp_args, _ in get_responses: - if method_name == "CalendarEvent/get": - for item in parse_event_get(resp_args): - events_by_id[item["id"]] = JMAPCalendarObject(data=item, parent=None) - - added = [events_by_id[i] for i in created_ids if i in events_by_id] - modified = [events_by_id[i] for i in updated_ids if i in events_by_id] - return added, modified, destroyed + return [], [], destroyed, new_sync_token + get_responses = self._request([build_event_get(session.account_id, ids=fetch_ids)]) + return self._assemble_sync_token_result( + get_responses, created_ids, updated_ids, destroyed, new_sync_token + ) def delete_event(self, event_id: str) -> None: """Delete a calendar event. @@ -498,17 +686,8 @@ def delete_event(self, event_id: str) -> None: JMAPMethodError: If the server rejects the delete. """ session = self._get_session() - call = build_event_set_destroy(session.account_id, [event_id]) - responses = self._request([call]) - - for method_name, resp_args, _ in responses: - if method_name == "CalendarEvent/set": - _, _, _, _, _, not_destroyed = parse_event_set(resp_args) - if event_id in not_destroyed: - self._raise_set_error(session, not_destroyed[event_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No CalendarEvent/set response") + responses = self._request([build_event_set_destroy(session.account_id, [event_id])]) + self._parse_delete_event_response(responses, session.api_url, event_id) def _get_object_by_uid( self, uid: str, calendar_id: str | None = None, parent: JMAPCalendar | None = None @@ -529,14 +708,8 @@ def get_task_lists(self) -> list[dict]: List of raw JMAP TaskList dicts as returned by the server. """ session = self._get_session() - call = build_task_list_get(session.account_id) - responses = self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "TaskList/get": - return parse_task_list_get(resp_args) - - return [] + responses = self._request([build_task_list_get(session.account_id)], using=_TASK_USING) + return self._parse_get_task_lists_response(responses) def create_task(self, task_list_id: str, title: str, **kwargs) -> str: """Create a task in a task list. @@ -567,15 +740,7 @@ def create_task(self, task_list_id: str, title: str, **kwargs) -> str: task_dict.update(kwargs) call = build_task_set_create(session.account_id, {"new-0": task_dict}) responses = self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/set": - created, _, _, not_created, _, _ = parse_task_set(resp_args) - if "new-0" in not_created: - self._raise_set_error(session, not_created["new-0"]) - return created["new-0"]["id"] - - raise JMAPMethodError(url=session.api_url, reason="No Task/set response") + return self._parse_create_task_response(responses, session.api_url) def get_task(self, task_id: str) -> dict: """Fetch a task by ID. @@ -590,21 +755,10 @@ def get_task(self, task_id: str) -> dict: JMAPMethodError: If the task is not found. """ session = self._get_session() - call = build_task_get(session.account_id, ids=[task_id]) - responses = self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/get": - items = resp_args.get("list", []) - if not items: - raise JMAPMethodError( - url=session.api_url, - reason=f"Task not found: {task_id}", - error_type="notFound", - ) - return items[0] - - raise JMAPMethodError(url=session.api_url, reason="No Task/get response") + responses = self._request( + [build_task_get(session.account_id, ids=[task_id])], using=_TASK_USING + ) + return self._parse_get_task_response(responses, session.api_url, task_id) def update_task(self, task_id: str, patch: dict) -> None: """Update a task with a partial patch. @@ -619,15 +773,7 @@ def update_task(self, task_id: str, patch: dict) -> None: session = self._get_session() call = build_task_set_update(session.account_id, {task_id: patch}) responses = self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/set": - _, _, _, _, not_updated, _ = parse_task_set(resp_args) - if task_id in not_updated: - self._raise_set_error(session, not_updated[task_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No Task/set response") + self._parse_update_task_response(responses, session.api_url, task_id) def delete_task(self, task_id: str) -> None: """Delete a task. @@ -639,14 +785,7 @@ def delete_task(self, task_id: str) -> None: JMAPMethodError: If the server rejects the delete. """ session = self._get_session() - call = build_task_set_destroy(session.account_id, [task_id]) - responses = self._request([call], using=_TASK_USING) - - for method_name, resp_args, _ in responses: - if method_name == "Task/set": - _, _, _, _, _, not_destroyed = parse_task_set(resp_args) - if task_id in not_destroyed: - self._raise_set_error(session, not_destroyed[task_id]) - return - - raise JMAPMethodError(url=session.api_url, reason="No Task/set response") + responses = self._request( + [build_task_set_destroy(session.account_id, [task_id])], using=_TASK_USING + ) + self._parse_delete_task_response(responses, session.api_url, task_id) diff --git a/caldav/jmap/convert/_patch.py b/caldav/jmap/convert/_patch.py new file mode 100644 index 00000000..db31a2ab --- /dev/null +++ b/caldav/jmap/convert/_patch.py @@ -0,0 +1,33 @@ +""" +RFC 8620 PatchObject helpers for CalendarEvent/set update calls. + +When updating an event, absent keys preserve the server's current value. +To delete an optional property the patch must set it to null explicitly. +""" + +from __future__ import annotations + +# Optional JSCalendar top-level properties that must be explicitly nulled in +# a CalendarEvent/set update when they are absent from the converted result. +# This ensures properties removed client-side (e.g. LOCATION deleted from +# the iCalendar) are actually removed on the server, not silently preserved. +_NULL_FOR_UPDATE: frozenset[str] = frozenset( + { + "description", + "color", + "locations", + "keywords", + "priority", + "privacy", + "freeBusyStatus", + "status", + "sequence", + "showWithoutTime", + "timeZone", + "recurrenceRules", + "excludedRecurrenceRules", + "recurrenceOverrides", + "participants", + "alerts", + } +) diff --git a/caldav/jmap/convert/_utils.py b/caldav/jmap/convert/_utils.py index 12b12263..475eca85 100644 --- a/caldav/jmap/convert/_utils.py +++ b/caldav/jmap/convert/_utils.py @@ -111,11 +111,12 @@ def _duration_to_timedelta(duration_str: str) -> timedelta: def _format_local_dt(dt: datetime | date) -> str: - """Format a datetime or date as a JSCalendar LocalDateTime or UTCDateTime string. + """Format a datetime or date as a JSCalendar LocalDateTime string. - JSCalendar uses: - - LocalDateTime: "2024-03-15T09:00:00" (no TZ suffix) - - UTCDateTime: "2024-03-15T09:00:00Z" (uppercase Z) + RFC 8984 requires LocalDateTime (no Z suffix) for override keys and RRULE + ``until`` values. Timezone information is stripped — callers must convert + UTC datetimes to the event's local timezone before calling if the event uses + TZID; for floating or all-day events the naive value is already correct. For date objects (all-day), uses T00:00:00 suffix. @@ -123,10 +124,8 @@ def _format_local_dt(dt: datetime | date) -> str: dt: A datetime (with or without tzinfo) or a date. Returns: - Formatted string suitable for use as a JSCalendar override key or datetime value. + Formatted string suitable for use as a JSCalendar override key or RRULE until. """ if isinstance(dt, datetime): - if dt.tzinfo is not None and dt.utcoffset() == timedelta(0): - return dt.strftime("%Y-%m-%dT%H:%M:%SZ") return dt.strftime("%Y-%m-%dT%H:%M:%S") return f"{dt.isoformat()}T00:00:00" diff --git a/caldav/jmap/convert/ical_to_jscal.py b/caldav/jmap/convert/ical_to_jscal.py index 00e9ce59..dbafd157 100644 --- a/caldav/jmap/convert/ical_to_jscal.py +++ b/caldav/jmap/convert/ical_to_jscal.py @@ -386,6 +386,17 @@ def ical_to_jscal(ical_str: str, calendar_id: str | None = None) -> dict: if location: jscal["locations"] = _location_str_to_jscal(str(location)) + status = master.get("STATUS") + if status: + _STATUS_ICAL_TO_JSCAL = { + "CONFIRMED": "confirmed", + "TENTATIVE": "tentative", + "CANCELLED": "cancelled", + } + jscal_status = _STATUS_ICAL_TO_JSCAL.get(str(status).upper()) + if jscal_status: + jscal["status"] = jscal_status + participants: dict = {} organizer = master.get("ORGANIZER") if organizer is not None: diff --git a/caldav/jmap/convert/jscal_to_ical.py b/caldav/jmap/convert/jscal_to_ical.py index 2a449d1b..4ed4c03d 100644 --- a/caldav/jmap/convert/jscal_to_ical.py +++ b/caldav/jmap/convert/jscal_to_ical.py @@ -353,6 +353,17 @@ def jscal_to_ical(jscal: dict) -> str: if loc_name: event.add("location", loc_name) + status = jscal.get("status") + if status: + _STATUS_JSCAL_TO_ICAL = { + "confirmed": "CONFIRMED", + "tentative": "TENTATIVE", + "cancelled": "CANCELLED", + } + ical_status = _STATUS_JSCAL_TO_ICAL.get(status) + if ical_status: + event.add("status", ical_status) + for rule in jscal.get("recurrenceRules") or []: ical_rule = _jscal_rrule_to_rrule(rule) if ical_rule: @@ -371,6 +382,15 @@ def jscal_to_ical(jscal: dict) -> str: rid_dt: datetime | date = datetime.strptime(override_key, "%Y-%m-%dT%H:%M:%SZ").replace( tzinfo=timezone.utc ) + elif show_without_time: + rid_dt = date.fromisoformat(override_key[:10]) + elif time_zone: + try: + rid_dt = datetime.strptime(override_key[:19], "%Y-%m-%dT%H:%M:%S").replace( + tzinfo=ZoneInfo(time_zone) + ) + except ZoneInfoNotFoundError: + rid_dt = datetime.strptime(override_key[:19], "%Y-%m-%dT%H:%M:%S") else: rid_dt = datetime.strptime(override_key[:19], "%Y-%m-%dT%H:%M:%S") @@ -381,7 +401,8 @@ def jscal_to_ical(jscal: dict) -> str: child.add("uid", uid) child.add("dtstamp", datetime.now(tz=timezone.utc)) child.add("recurrence-id", rid_dt) - child_start = patch.get("start", start_str) + # Default child start to the occurrence time (override key), not the master start. + child_start = patch.get("start", override_key) child_tz = patch.get("timeZone", time_zone) child_swt = patch.get("showWithoutTime", show_without_time) if child_start: diff --git a/caldav/jmap/objects/calendar.py b/caldav/jmap/objects/calendar.py index 28812ac9..6be47bd7 100644 --- a/caldav/jmap/objects/calendar.py +++ b/caldav/jmap/objects/calendar.py @@ -8,7 +8,7 @@ from __future__ import annotations from dataclasses import dataclass, field -from datetime import datetime +from datetime import datetime, timezone from typing import TYPE_CHECKING from caldav.jmap.objects.calendar_object import JMAPCalendarObject @@ -18,6 +18,17 @@ from caldav.jmap.client import JMAPClient +def _to_utcdate(dt: datetime) -> str: + """Convert a datetime to JMAP UTCDate format (YYYY-MM-DDTHH:MM:SSZ). + + Naive datetimes are assumed to be UTC. Aware datetimes are converted to + UTC before formatting. Microseconds are dropped as JMAP does not allow them. + """ + if dt.tzinfo is None: + dt = dt.replace(tzinfo=timezone.utc) + return dt.astimezone(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") + + @dataclass class JMAPCalendar: """A JMAP Calendar object. @@ -111,9 +122,9 @@ def search(self, **searchargs): start = searchargs.get("start") end = searchargs.get("end") if isinstance(start, datetime): - start = start.isoformat() + start = _to_utcdate(start) if isinstance(end, datetime): - end = end.isoformat() + end = _to_utcdate(end) return self._client._search( calendar_id=self.id, start=start, @@ -126,9 +137,9 @@ async def _async_search(self, **searchargs) -> list[JMAPCalendarObject]: start = searchargs.get("start") end = searchargs.get("end") if isinstance(start, datetime): - start = start.isoformat() + start = _to_utcdate(start) if isinstance(end, datetime): - end = end.isoformat() + end = _to_utcdate(end) return await self._client._search( calendar_id=self.id, start=start, diff --git a/docs/design/FULL_CODE_REVIEW_2026-06.md b/docs/design/FULL_CODE_REVIEW_2026-06.md index 988887c3..fa412342 100644 --- a/docs/design/FULL_CODE_REVIEW_2026-06.md +++ b/docs/design/FULL_CODE_REVIEW_2026-06.md @@ -138,7 +138,7 @@ Truncated/garbage iCalendar without DTSTAMP and without an `END:` line makes the DTSTAMP fixup logic it guards) is silently skipped. Should be `error.assert_` or a proper parse error. -### 1.13 `jmap/client.py:576` / `jmap/async_client.py:461` — `create_task` missing the guard `create_event` has `[code]` +### 1.13 `jmap/client.py:576` / `jmap/async_client.py:461` — `create_task` missing the guard `create_event` has `[code]` ✅ FIXED `create_event` handles an empty `created` dict with a descriptive `JMAPMethodError` (`client.py:294–298`); `create_task` does `created["new-0"]["id"]` unguarded → bare KeyError, bypassing the JMAP error @@ -308,7 +308,7 @@ explicitly). bodies (calendar PII, custom auth headers) to files that accumulate indefinitely. Files are 0600, and the niquests-applied Authorization header is added after the dump point, so exposure is limited — but a cleanup policy -or a documented warning would be appropriate. +or a documented warning would be appropriate. **Human notes:** This is in /tmp, so we should expect some kind of cleanup on the OS level. There does exist some security notes in the CHANGELOG for the revision adding the feature, but the CHANGELOG has been pruned, so it's needed to consult git history to find it - it should definitively be lifted up to a more visible place. **Ruled out** (checked, found safe): SSRF via server-returned hrefs (`_normalize_href` reduces absolute URLs to path-only); credential leak on @@ -319,32 +319,32 @@ cross-host redirects (auth applied via auth callable, stripped by ## 4. JMAP backend -### 4.1 `jmap/convert/jscal_to_ical.py:384` — override child VEVENT gets the master's DTSTART `[repro]` +### 4.1 `jmap/convert/jscal_to_ical.py:384` — override child VEVENT gets the master's DTSTART `[repro]` ✅ FIXED `child_start = patch.get("start", start_str)` defaults to the master start. An override that doesn't move the occurrence (e.g. title-only change — the common case) renders a child VEVENT with RECURRENCE-ID at the occurrence but DTSTART at the *master's* start, relocating the occurrence. Default must be the override key (`rid_dt`). -### 4.2 `jmap/convert/jscal_to_ical.py:375` — EXDATE/RECURRENCE-ID value-type mismatch `[repro]` +### 4.2 `jmap/convert/jscal_to_ical.py:375` — EXDATE/RECURRENCE-ID value-type mismatch `[repro]` ✅ FIXED Override keys are rendered as naive floating DATE-TIMEs regardless of the event's `timeZone`/`showWithoutTime`: a TZID-anchored event gets `EXDATE:20260620T100000` (floating — per RFC 5545 it does not match the instance, so the **excluded occurrence reappears**), and an all-day event gets a DATETIME EXDATE against a `VALUE=DATE` DTSTART. -### 4.3 `jmap/convert/ical_to_jscal.py:100` (via `_utils.py:129`) — `Z`-suffix in LocalDateTime slots `[repro]` +### 4.3 `jmap/convert/ical_to_jscal.py:100` (via `_utils.py:129`) — `Z`-suffix in LocalDateTime slots `[repro]` ✅ FIXED UTC inputs produce `...Z` strings for RRULE `until` and recurrenceOverrides keys; RFC 8984 requires LocalDateTime there. Strict servers reject with `invalidArguments`; lenient ones mis-set the boundary, and a `Z`-suffixed override key can never match a LocalDateTime occurrence key. -### 4.4 `jmap/convert/*` — STATUS dropped in both directions `[code]` +### 4.4 `jmap/convert/*` — STATUS dropped in both directions `[code]` ✅ FIXED Neither converter maps `STATUS` ↔ `status` (only participationStatus/freeBusyStatus exist). `STATUS:CANCELLED` round-trips to the JSCalendar default `confirmed`; cancelled meetings come back as active. -### 4.5 `jmap/client.py:346` / `async_client.py:233` — `update_event` patch never clears removed properties `[code]` +### 4.5 `jmap/client.py:346` / `async_client.py:233` — `update_event` patch never clears removed properties `[code]` ✅ FIXED The full converted object is sent as the RFC 8620 PatchObject; the converter only includes keys conditionally, so a property deleted client-side (e.g. LOCATION, VALARM) is simply *absent* from the patch and **persists on the @@ -373,7 +373,7 @@ producing drift bugs. modulo `await`. The build-side is already shared via `_JMAPClientBase` / `jmap/_methods`; moving the response-parsing glue into shared pure methods would shrink each sync/async method to ~3 lines. (The §1.13 and - §4.5 bugs are duplicated exactly because of this.) + §4.5 bugs are duplicated exactly because of this.) ✅ FIXED (commit ed3c47b0) 2. **`calendarobjectresource.py:1166–1205` — `_post_put` block pasted twice in sequence**; the second `elif r.status not in (204, 201)` is unreachable. Also factor the Etag/Schedule-Tag header→props snippet @@ -390,7 +390,7 @@ producing drift bugs. 5. **JMAP clients open a fresh HTTP connection per request** (async: `async with AsyncSession()` per `_request`; sync: module-level `requests.post`). `__exit__`/`__aexit__` already exist but do nothing — - hold one session in `_JMAPClientBase` and close it there. + hold one session in `_JMAPClientBase` and close it there. ✅ FIXED 6. **`Todo._async_complete_recurring_thisandfuture` copies ~60 lines of its sync twin** (the file says "TERRIBLY much code duplication here"), and the async safe-variant has drifted: it PUTs the completed copy twice. @@ -403,7 +403,7 @@ producing drift bugs. purelymail 404) must be maintained twice; the TODO at line 577 already acknowledges this. 8. **`search.py` sync/async driver loops duplicated** (~80 lines including - the Phase-1/Phase-2 exception-rethrow protocol and + the hase-1/Phase-2 exception-rethrow protocol and `_search_with_comptypes`). A small executor object with sync/async implementations would leave one driver. @@ -424,6 +424,8 @@ producing drift bugs. regression-testing each fixup against the exact server output it was written for. + **Human comment:** we've been discussing a bit moving this logic into the icalendar library - but it's hard to make good solutions, so we'll need to keep the stop-gap implementation as for now. + --- ## 7. Recommended priorities diff --git a/tests/test_jmap_integration.py b/tests/test_jmap_integration.py index 02f04fd3..255d249e 100644 --- a/tests/test_jmap_integration.py +++ b/tests/test_jmap_integration.py @@ -220,7 +220,7 @@ def test_event_sync(self, client, calendar_id): token_before = client.get_sync_token() event_id = client.create_event(calendar_id, _minimal_ical("Sync Test Event")) try: - added, _modified, _deleted = client.get_objects_by_sync_token(token_before) + added, _modified, _deleted, _new_token = client.get_objects_by_sync_token(token_before) assert any("Sync Test Event" in jscal_to_ical(a.get_data()) for a in added) finally: client.delete_event(event_id) @@ -280,7 +280,9 @@ async def test_event_sync(self, async_client, async_calendar_id): async_calendar_id, _minimal_ical("Async Sync Test Event") ) try: - added, _modified, _deleted = await async_client.get_objects_by_sync_token(token_before) + added, _modified, _deleted, _new_token = await async_client.get_objects_by_sync_token( + token_before + ) assert any("Async Sync Test Event" in jscal_to_ical(a.get_data()) for a in added) finally: await async_client.delete_event(event_id) diff --git a/tests/test_jmap_unit.py b/tests/test_jmap_unit.py index 4b7a6dc6..3d12cdd8 100644 --- a/tests/test_jmap_unit.py +++ b/tests/test_jmap_unit.py @@ -207,6 +207,8 @@ def test_picks_first_calendar_capable_account(self): assert session.account_id == "user_calendar" +from datetime import datetime, timezone + from caldav.jmap.objects.calendar import JMAPCalendar from caldav.jmap.objects.calendar_object import JMAPCalendarObject @@ -341,7 +343,9 @@ def capturing_post(*args, **kwargs): mock_resp.raise_for_status = MagicMock() return mock_resp - monkeypatch.setattr("caldav.jmap.client.requests.post", capturing_post) + mock_http = MagicMock() + mock_http.post.side_effect = capturing_post + client._http_session = mock_http cal = JMAPCalendar(id=calendar_id, name="Test") cal._client = client cal._is_async = False @@ -372,6 +376,26 @@ def test_calendar_search_with_date_range(self, monkeypatch): assert query_args["filter"]["after"] == "2026-01-01T00:00:00" assert query_args["filter"]["before"] == "2026-12-31T23:59:59" + def test_calendar_search_datetime_converted_to_utcdate(self, monkeypatch): + """§4.6: datetime.isoformat() produced wrong format for JMAP UTCDate. + Naive datetimes produce no Z, aware non-UTC produce +HH:MM offset; + JMAP requires ...Z (UTC, no microseconds).""" + import datetime as _dt + + resp = self._query_get_response([self._RAW_EVENT]) + cal, captured = self._capturing_calendar(monkeypatch, resp) + tz_plus2 = _dt.timezone(_dt.timedelta(hours=2)) + start_aware = datetime(2026, 6, 1, 12, 0, 0, tzinfo=tz_plus2) # +02:00 noon → UTC 10:00 + end_utc = datetime(2026, 12, 31, 23, 59, 59, tzinfo=timezone.utc) + cal.search(start=start_aware, end=end_utc) + query_args = captured["json"]["methodCalls"][0][1] + assert query_args["filter"]["after"] == "2026-06-01T10:00:00Z", ( + f"Expected UTC Z-format, got {query_args['filter']['after']!r}" + ) + assert query_args["filter"]["before"] == "2026-12-31T23:59:59Z", ( + f"Expected UTC Z-format, got {query_args['filter']['before']!r}" + ) + def test_calendar_search_ignores_unknown_params(self, monkeypatch): """Verify that unknown search parameters are silently ignored.""" resp = self._query_get_response([self._RAW_EVENT]) @@ -567,7 +591,9 @@ def _make_client_with_mocked_session(monkeypatch, api_response_json): mock_resp.status_code = 200 mock_resp.json.return_value = api_response_json mock_resp.raise_for_status = MagicMock() - monkeypatch.setattr("caldav.jmap.client.requests.post", lambda *a, **kw: mock_resp) + mock_http = MagicMock() + mock_http.post.return_value = mock_resp + client._http_session = mock_http return client @@ -585,6 +611,33 @@ def test_context_manager(self): with JMAPClient(url="http://x", username="u", password="p") as client: assert isinstance(client, JMAPClient) + def test_context_manager_closes_http_session(self): + mock_close = MagicMock() + mock_http = MagicMock() + mock_http.close = mock_close + with patch("caldav.jmap.client.requests.Session", return_value=mock_http): + client = JMAPClient(url="http://x", username="u", password="p") + with client: + assert client._http_session is mock_http + mock_close.assert_called_once() + assert client._http_session is None + + def test_http_session_reused_across_requests(self, monkeypatch): + client = JMAPClient(url=_JMAP_URL, username=_USERNAME, password=_PASSWORD) + client._session_cache = Session(api_url=_API_URL, account_id=_USERNAME, state="s") + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.json.return_value = {"methodResponses": []} + mock_resp.raise_for_status = MagicMock() + with patch("caldav.jmap.client.requests.Session") as MockSession: + mock_sess = MagicMock() + mock_sess.post.return_value = mock_resp + MockSession.return_value = mock_sess + client._request([("Calendar/get", {}, "c0")]) + client._request([("Calendar/get", {}, "c1")]) + MockSession.assert_called_once() + assert mock_sess.post.call_count == 2 + def test_build_auth_basic_when_username_given(self): client = JMAPClient(url="http://x", username="u", password="p") assert isinstance(client._auth, HTTPBasicAuth) @@ -645,7 +698,9 @@ def test_request_raises_auth_error_on_401(self, monkeypatch): mock_resp = MagicMock() mock_resp.status_code = 401 mock_resp.raise_for_status = MagicMock() - monkeypatch.setattr("caldav.jmap.client.requests.post", lambda *a, **kw: mock_resp) + mock_http = MagicMock() + mock_http.post.return_value = mock_resp + client._http_session = mock_http with pytest.raises(JMAPAuthError): client._request([("Calendar/get", {"accountId": _USERNAME, "ids": None}, "c0")]) @@ -899,7 +954,7 @@ def test_parse_event_set_partial_failure(self): assert not_destroyed["ev-old"]["type"] == "notFound" -from datetime import date, datetime, timedelta, timezone +from datetime import date, timedelta import icalendar as _icalendar @@ -968,8 +1023,9 @@ def test_duration_round_trip(self): assert _duration_to_timedelta(_timedelta_to_duration(td)) == td def test_format_local_dt_utc(self): + # RFC 8984: LocalDateTime slots (override keys, RRULE until) must not carry Z suffix. dt = datetime(2024, 6, 15, 9, 0, 0, tzinfo=timezone.utc) - assert _format_local_dt(dt) == "2024-06-15T09:00:00Z" + assert _format_local_dt(dt) == "2024-06-15T09:00:00" def test_format_local_dt_naive(self): dt = datetime(2024, 6, 15, 9, 0, 0) @@ -1604,6 +1660,112 @@ def test_update_event_drops_uid_from_patch(self, monkeypatch): patch = update_args["update"]["ev1"] assert "uid" not in patch + def test_update_event_nulls_removed_optional_properties(self, monkeypatch): + # RFC 8620 §3.3: absent keys in a PatchObject preserve the server value. + # To actually delete a property the patch must set it to null. + # An ical → jscal conversion that omits LOCATION/DESCRIPTION must send + # {"locations": null, "description": null, ...} so the server removes them. + _ICAL_WITH_LOCATION = ( + "BEGIN:VCALENDAR\r\nVERSION:2.0\r\n" + "BEGIN:VEVENT\r\n" + "UID:loc-uid@example.com\r\n" + "DTSTART:20240615T090000Z\r\n" + "SUMMARY:Event with Location\r\n" + "LOCATION:Old Conference Room\r\n" + "END:VEVENT\r\nEND:VCALENDAR\r\n" + ) + _ICAL_WITHOUT_LOCATION = ( + "BEGIN:VCALENDAR\r\nVERSION:2.0\r\n" + "BEGIN:VEVENT\r\n" + "UID:loc-uid@example.com\r\n" + "DTSTART:20240615T090000Z\r\n" + "SUMMARY:Event without Location\r\n" + "END:VEVENT\r\nEND:VCALENDAR\r\n" + ) + resp = self._set_response(updated={"ev1": None}) + client, captured = self._capturing_client(monkeypatch, resp) + # First, pretend the event had a location (we don't need to call create; just update) + client.update_event("ev1", _ICAL_WITHOUT_LOCATION) + patch = captured["json"]["methodCalls"][0][1]["update"]["ev1"] + # The patch must contain explicit null for 'locations' to remove it from the server + assert "locations" in patch + assert patch["locations"] is None + + def _sequence_client(self, responses): + """Return (client, captured) replaying ``responses`` POST-by-POST. + + ``captured["patches"]`` collects the ``update`` patch dict sent on each + CalendarEvent/set POST, in order. + """ + captured: dict = {"patches": []} + client = JMAPClient(url=_JMAP_URL, username=_USERNAME, password=_PASSWORD) + client._session_cache = Session(api_url=_API_URL, account_id=_USERNAME, state="state-abc") + seq = iter(responses) + + def post(*args, **kwargs): + body = kwargs.get("json", {}) + update = body["methodCalls"][0][1].get("update") + if update: + captured["patches"].append(dict(update["ev1"])) # copy: caller mutates in place + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.json.return_value = next(seq) + mock_resp.raise_for_status = MagicMock() + return mock_resp + + mock_http = MagicMock() + mock_http.post.side_effect = post + client._http_session = mock_http + return client, captured + + def test_update_event_retries_dropping_server_rejected_null_keys(self, monkeypatch): + # A server (e.g. Stalwart) rejects null-clearing of recurrence properties + # it does not support, reporting one offending property per response. + # update_event must drop each reported null-cleanup key and retry until + # the update succeeds — never failing on harmless cleanup. + def reject(prop): + return self._set_response( + notUpdated={ + "ev1": { + "type": "invalidProperties", + "description": "Invalid property.", + "properties": [prop], + } + } + ) + + responses = [ + reject("recurrenceRules"), + reject("excludedRecurrenceRules"), + self._set_response(updated={"ev1": None}), + ] + client, captured = self._sequence_client(responses) + client.update_event("ev1", self._MINIMAL_ICAL) + + assert len(captured["patches"]) == 3 + # First attempt nulled both recurrence keys; the final accepted patch dropped them. + assert captured["patches"][0]["recurrenceRules"] is None + assert "recurrenceRules" not in captured["patches"][2] + assert "excludedRecurrenceRules" not in captured["patches"][2] + + def test_update_event_does_not_drop_explicitly_set_property(self, monkeypatch): + # If the rejected property was actually assigned a value by the client + # (not null-cleanup), the rejection is genuine and must surface — no retry. + resp = self._set_response( + notUpdated={ + "ev1": { + "type": "invalidProperties", + "description": "Invalid property.", + "properties": ["title"], + } + } + ) + client, captured = self._sequence_client([resp]) + with pytest.raises(JMAPMethodError) as exc_info: + client.update_event("ev1", self._MINIMAL_ICAL) + assert exc_info.value.error_type == "invalidProperties" + assert len(captured["patches"]) == 1 # no retry + def test_delete_event_success(self, monkeypatch): resp = self._set_response(destroyed=["ev1"]) client = _make_client_with_mocked_session(monkeypatch, resp) @@ -1630,7 +1792,9 @@ def capturing_post(*args, **kwargs): mock_resp.raise_for_status = MagicMock() return mock_resp - monkeypatch.setattr("caldav.jmap.client.requests.post", capturing_post) + mock_http = MagicMock() + mock_http.post.side_effect = capturing_post + client._http_session = mock_http return client, captured def _query_get_response(self, items): @@ -1753,14 +1917,22 @@ def _make_client(self): client._session_cache = Session(api_url=_API_URL, account_id=_USERNAME, state="state-abc") return client - def test_get_sync_token_returns_state(self, monkeypatch): + def _mock_http(self, client, response=None, side_effect=None): + mock_http = MagicMock() + if side_effect is not None: + mock_http.post.side_effect = side_effect + elif response is not None: + mock_http.post.return_value = response + client._http_session = mock_http + return mock_http + + def test_get_sync_token_returns_state(self): resp = self._get_resp_with_state([], state="tok-1") - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - assert self._make_client().get_sync_token() == "tok-1" + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + assert client.get_sync_token() == "tok-1" - def test_get_sync_token_sends_empty_ids(self, monkeypatch): + def test_get_sync_token_sends_empty_ids(self): captured = {} resp = self._get_resp_with_state([]) @@ -1768,61 +1940,74 @@ def capturing_post(*args, **kwargs): captured["json"] = kwargs.get("json", {}) return self._make_mock(resp) - monkeypatch.setattr("caldav.jmap.client.requests.post", capturing_post) - self._make_client().get_sync_token() + client = self._make_client() + self._mock_http(client, side_effect=capturing_post) + client.get_sync_token() assert captured["json"]["methodCalls"][0][1]["ids"] == [] - def test_get_objects_no_changes(self, monkeypatch): + def test_get_objects_no_changes(self): resp = self._changes_resp() - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - added, modified, deleted = self._make_client().get_objects_by_sync_token("state-1") + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + added, modified, deleted, _ = client.get_objects_by_sync_token("state-1") assert added == [] and modified == [] and deleted == [] - def test_get_objects_deleted_returns_ids(self, monkeypatch): + def test_get_objects_deleted_returns_ids(self): resp = self._changes_resp(destroyed=["ev1"]) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - added, modified, deleted = self._make_client().get_objects_by_sync_token("state-1") + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + added, modified, deleted, _ = client.get_objects_by_sync_token("state-1") assert deleted == ["ev1"] and added == [] and modified == [] - def test_get_objects_added_returns_ical(self, monkeypatch): + def test_get_objects_added_returns_ical(self): changes_resp = self._changes_resp(created=["ev1"]) get_resp = self._get_resp_with_state([self._RAW_EVENT]) - mock_post = MagicMock( - side_effect=[self._make_mock(changes_resp), self._make_mock(get_resp)] + client = self._make_client() + self._mock_http( + client, + side_effect=[self._make_mock(changes_resp), self._make_mock(get_resp)], ) - monkeypatch.setattr("caldav.jmap.client.requests.post", mock_post) - added, modified, deleted = self._make_client().get_objects_by_sync_token("state-1") + added, modified, deleted, _ = client.get_objects_by_sync_token("state-1") assert len(added) == 1 assert isinstance(added[0], JMAPCalendarObject) assert added[0].id == "ev1" assert modified == [] and deleted == [] - def test_get_objects_modified_returns_ical(self, monkeypatch): + def test_get_objects_modified_returns_ical(self): changes_resp = self._changes_resp(updated=["ev1"]) get_resp = self._get_resp_with_state([self._RAW_EVENT]) - mock_post = MagicMock( - side_effect=[self._make_mock(changes_resp), self._make_mock(get_resp)] + client = self._make_client() + self._mock_http( + client, + side_effect=[self._make_mock(changes_resp), self._make_mock(get_resp)], ) - monkeypatch.setattr("caldav.jmap.client.requests.post", mock_post) - added, modified, deleted = self._make_client().get_objects_by_sync_token("state-1") + added, modified, deleted, _ = client.get_objects_by_sync_token("state-1") assert len(modified) == 1 assert isinstance(modified[0], JMAPCalendarObject) assert modified[0].id == "ev1" assert added == [] and deleted == [] - def test_get_objects_has_more_raises(self, monkeypatch): + def test_get_objects_has_more_raises(self): resp = self._changes_resp(created=["ev1"], has_more=True) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) with pytest.raises(JMAPMethodError) as exc_info: - self._make_client().get_objects_by_sync_token("state-1") + client.get_objects_by_sync_token("state-1") assert exc_info.value.error_type == "serverPartialFail" + def test_get_objects_returns_new_sync_token(self): + """§4.7: newState from /changes was discarded into _. Callers had no + way to chain sync calls without a separate get_sync_token() round-trip, + creating a race window where intervening changes would be silently missed.""" + resp = self._changes_resp(new_state="state-99") + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + result = client.get_objects_by_sync_token("state-1") + assert len(result) == 4, "expected 4-tuple (added, modified, deleted, new_sync_token)" + added, modified, deleted, new_token = result + assert new_token == "state-99" + assert added == [] and modified == [] and deleted == [] + def test_parse_event_changes_all_fields(self): resp_args = { "oldState": "s1", @@ -1969,25 +2154,32 @@ def _make_client(self): client._session_cache = Session(api_url=_API_URL, account_id=_USERNAME, state="state-abc") return client - def test_get_task_lists_returns_list(self, monkeypatch): + def _mock_http(self, client, response=None, side_effect=None): + mock_http = MagicMock() + if side_effect is not None: + mock_http.post.side_effect = side_effect + elif response is not None: + mock_http.post.return_value = response + client._http_session = mock_http + return mock_http + + def test_get_task_lists_returns_list(self): resp = self._tasklist_response([self._MINIMAL_TASKLIST]) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - result = self._make_client().get_task_lists() + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + result = client.get_task_lists() assert len(result) == 1 assert isinstance(result[0], dict) assert result[0]["name"] == "My Tasks" - def test_create_task_returns_server_id(self, monkeypatch): + def test_create_task_returns_server_id(self): resp = self._set_response(created={"new-0": {"id": "sv-task-1"}}) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - task_id = self._make_client().create_task("tl1", "Buy groceries") + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + task_id = client.create_task("tl1", "Buy groceries") assert task_id == "sv-task-1" - def test_create_task_passes_task_list_id(self, monkeypatch): + def test_create_task_passes_task_list_id(self): captured = {} resp = self._set_response(created={"new-0": {"id": "sv-task-1"}}) @@ -1995,71 +2187,74 @@ def capturing_post(*args, **kwargs): captured["json"] = kwargs.get("json", {}) return self._make_mock(resp) - monkeypatch.setattr("caldav.jmap.client.requests.post", capturing_post) - self._make_client().create_task("my-list", "Test Task") + client = self._make_client() + self._mock_http(client, side_effect=capturing_post) + client.create_task("my-list", "Test Task") create_args = captured["json"]["methodCalls"][0][1] assert create_args["create"]["new-0"]["taskListId"] == "my-list" - def test_create_task_raises_on_failure(self, monkeypatch): + def test_create_task_raises_on_failure(self): resp = self._set_response(notCreated={"new-0": {"type": "invalidArguments"}}) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) with pytest.raises(JMAPMethodError) as exc_info: - self._make_client().create_task("tl1", "Test") + client.create_task("tl1", "Test") assert exc_info.value.error_type == "invalidArguments" - def test_get_task_returns_task_object(self, monkeypatch): + def test_create_task_raises_jmap_error_when_created_is_empty(self): + """§1.13: create_task must raise JMAPMethodError (not KeyError) when the server + returns a Task/set response with an empty 'created' dict and no 'notCreated' entry.""" + resp = self._set_response(created={}, notCreated={}) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + with pytest.raises(JMAPMethodError): + client.create_task("tl1", "Test") + + def test_get_task_returns_task_object(self): resp = self._get_response([self._MINIMAL_TASK]) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - task = self._make_client().get_task("task1") + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + task = client.get_task("task1") assert isinstance(task, dict) assert task["id"] == "task1" - def test_get_task_raises_on_not_found(self, monkeypatch): + def test_get_task_raises_on_not_found(self): resp = self._get_response([]) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) with pytest.raises(JMAPMethodError) as exc_info: - self._make_client().get_task("missing") + client.get_task("missing") assert exc_info.value.error_type == "notFound" - def test_update_task_success(self, monkeypatch): + def test_update_task_success(self): resp = self._set_response(updated={"task1": None}) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - self._make_client().update_task("task1", {"title": "Updated"}) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + client.update_task("task1", {"title": "Updated"}) - def test_update_task_raises_on_failure(self, monkeypatch): + def test_update_task_raises_on_failure(self): resp = self._set_response(notUpdated={"task1": {"type": "notFound"}}) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) with pytest.raises(JMAPMethodError) as exc_info: - self._make_client().update_task("task1", {"title": "X"}) + client.update_task("task1", {"title": "X"}) assert exc_info.value.error_type == "notFound" - def test_delete_task_success(self, monkeypatch): + def test_delete_task_success(self): resp = self._set_response(destroyed=["task1"]) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) - self._make_client().delete_task("task1") + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) + client.delete_task("task1") - def test_delete_task_raises_on_failure(self, monkeypatch): + def test_delete_task_raises_on_failure(self): resp = self._set_response(notDestroyed={"task1": {"type": "notFound"}}) - monkeypatch.setattr( - "caldav.jmap.client.requests.post", lambda *a, **kw: self._make_mock(resp) - ) + client = self._make_client() + self._mock_http(client, self._make_mock(resp)) with pytest.raises(JMAPMethodError) as exc_info: - self._make_client().delete_task("task1") + client.delete_task("task1") assert exc_info.value.error_type == "notFound" - def test_task_requests_use_task_capability(self, monkeypatch): + def test_task_requests_use_task_capability(self): captured = {} resp = self._tasklist_response([self._MINIMAL_TASKLIST]) @@ -2067,8 +2262,9 @@ def capturing_post(*args, **kwargs): captured["json"] = kwargs.get("json", {}) return self._make_mock(resp) - monkeypatch.setattr("caldav.jmap.client.requests.post", capturing_post) - self._make_client().get_task_lists() + client = self._make_client() + self._mock_http(client, side_effect=capturing_post) + client.get_task_lists() assert TASK_CAPABILITY in captured["json"]["using"] assert CALENDAR_CAPABILITY not in captured["json"]["using"] @@ -2223,6 +2419,33 @@ async def test_context_manager(self): async with AsyncJMAPClient(url=_JMAP_URL, username=_USERNAME, password=_PASSWORD) as client: assert isinstance(client, AsyncJMAPClient) + @pytest.mark.asyncio + async def test_context_manager_closes_http_session(self, monkeypatch): + mock_close = AsyncMock() + mock_http = MagicMock() + mock_http.close = mock_close + mock_http.headers = MagicMock() + monkeypatch.setattr("caldav.jmap.async_client.AsyncSession", lambda: mock_http) + client = AsyncJMAPClient(url=_JMAP_URL, username=_USERNAME, password=_PASSWORD) + async with client: + assert client._http_session is mock_http + mock_close.assert_called_once() + assert client._http_session is None + + @pytest.mark.asyncio + async def test_http_session_reused_across_requests(self, monkeypatch): + client = self._make_client() + mock_resp = self._make_mock_response({"methodResponses": []}) + mock_http = MagicMock() + mock_http.post = AsyncMock(return_value=mock_resp) + mock_http.headers = MagicMock() + with patch("caldav.jmap.async_client.AsyncSession") as MockAsyncSession: + MockAsyncSession.return_value = mock_http + await client._request([("Calendar/get", {}, "c0")]) + await client._request([("Calendar/get", {}, "c1")]) + MockAsyncSession.assert_called_once() + assert mock_http.post.call_count == 2 + @pytest.mark.asyncio async def test_get_calendars_returns_list(self, monkeypatch): cal = {"id": "cal1", "name": "Personal", "isSubscribed": True, "myRights": {}} @@ -2327,7 +2550,7 @@ async def test_get_objects_no_changes(self, monkeypatch): mock_http.__aexit__ = AsyncMock(return_value=None) mock_http.post = AsyncMock(return_value=self._make_mock_response(self._changes_resp())) monkeypatch.setattr("caldav.jmap.async_client.AsyncSession", lambda: mock_http) - added, modified, deleted = await self._make_client().get_objects_by_sync_token("state-1") + added, modified, deleted, _ = await self._make_client().get_objects_by_sync_token("state-1") assert added == [] and modified == [] and deleted == [] @pytest.mark.asyncio @@ -2339,7 +2562,7 @@ async def test_get_objects_deleted_returns_ids(self, monkeypatch): return_value=self._make_mock_response(self._changes_resp(destroyed=["ev1"])) ) monkeypatch.setattr("caldav.jmap.async_client.AsyncSession", lambda: mock_http) - added, modified, deleted = await self._make_client().get_objects_by_sync_token("state-1") + added, modified, deleted, _ = await self._make_client().get_objects_by_sync_token("state-1") assert deleted == ["ev1"] and added == [] and modified == [] @pytest.mark.asyncio @@ -2354,7 +2577,7 @@ async def test_get_objects_added_returns_ical(self, monkeypatch): ] ) monkeypatch.setattr("caldav.jmap.async_client.AsyncSession", lambda: mock_http) - added, modified, deleted = await self._make_client().get_objects_by_sync_token("state-1") + added, modified, deleted, _ = await self._make_client().get_objects_by_sync_token("state-1") assert len(added) == 1 assert isinstance(added[0], JMAPCalendarObject) assert added[0].id == "ev-async-1" @@ -2534,7 +2757,7 @@ async def test_get_objects_modified_returns_ical(self, monkeypatch): ] ) monkeypatch.setattr("caldav.jmap.async_client.AsyncSession", lambda: mock_http) - added, modified, deleted = await self._make_client().get_objects_by_sync_token("state-1") + added, modified, deleted, _ = await self._make_client().get_objects_by_sync_token("state-1") assert len(modified) == 1 assert isinstance(modified[0], JMAPCalendarObject) assert modified[0].id == "ev-async-1" @@ -2548,3 +2771,126 @@ async def test_create_task_passes_task_list_id(self, monkeypatch): create_args = captured["json"]["methodCalls"][0][1] new_task = create_args["create"]["new-0"] assert new_task["taskListId"] == "tl-target" + + +class TestOverrideWithoutStartUsesOccurrenceTime: + """§4.1: override child VEVENT must use occurrence time as DTSTART, not master start.""" + + def test_title_only_override_dtstart_equals_occurrence(self): + # Master: 2024-06-17T09:00:00Z (UTC), weekly recurrence. + # Override for 2024-06-24T09:00:00Z changes only title — no "start" in patch. + # Child DTSTART must be 20240624T090000Z, not 20240617T090000Z. + jscal = { + "uid": "override-dtstart@example.com", + "title": "Master Title", + "start": "2024-06-17T09:00:00Z", + "duration": "PT1H", + "recurrenceRules": [{"@type": "RecurrenceRule", "frequency": "weekly"}], + "recurrenceOverrides": { + "2024-06-24T09:00:00Z": {"title": "Changed Title"}, + }, + } + result = jscal_to_ical(jscal) + import icalendar as _ic + + cal = _ic.Calendar.from_ical(result) + events = [c for c in cal.subcomponents if isinstance(c, _ic.Event)] + assert len(events) == 2 + child = next(e for e in events if e.get("RECURRENCE-ID") is not None) + # DTSTART of the child must match its own occurrence, not the master start + child_dtstart = child["DTSTART"].dt + if hasattr(child_dtstart, "utctimetuple"): + import datetime as _dt + + assert child_dtstart == _dt.datetime(2024, 6, 24, 9, 0, 0, tzinfo=_dt.timezone.utc) + else: + assert str(child_dtstart) == "2024-06-24" + + +class TestExdateValueType: + """§4.2: EXDATE value type must match DTSTART (TZID or DATE, not floating).""" + + def test_exdate_for_tzid_event_has_tzid_param(self): + # A TZID-anchored event's excluded override must produce EXDATE with TZID, + # not a floating EXDATE (which per RFC 5545 won't match the instance). + jscal = _minimal_jscal( + start="2024-06-17T14:00:00", + timeZone="Europe/Berlin", + recurrenceRules=[{"@type": "RecurrenceRule", "frequency": "weekly"}], + recurrenceOverrides={"2024-06-24T14:00:00": {"excluded": True}}, + ) + result = jscal_to_ical(jscal) + # Must have TZID on EXDATE; a plain EXDATE:... without TZID is a floating datetime + assert "EXDATE;TZID=Europe/Berlin:" in result + + def test_exdate_for_allday_event_is_date_value(self): + jscal = { + "uid": "allday-exdate@example.com", + "title": "All Day Recurring", + "start": "2024-06-17T00:00:00", + "showWithoutTime": True, + "duration": "P1D", + "recurrenceRules": [{"@type": "RecurrenceRule", "frequency": "weekly"}], + "recurrenceOverrides": {"2024-06-24T00:00:00": {"excluded": True}}, + } + result = jscal_to_ical(jscal) + # All-day EXDATE must be a DATE value (8-digit YYYYMMDD, not YYYYMMDDTHHMMSS datetime). + # The icalendar library may or may not emit explicit VALUE=DATE — either form is acceptable. + assert "EXDATE" in result + assert "20240624" in result + assert "20240624T" not in result # must not be a datetime + + +class TestStatusMapping: + """§4.4: STATUS must be mapped in both ical→jscal and jscal→ical directions.""" + + def test_ical_status_cancelled_to_jscal(self): + ical = _make_ical( + "DTSTART:20240615T100000Z\r\nSUMMARY:Cancelled Meeting\r\nSTATUS:CANCELLED\r\n" + ) + result = ical_to_jscal(ical) + assert result.get("status") == "cancelled" + + def test_ical_status_tentative_to_jscal(self): + ical = _make_ical( + "DTSTART:20240615T100000Z\r\nSUMMARY:Tentative Meeting\r\nSTATUS:TENTATIVE\r\n" + ) + result = ical_to_jscal(ical) + assert result.get("status") == "tentative" + + def test_ical_status_confirmed_to_jscal(self): + ical = _make_ical( + "DTSTART:20240615T100000Z\r\nSUMMARY:Confirmed Meeting\r\nSTATUS:CONFIRMED\r\n" + ) + result = ical_to_jscal(ical) + assert result.get("status") == "confirmed" + + def test_ical_no_status_omits_jscal_status(self): + ical = _make_ical("DTSTART:20240615T100000Z\r\nSUMMARY:No Status\r\n") + result = ical_to_jscal(ical) + assert "status" not in result + + def test_jscal_status_cancelled_to_ical(self): + result = jscal_to_ical(_minimal_jscal(status="cancelled")) + assert "STATUS:CANCELLED" in result + + def test_jscal_status_tentative_to_ical(self): + result = jscal_to_ical(_minimal_jscal(status="tentative")) + assert "STATUS:TENTATIVE" in result + + def test_jscal_status_confirmed_to_ical(self): + result = jscal_to_ical(_minimal_jscal(status="confirmed")) + assert "STATUS:CONFIRMED" in result + + def test_jscal_no_status_omits_ical_status(self): + result = jscal_to_ical(_minimal_jscal()) + assert "STATUS:" not in result + + def test_status_cancelled_round_trips(self): + original = _make_ical( + "DTSTART:20240615T100000Z\r\nSUMMARY:Cancelled\r\nSTATUS:CANCELLED\r\n" + ) + jscal = ical_to_jscal(original) + assert jscal.get("status") == "cancelled" + round_tripped = jscal_to_ical(jscal) + assert "STATUS:CANCELLED" in round_tripped