fix(jmap): correctness fixes + dedup from June 2026 review#688
Open
tobixen wants to merge 1 commit into
Open
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( |
Collaborator
|
Hi, is it okay if I review this on Sunday or later? I'm currently drowned in a lot of business work right now. |
Member
Author
|
Sure. While I had hoped to release caldav 3.3.0 this week, realistically I will not manage. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.