fix(jmap): correctness fixes + dedup from June 2026 review#688
Conversation
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
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| # 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 = ( |
| # 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( |
|
Hi, is it okay if I review this on Sunday or later? I'm currently drowned in a lot of business work right now. |
|
Sure. While I had hoped to release caldav 3.3.0 this week, realistically I will not manage. |
SashankBhamidi
left a comment
SashankBhamidi
left a comment
There was a problem hiding this comment.
Thanks for running the full codebase review and pulling me in on the JMAP sections.
Most of this is correct and the §5.1 refactor is genuinely the right move structurally. Having every public async method be a three-line wrapper over _request() and a shared static parser means future fixes no longer need to be made in two places, which is exactly how the §1.3 create_task bug got there in the first place. The §4.5 implementation is also more careful than the original finding required: the _unsupported_null_keys retry loop only retries when every property in the server rejection is null-cleanup we injected, terminates because each iteration strictly shrinks the patch, and surfaces genuine invalidProperties errors without retrying. I traced through it.
One finding needs fixing before merge. The §4.3 change to _format_local_dt is correct in what it does but leaves the callers in an inconsistent state for TZID events with RRULE UNTIL clauses. Details in the inline comment. The STATUS maps and the dead test variable are non-blocking.
On the two bot findings: _NULL_FOR_UPDATE is not unused. It is imported at client.py:46 and used at line 178. Dismiss that one. The _ICAL_WITH_LOCATION finding is correct, noted below.
| @@ -111,22 +111,21 @@ def _duration_to_timedelta(duration_str: str) -> timedelta: | |||
|
|
|||
|
|
|||
| def _format_local_dt(dt: datetime | date) -> str: | |||
There was a problem hiding this comment.
The docstring now says "callers must convert UTC datetimes to the event's local timezone before calling if the event uses TZID" but none of the three callers in ical_to_jscal.py actually do that (at lines 100, 163, and 312 of that file). I ran it to confirm the data bug:
# TZID=Europe/Berlin event, UNTIL=20240701T120000Z in the RRULE
jscal = ical_to_jscal(ical)
jscal["recurrenceRules"][0]["until"]
# => "2024-07-01T12:00:00" (UTC wall-clock, wrong)
# should be "2024-07-01T14:00:00" (Berlin local, UTC+2)The round-trip back through jscal_to_ical produces UNTIL=20240701T120000 with no Z suffix, which RFC 5545 §3.3.10 forbids for TZID events. The recurrence series ends two hours early for UTC+2 users. The same applies at ical_to_jscal.py:163 when an EXDATE is expressed in UTC on a TZID event, though that form is less common since most clients emit EXDATE with the TZID parameter matching DTSTART.
The fix is to thread the event timezone into _rrule_to_jscal and _exdate_to_overrides so they can call dt.astimezone(ZoneInfo(tz)).replace(tzinfo=None) before _format_local_dt. The timezone is already extracted at ical_to_jscal.py:64 from dtstart_prop.params.get("TZID") but is not passed to either helper. I would keep this in the same PR rather than deferring it since the UNTIL case is silent data corruption on a common event pattern.
| 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) |
There was a problem hiding this comment.
The STATUS mapping dict is defined inside the if status: block on every call. Every other mapping in this file (_PRIVACY_TO_CLASS, _FREE_BUSY_TO_TRANSP, _PARTSTAT_MAP, _KIND_TO_CUTYPE) is a module-level constant. Non-blocking, but inconsistent with the rest of the file. Hoist _STATUS_JSCAL_TO_ICAL to module level and simplify this block to:
| 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) | |
| status = jscal.get("status") | |
| if status: | |
| ical_status = _STATUS_JSCAL_TO_ICAL.get(status) | |
| if ical_status: | |
| event.add("status", ical_status) |
| 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 |
There was a problem hiding this comment.
Same issue: _STATUS_ICAL_TO_JSCAL is rebuilt on every call. Should be a module-level constant alongside _CLASS_MAP, _PARTSTAT_MAP, etc.
| 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 | |
| status = master.get("STATUS") | |
| if status: | |
| jscal_status = _STATUS_ICAL_TO_JSCAL.get(str(status).upper()) | |
| if jscal_status: | |
| jscal["status"] = jscal_status |
| _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" | ||
| ) |
There was a problem hiding this comment.
_ICAL_WITH_LOCATION is assigned but never passed to anything. Only _ICAL_WITHOUT_LOCATION is used at line 1688. Looks like scaffolding from an earlier draft. Non-blocking but should be dropped.
| _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" | |
| ) |
Sorry for the delay! Back to you, good to merge after my comments have been addressed. I'll be afk for a few days so feel free to merge. |
|
Well, I'm also delayed, hoped to get v3.3.0 launched yesterday, but it seems like there will be several days until I have time looking more into the caldav library. |
2 participants
I managed to run a full code review of the caldav library with the Claude Fable model before it was yanked, and I've had Claude Sonnet and Claude Opus helping me fixing the things. I'm still busy reviewing, rewording, squashing similar commits together, etc.
The experimental JMAP support was written by @SashankBhamidi - perhaps you could help me reviewing this particular changeset? (or rewrite it completely if you prefer that).
The rest of this message is AI-generated
JMAP fixes from the June 2026 code review
This consolidates the JMAP-related findings from the full codebase review
(
docs/design/FULL_CODE_REVIEW_2026-06.md) into a single commit, so the JMAPbackend can be fixed on
masterindependently of the larger v3.3.0 work.Correctness fixes
create_task()was missing the empty-created-dict guard thatcreate_event()already had → bareKeyErrorinstead ofJMAPMethodError(sync + async).
jscal_to_ical: override child VEVENT with nostartpatch key tookDTSTART from the master instead of the override key — relocated every
title-only override to the master's first occurrence.
jscal_to_ical: EXDATE / RECURRENCE-ID were always emitted as naivefloating DATE-TIMEs; a floating EXDATE on a TZID event matches no instance, so
excluded occurrences reappeared. Now parsed with the event's value type.
_format_local_dt(): stripped theZsuffix — RFC 8984 LocalDateTime(override keys,
until) must be suffix-less or strict servers won't match.(
CANCELLEDround-tripped asconfirmed). Added the CONFIRMED/TENTATIVE/CANCELLED mappings.
update_eventsent the full object as the PatchObject, so removedproperties (LOCATION, VALARM, …) silently persisted. Absent optional props are
now set to
null(RFC 8620 §3.3 semantics).search()passed datetimes throughisoformat()instead of the...ZUTCDate format JMAP requires.get_objects_by_sync_token()discardednewState, forcing a raceyfollow-up
get_sync_token(). Now returns(added, modified, deleted, new_sync_token).Dedup / perf
_JMAPClientBase; sync/async public methods are now thin wrappers(
async_client.py550 → 415 lines), so future fixes live in one place.Testing
tests/test_jmap_unit.py— 266 passed.