fix(jmap): correctness fixes + dedup from June 2026 review#688
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
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.
| 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. |
tobixen
force-pushed
the
jmap-code-review-fixes
branch
from
0204af4 to
d23eafd
Compare
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. Because LocalDateTime is in the event's own time zone, the event TZID is now threaded into _rrule_to_jscal / _exdate_to_overrides so a UTC UNTIL/EXDATE on a TZID event is shifted to event-local wall-clock (a UTC+2 series was ending two hours early); the jscal->ical direction converts the local until back to UTC, since RFC 5545 §3.3.10 requires a UTC UNTIL for a TZID DTSTART. (PR review, @SashankBhamidi.) Also hoisted the STATUS maps to module-level constants and dropped a dead test variable per the same review. §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 Co-authored-by: Claude Sonnet 4.6 and Opus 4.8 <noreply@anthropic.com> Co-authored-by: Sashank Bhamidi <hello@sashank.wiki>
tobixen
force-pushed
the
jmap-code-review-fixes
branch
from
d23eafd to
43226c5
Compare
|
Thanks @SashankBhamidi — I had Claude go through this, I've looked through the changes and it seems like all the points you've raised have been resolved - do you agree? I've also verified that the new tests fails on 0204af4 but passes with 43226c5. I'm also asking it to come up with some integration tests. Below is the AI-generated description of the changes done. §4.3 (blocking) — fixed. Threaded the event TZID into Nits — done. Hoisted Bot I left the RECURRENCE-ID path at |
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.