Repository navigation
CalDAV adapter - #33
Conversation
Documentation build overview
23 files changed ·
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
I'm adding those checks to the caldav-server-tester.
This has been fixed in a PR now.
I will look into this as well. |
tobixen
left a comment
There was a problem hiding this comment.
It would be nice if the integration tests could be run towards arbitrary servers. I do have a list of servers I test the caldav library towards. I also have on my list to make the list exportable from the caldav library, making it easier to maintain it in one place and run different libraries towards it - see python-caldav/caldav#817
The text below the line is generated by Claude Opus 5.5
This is a lot of careful work, and the recorded tapes plus live scenarios across five servers are great. I reviewed c40782b against python-caldav 3.6.0. The 243 CalDAV unit tests pass. The points below come from reading the code; I haven't reproduced them. The first six look like real bugs. The rest are robustness and tidy-up points.
… calls, and skip unchanged members
|
Thanks @tobixen, really useful review :) Fixed most of it in a710a61 and answered the rest in the threads. |
tobixen
left a comment
There was a problem hiding this comment.
Disclaimer: AI-generated text below the line, generated by Claude Opus 5.5
@SashankBhamidi, the fixes in a710a61 look right to me, and the three open threads (weak ETags, a UID taken on an update, the 507 workaround) are fine as later work. Approving.
As I said I would, your page's checks are now probes in caldav-server-tester (python-caldav/caldav-server-tester#36, python-caldav/caldav-server-tester#37). The results are python-caldav compatibility hints (python-caldav/caldav#822), measured on 22 servers. Once that's released, a DAVClient configured with a server's name carries them as client.features, so the adapter can ask instead of hard-coding servers. These matter for the adapter:
sync-token.delete.created-and-deleted: Stalwart depends on what else changed, not on the version. On 0.16.11, a member created and deleted between two reports is left out unless a member that existed at the old token also changed in the same interval:
| changes between the two reports | reported |
|---|---|
| create e, delete e | nothing |
| create e, delete e, create f | f only |
| create e, delete e, change or delete an existing a | e as 404, and a |
That's why our first probe passed Stalwart and your page didn't. The probe now tests the short-lived member in an interval of its own. Xandikos and Bedework 5 never report it.
sync-token.recreated is broken on Nextcloud (and ecloud) and Synology. A member deleted and re-created at the same href is reported only as a 404, with no changed entry; I checked by hand that a GET answers 200 while the sync says 404. The adapter then marks an existing object REMOVED, and nothing brings it back short of a full listing or a later change. Worth a row. Stalwart's twice-reported case is a quirk.
sync-token.initial-sync: CCS lists the collection itself, like Stalwart; the adapter's filter covers both. Radicale lists deleted members with a 404.
delete.if-match is broken on Bedework (3 and 5), Zimbra, Posteo and SOGo: a DELETE whose If-Match no longer matches deletes the object anyway, so a conditional delete can remove a remote edit there.
delete.missing.if-match: Baïkal, Nextcloud, Davis, CCS, OX, Purelymail, Robur and Infomaniak answer a DELETE with If-Match on a missing member with 412. The adapter already leaves such a delete pending, so this only widens the "a tested server" sentence.
multiget.missing-href: Baïkal, Nextcloud, DAViCal, Davis and Infomaniak leave the href out of the multistatus, which the adapter handles. Posteo fails the whole calendar-multiget with a 404 instead, so a member removed between report and multiget would make each sync raise there.
save.etag.matches-stored: Radicale, Xandikos, CCS and Infomaniak reorder properties, SOGo adds CLASS, Zimbra and Bedework 5 add properties, and all of them still answer the PUT with a strong ETag (RFC 4791 section 5.3.4 forbids that). Clearing the version after a push, as the adapter does, is the right call.
multiget.limit, propfind.limit: Stalwart stops at WebDav.maxResults with a 507; caldav's Stalwart test container now runs with 50, so this shows up with few objects.
python-caldav, from 3.7.0:
ResponseError.statusis set for failed replies (python-caldav/caldav@824e26e3), for the 507 thread.sync-collectiongoes out withDepth: 0, so the Depth: 1 sentence indocs/explanation/adapters.rstneeds a version.- Infomaniak adds an empty propstat with 418 to a removed member in a sync reply, which made python-caldav raise on every sync after a deletion: python-caldav/caldav#821.
|
Thanks @tobixen, the server-tester results helped a lot. I've handled them in this PR itself: a re-created member on Nextcloud and Synology is fetched now, Posteo's multiget 404 falls back to one member at a time, the 507 is read from the status, and the Depth sentence has a version. Your results are on the compatibility page too, credited to the tester. Could you have another look when you get time? |
|
LGTM |
Closes #10.
This brings in the CalDAV adapter,
CalDAVAdapterand its async twin, behind a newcaldavextra. I didn't want the sync and async versions drifting apart, so the whole sync flow is written once inadapters/_flow.py, and the two adapters only drive it differently. The JMAP adapter will sit on the same base.For testing, I recorded real conversations with Radicale, Xandikos, Cyrus, Stalwart and Baïkal, and the unit tests replay those. The integration tests run the same scenarios live against all five, including a token that actually expires and a login that gets refused.
Along the way I had to go past the issue text in a few places:
raw_data,precondition="etag"andIf-None-Match: *onsave(no_overwrite=True), so I bumped the floor to it, and creates now go throughsave()instead ofput().%40from a sync report and@from a multiget, so object ids are now the href's path in one spelling.LogicalKeyTakenfor creates.The servers had some surprises too, all on the compatibility page. The one that worried me most: Baïkal accepts any token with its own prefix, even one it never issued, and simply returns no changes. A client holding such a token would miss changes without any error.
@tobixen, two python-caldav things I noticed:
sync-collectiongoes out withDepth: 1, though RFC 6578 section 3.2 only defines it forDepth: 0. Every server I tried answers it anyway.propfindcomes back as aResponseErrorwithstatusset to None, so for now I'm reading it from the message.One cost to know about: canonical fingerprints take around 2.4 s for 20,000 events, mostly inside icalendar's parser. I've added a benchmark scenario so we can keep an eye on it.