From ff6db300ed2ec21fc911b03069929f5b18bcbd79 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 11:15:15 +0100 Subject: [PATCH 01/23] Fix bugs identified in v3.0 code review Must-fix items: - xml_parsers.py: fix NameError in parse_calendar_multiget_response (missing _ prefix) - compatibility_hints.py: replace breakpoint() with ValueError for unknown feature type - calendarobjectresource.py: fix _set_deprecated_vobject_instance calling getter instead of setter - calendarobject_ops.py: replace uuid.uuid1() with uuid.uuid4() to avoid MAC address leak - davclient.py: narrow bare except clauses (TypeError for teardown, Exception for check_dav_support) - async_davclient.py: add url.unauth() to strip credentials from stored URL (prevents log leaks) Should-fix items: - async_davclient.py: add httpx-compatible _HttpxBearerAuth class; use it for bearer on httpx path - response.py: guard tree[0] access with len(tree) > 0 check - davobject.py: replace production-unsafe assert with explicit ValueError - calendar_ops.py, collection.py: add usedforsecurity=False to MD5 calls for FIPS compliance Co-Authored-By: Claude --- caldav/async_davclient.py | 18 +++++++++++++++++- caldav/calendarobjectresource.py | 2 +- caldav/collection.py | 2 +- caldav/compatibility_hints.py | 2 +- caldav/davclient.py | 4 ++-- caldav/davobject.py | 3 ++- caldav/operations/calendar_ops.py | 2 +- caldav/operations/calendarobject_ops.py | 2 +- caldav/protocol/xml_parsers.py | 2 +- caldav/response.py | 2 +- 10 files changed, 28 insertions(+), 11 deletions(-) diff --git a/caldav/async_davclient.py b/caldav/async_davclient.py index 9712ffe8..53654291 100644 --- a/caldav/async_davclient.py +++ b/caldav/async_davclient.py @@ -34,6 +34,17 @@ _H2_AVAILABLE = True except ImportError: pass + + class _HttpxBearerAuth(httpx.Auth): + """httpx-compatible bearer token auth.""" + + def __init__(self, password: str) -> None: + self.password = password + + def auth_flow(self, request): + request.headers["Authorization"] = f"Bearer {self.password}" + yield request + except ImportError: pass @@ -237,6 +248,8 @@ def __init__( # Use explicit None check to preserve empty strings (needed for servers with no auth) self.username = username if username is not None else url_username self.password = password if password is not None else url_password + # Strip credentials from stored URL to avoid leaking them in log messages + self.url = self.url.unauth() # Setup authentication self.auth = auth @@ -846,7 +859,10 @@ def build_auth_object(self, auth_types: list[str] | None = None) -> None: # Build auth object - use appropriate classes for httpx or niquests if auth_type == "bearer": - self.auth = HTTPBearerAuth(self.password) + if _USE_HTTPX: + self.auth = _HttpxBearerAuth(self.password) + else: + self.auth = HTTPBearerAuth(self.password) elif auth_type == "digest": if _USE_HTTPX: self.auth = httpx.DigestAuth(self.username, self.password) diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index 7632f606..1790e6b6 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -1245,7 +1245,7 @@ def _set_deprecated_vobject_instance(self, inst: "vobject.base.Component"): DeprecationWarning, stacklevel=2, ) - return self._get_vobject_instance(inst) + return self._set_vobject_instance(inst) vobject_instance: "vobject.base.VBase" = property( _get_vobject_instance, diff --git a/caldav/collection.py b/caldav/collection.py index 1134c228..b0c6536d 100644 --- a/caldav/collection.py +++ b/caldav/collection.py @@ -1653,7 +1653,7 @@ def _generate_fake_sync_token(self, objects: list["CalendarObjectResource"]) -> etags.append(str(obj.url.canonical())) etags.sort() ## Consistent ordering combined = "|".join(etags) - hash_value = hashlib.md5(combined.encode()).hexdigest() + hash_value = hashlib.md5(combined.encode(), usedforsecurity=False).hexdigest() return f"fake-{hash_value}" def get_objects_by_sync_token( diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index 4de73bdf..14e278cb 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -462,7 +462,7 @@ def _default(self, feature_info): elif feature_type in ('tests-behaviour', 'client-hints'): return { } else: - breakpoint() + raise ValueError(f"Unknown feature type: {feature_type!r}") def is_supported(self, feature, return_type=bool, return_defaults=True, accept_fragile=False): """Work in progress diff --git a/caldav/davclient.py b/caldav/davclient.py index 808a1bc4..3957f2da 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -382,7 +382,7 @@ def __exit__( if hasattr(self, "teardown"): try: self.teardown() - except: + except TypeError: self.teardown(self) def close(self) -> None: @@ -694,7 +694,7 @@ def check_dav_support(self) -> str | None: ## element that should come with caldav extras. ## Anyway, packing this into a try-except in case it fails. response = self.options(self.principal().url) - except: + except Exception: response = self.options(str(self.url)) return response.headers.get("DAV", None) diff --git a/caldav/davobject.py b/caldav/davobject.py index 2d4cf018..444ebcbc 100644 --- a/caldav/davobject.py +++ b/caldav/davobject.py @@ -96,7 +96,8 @@ def __init__( self.url = None else: self.url = URL.objectify(url) - assert " " not in str(self.url) + if self.url is not None and " " in str(self.url): + raise ValueError(f"URL must not contain spaces: {self.url!r}") @property def canonical_url(self) -> str: diff --git a/caldav/operations/calendar_ops.py b/caldav/operations/calendar_ops.py index 2bdbfdd2..07630b46 100644 --- a/caldav/operations/calendar_ops.py +++ b/caldav/operations/calendar_ops.py @@ -129,7 +129,7 @@ def _generate_fake_sync_token(etags_and_urls: list[tuple[str | None, str]]) -> s parts.sort() # Consistent ordering combined = "|".join(parts) - hash_value = hashlib.md5(combined.encode()).hexdigest() + hash_value = hashlib.md5(combined.encode(), usedforsecurity=False).hexdigest() return f"fake-{hash_value}" diff --git a/caldav/operations/calendarobject_ops.py b/caldav/operations/calendarobject_ops.py index c31de227..8c95c676 100644 --- a/caldav/operations/calendarobject_ops.py +++ b/caldav/operations/calendarobject_ops.py @@ -42,7 +42,7 @@ class CalendarObjectData: def _generate_uid() -> str: """Generate a new UID for a calendar object.""" - return str(uuid.uuid1()) + return str(uuid.uuid4()) def _generate_url(parent_url: str, uid: str) -> str: diff --git a/caldav/protocol/xml_parsers.py b/caldav/protocol/xml_parsers.py index 6f099f11..1876ac22 100644 --- a/caldav/protocol/xml_parsers.py +++ b/caldav/protocol/xml_parsers.py @@ -257,7 +257,7 @@ def _parse_calendar_multiget_response( Returns: List of CalendarQueryResult with calendar data """ - return parse_calendar_query_response(body, status_code, huge_tree) + return _parse_calendar_query_response(body, status_code, huge_tree) # Helper functions diff --git a/caldav/response.py b/caldav/response.py index bd2feef9..0c1d3bfa 100644 --- a/caldav/response.py +++ b/caldav/response.py @@ -166,7 +166,7 @@ def _strip_to_multistatus(self) -> _Element | list[_Element]: simple XPath query, but I'm not much into XPath) """ tree = self.tree - if tree.tag == "xml" and tree[0].tag == dav.MultiStatus.tag: + if tree.tag == "xml" and len(tree) > 0 and tree[0].tag == dav.MultiStatus.tag: return tree[0] if tree.tag == dav.MultiStatus.tag: return self.tree From ee9d1bbdb3b738af99aab1fdc7e05daf89eb78ce Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 13:20:44 +0100 Subject: [PATCH 02/23] Update V3_CODE_REVIEW.md to reflect fixed issues Mark all "Must Fix" and "Should Fix" items as done (commit ff6db30). Add status column to bugs table. Move two remaining async issues into the v3.1+ list. Reword the executive summary accordingly. Co-Authored-By: Claude --- docs/design/V3_CODE_REVIEW.md | 91 ++++++++++++++++++----------------- 1 file changed, 48 insertions(+), 43 deletions(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index 13f5aaee..cf697848 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -5,6 +5,9 @@ **Branch:** v3.0-dev (119 commits since v2.2.6) **Scope:** All changes between tags v2.2.6 and HEAD +> **Status update (commit ff6db30):** All "Must Fix" and "Should Fix" items from §8 have been +> resolved. Remaining open items are v3.1+ and v4.0 work. + ## Executive Summary The v3.0 release is a major architectural refactoring introducing Sans-I/O separation, full async support, and comprehensive API modernization -- all while maintaining backward compatibility with v2.x. The scope is large: 159 files changed, ~25,900 lines added, ~4,500 removed. @@ -12,11 +15,11 @@ The v3.0 release is a major architectural refactoring introducing Sans-I/O separ **Overall assessment:** The architecture is well-designed and the codebase is in good shape for an alpha release. The Sans-I/O protocol layer is clean and testable. However, there are several bugs that should be fixed before a stable release, significant code duplication between sync/async paths (~650 lines), and some test coverage gaps. **Key findings:** -- 3 bugs that will cause runtime errors -- 1 security concern (UUID1 leaks MAC address in calendar UIDs) +- ~~3 bugs that will cause runtime errors~~ **fixed** +- ~~1 security concern (UUID1 leaks MAC address in calendar UIDs)~~ **fixed** - ~650 lines of sync/async duplication across domain objects - Test coverage gaps in discovery module and sync client unit tests -- `breakpoint()` left in production code +- ~~`breakpoint()` left in production code~~ **fixed** --- @@ -30,12 +33,12 @@ The protocol layer separates XML construction/parsing from I/O. This is the stro |------|-------|--------|---------| | `types.py` | 243 | 9/10 | Frozen dataclasses: DAVRequest, DAVResponse, PropfindResult, CalendarQueryResult | | `xml_builders.py` | 428 | 7/10 | Pure functions building XML for PROPFIND, calendar-query, MKCALENDAR, etc. | -| `xml_parsers.py` | 455 | 5/10 | Parse XML responses into typed results -- has a runtime bug | +| `xml_parsers.py` | 455 | 5/10 | Parse XML responses into typed results | | `__init__.py` | 46 | 8/10 | Clean re-exports | **Issues found:** -1. **BUG: NameError in `xml_parsers.py:260`** -- `parse_calendar_multiget_response()` calls `parse_calendar_query_response()` (no leading underscore), but only `_parse_calendar_query_response()` is defined. This will crash at runtime when calendar-multiget responses are parsed. +1. ~~**BUG: NameError in `xml_parsers.py:260`**~~ **FIXED** -- `parse_calendar_multiget_response()` was calling `parse_calendar_query_response()` (missing leading underscore). Fixed by adding the `_` prefix. 2. **Dead code in `xml_builders.py`** -- `_to_utc_date_string()` (line 401), `_build_freebusy_query_body()` (line 189), and `_build_mkcol_body()` (line 200) have zero callers. @@ -59,11 +62,11 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. **Issues found:** -1. **SECURITY: UUID1 leaks MAC address (`calendarobject_ops.py:55`)** -- `uuid.uuid1()` embeds the host MAC address into generated calendar UIDs. Since calendar events are routinely shared, this is a privacy leak. Should use `uuid.uuid4()`. +1. ~~**SECURITY: UUID1 leaks MAC address (`calendarobject_ops.py:55`)**~~ **FIXED** -- Replaced `uuid.uuid1()` with `uuid.uuid4()`. 2. **`search_ops.py` mutates inputs (line 381-389)** -- `_build_search_xml_query` calls `setattr(searcher, flag, True)` on the passed-in searcher object. This violates the "pure functions" contract and causes side effects in callers. -3. **MD5 in FIPS environments (`calendar_ops.py:132`)** -- `hashlib.md5()` without `usedforsecurity=False` will fail in FIPS-mode environments. Used for fake sync tokens, not for security. +3. ~~**MD5 in FIPS environments (`calendar_ops.py:132`)**~~ **FIXED** -- Added `usedforsecurity=False` to both `calendar_ops.py` and `collection.py`. 4. **Duplicate URL quoting** -- `quote(uid.replace("/", "%2F"))` pattern appears at both lines 62 and 138 in `calendarobject_ops.py`. @@ -77,7 +80,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. **Issues found:** -1. **Unguarded index access (line 169)** -- `_strip_to_multistatus` accesses `tree[0]` without checking `len(tree) > 0` when `tree.tag == "xml"`. The equivalent code in `xml_parsers.py:279` does guard this. +1. ~~**Unguarded index access (line 169)**~~ **FIXED** -- Added `len(tree) > 0` guard before `tree[0]` access in `_strip_to_multistatus`, matching the equivalent guard in `xml_parsers.py`. 2. **Thread-unsafe** -- `self.objects`, `self.results`, `self._responses` are mutable instance state set during parsing. If a response object is shared between threads, results could be corrupt. @@ -100,9 +103,9 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. **Issues found:** -1. **BUG: `HTTPBearerAuth` incompatible with httpx (line 862)** -- `HTTPBearerAuth` is a requests/niquests `AuthBase` subclass that implements `__call__(self, r)`. httpx uses a different auth protocol (`httpx.Auth` with `auth_flow`). Bearer auth will fail at runtime on the httpx code path. +1. ~~**BUG: `HTTPBearerAuth` incompatible with httpx**~~ **FIXED** -- Added `_HttpxBearerAuth(httpx.Auth)` class with `auth_flow` generator inside the httpx import block. `build_auth_object` now uses it on the httpx path and falls back to `HTTPBearerAuth` for niquests. -2. **BUG: Missing `url.unauth()` call** -- The sync client strips credentials from URLs at `davclient.py:342`, but the async client never does. Credentials embedded in URLs could leak in log messages. +2. ~~**BUG: Missing `url.unauth()` call**~~ **FIXED** -- Added `self.url = self.url.unauth()` after credentials are extracted from the URL, matching the sync client behaviour. 3. **`_auto_url` blocks the event loop** -- RFC6764 discovery performs synchronous DNS lookups and HTTP requests inside `AsyncDAVClient.__init__()`, which is called from `async get_davclient()`. This blocks the event loop. @@ -118,7 +121,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. **Issues found:** -1. **Bare `except:` clauses (lines 367, 679)** -- Catches `SystemExit` and `KeyboardInterrupt`. Should be `except Exception:`. +1. ~~**Bare `except:` clauses (lines 367, 679)**~~ **FIXED** -- Narrowed to `except TypeError` for the test teardown fallback (the expected exception when `teardown()` takes an argument), and `except Exception` for `check_dav_support` (which intentionally catches anything from principal lookup and falls back to root URL). 2. **`NotImplementedError` for auth failures (line 996)** -- When no supported auth scheme is found, `NotImplementedError` is raised. Should be `AuthorizationError`. @@ -153,7 +156,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Rating: 7/10** -- Solid dual-mode foundation. **Issues:** -1. **Production-unsafe assert (`line 99`)** -- `assert " " not in str(self.url)` will be silently removed with `-O`. Also passes when `url=None`. +1. ~~**Production-unsafe assert (`line 99`)**~~ **FIXED** -- Replaced `assert " " not in str(self.url)` with an explicit `ValueError` (also fixed the `url=None` case that the assert silently passed). 2. **Return type lies for async** -- Methods like `get_property()`, `get_properties()`, `delete()` return coroutines when used with async clients, but annotations say `str | None`, `dict`, `None`. 3. **`set_properties` regression** -- Changed from per-property status checking to HTTP status-only checking, losing ability to detect partial PROPPATCH failures. @@ -163,7 +166,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Issues:** 1. **Missing deprecation warnings** -- `calendars()`, `events()`, `todos()` etc. have docstring notes but no `warnings.warn()` calls (unlike `date_search` and `davobject.name` which do emit). -2. **`_generate_fake_sync_token` uses MD5 (line 1655)** -- Same FIPS concern as calendar_ops.py. +2. ~~**`_generate_fake_sync_token` uses MD5 (line 1655)**~~ **FIXED** -- Added `usedforsecurity=False`. 3. **`Principal._async_get_property` overrides parent (line 352)** with incompatible implementation. ### 3.3 CalendarObjectResource (`caldav/calendarobjectresource.py`) @@ -171,7 +174,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Rating: 7/10** -- Good DataState integration but large (1,919 lines). **Issues:** -1. **BUG: `_set_deprecated_vobject_instance` (line 1248)** -- Calls `self._get_vobject_instance(inst)` but `_get_vobject_instance` takes no arguments. Will raise `TypeError` when invoked via `event.instance = some_vobject`. +1. ~~**BUG: `_set_deprecated_vobject_instance` (line 1248)**~~ **FIXED** -- Was calling `_get_vobject_instance(inst)` (getter, wrong number of arguments); fixed to call `_set_vobject_instance(inst)`. 2. **`id` setter is a no-op (line 123)** -- `id` passed to constructor is silently ignored. 3. **`_async_load` missing multiget fallback** -- Sync `load()` has `load_by_multiget()` fallback, async does not. 4. **Dual data model risk** -- Old `_data`/`_vobject_instance`/`_icalendar_instance` coexist with new `_state`. Manual sync at lines 1206, 1279 could desynchronize. @@ -203,7 +206,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Rating: 7/10** -- Comprehensive server database. **Issues:** -1. **`breakpoint()` in production code (line 443)** -- `FeatureSet._default()` has `breakpoint()` in the `else` branch. Will pause execution in production. +1. ~~**`breakpoint()` in production code (line 443)**~~ **FIXED** -- Replaced with `raise ValueError(f"Unknown feature type: {feature_type!r}")`. 2. **Deprecated `incompatibility_description` dict still present (lines 660-778)** -- Marked "TO BE REMOVED" with 30+ entries. 3. **`# fmt: off` for entire 1,366-line file** -- Should scope it to just the dict definitions. @@ -263,37 +266,37 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he ## 7. Bugs Summary (Ordered by Severity) -| # | Severity | Location | Description | -|---|---|---|---| -| 1 | HIGH | `xml_parsers.py:260` | NameError: calls `parse_calendar_query_response` (missing underscore) | -| 2 | HIGH | `async_davclient.py:862` | `HTTPBearerAuth` incompatible with httpx -- bearer auth broken on httpx path | -| 3 | MEDIUM | `calendarobjectresource.py:1248` | `_set_deprecated_vobject_instance` passes arg to no-arg getter | -| 4 | MEDIUM | `compatibility_hints.py:443` | `breakpoint()` in production code path | -| 5 | MEDIUM | `async_davclient.py` | Missing `url.unauth()` -- credential leak in logs | -| 6 | MEDIUM | `calendarobject_ops.py:55` | UUID1 leaks MAC address in calendar UIDs | -| 7 | LOW | `davclient.py:367,679` | Bare `except:` catches SystemExit/KeyboardInterrupt | -| 8 | LOW | `response.py:169` | Unguarded `tree[0]` access | -| 9 | LOW | `davobject.py:99` | Production-unsafe `assert` for URL validation | +| # | Severity | Location | Description | Status | +|---|---|---|---|---| +| 1 | HIGH | `xml_parsers.py:260` | NameError: calls `parse_calendar_query_response` (missing underscore) | **FIXED** | +| 2 | HIGH | `async_davclient.py` | `HTTPBearerAuth` incompatible with httpx -- bearer auth broken on httpx path | **FIXED** | +| 3 | MEDIUM | `calendarobjectresource.py:1248` | `_set_deprecated_vobject_instance` passes arg to no-arg getter | **FIXED** | +| 4 | MEDIUM | `compatibility_hints.py:443` | `breakpoint()` in production code path | **FIXED** | +| 5 | MEDIUM | `async_davclient.py` | Missing `url.unauth()` -- credential leak in logs | **FIXED** | +| 6 | MEDIUM | `calendarobject_ops.py:55` | UUID1 leaks MAC address in calendar UIDs | **FIXED** | +| 7 | LOW | `davclient.py:367,679` | Bare `except:` catches SystemExit/KeyboardInterrupt | **FIXED** | +| 8 | LOW | `response.py:169` | Unguarded `tree[0]` access | **FIXED** | +| 9 | LOW | `davobject.py:99` | Production-unsafe `assert` for URL validation | **FIXED** | --- ## 8. Recommendations -### For v3.0 Stable Release (Must Fix) +### For v3.0 Stable Release (Must Fix) -- ALL DONE ✓ -1. Fix the NameError in `xml_parsers.py:260` (add underscore) -2. Remove `breakpoint()` from `compatibility_hints.py:443` -3. Fix `_set_deprecated_vobject_instance` to call setter not getter -4. Replace `uuid.uuid1()` with `uuid.uuid4()` in `calendarobject_ops.py:55` -5. Fix bare `except:` to `except Exception:` in `davclient.py` -6. Add `url.unauth()` call to `AsyncDAVClient.__init__` +1. ~~Fix the NameError in `xml_parsers.py:260` (add underscore)~~ +2. ~~Remove `breakpoint()` from `compatibility_hints.py:443`~~ +3. ~~Fix `_set_deprecated_vobject_instance` to call setter not getter~~ +4. ~~Replace `uuid.uuid1()` with `uuid.uuid4()` in `calendarobject_ops.py:55`~~ +5. ~~Fix bare `except:` to narrower exception types in `davclient.py`~~ +6. ~~Add `url.unauth()` call to `AsyncDAVClient.__init__`~~ -### For v3.0 Stable Release (Should Fix) +### For v3.0 Stable Release (Should Fix) -- ALL DONE ✓ -7. Fix `HTTPBearerAuth` for httpx path (or document httpx+bearer as unsupported) -8. Add guard for `tree[0]` access in `response.py:169` -9. Replace production `assert` with proper validation in `davobject.py:99` -10. Add `usedforsecurity=False` to MD5 calls for FIPS compliance +7. ~~Fix `HTTPBearerAuth` for httpx path~~ +8. ~~Add guard for `tree[0]` access in `response.py:169`~~ +9. ~~Replace production `assert` with proper validation in `davobject.py:99`~~ +10. ~~Add `usedforsecurity=False` to MD5 calls for FIPS compliance~~ ### For v3.1+ @@ -303,15 +306,17 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he 14. Add discovery module tests 15. Add missing `warnings.warn()` to all deprecated methods 16. Remove dead code in `xml_builders.py` -17. Move `_auto_url` from `davclient.py` to shared module +17. Move `_auto_url` from `davclient.py` to shared module (also fixes event-loop blocking in async client) 18. Make `search_ops._build_search_xml_query` not mutate its input +19. Fix `NotImplementedError` for auth failures in `davclient.py` -- raise `AuthorizationError` instead +20. Fix `_auto_url` blocking the event loop in `AsyncDAVClient.__init__` ### For v4.0 -19. Address implicit data conversion side effects (issue #613) -20. Consider splitting `collection.py` (2,054 lines) and `calendarobjectresource.py` (1,919 lines) -21. Fix return type annotations for async-capable methods (use `@overload`) -22. Remove `incompatibility_description` dict from compatibility_hints.py +21. Address implicit data conversion side effects (issue #613) +22. Consider splitting `collection.py` (2,054 lines) and `calendarobjectresource.py` (1,919 lines) +23. Fix return type annotations for async-capable methods (use `@overload`) +24. Remove `incompatibility_description` dict from compatibility_hints.py --- From f7c51c91d92595db7142e3d9ca9813dacd41c0e4 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 13:49:12 +0100 Subject: [PATCH 03/23] Reduce sync/async duplication across davobject, collection, and clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - davobject.py: Extract _build_xml_body() and _build_propfind_root() helpers; replace four identical XML-serialisation blocks and two identical propfind-root-builder blocks with calls to these helpers. _query_properties / _async_query_properties each reduce to one line. - collection.py CalendarSet.get_calendars: Migrate sync path from the old children()/_expand_simple_props() API to the protocol layer (client.propfind + response.results), matching the async path. Add _calendars_from_results() as the single shared result processor; both get_calendars and _async_calendars now reduce to two lines each. - base_client.py: Add _build_principal_search_query() and _parse_principal_search_response() so that both DAVClient and AsyncDAVClient.search_principals share all build/parse logic. Hoist get_events() and get_todos() into BaseDAVClient — the non-async wrappers work for both clients because calling an async search_calendar() without await returns an awaitable coroutine. - davclient.py / async_davclient.py: Shrink search_principals to 4 lines each (body = build; response = report; status check; parse). Remove the now-redundant get_events and get_todos overrides. Net: -177 lines, 0 new tests needed (existing 460 all pass). Co-Authored-By: Claude --- caldav/async_davclient.py | 90 ++------------------------------------- caldav/base_client.py | 61 ++++++++++++++++++++++++++ caldav/collection.py | 75 +++++++++----------------------- caldav/davclient.py | 85 ++---------------------------------- caldav/davobject.py | 86 +++++++++++-------------------------- 5 files changed, 112 insertions(+), 285 deletions(-) diff --git a/caldav/async_davclient.py b/caldav/async_davclient.py index 53654291..cd1eec41 100644 --- a/caldav/async_davclient.py +++ b/caldav/async_davclient.py @@ -996,51 +996,6 @@ async def _get_calendar_home_set(self, principal: "Principal") -> str | None: return extract_home_set(response.results) - async def get_events( - self, - calendar: "Calendar", - start: Any | None = None, - end: Any | None = None, - ) -> list["Event"]: - """Get events from a calendar. - - This is a convenience method that searches for VEVENT objects in the - calendar, optionally filtered by date range. - - Args: - calendar: Calendar to search - start: Start of date range (optional) - end: End of date range (optional) - - Returns: - List of Event objects. - - Example: - from datetime import datetime - events = await client.get_events( - calendar, - start=datetime(2024, 1, 1), - end=datetime(2024, 12, 31) - ) - """ - return await self.search_calendar(calendar, event=True, start=start, end=end) - - async def get_todos( - self, - calendar: "Calendar", - include_completed: bool = False, - ) -> list["Todo"]: - """Get todos from a calendar. - - Args: - calendar: Calendar to search - include_completed: Whether to include completed todos - - Returns: - List of Todo objects. - """ - return await self.search_calendar(calendar, todo=True, include_completed=include_completed) - async def search_calendar( self, calendar: "Calendar", @@ -1125,50 +1080,11 @@ async def search_principals(self, name: str | None = None) -> list["Principal"]: Raises: ReportError: If the server doesn't support principal search """ - from lxml import etree - - from caldav.collection import CalendarSet, Principal - from caldav.elements import cdav, dav - - if name: - name_filter = [ - dav.PropertySearch() + [dav.Prop() + [dav.DisplayName()]] + dav.Match(value=name) - ] - else: - name_filter = [] - - query = ( - dav.PrincipalPropertySearch() - + name_filter - + [dav.Prop(), cdav.CalendarHomeSet(), dav.DisplayName()] - ) - response = await self.report(str(self.url), etree.tostring(query.xmlelement())) - + body = self._build_principal_search_query(name) + response = await self.report(str(self.url), body) if response.status >= 300: raise error.ReportError(f"{response.status} {response.reason} - {response.raw}") - - principal_dict = response._find_objects_and_props() - ret = [] - for x in principal_dict: - p = principal_dict[x] - if dav.DisplayName.tag not in p: - continue - pname = p[dav.DisplayName.tag].text - error.assert_(not p[dav.DisplayName.tag].getchildren()) - error.assert_(not p[dav.DisplayName.tag].items()) - chs = p[cdav.CalendarHomeSet.tag] - error.assert_(not chs.items()) - error.assert_(not chs.text) - chs_href = chs.getchildren() - error.assert_(len(chs_href) == 1) - error.assert_(not chs_href[0].items()) - error.assert_(not chs_href[0].getchildren()) - chs_url = chs_href[0].text - calendar_home_set = CalendarSet(client=self, url=chs_url) - ret.append( - Principal(client=self, url=x, name=pname, calendar_home_set=calendar_home_set) - ) - return ret + return self._parse_principal_search_response(response._find_objects_and_props()) async def principals(self, name: str | None = None) -> list["Principal"]: """ diff --git a/caldav/base_client.py b/caldav/base_client.py index 43cc378c..57e47fe0 100644 --- a/caldav/base_client.py +++ b/caldav/base_client.py @@ -193,6 +193,67 @@ def _raise_authorization_error(self, url_str: str, reason_source: Any) -> NoRetu reason = "None given" raise error.AuthorizationError(url=url_str, reason=reason) + def _build_principal_search_query(self, name: str | None) -> bytes: + """Build the XML body for a principal-property-search REPORT.""" + from lxml import etree + + from caldav.elements import cdav, dav + + name_filter = ( + [dav.PropertySearch() + [dav.Prop() + [dav.DisplayName()]] + dav.Match(value=name)] + if name + else [] + ) + query = ( + dav.PrincipalPropertySearch() + + name_filter + + [dav.Prop(), cdav.CalendarHomeSet(), dav.DisplayName()] + ) + return etree.tostring(query.xmlelement()) + + def _parse_principal_search_response(self, principal_dict: dict) -> list: + """Parse principal-property-search REPORT results into Principal objects.""" + from caldav.collection import CalendarSet, Principal + from caldav.elements import cdav, dav + + ret = [] + for x in principal_dict: + p = principal_dict[x] + if dav.DisplayName.tag not in p: + continue + name = p[dav.DisplayName.tag].text + error.assert_(not p[dav.DisplayName.tag].getchildren()) + error.assert_(not p[dav.DisplayName.tag].items()) + chs = p[cdav.CalendarHomeSet.tag] + error.assert_(not chs.items()) + error.assert_(not chs.text) + chs_href = chs.getchildren() + error.assert_(len(chs_href) == 1) + error.assert_(not chs_href[0].items()) + error.assert_(not chs_href[0].getchildren()) + chs_url = chs_href[0].text + calendar_home_set = CalendarSet(client=self, url=chs_url) + ret.append( + Principal(client=self, url=x, name=name, calendar_home_set=calendar_home_set) + ) + return ret + + def get_events(self, calendar: Any, start: Any = None, end: Any = None) -> Any: + """Get events from a calendar, optionally filtered by date range. + + For sync clients returns a list directly. + For async clients returns a coroutine that must be awaited. + """ + return self.search_calendar(calendar, event=True, start=start, end=end) + + def get_todos(self, calendar: Any, include_completed: bool = False) -> Any: + """Get todos from a calendar. + + For sync clients returns a list directly. + For async clients returns a coroutine that must be awaited. + """ + return self.search_calendar(calendar, todo=True, include_completed=include_completed) + @abstractmethod def build_auth_object(self, auth_types: list[str] | None = None) -> None: """ diff --git a/caldav/collection.py b/caldav/collection.py index b0c6536d..b3c03014 100644 --- a/caldav/collection.py +++ b/caldav/collection.py @@ -58,6 +58,18 @@ class CalendarSet(DAVObject): A CalendarSet is a set of calendars. """ + def _calendars_from_results(self, results) -> list["Calendar"]: + """Convert PropfindResult list into Calendar objects.""" + from caldav.operations.calendarset_ops import ( + _extract_calendars_from_propfind_results, + ) + + calendar_infos = _extract_calendars_from_propfind_results(results) + return [ + Calendar(client=self.client, url=info.url, name=info.name, id=info.cal_id, parent=self) + for info in calendar_infos + ] + def get_calendars(self) -> list["Calendar"]: """ List all calendar collections in this set. @@ -74,69 +86,20 @@ def get_calendars(self) -> list["Calendar"]: Example (async): calendars = await calendar_set.get_calendars() """ - # Delegate to client for dual-mode support if self.is_async_client: return self._async_calendars() - cals = [] - data = self.children(cdav.Calendar.tag) - for c_url, c_type, c_name in data: - try: - cal_id = c_url.split("/")[-2] - if not cal_id: - continue - except Exception: - log.error(f"Calendar {c_name} has unexpected url {c_url}") - cal_id = None - cals.append(Calendar(self.client, id=cal_id, url=c_url, parent=self, name=c_name)) - - return cals - - async def _async_calendars(self) -> list["Calendar"]: - """Async implementation of calendars() using the client.""" - from caldav.operations.base import _is_calendar_resource as is_calendar_resource - from caldav.operations.calendarset_ops import ( - _extract_calendar_id_from_url as extract_calendar_id_from_url, + response = self.client.propfind( + str(self.url), props=self.client.CALENDAR_LIST_PROPS, depth=1 ) + return self._calendars_from_results(response.results) - # Fetch calendars via PROPFIND + async def _async_calendars(self) -> list["Calendar"]: + """Async implementation of get_calendars().""" response = await self.client.propfind( - str(self.url), - props=[ - "{DAV:}resourcetype", - "{DAV:}displayname", - "{urn:ietf:params:xml:ns:caldav}supported-calendar-component-set", - "{http://apple.com/ns/ical/}calendar-color", - "{http://calendarserver.org/ns/}getctag", - ], - depth=1, + str(self.url), props=self.client.CALENDAR_LIST_PROPS, depth=1 ) - - # Process results to extract calendars - calendars = [] - for result in response.results or []: - # Check if this is a calendar resource - if not is_calendar_resource(result.properties): - continue - - # Extract calendar info - url = result.href - name = result.properties.get("{DAV:}displayname") - cal_id = extract_calendar_id_from_url(url) - - if not cal_id: - continue - - cal = Calendar( - client=self.client, - url=url, - name=name, - id=cal_id, - parent=self, - ) - calendars.append(cal) - - return calendars + return self._calendars_from_results(response.results) def calendars(self) -> list["Calendar"]: """ diff --git a/caldav/davclient.py b/caldav/davclient.py index 3957f2da..6976296d 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -409,48 +409,14 @@ def search_principals(self, name=None): Raises: ReportError: If the server doesn't support principal search """ - if name: - name_filter = [ - dav.PropertySearch() + [dav.Prop() + [dav.DisplayName()]] + dav.Match(value=name) - ] - else: - name_filter = [] - - query = ( - dav.PrincipalPropertySearch() - + name_filter - + [dav.Prop(), cdav.CalendarHomeSet(), dav.DisplayName()] - ) - response = self.report(self.url, etree.tostring(query.xmlelement())) - + body = self._build_principal_search_query(name) + response = self.report(self.url, body) ## Possibly we should follow redirects (response status 3xx), but as ## for now we're just treating it in the same way as 4xx and 5xx - ## probably the server did not support the operation if response.status >= 300: raise error.ReportError(f"{response.status} {response.reason} - {response.raw}") - - principal_dict = response._find_objects_and_props() - ret = [] - for x in principal_dict: - p = principal_dict[x] - if dav.DisplayName.tag not in p: - continue - name = p[dav.DisplayName.tag].text - error.assert_(not p[dav.DisplayName.tag].getchildren()) - error.assert_(not p[dav.DisplayName.tag].items()) - chs = p[cdav.CalendarHomeSet.tag] - error.assert_(not chs.items()) - error.assert_(not chs.text) - chs_href = chs.getchildren() - error.assert_(len(chs_href) == 1) - error.assert_(not chs_href[0].items()) - error.assert_(not chs_href[0].getchildren()) - chs_url = chs_href[0].text - calendar_home_set = CalendarSet(client=self, url=chs_url) - ret.append( - Principal(client=self, url=x, name=name, calendar_home_set=calendar_home_set) - ) - return ret + return self._parse_principal_search_response(response._find_objects_and_props()) def principals(self, name=None): """ @@ -585,51 +551,6 @@ def _get_calendar_home_set(self, principal: Principal) -> str | None: return extract_home_set(response.results) - def get_events( - self, - calendar: Calendar, - start: Any | None = None, - end: Any | None = None, - ) -> list["Event"]: - """Get events from a calendar. - - This is a convenience method that searches for VEVENT objects in the - calendar, optionally filtered by date range. - - Args: - calendar: Calendar to search - start: Start of date range (optional) - end: End of date range (optional) - - Returns: - List of Event objects. - - Example: - from datetime import datetime - events = client.get_events( - calendar, - start=datetime(2024, 1, 1), - end=datetime(2024, 12, 31) - ) - """ - return self.search_calendar(calendar, event=True, start=start, end=end) - - def get_todos( - self, - calendar: Calendar, - include_completed: bool = False, - ) -> list["Todo"]: - """Get todos from a calendar. - - Args: - calendar: Calendar to search - include_completed: Whether to include completed todos - - Returns: - List of Todo objects. - """ - return self.search_calendar(calendar, todo=True, include_completed=include_completed) - def search_calendar( self, calendar: Calendar, diff --git a/caldav/davobject.py b/caldav/davobject.py index 444ebcbc..d43352c3 100644 --- a/caldav/davobject.py +++ b/caldav/davobject.py @@ -171,6 +171,26 @@ def children(self, type: str | None = None) -> list[tuple[URL, Any, Any]]: ## the properties we've already fetched return c + def _build_xml_body(self, root) -> bytes | str: + """Serialize a DAV element (or raw bytes/str) to a request body.""" + if root: + if hasattr(root, "xmlelement"): + return etree.tostring( + root.xmlelement(), + encoding="utf-8", + xml_declaration=True, + pretty_print=error.debug_dump_communication, + ) + return root + return "" + + def _build_propfind_root(self, props): + """Build a Propfind XML element from a list of props, or return None.""" + if props is not None and len(props) > 0: + prop = dav.Prop() + props + return dav.Propfind() + prop + return None + def _query_properties(self, props: Sequence[BaseElement] | None = None, depth: int = 0): """ This is an internal method for doing a propfind query. It's a @@ -182,25 +202,13 @@ def _query_properties(self, props: Sequence[BaseElement] | None = None, depth: i if self.is_async_client: return self._async_query_properties(props, depth) - root = None - # build the propfind request - if props is not None and len(props) > 0: - prop = dav.Prop() + props - root = dav.Propfind() + prop - - return self._query(root, depth) + return self._query(self._build_propfind_root(props), depth) async def _async_query_properties( self, props: Sequence[BaseElement] | None = None, depth: int = 0 ): """Async implementation of _query_properties.""" - root = None - # build the propfind request - if props is not None and len(props) > 0: - prop = dav.Prop() + props - root = dav.Propfind() + prop - - return await self._async_query(root, depth) + return await self._async_query(self._build_propfind_root(props), depth) def _query( self, @@ -220,17 +228,7 @@ def _query( if self.is_async_client: return self._async_query(root, depth, query_method, url, expected_return_value) - body = "" - if root: - if hasattr(root, "xmlelement"): - body = etree.tostring( - root.xmlelement(), - encoding="utf-8", - xml_declaration=True, - pretty_print=error.debug_dump_communication, - ) - else: - body = root + body = self._build_xml_body(root) if url is None: url = self.url ret = getattr(self.client, query_method)(url, body, depth) @@ -257,17 +255,7 @@ async def _async_query( expected_return_value=None, ): """Async implementation of _query.""" - body = "" - if root: - if hasattr(root, "xmlelement"): - body = etree.tostring( - root.xmlelement(), - encoding="utf-8", - xml_declaration=True, - pretty_print=error.debug_dump_communication, - ) - else: - body = root + body = self._build_xml_body(root) if url is None: url = self.url ret = await getattr(self.client, query_method)(url, body, depth) @@ -493,18 +481,7 @@ def set_properties(self, props: Any | None = None) -> Self: prop = dav.Prop() + props set_elem = dav.Set() + prop root = dav.PropertyUpdate() + set_elem - - body = "" - if root: - if hasattr(root, "xmlelement"): - body = etree.tostring( - root.xmlelement(), - encoding="utf-8", - xml_declaration=True, - pretty_print=error.debug_dump_communication, - ) - else: - body = root + body = self._build_xml_body(root) if self.url is None: raise ValueError("Unexpected value None for self.url") @@ -524,18 +501,7 @@ async def _async_set_properties(self, props: Any | None = None) -> Self: prop = dav.Prop() + props set_elem = dav.Set() + prop root = dav.PropertyUpdate() + set_elem - - body = "" - if root: - if hasattr(root, "xmlelement"): - body = etree.tostring( - root.xmlelement(), - encoding="utf-8", - xml_declaration=True, - pretty_print=error.debug_dump_communication, - ) - else: - body = root + body = self._build_xml_body(root) if self.url is None: raise ValueError("Unexpected value None for self.url") From 2153c92a3082facee446bfa2ec34ccc48af6773b Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 14:12:48 +0100 Subject: [PATCH 04/23] Update V3_CODE_REVIEW.md to reflect sync/async deduplication (commit f7c51c9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mark recommendation 11 as done; update §5 duplication tables to show which items are fixed and revised line-count estimates. Co-Authored-By: Claude --- docs/design/V3_CODE_REVIEW.md | 43 +++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index cf697848..c12706f4 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -7,6 +7,9 @@ > **Status update (commit ff6db30):** All "Must Fix" and "Should Fix" items from §8 have been > resolved. Remaining open items are v3.1+ and v4.0 work. +> +> **Status update (commit f7c51c9):** Recommendation 11 largely addressed — sync/async +> duplication reduced by ~177 lines across five files (see §5 for details). ## Executive Summary @@ -17,7 +20,7 @@ The v3.0 release is a major architectural refactoring introducing Sans-I/O separ **Key findings:** - ~~3 bugs that will cause runtime errors~~ **fixed** - ~~1 security concern (UUID1 leaks MAC address in calendar UIDs)~~ **fixed** -- ~650 lines of sync/async duplication across domain objects +- ~~650 lines of sync/async duplication across domain objects~~ reduced to ~475 lines (commit f7c51c9) - Test coverage gaps in discovery module and sync client unit tests - ~~`breakpoint()` left in production code~~ **fixed** @@ -214,26 +217,26 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he ## 5. Code Duplication Analysis -### 5.1 Sync/Async Client Duplication (~70 lines) +### 5.1 Sync/Async Client Duplication (~30 lines remaining) -| Code Section | davclient.py | async_davclient.py | Similarity | +| Code Section | davclient.py | async_davclient.py | Similarity | Status | +|---|---|---|---|---| +| `search_principals` | 376-435 | 1107-1168 | ~95% (copy-paste + await) | **FIXED** — build/parse hoisted to `BaseDAVClient` | +| `_get_calendar_home_set` | 548-568 | 974-994 | ~95% | open | +| `get_events` | 570-597 | 996-1023 | ~95% | **FIXED** — hoisted to `BaseDAVClient` | +| `get_todos` | 599-613 | 1025-1039 | ~95% | **FIXED** — hoisted to `BaseDAVClient` | +| `propfind` response parsing | 280-320 | 750-790 | ~90% | open | +| Auth type extraction | 180-210 | 420-450 | ~100% | already in `BaseDAVClient` | +| Factory functions | 1015-1078 | 1312-1431 | ~80% | open | + +### 5.2 Domain Object Async/Sync Duplication (~445 lines remaining) + +| File | Duplicated pairs | Approx. lines | Notes | |---|---|---|---| -| `search_principals` | 376-435 | 1107-1168 | ~95% (copy-paste + await) | -| `_get_calendar_home_set` | 548-568 | 974-994 | ~95% | -| `get_events` | 570-597 | 996-1023 | ~95% | -| `get_todos` | 599-613 | 1025-1039 | ~95% | -| `propfind` response parsing | 280-320 | 750-790 | ~90% | -| Auth type extraction | 180-210 | 420-450 | ~100% | -| Factory functions | 1015-1078 | 1312-1431 | ~80% | - -### 5.2 Domain Object Async/Sync Duplication (~580 lines) - -| File | Duplicated pairs | Approx. lines | -|---|---|---| -| davobject.py | 6 method pairs | ~180 | -| collection.py | 8 method pairs | ~250 | -| calendarobjectresource.py | 4 method pairs | ~100 | -| search.py | 2 method pairs | ~50 | +| davobject.py | 6 method pairs | ~~180~~ ~140 | `_build_xml_body` / `_build_propfind_root` extracted; `_query`/`_async_query` and `set_properties`/`_async_set_properties` shortened; `_query_properties`/`_async_query_properties` each now 1 line | +| collection.py | 8 method pairs | ~~250~~ ~215 | `CalendarSet.get_calendars` sync migrated to protocol layer; shared `_calendars_from_results` eliminates 30+ lines of duplication | +| calendarobjectresource.py | 4 method pairs | ~100 | open | +| search.py | 2 method pairs | ~50 | open | ### 5.3 Protocol/Response Duplication @@ -300,7 +303,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he ### For v3.1+ -11. Reduce sync/async client duplication (move `search_principals`, `get_events`, `get_todos` to operations layer) +11. ~~Reduce sync/async client duplication (`search_principals`, `get_events`, `get_todos`)~~ **DONE** — all three hoisted to `BaseDAVClient`; XML build/parse extracted to `_build_principal_search_query` / `_parse_principal_search_response`; `_build_xml_body` / `_build_propfind_root` helpers added to `DAVObject`; `CalendarSet.get_calendars` sync path migrated to protocol layer 12. Consolidate `response.py` and `protocol/xml_parsers.py` duplication 13. Add sync DAVClient unit tests mirroring async test structure 14. Add discovery module tests From ad042bb952c6c76e94eb51c81f582eb69692a118 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 14:23:44 +0100 Subject: [PATCH 05/23] Fix dead code, CalendarInfo collision, and response/xml_parsers duplication MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dead code (xml_builders.py): - Delete _build_freebusy_query_body, _build_mkcol_body, _to_utc_date_string — confirmed zero callers across the codebase. The _to_utc_date_string in xml_builders was a never-wired duplicate of the one in elements/cdav.py. CalendarInfo name collision (protocol/types.py, protocol/__init__.py): - Delete the CalendarInfo dataclass from protocol/types.py and remove it from __init__.py. It was never instantiated anywhere; the real CalendarInfo lives in operations/calendarset_ops.py (different fields, actively used). response.py / xml_parsers.py duplication: - Extract _normalize_href() into xml_parsers.py, consolidating the identical %2540 Confluence workaround + absolute-URL-to-path conversion from both _parse_response_element (xml_parsers) and _parse_response (response.py). - _parse_response_element now calls _normalize_href (4 lines -> 1). - response.py._strip_to_multistatus now delegates to the xml_parsers _strip_to_multistatus function instead of reimplementing it. - response.py.validate_status now delegates to xml_parsers._validate_status. - response.py._parse_response now calls _normalize_href; the trailing absolute-URL conversion block is removed (now inside _normalize_href). - Drop the unused unquote and URL imports from response.py. The propstat-loop duplication (_find_objects_and_props vs _extract_properties) is intentionally left: _find_objects_and_props must return raw _Element objects for expand_simple_props() to work, while _extract_properties returns plain Python values. Converging them would require a public API change. All 460 unit tests pass. Co-Authored-By: Claude --- caldav/protocol/__init__.py | 2 - caldav/protocol/types.py | 22 --------- caldav/protocol/xml_builders.py | 82 --------------------------------- caldav/protocol/xml_parsers.py | 28 +++++++---- caldav/response.py | 36 ++++----------- 5 files changed, 30 insertions(+), 140 deletions(-) diff --git a/caldav/protocol/__init__.py b/caldav/protocol/__init__.py index 74c89eaa..ddf68acc 100644 --- a/caldav/protocol/__init__.py +++ b/caldav/protocol/__init__.py @@ -17,7 +17,6 @@ """ from .types import ( - CalendarInfo, CalendarQueryResult, DAVMethod, DAVRequest, @@ -36,7 +35,6 @@ "DAVRequest", "DAVResponse", # Result types - "CalendarInfo", "CalendarQueryResult", "MultiGetResult", "MultistatusResponse", diff --git a/caldav/protocol/types.py b/caldav/protocol/types.py index 457db79d..13697e1a 100644 --- a/caldav/protocol/types.py +++ b/caldav/protocol/types.py @@ -219,25 +219,3 @@ class PrincipalInfo: calendar_home_set: str | None = None displayname: str | None = None calendar_user_address_set: list[str] = field(default_factory=list) - - -@dataclass -class CalendarInfo: - """ - Information about a calendar collection. - - Attributes: - url: Calendar URL - displayname: Display name - description: Calendar description - color: Calendar color (vendor extension) - supported_components: List of supported component types (VEVENT, VTODO, etc.) - ctag: Calendar CTag for change detection - """ - - url: str - displayname: str | None = None - description: str | None = None - color: str | None = None - supported_components: list[str] = field(default_factory=list) - ctag: str | None = None diff --git a/caldav/protocol/xml_builders.py b/caldav/protocol/xml_builders.py index f744d1eb..6009ca83 100644 --- a/caldav/protocol/xml_builders.py +++ b/caldav/protocol/xml_builders.py @@ -235,25 +235,6 @@ def _build_sync_collection_body( return etree.tostring(sync_collection.xmlelement(), encoding="utf-8", xml_declaration=True) -def _build_freebusy_query_body( - start: datetime, - end: datetime, -) -> bytes: - """ - Build free-busy-query REPORT request body. - - Args: - start: Start of free-busy period - end: End of free-busy period - - Returns: - UTF-8 encoded XML bytes - """ - root = cdav.FreeBusyQuery() + [cdav.TimeRange(start, end)] - - return etree.tostring(root.xmlelement(), encoding="utf-8", xml_declaration=True) - - def _build_mkcalendar_body( displayname: str | None = None, description: str | None = None, @@ -298,39 +279,6 @@ def _build_mkcalendar_body( return etree.tostring(mkcalendar.xmlelement(), encoding="utf-8", xml_declaration=True) -def _build_mkcol_body( - displayname: str | None = None, - resource_types: list[BaseElement] | None = None, -) -> bytes: - """ - Build MKCOL (extended) request body. - - Args: - displayname: Collection display name - resource_types: List of resource type elements - - Returns: - UTF-8 encoded XML bytes - """ - prop = dav.Prop() - - if displayname: - prop += dav.DisplayName(displayname) - - if resource_types: - rt = dav.ResourceType() - for rt_elem in resource_types: - rt += rt_elem - prop += rt - else: - prop += dav.ResourceType() + dav.Collection() - - set_elem = dav.Set() + prop - mkcol = dav.Mkcol() + set_elem - - return etree.tostring(mkcol.xmlelement(), encoding="utf-8", xml_declaration=True) - - # Property name to element mapping @@ -396,33 +344,3 @@ def _prop_name_to_element(name: str, value: Any | None = None) -> BaseElement | return cls() return None - - -def _to_utc_date_string(ts: datetime) -> str: - """ - Convert datetime to UTC date string for CalDAV. - - Args: - ts: datetime object (may or may not have timezone) - - Returns: - UTC date string in format YYYYMMDDTHHMMSSZ - """ - from datetime import timezone - - utc_tz = timezone.utc - - if ts.tzinfo is None: - # Assume local time, convert to UTC - try: - ts = ts.astimezone(utc_tz) - except Exception: - # For very old Python versions or edge cases - import tzlocal - - ts = ts.replace(tzinfo=tzlocal.get_localzone()) - ts = ts.astimezone(utc_tz) - else: - ts = ts.astimezone(utc_tz) - - return ts.strftime("%Y%m%dT%H%M%SZ") diff --git a/caldav/protocol/xml_parsers.py b/caldav/protocol/xml_parsers.py index 1876ac22..c14452ec 100644 --- a/caldav/protocol/xml_parsers.py +++ b/caldav/protocol/xml_parsers.py @@ -263,6 +263,25 @@ def _parse_calendar_multiget_response( # Helper functions +def _normalize_href(text: str) -> str: + """ + Normalize an href string from a DAV response element. + + Handles the Confluence double-encoding bug (%2540 → %40) and converts + absolute URLs to path-only strings so callers always work with paths. + """ + # Fix for https://github.com/python-caldav/caldav/issues/471 + # Confluence server quotes the user email twice. + if "%2540" in text: + text = text.replace("%2540", "%40") + href = unquote(text) + # Ref https://github.com/python-caldav/caldav/issues/435 + # Some servers return absolute URLs; convert to path. + if ":" in href: + href = unquote(URL(href).path) + return href + + def _strip_to_multistatus(tree: _Element) -> _Element | list[_Element]: """ Strip outer elements to get to the multistatus content. @@ -301,14 +320,7 @@ def _parse_response_element( status = elem.text _validate_status(status) elif elem.tag == dav.Href.tag: - # Fix for double-encoded URLs (e.g., Confluence) - text = elem.text or "" - if "%2540" in text: - text = text.replace("%2540", "%40") - href = unquote(text) - # Convert absolute URLs to paths - if ":" in href: - href = unquote(URL(href).path) + href = _normalize_href(elem.text or "") elif elem.tag == dav.PropStat.tag: propstats.append(elem) diff --git a/caldav/response.py b/caldav/response.py index 0c1d3bfa..0129d30b 100644 --- a/caldav/response.py +++ b/caldav/response.py @@ -9,7 +9,6 @@ import warnings from collections.abc import Iterable from typing import TYPE_CHECKING, Any, cast -from urllib.parse import unquote from lxml import etree from lxml.etree import _Element @@ -18,7 +17,13 @@ from caldav.elements.base import BaseElement from caldav.lib import error from caldav.lib.python_utilities import to_normal_str -from caldav.lib.url import URL +from caldav.protocol.xml_parsers import ( + _normalize_href, + _validate_status, +) +from caldav.protocol.xml_parsers import ( + _strip_to_multistatus as _proto_strip, +) if TYPE_CHECKING: # Protocol for HTTP response objects (works with httpx, niquests, requests) @@ -165,12 +170,7 @@ def _strip_to_multistatus(self) -> _Element | list[_Element]: (The equivalent of this method could probably be found with a simple XPath query, but I'm not much into XPath) """ - tree = self.tree - if tree.tag == "xml" and len(tree) > 0 and tree[0].tag == dav.MultiStatus.tag: - return tree[0] - if tree.tag == dav.MultiStatus.tag: - return self.tree - return [self.tree] + return _proto_strip(self.tree) def validate_status(self, status: str) -> None: """ @@ -181,13 +181,7 @@ def validate_status(self, status: str) -> None: makes sense to me, but I've only seen it from SOGo, and it's not in accordance with the examples in rfc6578. """ - if ( - " 200 " not in status - and " 201 " not in status - and " 207 " not in status - and " 404 " not in status - ): - raise error.ResponseError(status) + _validate_status(status) def _parse_response(self, response: _Element) -> tuple[str, list[_Element], Any | None]: """ @@ -208,11 +202,7 @@ def _parse_response(self, response: _Element) -> tuple[str, list[_Element], Any self.validate_status(status) elif elem.tag == dav.Href.tag: assert not href - # Fix for https://github.com/python-caldav/caldav/issues/471 - # Confluence server quotes the user email twice. We unquote it manually. - if "%2540" in elem.text: - elem.text = elem.text.replace("%2540", "%40") - href = unquote(elem.text) + href = _normalize_href(elem.text or "") elif elem.tag == dav.PropStat.tag: propstats.append(elem) elif elem.tag == "{DAV:}error": @@ -232,12 +222,6 @@ def _parse_response(self, response: _Element) -> tuple[str, list[_Element], Any error.assert_(href) if check_404: error.assert_("404" in status) - ## TODO: is this safe/sane? - ## Ref https://github.com/python-caldav/caldav/issues/435 the paths returned may be absolute URLs, - ## but the caller expects them to be paths. Could we have issues when a server has same path - ## but different URLs for different elements? Perhaps href should always be made into an URL-object? - if ":" in href: - href = unquote(URL(href).path) return (cast(str, href), propstats, status) def _find_objects_and_props(self) -> dict[str, dict[str, _Element]]: From 8e5cac817358e863310fcd7e2585f3ee25803fc6 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 14:24:27 +0100 Subject: [PATCH 06/23] Update V3_CODE_REVIEW.md to reflect dead-code/collision/duplication fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mark §1.1 issues 2, 3, 4 as fixed; upgrade response.py rating from 6 to 7. Co-Authored-By: Claude --- docs/design/V3_CODE_REVIEW.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index c12706f4..30bd0fea 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -10,6 +10,9 @@ > > **Status update (commit f7c51c9):** Recommendation 11 largely addressed — sync/async > duplication reduced by ~177 lines across five files (see §5 for details). +> +> **Status update (commit ad042bb):** §1.1 issues 2, 3, 4 fixed (dead code deleted, +> CalendarInfo collision resolved, response/xml_parsers duplication reduced). ## Executive Summary @@ -43,11 +46,11 @@ The protocol layer separates XML construction/parsing from I/O. This is the stro 1. ~~**BUG: NameError in `xml_parsers.py:260`**~~ **FIXED** -- `parse_calendar_multiget_response()` was calling `parse_calendar_query_response()` (missing leading underscore). Fixed by adding the `_` prefix. -2. **Dead code in `xml_builders.py`** -- `_to_utc_date_string()` (line 401), `_build_freebusy_query_body()` (line 189), and `_build_mkcol_body()` (line 200) have zero callers. +2. ~~**Dead code in `xml_builders.py`**~~ **FIXED** -- Deleted `_build_freebusy_query_body`, `_build_mkcol_body`, and `_to_utc_date_string` (the last was a never-wired duplicate of the real `_to_utc_date_string` in `elements/cdav.py`). -3. **`CalendarInfo` name collision** -- `protocol/types.py:149` and `operations/calendarset_ops.py:24` define different dataclasses named `CalendarInfo` with different fields. Both are exported from their respective `__init__.py`. +3. ~~**`CalendarInfo` name collision**~~ **FIXED** -- Deleted `CalendarInfo` from `protocol/types.py` (it was never instantiated); the canonical one in `operations/calendarset_ops.py` is unaffected. -4. **Heavy duplication with `response.py`** -- Multistatus stripping, status validation, response element parsing, and the Confluence `%2540` workaround are duplicated nearly verbatim between `xml_parsers.py` and `response.py`. +4. ~~**Heavy duplication with `response.py`**~~ **PARTIALLY FIXED** -- Extracted `_normalize_href()` (Confluence %2540 fix + absolute-URL-to-path) into `xml_parsers.py`; `response.py._strip_to_multistatus` and `validate_status` now delegate to their `xml_parsers` counterparts. The propstat-loop duplication (`_find_objects_and_props` vs `_extract_properties`) is intentionally left: converging them requires changing the return type of the public `expand_simple_props()` API. ### 1.2 Operations Layer (`caldav/operations/`) @@ -79,7 +82,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. ### 1.4 Response Handling (`caldav/response.py`) -**Rating: 6/10** -- `BaseDAVResponse` provides shared XML parsing for sync/async clients, but has significant duplication with the protocol layer and thread-unsafe mutable state. +**Rating: 7/10** -- `BaseDAVResponse` provides shared XML parsing for sync/async clients. The main duplication with the protocol layer has been reduced; remaining open item is thread-unsafe mutable state. **Issues found:** From f5c242dc446cefd7b719800e0f18ae01e1a07135 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 15:09:23 +0100 Subject: [PATCH 07/23] =?UTF-8?q?Update=20V3=5FCODE=5FREVIEW.md:=20=C2=A75?= =?UTF-8?q?.3=20detail,=20=C2=A78=20items=2012+16,=20Appendix=20A=20line?= =?UTF-8?q?=20counts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - §1.1 table: update line counts for types.py (221), xml_builders.py (346), xml_parsers.py (467); bump xml_parsers.py rating from 5/10 to 6/10 - §5.3: replace one-liner with a table showing which of the five duplicated pieces are now fixed vs still open, and why - §8: strike through item 16 (dead code removed); mark item 12 as partially done with a pointer to §5.3 - Appendix A: update line counts for response.py, types.py, xml_builders.py, xml_parsers.py to reflect current state Co-Authored-By: Claude --- docs/design/V3_CODE_REVIEW.md | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index 30bd0fea..22cdbff8 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -37,9 +37,9 @@ The protocol layer separates XML construction/parsing from I/O. This is the stro | File | Lines | Rating | Purpose | |------|-------|--------|---------| -| `types.py` | 243 | 9/10 | Frozen dataclasses: DAVRequest, DAVResponse, PropfindResult, CalendarQueryResult | -| `xml_builders.py` | 428 | 7/10 | Pure functions building XML for PROPFIND, calendar-query, MKCALENDAR, etc. | -| `xml_parsers.py` | 455 | 5/10 | Parse XML responses into typed results | +| `types.py` | ~~243~~ 221 | 9/10 | Frozen dataclasses: DAVRequest, DAVResponse, PropfindResult, CalendarQueryResult | +| `xml_builders.py` | ~~428~~ 346 | 7/10 | Pure functions building XML for PROPFIND, calendar-query, MKCALENDAR, etc. | +| `xml_parsers.py` | ~~455~~ 467 | 6/10 | Parse XML responses into typed results | | `__init__.py` | 46 | 8/10 | Clean re-exports | **Issues found:** @@ -241,9 +241,17 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he | calendarobjectresource.py | 4 method pairs | ~100 | open | | search.py | 2 method pairs | ~50 | open | -### 5.3 Protocol/Response Duplication +### 5.3 Protocol/Response Duplication (partially resolved) -`response.py` and `protocol/xml_parsers.py` share five pieces of nearly identical logic (multistatus stripping, status validation, response element parsing, `%2540` workaround, propstat loops). +`response.py` and `protocol/xml_parsers.py` originally shared five pieces of nearly identical logic. Three have been consolidated: + +| Piece | Status | +|---|---| +| Multistatus stripping | **FIXED** — `response._strip_to_multistatus` now delegates to `xml_parsers._strip_to_multistatus` | +| Status validation | **FIXED** — `response.validate_status` now delegates to `xml_parsers._validate_status` | +| `%2540` Confluence workaround + absolute-URL-to-path | **FIXED** — extracted as `xml_parsers._normalize_href`; called from both `_parse_response_element` and `response._parse_response` | +| Response element parsing (`_parse_response` vs `_parse_response_element`) | open — too much divergence in error handling to merge without risk | +| Propstat loops (`_find_objects_and_props` vs `_extract_properties`) | open — `_find_objects_and_props` must return raw `_Element` objects for `expand_simple_props()`; merging requires a public API change | --- @@ -307,11 +315,11 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he ### For v3.1+ 11. ~~Reduce sync/async client duplication (`search_principals`, `get_events`, `get_todos`)~~ **DONE** — all three hoisted to `BaseDAVClient`; XML build/parse extracted to `_build_principal_search_query` / `_parse_principal_search_response`; `_build_xml_body` / `_build_propfind_root` helpers added to `DAVObject`; `CalendarSet.get_calendars` sync path migrated to protocol layer -12. Consolidate `response.py` and `protocol/xml_parsers.py` duplication +12. ~~Consolidate `response.py` and `protocol/xml_parsers.py` duplication~~ **PARTIALLY DONE** — three of five duplicated pieces consolidated (see §5.3); response-element parsing and propstat loops remain 13. Add sync DAVClient unit tests mirroring async test structure 14. Add discovery module tests 15. Add missing `warnings.warn()` to all deprecated methods -16. Remove dead code in `xml_builders.py` +16. ~~Remove dead code in `xml_builders.py`~~ **DONE** — deleted `_build_freebusy_query_body`, `_build_mkcol_body`, `_to_utc_date_string` 17. Move `_auto_url` from `davclient.py` to shared module (also fixes event-loop blocking in async client) 18. Make `search_ops._build_search_xml_query` not mutate its input 19. Fix `NotImplementedError` for auth failures in `davclient.py` -- raise `AuthorizationError` instead @@ -332,13 +340,13 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he |------|-------|---------| | `caldav/async_davclient.py` | 1,431 | Async HTTP client | | `caldav/base_client.py` | 480 | Shared client ABC | -| `caldav/response.py` | 390 | Shared response parsing | +| `caldav/response.py` | ~~390~~ 374 | Shared response parsing | | `caldav/datastate.py` | 246 | Data representation state machine | | `caldav/aio.py` | 93 | Async entry point | | `caldav/lib/auth.py` | 69 | Shared auth utilities | -| `caldav/protocol/types.py` | 243 | Request/response dataclasses | -| `caldav/protocol/xml_builders.py` | 428 | XML construction | -| `caldav/protocol/xml_parsers.py` | 455 | XML parsing | +| `caldav/protocol/types.py` | ~~243~~ 221 | Request/response dataclasses | +| `caldav/protocol/xml_builders.py` | ~~428~~ 346 | XML construction | +| `caldav/protocol/xml_parsers.py` | ~~455~~ 467 | XML parsing | | `caldav/operations/base.py` | 189 | Query specifications | | `caldav/operations/search_ops.py` | 445 | Search query building | | `caldav/operations/calendarobject_ops.py` | 531 | Calendar object ops | From 01b33528026dc83593591544193839df4f3f0151 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 15:34:34 +0100 Subject: [PATCH 08/23] Extract _quote_uid() helper to eliminate duplicate URL-quoting logic The pattern quote(uid.replace("/", "%2F")) was duplicated in both _generate_url() and _find_id_and_path() in calendarobject_ops.py. Extracted into a single _quote_uid() helper, both callers now use it. Co-Authored-By: Claude --- caldav/operations/calendarobject_ops.py | 15 ++++++++++++--- docs/design/V3_CODE_REVIEW.md | 2 +- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/caldav/operations/calendarobject_ops.py b/caldav/operations/calendarobject_ops.py index 8c95c676..fa7ff54f 100644 --- a/caldav/operations/calendarobject_ops.py +++ b/caldav/operations/calendarobject_ops.py @@ -45,6 +45,16 @@ def _generate_uid() -> str: return str(uuid.uuid4()) +def _quote_uid(uid: str) -> str: + """ + URL-quote a UID for use in a CalDAV object URL. + + Slashes are double-quoted (replaced with %2F before percent-encoding) + per https://github.com/python-caldav/caldav/issues/143. + """ + return quote(uid.replace("/", "%2F")) + + def _generate_url(parent_url: str, uid: str) -> str: """ Generate a URL for a calendar object based on its UID. @@ -58,8 +68,7 @@ def _generate_url(parent_url: str, uid: str) -> str: Returns: Full URL for the calendar object """ - # Double-quote slashes per https://github.com/python-caldav/caldav/issues/143 - quoted_uid = quote(uid.replace("/", "%2F")) + quoted_uid = _quote_uid(uid) if not parent_url.endswith("/"): parent_url += "/" return f"{parent_url}{quoted_uid}.ics" @@ -135,7 +144,7 @@ def _find_id_and_path( if given_path: path = given_path else: - path = quote(uid.replace("/", "%2F")) + ".ics" + path = _quote_uid(uid) + ".ics" return uid, path diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index 22cdbff8..e15ab415 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -74,7 +74,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. 3. ~~**MD5 in FIPS environments (`calendar_ops.py:132`)**~~ **FIXED** -- Added `usedforsecurity=False` to both `calendar_ops.py` and `collection.py`. -4. **Duplicate URL quoting** -- `quote(uid.replace("/", "%2F"))` pattern appears at both lines 62 and 138 in `calendarobject_ops.py`. +4. ~~**Duplicate URL quoting**~~ **FIXED** -- Extracted `_quote_uid()` helper in `calendarobject_ops.py`; both `_generate_url` and `_find_id_and_path` now call it instead of repeating the pattern inline. ### 1.3 Data State Management (`caldav/datastate.py`) From 7e7919e52c2e598de5160c0a656c69b7829017e2 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 20:25:44 +0100 Subject: [PATCH 09/23] Add Stalwart to Docker test server framework; fix expand_simple_props return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stalwart (https://stalw.art/) is an all-in-one mail & collaboration server that added CalDAV/CardDAV support in 2024/2025. Adds the full Docker test server integration: - tests/docker-test-servers/stalwart/: docker-compose.yml (port 8809), start.sh, stop.sh, setup_stalwart.sh (creates domain + user via REST API) - tests/test_servers/docker.py: StalwartTestServer class - caldav/compatibility_hints.py: empty stalwart hints dict (to be filled after test runs) - tests/caldav_test_servers.yaml.example: stalwart entry Setup notes: - Admin password set via STALWART_ADMIN_PASSWORD env var - Domain must be created before user (POST /api/principal) - CalDAV home: /dav/cal// - Startup takes ~25s; healthcheck start_period accounts for this Also fixes a missing `return` in the expand_simple_props wrapper introduced by the _expand_simple_props refactor — without it every calendar-query REPORT result was discarded (returned None), causing TypeError on iteration. Co-Authored-By: Claude --- caldav/compatibility_hints.py | 7 ++ caldav/response.py | 8 +- tests/caldav_test_servers.yaml.example | 8 ++ .../stalwart/docker-compose.yml | 18 +++ .../stalwart/setup_stalwart.sh | 106 ++++++++++++++++++ tests/docker-test-servers/stalwart/start.sh | 23 ++++ tests/docker-test-servers/stalwart/stop.sh | 12 ++ tests/test_servers/docker.py | 32 +++++- 8 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 tests/docker-test-servers/stalwart/docker-compose.yml create mode 100755 tests/docker-test-servers/stalwart/setup_stalwart.sh create mode 100755 tests/docker-test-servers/stalwart/start.sh create mode 100755 tests/docker-test-servers/stalwart/stop.sh diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index 14e278cb..ead7f9e5 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -1362,6 +1362,13 @@ def dotted_feature_set_list(self, compact=False): ], } +## Stalwart - all-in-one mail & collaboration server (CalDAV added 2024/2025) +## https://stalw.art/ +## CalDAV served at /dav/cal// over HTTP on port 8080. +## Feature support mostly unknown until tested; starting with empty hints. +stalwart = { +} + ## Lots of transient problems with purelymail purelymail = { ## Purelymail claims that the search indexes are "lazily" populated, diff --git a/caldav/response.py b/caldav/response.py index 0129d30b..901f1223 100644 --- a/caldav/response.py +++ b/caldav/response.py @@ -224,6 +224,8 @@ def _parse_response(self, response: _Element) -> tuple[str, list[_Element], Any error.assert_("404" in status) return (cast(str, href), propstats, status) + ## TODO: there is currently quite some overlapping with the protocol.xml_parsers + ## we should refactor def _find_objects_and_props(self) -> dict[str, dict[str, _Element]]: """Internal implementation of find_objects_and_props without deprecation warning.""" self.objects: dict[str, dict[str, _Element]] = {} @@ -337,7 +339,11 @@ def _expand_simple_prop( return values[0] ## TODO: word "expand" does not feel quite right. - def expand_simple_props( + ## TODO: I'm considering to deprecate this in v4 + def expand_simple_props(self, *largs, **kwargs) -> dict[str, dict[str, str]]: + return self._expand_simple_props(*largs, **kwargs) + + def _expand_simple_props( self, props: Iterable[BaseElement] | None = None, multi_value_props: Iterable[Any] | None = None, diff --git a/tests/caldav_test_servers.yaml.example b/tests/caldav_test_servers.yaml.example index 48fa1ccb..ee9efcf1 100644 --- a/tests/caldav_test_servers.yaml.example +++ b/tests/caldav_test_servers.yaml.example @@ -109,6 +109,14 @@ test-servers: username: ${ZIMBRA_USERNAME:-testuser@zimbra.io} password: ${ZIMBRA_PASSWORD:-testpass} + stalwart: + type: docker + enabled: ${TEST_STALWART:-false} + host: ${STALWART_HOST:-localhost} + port: ${STALWART_PORT:-8809} + username: ${STALWART_USERNAME:-testuser} + password: ${STALWART_PASSWORD:-testpass} + # ========================================================================= # External/private servers (your own CalDAV server) # ========================================================================= diff --git a/tests/docker-test-servers/stalwart/docker-compose.yml b/tests/docker-test-servers/stalwart/docker-compose.yml new file mode 100644 index 00000000..f6682537 --- /dev/null +++ b/tests/docker-test-servers/stalwart/docker-compose.yml @@ -0,0 +1,18 @@ +services: + stalwart: + image: stalwartlabs/stalwart:latest + container_name: stalwart-test + ports: + - "8809:8080" + environment: + - STALWART_ADMIN_PASSWORD=adminpass + tmpfs: + - /opt/stalwart/data:size=500m + - /opt/stalwart/logs:size=50m + - /opt/stalwart/etc:size=10m + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8080/"] + interval: 5s + timeout: 3s + retries: 15 + start_period: 25s diff --git a/tests/docker-test-servers/stalwart/setup_stalwart.sh b/tests/docker-test-servers/stalwart/setup_stalwart.sh new file mode 100755 index 00000000..4fddb0d6 --- /dev/null +++ b/tests/docker-test-servers/stalwart/setup_stalwart.sh @@ -0,0 +1,106 @@ +#!/bin/bash +# Setup script for Stalwart test server. +# Creates the test domain and user via the management REST API. +# +# Stalwart requires: +# 1. A domain principal before a user can be created with that email. +# 2. A user principal with a plain-text secret (Stalwart hashes it internally). +# +# CalDAV is served at /dav/cal// over plain HTTP on port 8080. + +set -e + +CONTAINER_NAME="stalwart-test" +HOST_PORT="${STALWART_PORT:-8809}" +ADMIN_USER="admin" +ADMIN_PASSWORD="adminpass" +DOMAIN="example.org" +TEST_USER="${STALWART_USERNAME:-testuser}" +TEST_PASSWORD="${STALWART_PASSWORD:-testpass}" +API_BASE="http://localhost:${HOST_PORT}/api" + +api_post() { + local endpoint="$1" + local body="$2" + curl -s -X POST "${API_BASE}${endpoint}" \ + -H "Content-Type: application/json" \ + -H "Accept: application/json" \ + -u "${ADMIN_USER}:${ADMIN_PASSWORD}" \ + -d "${body}" +} + +echo "Waiting for Stalwart HTTP endpoint to be ready..." +max_attempts=60 +for i in $(seq 1 $max_attempts); do + if curl -s -o /dev/null -w "%{http_code}" "http://localhost:${HOST_PORT}/" 2>/dev/null | grep -q "200"; then + echo "Stalwart is ready" + break + fi + if [ $i -eq $max_attempts ]; then + echo "Stalwart did not start in time" + docker logs "$CONTAINER_NAME" 2>&1 | tail -20 + exit 1 + fi + echo -n "." + sleep 1 +done + +echo "" +echo "Creating domain '${DOMAIN}'..." +RESULT=$(api_post "/principal" "{\"type\": \"domain\", \"name\": \"${DOMAIN}\"}") +if echo "$RESULT" | grep -q '"error"'; then + if echo "$RESULT" | grep -q '"fieldAlreadyExists"'; then + echo "Domain already exists (OK)" + else + echo "Warning: domain creation returned: $RESULT" + fi +fi + +echo "Creating test user '${TEST_USER}'..." +RESULT=$(api_post "/principal" "{ + \"type\": \"individual\", + \"name\": \"${TEST_USER}\", + \"secrets\": [\"${TEST_PASSWORD}\"], + \"emails\": [\"${TEST_USER}@${DOMAIN}\"], + \"roles\": [\"user\"] +}") +if echo "$RESULT" | grep -q '"error"'; then + if echo "$RESULT" | grep -q '"fieldAlreadyExists"'; then + echo "User already exists (OK)" + else + echo "Error creating user: $RESULT" + exit 1 + fi +else + echo "User created: $RESULT" +fi + +echo "" +echo "Verifying CalDAV access..." +max_caldav_attempts=15 +for i in $(seq 1 $max_caldav_attempts); do + RESPONSE=$(curl -s -X PROPFIND \ + -H "Depth: 0" \ + -u "${TEST_USER}:${TEST_PASSWORD}" \ + "http://localhost:${HOST_PORT}/dav/cal/${TEST_USER}/" 2>/dev/null) + if echo "$RESPONSE" | grep -qi "multistatus\|collection"; then + echo "CalDAV is accessible" + break + fi + if [ $i -eq $max_caldav_attempts ]; then + echo "Warning: CalDAV access test failed after ${max_caldav_attempts} attempts" + echo "Response: $RESPONSE" + echo "Continuing anyway..." + break + fi + echo -n "." + sleep 2 +done + +echo "" +echo "Stalwart setup complete!" +echo "" +echo "Credentials:" +echo " Admin: ${ADMIN_USER} / ${ADMIN_PASSWORD} (web UI: http://localhost:${HOST_PORT})" +echo " Test user: ${TEST_USER} / ${TEST_PASSWORD}" +echo " CalDAV URL: http://localhost:${HOST_PORT}/dav/cal/${TEST_USER}/" diff --git a/tests/docker-test-servers/stalwart/start.sh b/tests/docker-test-servers/stalwart/start.sh new file mode 100755 index 00000000..b9e7d0fc --- /dev/null +++ b/tests/docker-test-servers/stalwart/start.sh @@ -0,0 +1,23 @@ +#!/bin/bash +# Quick start script for Stalwart test server +# +# Usage: ./start.sh + +set -e + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cd "$SCRIPT_DIR" + +echo "Creating and starting Stalwart container..." +docker-compose up -d + +echo "Running setup (waits for HTTP readiness, creates domain and test user)..." +bash "$SCRIPT_DIR/setup_stalwart.sh" + +echo "" +echo "Run tests from project root:" +echo " cd ../../.." +echo " TEST_STALWART=true pytest" +echo "" +echo "To stop Stalwart: ./stop.sh" +echo "To view logs: docker-compose logs -f stalwart" diff --git a/tests/docker-test-servers/stalwart/stop.sh b/tests/docker-test-servers/stalwart/stop.sh new file mode 100755 index 00000000..05760c1f --- /dev/null +++ b/tests/docker-test-servers/stalwart/stop.sh @@ -0,0 +1,12 @@ +#!/bin/bash +# Stop script for Stalwart test server + +set -e + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cd "$SCRIPT_DIR" + +echo "Stopping Stalwart and removing volumes..." +docker-compose down -v + +echo "Stalwart stopped and volumes removed" diff --git a/tests/test_servers/docker.py b/tests/test_servers/docker.py index 645dfd0c..ce6f7860 100644 --- a/tests/test_servers/docker.py +++ b/tests/test_servers/docker.py @@ -2,7 +2,7 @@ Docker-based test server implementations. This module provides test server implementations for servers that run -in Docker containers: Baikal, Nextcloud, Cyrus, SOGo, Bedework, DAViCal, Davis, CCS, and Zimbra. +in Docker containers: Baikal, Nextcloud, Cyrus, SOGo, Bedework, DAViCal, Davis, CCS, Zimbra, and Stalwart. """ import os @@ -349,6 +349,35 @@ def is_accessible(self) -> bool: return False +class StalwartTestServer(DockerTestServer): + """ + Stalwart all-in-one mail & collaboration server in Docker. + + Stalwart added CalDAV/CardDAV support in 2024/2025. Uses plain HTTP on + port 8080 for both the admin interface and CalDAV access. Calendar home + for a user is at /dav/cal//. + """ + + name = "Stalwart" + + def __init__(self, config: dict[str, Any] | None = None) -> None: + config = config or {} + config.setdefault("host", os.environ.get("STALWART_HOST", "localhost")) + config.setdefault("port", int(os.environ.get("STALWART_PORT", "8809"))) + config.setdefault("username", os.environ.get("STALWART_USERNAME", "testuser")) + config.setdefault("password", os.environ.get("STALWART_PASSWORD", "testpass")) + if "features" not in config: + config["features"] = compatibility_hints.stalwart.copy() + super().__init__(config) + + def _default_port(self) -> int: + return 8809 + + @property + def url(self) -> str: + return f"http://{self.host}:{self.port}/dav/cal/{self.username}/" + + # Register server classes register_server_class("baikal", BaikalTestServer) register_server_class("nextcloud", NextcloudTestServer) @@ -359,3 +388,4 @@ def is_accessible(self) -> bool: register_server_class("davis", DavisTestServer) register_server_class("ccs", CCSTestServer) register_server_class("zimbra", ZimbraTestServer) +register_server_class("stalwart", StalwartTestServer) From 547cbf8fda03b6bc9e9b5e585f2f40d68c0eb6cf Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Thu, 26 Feb 2026 20:26:43 +0100 Subject: [PATCH 10/23] code review comments --- docs/design/V3_CODE_REVIEW.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index e15ab415..ba921c4c 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -171,7 +171,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Rating: 6/10** -- Functional but at 2,054 lines is the largest file and could benefit from extraction. **Issues:** -1. **Missing deprecation warnings** -- `calendars()`, `events()`, `todos()` etc. have docstring notes but no `warnings.warn()` calls (unlike `date_search` and `davobject.name` which do emit). +1. **Missing deprecation warnings** -- `calendars()`, `events()`, `todos()` etc. have docstring notes but no `warnings.warn()` calls (unlike `date_search` and `davobject.name` which do emit). *Comment:* This is intentional. Those are "core features" that was suddenly changed between v2 and v3 - we want people to upgrade to v3 without getting a lot of deprecation warnings thrown in their face. Those warnings will be added in v4. 2. ~~**`_generate_fake_sync_token` uses MD5 (line 1655)**~~ **FIXED** -- Added `usedforsecurity=False`. 3. **`Principal._async_get_property` overrides parent (line 352)** with incompatible implementation. From 819fdd7603823df8861dcd847159c1e5a8f08b81 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 11:19:54 +0100 Subject: [PATCH 11/23] Update V3_CODE_REVIEW.md: note residual URL-quoting in calendarobjectresource.py Co-Authored-By: Claude --- docs/design/V3_CODE_REVIEW.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index ba921c4c..fd0d785a 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -74,7 +74,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. 3. ~~**MD5 in FIPS environments (`calendar_ops.py:132`)**~~ **FIXED** -- Added `usedforsecurity=False` to both `calendar_ops.py` and `collection.py`. -4. ~~**Duplicate URL quoting**~~ **FIXED** -- Extracted `_quote_uid()` helper in `calendarobject_ops.py`; both `_generate_url` and `_find_id_and_path` now call it instead of repeating the pattern inline. +4. ~~**Duplicate URL quoting**~~ **FIXED** -- Extracted `_quote_uid()` helper in `calendarobject_ops.py`; both `_generate_url` and `_find_id_and_path` now call it instead of repeating the pattern inline. The same pattern still exists in `calendarobjectresource.py:900` (`_generate_url` OOP method), but lives in a different layer and uses `URL.join` — separate refactor. ### 1.3 Data State Management (`caldav/datastate.py`) From 5aa844739b0040ff04a2779f70caa6541497b5aa Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 13:07:58 +0100 Subject: [PATCH 12/23] Use _quote_uid() in calendarobjectresource._generate_url, drop quote import Removes the last inline quote(uid.replace("/", "%2F")) in favour of the shared _quote_uid() helper from calendarobject_ops. Co-Authored-By: Claude --- caldav/calendarobjectresource.py | 5 +++-- docs/design/V3_CODE_REVIEW.md | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index 1790e6b6..463136e1 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -18,7 +18,7 @@ from collections import defaultdict from datetime import datetime, timedelta, timezone from typing import TYPE_CHECKING, Any, Optional -from urllib.parse import ParseResult, SplitResult, quote +from urllib.parse import ParseResult, SplitResult import icalendar from dateutil.rrule import rrulestr @@ -53,6 +53,7 @@ from .lib.error import errmsg from .lib.python_utilities import to_normal_str, to_unicode, to_wire from .lib.url import URL +from .operations.calendarobject_ops import _quote_uid log = logging.getLogger("caldav") @@ -897,7 +898,7 @@ def _generate_url(self): ## See https://github.com/python-caldav/caldav/issues/143 for the rationale behind double-quoting slashes ## TODO: should try to wrap my head around issues that arises when id contains weird characters. maybe it's ## better to generate a new uuid here, particularly if id is in some unexpected format. - url = self.parent.url.join(quote(self.id.replace("/", "%2F")) + ".ics") + url = self.parent.url.join(_quote_uid(self.id) + ".ics") assert " " not in str(url) return url diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index fd0d785a..f346021f 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -74,7 +74,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. 3. ~~**MD5 in FIPS environments (`calendar_ops.py:132`)**~~ **FIXED** -- Added `usedforsecurity=False` to both `calendar_ops.py` and `collection.py`. -4. ~~**Duplicate URL quoting**~~ **FIXED** -- Extracted `_quote_uid()` helper in `calendarobject_ops.py`; both `_generate_url` and `_find_id_and_path` now call it instead of repeating the pattern inline. The same pattern still exists in `calendarobjectresource.py:900` (`_generate_url` OOP method), but lives in a different layer and uses `URL.join` — separate refactor. +4. ~~**Duplicate URL quoting**~~ **FIXED** -- Extracted `_quote_uid()` helper in `calendarobject_ops.py`; `_generate_url`, `_find_id_and_path`, and `calendarobjectresource.CalendarObjectResource._generate_url` all now call it. `quote` import removed from `calendarobjectresource.py`. ### 1.3 Data State Management (`caldav/datastate.py`) From e49856f064464be7fb4a2c32e42d2a884a1564f8 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 13:10:47 +0100 Subject: [PATCH 13/23] fighting more with tests and compatibility --- caldav/compatibility_hints.py | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index ead7f9e5..a030ea26 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -88,6 +88,8 @@ class FeatureSet: "extra_keys": { "interval": "Rate limiting window, in seconds", "count": "Max number of requests to send within the interval", + "max_sleep": "Max sleep when hitting a 429 or 503 with retry-after, in seconds", + "default_sleep": "Sleep for this long when hitting a 429, in seconds" }}, "search-cache": { "type": "server-peculiarity", @@ -951,8 +953,7 @@ def dotted_feature_set_list(self, compact=False): zimbra = { 'auto-connect.url': {'basepath': '/dav/'}, 'delete-calendar': {'support': 'fragile', 'behaviour': 'may move to trashbin instead of deleting immediately'}, - ## save-load.get-by-url was unsupported in older Zimbra versions (GET to - ## valid calendar object URLs returned 404), but works in zimbra/zcs-foss:latest + 'save-load.get-by-url': {'support': 'fragile', 'behaviour': '404 most of the time - but sometimes 200. Weird, should be investigated more'}, ## Zimbra treats same-UID events across calendars as aliases of the same event 'save.duplicate-uid.cross-calendar': {'support': 'unsupported'}, 'search.recurrences.expanded.exception': {'support': 'unsupported'}, ## TODO: verify @@ -1024,22 +1025,10 @@ def dotted_feature_set_list(self, compact=False): "search.time-range.todo": { "support": "unsupported" }, - "search.text.case-sensitive": { - "support": "unsupported" - }, - "search.text.case-insensitive": { - "support": "unsupported" - }, - "search.text.category": { - "support": "ungraceful" - }, + "search.text": False, ## sometimes ungraceful "search.is-not-defined": { "support": "fragile" }, - "search.text.by-uid": { - "support": "fragile", - "behaviour": "sometimes the text search delivers everything, other times it doesn't deliver anything. When the text search delivers everything, then the post-filtering will save the day" - }, "search.recurrences.includes-implicit": { "support": "unsupported", "behaviour": "cannot reliably test due to broken comp-type filtering" @@ -1367,6 +1356,14 @@ def dotted_feature_set_list(self, compact=False): ## CalDAV served at /dav/cal// over HTTP on port 8080. ## Feature support mostly unknown until tested; starting with empty hints. stalwart = { + 'rate-limit': { + 'default_sleep': 8, + 'max_sleep': 60 + }, + 'create-calendar.auto': True, + 'principal-search': {'support': 'ungraceful'}, + 'search.recurrences.expanded.exception': False, + 'search.time-range.alarm': False, } ## Lots of transient problems with purelymail From 640a313b56d994a04227529b08eec43084741f55 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 13:18:12 +0100 Subject: [PATCH 14/23] Symmetric adaptive rate-limit retry in AsyncDAVClient; fix two bugs in sync Sync (davclient.py) bugs fixed: - Auto-detect used `if rate_limit:` on the dict returned by is_supported(), which is truthy even for the default {'enable': False}. Changed to `if rate_limit and rate_limit.get('enable'):` - `rate_limit_time_slept > self.rate_limit_max_sleep` crashed with TypeError when rate_limit_max_sleep is None. Guarded with `is not None` check. Async (async_davclient.py) brought to parity: - rate_limit_handle now Optional[bool] = None with same features-based auto-detect as sync. - request() gains rate_limit_time_slept: float = 0 and the same adaptive backoff loop: each retry adds half the already-slept time; stops when rate_limit_max_sleep is exceeded. Retries recursively via self.request() rather than calling _async_request() directly (enabling multi-retry). Tests added for both clients: - test_rate_limit_adaptive_sleep_increases_on_repeated_retries - test_rate_limit_max_sleep_stops_adaptive_retries Co-Authored-By: Claude --- caldav/async_davclient.py | 33 +++++++++++++++++++++++++++------ caldav/davclient.py | 22 +++++++++++++++++++--- tests/test_async_davclient.py | 34 ++++++++++++++++++++++++++++++++++ tests/test_caldav_unit.py | 31 +++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 9 deletions(-) diff --git a/caldav/async_davclient.py b/caldav/async_davclient.py index cd1eec41..da928c3a 100644 --- a/caldav/async_davclient.py +++ b/caldav/async_davclient.py @@ -155,7 +155,7 @@ def __init__( features: FeatureSet | dict | str | None = None, enable_rfc6764: bool = True, require_tls: bool = True, - rate_limit_handle: bool = False, + rate_limit_handle: Optional[bool] = None, rate_limit_default_sleep: Optional[int] = None, rate_limit_max_sleep: Optional[int] = None, ) -> None: @@ -178,7 +178,8 @@ def __init__( enable_rfc6764: Enable RFC6764 DNS-based service discovery. require_tls: Require TLS for discovered services (security consideration). rate_limit_handle: When True, automatically sleep and retry on 429/503 - responses. When False (default), raise RateLimitError immediately. + responses. When None (default), auto-detected from server features. + When False, raise RateLimitError immediately. rate_limit_default_sleep: Fallback sleep seconds when the server's 429 response omits a Retry-After header. None (default) means raise rather than sleeping when no Retry-After is provided. @@ -271,6 +272,16 @@ def __init__( } self.headers.update(headers) + rate_limit = self.features.is_supported("rate-limit", dict) + if rate_limit_handle is None: + if rate_limit and rate_limit.get("enable"): + rate_limit_handle = True + if "default_sleep" in rate_limit: + rate_limit_default_sleep = rate_limit["default_sleep"] + if "max_sleep" in rate_limit: + rate_limit_max_sleep = rate_limit["max_sleep"] + else: + rate_limit_handle = False self.rate_limit_handle = rate_limit_handle self.rate_limit_default_sleep = rate_limit_default_sleep self.rate_limit_max_sleep = rate_limit_max_sleep @@ -352,13 +363,16 @@ async def request( method: str = "GET", body: str = "", headers: Mapping[str, str] | None = None, + rate_limit_time_slept: float = 0, ) -> AsyncDAVResponse: """ Send an async HTTP request, with optional rate-limit sleep-and-retry. Catches RateLimitError from _async_request. When rate_limit_handle is - True and a usable sleep duration is available, sleeps then retries once. - Otherwise re-raises immediately. + True and a usable sleep duration is available, sleeps then retries with + adaptive backoff (each retry adds half the already-slept time). Stops + retrying when rate_limit_max_sleep is exceeded or no sleep duration is + available. Otherwise re-raises immediately. """ try: return await self._async_request(url, method, body, headers) @@ -370,10 +384,17 @@ async def request( self.rate_limit_default_sleep, self.rate_limit_max_sleep, ) - if sleep_seconds is None: + if rate_limit_time_slept: + sleep_seconds += rate_limit_time_slept / 2 + if sleep_seconds is None or ( + self.rate_limit_max_sleep is not None + and rate_limit_time_slept > self.rate_limit_max_sleep + ): raise await asyncio.sleep(sleep_seconds) - return await self._async_request(url, method, body, headers) + return await self.request( + url, method, body, headers, rate_limit_time_slept + sleep_seconds + ) async def _async_request( self, diff --git a/caldav/davclient.py b/caldav/davclient.py index 6976296d..7b04d5ed 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -211,7 +211,7 @@ def __init__( features: FeatureSet | dict | str = None, enable_rfc6764: bool = True, require_tls: bool = True, - rate_limit_handle: bool = False, + rate_limit_handle: Optional[bool] = None, rate_limit_default_sleep: Optional[int] = None, rate_limit_max_sleep: Optional[int] = None, ) -> None: @@ -358,6 +358,16 @@ def __init__( self._principal = None + rate_limit = self.features.is_supported("rate-limit", dict) + if rate_limit_handle is None: + if rate_limit and rate_limit.get("enable"): + rate_limit_handle = True + if "default_sleep" in rate_limit: + rate_limit_default_sleep = rate_limit["default_sleep"] + if "max_sleep" in rate_limit: + rate_limit_max_sleep = rate_limit["max_sleep"] + else: + rate_limit_handle = False self.rate_limit_handle = rate_limit_handle self.rate_limit_default_sleep = rate_limit_default_sleep self.rate_limit_max_sleep = rate_limit_max_sleep @@ -860,6 +870,7 @@ def request( method: str = "GET", body: str = "", headers: Mapping[str, str] = None, + rate_limit_time_slept=0, ) -> DAVResponse: """ Send a generic HTTP request. @@ -885,10 +896,15 @@ def request( self.rate_limit_default_sleep, self.rate_limit_max_sleep, ) - if sleep_seconds is None: + if rate_limit_time_slept: + sleep_seconds += rate_limit_time_slept / 2 + if sleep_seconds is None or ( + self.rate_limit_max_sleep is not None + and rate_limit_time_slept > self.rate_limit_max_sleep + ): raise time.sleep(sleep_seconds) - return self._sync_request(url, method, body, headers) + return self.request(url, method, body, headers, rate_limit_time_slept + sleep_seconds) def _sync_request( self, diff --git a/tests/test_async_davclient.py b/tests/test_async_davclient.py index 8e3f2995..3ad5a3b1 100644 --- a/tests/test_async_davclient.py +++ b/tests/test_async_davclient.py @@ -935,3 +935,37 @@ async def test_rate_limit_max_sleep_zero_raises(self): ) with pytest.raises(error.RateLimitError): await client.request("/") + + @pytest.mark.asyncio + async def test_rate_limit_adaptive_sleep_increases_on_repeated_retries(self): + """On repeated 429s the sleep grows: first sleep uses Retry-After, second adds half of already-slept.""" + ok_response = self._make_response(200) + client = AsyncDAVClient(url="http://cal.example.com/", rate_limit_handle=True) + client.session.request = AsyncMock( + side_effect=[ + self._make_response(429, {"Retry-After": "4"}), + self._make_response(429, {"Retry-After": "4"}), + ok_response, + ] + ) + with patch("caldav.async_davclient.asyncio.sleep", new_callable=AsyncMock) as mock_sleep: + response = await client.request("/") + assert mock_sleep.call_count == 2 + sleeps = [c.args[0] for c in mock_sleep.call_args_list] + assert sleeps[0] == 4.0 + assert sleeps[1] == 6.0 # 4 + 4/2 + assert response.status == 200 + assert client.session.request.call_count == 3 + + @pytest.mark.asyncio + async def test_rate_limit_max_sleep_stops_adaptive_retries(self): + """When accumulated sleep exceeds rate_limit_max_sleep, retrying stops.""" + client = AsyncDAVClient( + url="http://cal.example.com/", rate_limit_handle=True, rate_limit_max_sleep=5 + ) + client.session.request = AsyncMock( + return_value=self._make_response(429, {"Retry-After": "4"}) + ) + with patch("caldav.async_davclient.asyncio.sleep", new_callable=AsyncMock): + with pytest.raises(error.RateLimitError): + await client.request("/") diff --git a/tests/test_caldav_unit.py b/tests/test_caldav_unit.py index 4f2f0322..2321789f 100755 --- a/tests/test_caldav_unit.py +++ b/tests/test_caldav_unit.py @@ -2019,3 +2019,34 @@ def test_rate_limit_max_sleep_zero_raises(self, mocked): ) with pytest.raises(error.RateLimitError): client.request("/") + + @mock.patch("caldav.davclient.requests.Session.request") + def test_rate_limit_adaptive_sleep_increases_on_repeated_retries(self, mocked): + """On repeated 429s the sleep grows: first sleep uses Retry-After, second adds half of already-slept.""" + ok_response = mock.MagicMock() + ok_response.status_code = 200 + ok_response.headers = {} + mocked.side_effect = [ + self._make_response(429, {"Retry-After": "4"}), + self._make_response(429, {"Retry-After": "4"}), + ok_response, + ] + client = DAVClient(url="http://cal.example.com/", rate_limit_handle=True) + with mock.patch("caldav.davclient.time.sleep") as mock_sleep: + response = client.request("/") + assert mock_sleep.call_count == 2 + assert mock_sleep.call_args_list[0] == mock.call(4.0) + assert mock_sleep.call_args_list[1] == mock.call(6.0) # 4 + 4/2 + assert response.status == 200 + assert mocked.call_count == 3 + + @mock.patch("caldav.davclient.requests.Session.request") + def test_rate_limit_max_sleep_stops_adaptive_retries(self, mocked): + """When accumulated sleep exceeds rate_limit_max_sleep, retrying stops.""" + mocked.return_value = self._make_response(429, {"Retry-After": "4"}) + client = DAVClient( + url="http://cal.example.com/", rate_limit_handle=True, rate_limit_max_sleep=5 + ) + with mock.patch("caldav.davclient.time.sleep"): + with pytest.raises(error.RateLimitError): + client.request("/") From 85984853edda18e6bd1de821cf333cfea0da6c2c Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 13:49:53 +0100 Subject: [PATCH 15/23] Update V3_CODE_REVIEW.md: rate-limit fixes and async parity Co-Authored-By: Claude --- docs/design/V3_CODE_REVIEW.md | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index f346021f..7381edc6 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -113,13 +113,16 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. 2. ~~**BUG: Missing `url.unauth()` call**~~ **FIXED** -- Added `self.url = self.url.unauth()` after credentials are extracted from the URL, matching the sync client behaviour. -3. **`_auto_url` blocks the event loop** -- RFC6764 discovery performs synchronous DNS lookups and HTTP requests inside `AsyncDAVClient.__init__()`, which is called from `async get_davclient()`. This blocks the event loop. +3. ~~**Rate-limit handling asymmetry**~~ **FIXED** -- `AsyncDAVClient.request()` now has `rate_limit_time_slept: float = 0`, the same adaptive backoff loop as the sync client (each retry adds half the already-slept time), and the same features-based auto-detect of `rate_limit_handle`. Retries recursively via `self.request()` instead of calling `_async_request()` directly. -4. **Sync import dependency** -- `async_davclient.py:198` imports from `caldav.davclient`, pulling the sync HTTP stack into async contexts. `_auto_url` should live in `caldav/config.py` or `caldav/discovery.py`. +4. **`_auto_url` blocks the event loop** -- RFC6764 discovery performs synchronous DNS lookups and HTTP requests inside `AsyncDAVClient.__init__()`, which is called from `async get_davclient()`. This blocks the event loop. -5. **Password encoding asymmetry** -- Sync client encodes password to bytes eagerly (`davclient.py:329`), async does not. This creates different code paths for auth building. +5. **Sync import dependency** -- `async_davclient.py:198` imports from `caldav.davclient`, pulling the sync HTTP stack into async contexts. `_auto_url` should live in `caldav/config.py` or `caldav/discovery.py`. + +6. **Password encoding asymmetry** -- Sync client encodes password to bytes eagerly (`davclient.py:329`), async does not. This creates different code paths for auth building. + +7. **Response parsing boilerplate** -- The pattern `if response.status in (200, 207) and response._raw: ...` is repeated in ~5 methods. Should be a helper. -6. **Response parsing boilerplate** -- The pattern `if response.status in (200, 207) and response._raw: ...` is repeated in ~5 methods. Should be a helper. ### 2.3 Sync Client (`caldav/davclient.py`) @@ -129,11 +132,13 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. 1. ~~**Bare `except:` clauses (lines 367, 679)**~~ **FIXED** -- Narrowed to `except TypeError` for the test teardown fallback (the expected exception when `teardown()` takes an argument), and `except Exception` for `check_dav_support` (which intentionally catches anything from principal lookup and falls back to root URL). -2. **`NotImplementedError` for auth failures (line 996)** -- When no supported auth scheme is found, `NotImplementedError` is raised. Should be `AuthorizationError`. +2. ~~**Rate-limit auto-detect always enabled**~~ **FIXED** -- `is_supported('rate-limit', dict)` returns `{'enable': False}` (a truthy non-empty dict) for any unconfigured client; the `if rate_limit:` check was incorrectly auto-enabling rate limiting for everyone. Changed to `if rate_limit and rate_limit.get('enable'):`. Also fixed a `TypeError` crash when `rate_limit_max_sleep is None` and the `>` comparison was attempted. + +3. **`NotImplementedError` for auth failures (line 996)** -- When no supported auth scheme is found, `NotImplementedError` is raised. Should be `AuthorizationError`. -3. **Type annotation gaps** -- Multiple `headers: Mapping[str, str] = None` parameters where `None` is not in the union type. +4. **Type annotation gaps** -- Multiple `headers: Mapping[str, str] = None` parameters where `None` is not in the union type. -4. **`propfind` API divergence** -- Sync version (line 754) takes `props=None` which can be either XML string or property list. Async version (line 512) has separate `body` and `props` parameters. +5. **`propfind` API divergence** -- Sync version (line 754) takes `props=None` which can be either XML string or property list. Async version (line 512) has separate `body` and `props` parameters. ### 2.4 Lazy Imports (`caldav/__init__.py`) @@ -261,10 +266,10 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he |---|---|---|---| | `caldav/protocol/` | Excellent | 9/10 | Pure unit tests in test_protocol.py | | `caldav/operations/` | Excellent | 9/10 | 6 dedicated test files | -| `caldav/async_davclient.py` | Good | 8/10 | test_async_davclient.py (821 lines) | +| `caldav/async_davclient.py` | Good | 8/10 | test_async_davclient.py; adaptive rate-limit tests added | | `caldav/datastate.py` | Good | 7/10 | Covered through calendarobject tests | | `caldav/search.py` | Good | 7/10 | test_search.py + integration tests | -| `caldav/davclient.py` | Poor | 4/10 | Only integration tests, no unit tests | +| `caldav/davclient.py` | Low | 5/10 | Rate-limit unit tests added to test_caldav_unit.py; rest still integration-only | | `caldav/collection.py` | Moderate | 6/10 | Integration tests cover most paths | | `caldav/discovery.py` | None | 0/10 | Zero dedicated tests | | `caldav/config.py` | Poor | 3/10 | Module docstring says "test coverage is poor" | From cc2bb9d6972a49add08e6f510cdade55eb060fa1 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 13:57:22 +0100 Subject: [PATCH 16/23] testing the rate limits a bit --- caldav/compatibility_hints.py | 15 ++++++++++----- .../nextcloud/setup_nextcloud.sh | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index a030ea26..8e045dd2 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -84,7 +84,7 @@ class FeatureSet: "description": "Principal has one or more calendars. Some servers and providers comes with a pre-defined calendar for each user, for other servers a calendar has to be explicitly created (supported means there exists a calendar - it may be because the calendar was already provisioned together with the principal, or it may be because a calendar was created manually, the checks can't see the difference)"}, "rate-limit": { "type": "client-feature", - "description": "client (or test code) must not send requests too fast", + "description": "client (or test code) must sleep a bit between requests. Pro-active rate limiting is done through interval and count, server-flagged rate-limiting is controlled through default_sleep/max_sleep", "extra_keys": { "interval": "Rate limiting window, in seconds", "count": "Max number of requests to send within the interval", @@ -938,9 +938,11 @@ def dotted_feature_set_list(self, compact=False): ## TODO: this applies only to test runs, not to ordinary usage 'rate-limit': { 'enable': True, - 'interval': 10, + 'interval': 2, 'count': 1, - 'description': "It's needed to manually empty trashbin frequently when running tests. Since this oepration takes some time and/or there are some caches, it's needed to run tests slowly, even when hammering the 'empty thrashbin' frequently", + 'default_sleep': 2, + 'max_sleep': 30, + 'description': "It's needed to manually empty trashbin frequently when running tests. Since this operation takes some time and/or there are some caches, it's needed to run tests slowly, even when hammering the 'empty thrashbin' frequently", }, 'auto-connect.url': { 'basepath': '/remote.php/dav', @@ -1357,7 +1359,8 @@ def dotted_feature_set_list(self, compact=False): ## Feature support mostly unknown until tested; starting with empty hints. stalwart = { 'rate-limit': { - 'default_sleep': 8, + 'enable': True, + 'default_sleep': 3, 'max_sleep': 60 }, 'create-calendar.auto': True, @@ -1415,8 +1418,10 @@ def dotted_feature_set_list(self, compact=False): }, 'rate-limit': { 'enable': True, - 'interval': 4, + 'interval': 1, 'count': 1, + 'default_sleep': 4, + 'max_sleep': 30 }, 'search.comp-type-optional': {'support': 'fragile', 'description': 'unexpected results from date-search without comp-type - but only sometimes - TODO: research more'}, 'search.recurrences.expanded': {'support': 'unsupported'}, diff --git a/tests/docker-test-servers/nextcloud/setup_nextcloud.sh b/tests/docker-test-servers/nextcloud/setup_nextcloud.sh index dc30579e..6e2f0a8e 100755 --- a/tests/docker-test-servers/nextcloud/setup_nextcloud.sh +++ b/tests/docker-test-servers/nextcloud/setup_nextcloud.sh @@ -38,7 +38,7 @@ echo "Enabling contacts app..." docker exec $CONTAINER_NAME php occ app:enable contacts || true echo "Disabling rate limiting for testing..." -docker exec $CONTAINER_NAME php occ config:system:set ratelimit.enabled --value=false --type=boolean || true +#docker exec $CONTAINER_NAME php occ config:system:set ratelimit.enabled --value=false --type=boolean || true docker exec $CONTAINER_NAME php occ app:disable bruteforcesettings || true docker exec $CONTAINER_NAME php occ config:system:set auth.bruteforce.protection.enabled --value=false --type=boolean || true From 3b88454006fd10e34ec1aed20366420b091a18aa Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 14:37:18 +0100 Subject: [PATCH 17/23] Fix TypesFactory shadowing, read_config return value, and StopIteration pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - caldav/search.py: rename module-level instance `TypesFactory = TypesFactory()` to `_icalendar_types` to avoid shadowing the imported class name - caldav/config.py: change `return None` to `return {}` when no config file is found in default locations, for consistency with all other return paths - caldav/calendarobjectresource.py: replace try/except StopIteration in load_by_multiget() with next(gen, None) sentinel pattern — using StopIteration for control flow is deprecated since PEP 479 (Python 3.7+) - docs/design/V3_CODE_REVIEW.md: update code review notes to reflect fixes Co-Authored-By: Claude --- caldav/calendarobjectresource.py | 6 +++--- caldav/config.py | 2 +- caldav/search.py | 2 +- docs/design/V3_CODE_REVIEW.md | 6 +++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index 463136e1..d210e632 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -784,14 +784,14 @@ def load_by_multiget(self) -> Self: """ error.assert_(self.url) mydata = self.parent._multiget(event_urls=[self.url], raise_notfound=True) - try: - url, self.data = next(mydata) - except StopIteration: + url_data = next(mydata, None) + if url_data is None: ## We shouldn't come here. Something is wrong. ## TODO: research it ## As of 2025-05-20, this code section is used by ## TestForServerECloud::testCreateOverwriteDeleteEvent raise error.NotFoundError(self.url) + url, self.data = url_data error.assert_(self.data) error.assert_(next(mydata, None) is None) return self diff --git a/caldav/config.py b/caldav/config.py index 25d0819d..a566f750 100644 --- a/caldav/config.py +++ b/caldav/config.py @@ -88,7 +88,7 @@ def read_config(fn, interactive_error=False): cfg = read_config(config_file) if cfg: return cfg - return None + return {} ## This can probably be refactored into fewer lines ... try: diff --git a/caldav/search.py b/caldav/search.py index a7105a4a..7ace8c1e 100644 --- a/caldav/search.py +++ b/caldav/search.py @@ -22,7 +22,7 @@ ) from .collection import Calendar as AsyncCalendar -TypesFactory = TypesFactory() +_icalendar_types = TypesFactory() # Re-export for backward compatibility diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index 7381edc6..b4b45956 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -134,7 +134,7 @@ Pure functions for CalDAV business logic. Well-structured but has some issues. 2. ~~**Rate-limit auto-detect always enabled**~~ **FIXED** -- `is_supported('rate-limit', dict)` returns `{'enable': False}` (a truthy non-empty dict) for any unconfigured client; the `if rate_limit:` check was incorrectly auto-enabling rate limiting for everyone. Changed to `if rate_limit and rate_limit.get('enable'):`. Also fixed a `TypeError` crash when `rate_limit_max_sleep is None` and the `>` comparison was attempted. -3. **`NotImplementedError` for auth failures (line 996)** -- When no supported auth scheme is found, `NotImplementedError` is raised. Should be `AuthorizationError`. +3. ~~**`NotImplementedError` for auth failures (line 996)**~~ **ALREADY FIXED** -- Code already calls `self._raise_authorization_error()` which raises `AuthorizationError`. Issue was stale at time of review. 4. **Type annotation gaps** -- Multiple `headers: Mapping[str, str] = None` parameters where `None` is not in the union type. @@ -186,7 +186,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Issues:** 1. ~~**BUG: `_set_deprecated_vobject_instance` (line 1248)**~~ **FIXED** -- Was calling `_get_vobject_instance(inst)` (getter, wrong number of arguments); fixed to call `_set_vobject_instance(inst)`. -2. **`id` setter is a no-op (line 123)** -- `id` passed to constructor is silently ignored. +2. **`id` setter is a no-op (line 123)** -- Intentional design: UID lives in the iCalendar data, not as a separate field. The setter exists to satisfy the parent class `__init__` without side effects; removing it would cause silent writes that don't take effect. Not a bug. 3. **`_async_load` missing multiget fallback** -- Sync `load()` has `load_by_multiget()` fallback, async does not. 4. **Dual data model risk** -- Old `_data`/`_vobject_instance`/`_icalendar_instance` coexist with new `_state`. Manual sync at lines 1206, 1279 could desynchronize. @@ -218,7 +218,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Issues:** 1. ~~**`breakpoint()` in production code (line 443)**~~ **FIXED** -- Replaced with `raise ValueError(f"Unknown feature type: {feature_type!r}")`. -2. **Deprecated `incompatibility_description` dict still present (lines 660-778)** -- Marked "TO BE REMOVED" with 30+ entries. +2. **`incompatibility_description` dict still present (lines 660-778)** -- Marked "TO BE REMOVED" but still referenced by `test_caldav.py` (lines 722, 727, 754). Cannot be deleted until the test code is updated. 3. **`# fmt: off` for entire 1,366-line file** -- Should scope it to just the dict definitions. --- From 33096e3854f7c7a40dfd4945e334c5134fd93c08 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 14:38:39 +0100 Subject: [PATCH 18/23] Inline _get_calendar_home_set() to remove sync/async duplication The sync and async variants were identical except for await. Inline the three-line body at each call site and delete both methods. Co-Authored-By: Claude --- caldav/async_davclient.py | 32 +++++++++----------------------- caldav/davclient.py | 32 +++++++++----------------------- 2 files changed, 18 insertions(+), 46 deletions(-) diff --git a/caldav/async_davclient.py b/caldav/async_davclient.py index da928c3a..7fa9f6ef 100644 --- a/caldav/async_davclient.py +++ b/caldav/async_davclient.py @@ -967,12 +967,20 @@ async def get_calendars(self, principal: Optional["Principal"] = None) -> list[" from caldav.operations.calendarset_ops import ( _extract_calendars_from_propfind_results as extract_calendars, ) + from caldav.operations.principal_ops import ( + _extract_calendar_home_set_from_results as extract_home_set, + ) if principal is None: principal = await self.get_principal() # Get calendar-home-set from principal - calendar_home_url = await self._get_calendar_home_set(principal) + response = await self.propfind( + str(principal.url), + props=self.CALENDAR_HOME_SET_PROPS, + depth=0, + ) + calendar_home_url = extract_home_set(response.results) if not calendar_home_url: return [] @@ -995,28 +1003,6 @@ async def get_calendars(self, principal: Optional["Principal"] = None) -> list[" for info in calendar_infos ] - async def _get_calendar_home_set(self, principal: "Principal") -> str | None: - """Get the calendar-home-set URL for a principal. - - Args: - principal: Principal object - - Returns: - Calendar home set URL or None - """ - from caldav.operations.principal_ops import ( - _extract_calendar_home_set_from_results as extract_home_set, - ) - - # Try to get from principal properties - response = await self.propfind( - str(principal.url), - props=self.CALENDAR_HOME_SET_PROPS, - depth=0, - ) - - return extract_home_set(response.results) - async def search_calendar( self, calendar: "Calendar", diff --git a/caldav/davclient.py b/caldav/davclient.py index 7b04d5ed..61578083 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -509,12 +509,20 @@ def get_calendars(self, principal: Principal | None = None) -> list[Calendar]: from caldav.operations.calendarset_ops import ( _extract_calendars_from_propfind_results as extract_calendars, ) + from caldav.operations.principal_ops import ( + _extract_calendar_home_set_from_results as extract_home_set, + ) if principal is None: principal = self.principal() # Get calendar-home-set from principal - calendar_home_url = self._get_calendar_home_set(principal) + response = self.propfind( + str(principal.url), + props=self.CALENDAR_HOME_SET_PROPS, + depth=0, + ) + calendar_home_url = extract_home_set(response.results) if not calendar_home_url: # Fall back to the principal URL as calendar home # (some servers like GMX don't support calendar-home-set) @@ -539,28 +547,6 @@ def get_calendars(self, principal: Principal | None = None) -> list[Calendar]: for info in calendar_infos ] - def _get_calendar_home_set(self, principal: Principal) -> str | None: - """Get the calendar-home-set URL for a principal. - - Args: - principal: Principal object - - Returns: - Calendar home set URL or None - """ - from caldav.operations.principal_ops import ( - _extract_calendar_home_set_from_results as extract_home_set, - ) - - # Try to get from principal properties - response = self.propfind( - str(principal.url), - props=self.CALENDAR_HOME_SET_PROPS, - depth=0, - ) - - return extract_home_set(response.results) - def search_calendar( self, calendar: Calendar, From 8cacbc0b09a38af18b314c392c1758ae49821f3b Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 14:42:21 +0100 Subject: [PATCH 19/23] Fix Principal._async_get_property and add async multiget fallback in _async_load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - collection.py: delete Principal._async_get_property override — it had an incompatible signature (missing use_cached and **passthrough) and reimplemented PROPFIND logic that the parent DAVObject._async_get_property already handles correctly. Deleting the override restores LSP compliance. - collection.py: add Calendar._async_multiget() as the async counterpart to _multiget(), returning a list of (url, data) tuples. - calendarobjectresource.py: add _async_load_by_multiget() using the new _async_multiget(), mirroring sync load_by_multiget(). - calendarobjectresource.py: update _async_load() to match sync load() fallback behaviour — on NotFoundError: try multiget first, then re-fetch by UID; on other exceptions: try multiget (previously just re-raised). - docs/design/V3_CODE_REVIEW.md: mark both items as fixed. Co-Authored-By: Claude --- caldav/calendarobjectresource.py | 40 ++++++++++++++++++++++---------- caldav/collection.py | 36 ++++++++++++++-------------- docs/design/V3_CODE_REVIEW.md | 4 ++-- 3 files changed, 48 insertions(+), 32 deletions(-) diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index d210e632..fe211cad 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -753,23 +753,28 @@ async def _async_load(self, only_if_unloaded: bool = False) -> Self: raise error.NotFoundError(errmsg(r)) self.data = r.raw # type: ignore except error.NotFoundError: - # Fallback: re-fetch by UID (server may have changed the URL) uid = self.id - if uid and self.parent and hasattr(self.parent, "get_object_by_uid"): + if uid: + # Fallback 1: try multiget (REPORT may work even when GET fails) try: - obj = await self.parent.get_object_by_uid(uid) - if obj: - self.url = obj.url - self.data = obj.data - if hasattr(obj, "props"): - self.props.update(obj.props) - return self - except error.NotFoundError: + return await self._async_load_by_multiget() + except Exception: pass + # Fallback 2: re-fetch by UID (server may have changed the URL) + if self.parent and hasattr(self.parent, "get_object_by_uid"): + try: + obj = await self.parent.get_object_by_uid(uid) + if obj: + self.url = obj.url + self.data = obj.data + if hasattr(obj, "props"): + self.props.update(obj.props) + return self + except error.NotFoundError: + pass raise except Exception: - # Note: load_by_multiget is sync-only, not supported in async mode yet - raise + return await self._async_load_by_multiget() if "Etag" in r.headers: self.props[dav.GetEtag.tag] = r.headers["Etag"] @@ -777,6 +782,17 @@ async def _async_load(self, only_if_unloaded: bool = False) -> Self: self.props[cdav.ScheduleTag.tag] = r.headers["Schedule-Tag"] return self + async def _async_load_by_multiget(self) -> Self: + """Async implementation of load_by_multiget.""" + error.assert_(self.url) + items = await self.parent._async_multiget(event_urls=[self.url], raise_notfound=True) + if not items: + raise error.NotFoundError(self.url) + _url, self.data = items[0] + error.assert_(self.data) + error.assert_(len(items) == 1) + return self + def load_by_multiget(self) -> Self: """ Some servers do not accept a GET, but we can still do a REPORT diff --git a/caldav/collection.py b/caldav/collection.py index b3c03014..2d60e019 100644 --- a/caldav/collection.py +++ b/caldav/collection.py @@ -312,24 +312,6 @@ async def create( return principal - async def _async_get_property(self, prop): - """Async version of get_property for use with async clients.""" - if self.url is None: - raise ValueError("Unexpected value None for self.url") - - response = await self.client.propfind( - str(self.url), - props=[prop.tag if hasattr(prop, "tag") else str(prop)], - depth=0, - ) - - if response.results: - for result in response.results: - value = result.properties.get(prop.tag if hasattr(prop, "tag") else str(prop)) - if value is not None: - return value - return None - def make_calendar( self, name: str | None = None, @@ -1001,6 +983,24 @@ def multiget(self, event_urls: Iterable[URL], raise_notfound: bool = False) -> I parent=self, ) + async def _async_multiget( + self, event_urls: Iterable[URL], raise_notfound: bool = False + ) -> list[tuple[str, str]]: + """Async version of _multiget — returns a list of (url, data) tuples.""" + if self.url is None: + raise ValueError("Unexpected value None for self.url") + + prop = dav.Prop() + cdav.CalendarData() + root = cdav.CalendarMultiGet() + prop + [dav.Href(value=u.path) for u in event_urls] + response = await self._async_query(root, None, "report") + results = response.expand_simple_props([cdav.CalendarData()]) + if raise_notfound: + for href in response.statuses: + status = response.statuses[href] + if status and "404" in status: + raise error.NotFoundError(f"Status {status} in {href}") + return [(r, results[r][cdav.CalendarData.tag]) for r in results] + def calendar_multiget(self, *largs, **kwargs): """ get multiple events' data diff --git a/docs/design/V3_CODE_REVIEW.md b/docs/design/V3_CODE_REVIEW.md index b4b45956..4f136972 100644 --- a/docs/design/V3_CODE_REVIEW.md +++ b/docs/design/V3_CODE_REVIEW.md @@ -178,7 +178,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Issues:** 1. **Missing deprecation warnings** -- `calendars()`, `events()`, `todos()` etc. have docstring notes but no `warnings.warn()` calls (unlike `date_search` and `davobject.name` which do emit). *Comment:* This is intentional. Those are "core features" that was suddenly changed between v2 and v3 - we want people to upgrade to v3 without getting a lot of deprecation warnings thrown in their face. Those warnings will be added in v4. 2. ~~**`_generate_fake_sync_token` uses MD5 (line 1655)**~~ **FIXED** -- Added `usedforsecurity=False`. -3. **`Principal._async_get_property` overrides parent (line 352)** with incompatible implementation. +3. ~~**`Principal._async_get_property` overrides parent (line 352)** with incompatible implementation.~~ **FIXED** -- Deleted the override; parent `DAVObject._async_get_property` handles it correctly. ### 3.3 CalendarObjectResource (`caldav/calendarobjectresource.py`) @@ -187,7 +187,7 @@ Minor: `WWW-Authenticate` parsing (line 31) splits on commas, which fails for he **Issues:** 1. ~~**BUG: `_set_deprecated_vobject_instance` (line 1248)**~~ **FIXED** -- Was calling `_get_vobject_instance(inst)` (getter, wrong number of arguments); fixed to call `_set_vobject_instance(inst)`. 2. **`id` setter is a no-op (line 123)** -- Intentional design: UID lives in the iCalendar data, not as a separate field. The setter exists to satisfy the parent class `__init__` without side effects; removing it would cause silent writes that don't take effect. Not a bug. -3. **`_async_load` missing multiget fallback** -- Sync `load()` has `load_by_multiget()` fallback, async does not. +3. ~~**`_async_load` missing multiget fallback**~~ **FIXED** -- Added `_async_multiget()` to `Calendar` and `_async_load_by_multiget()` to `CalendarObjectResource`; `_async_load` now has the same fallback chain as sync `load()`. 4. **Dual data model risk** -- Old `_data`/`_vobject_instance`/`_icalendar_instance` coexist with new `_state`. Manual sync at lines 1206, 1279 could desynchronize. ### 3.4 Search (`caldav/search.py`) From edf5c709afab8c46376af9f8e226fcf5f717b56b Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 15:44:25 +0100 Subject: [PATCH 20/23] Add [Unreleased] section to CHANGELOG Covers changes since 3.0.0a2: rate-limit bug fixes (sync + async), async multiget fallback, Principal._async_get_property fix, URL quoting fix, expand_simple_props fix, and Stalwart test server support. I'm also upgrading the icalendar dependency to icalendar7. Co-Authored-By: Claude --- CHANGELOG.md | 23 +++++++++++++++++++++++ pyproject.toml | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 293db8b2..fc3ee40e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,29 @@ Changelogs prior to v2.0 is pruned, but was available in the v2.x releases This project should adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html), though for pre-releases PEP 440 takes precedence. +## [Unreleased] + +### Breaking Changes + +* The icalendar dependency is updated from 6 to 7 - not because 3.0 depends on icalendar7, but because I'm planning to use icalendar7-features in some upcoming 3.x. If this causes problems for you, just reach out and I will downgrade the dependency, release a new 3.0.1, and possibly procrastinate the icalendar7-stuff until 4.0. + +### Added + +* **Stalwart CalDAV server** added to Docker test server framework. +* **Async multiget fallback in `_async_load()`** -- `CalendarObjectResource._async_load()` now has the same two-stage fallback as sync `load()`: on a 404, first tries calendar-multiget REPORT, then re-fetches by UID. Previously the async path only had the UID re-fetch fallback. + +### Fixed + +* Fixed two bugs in `DAVClient` rate-limit auto-detection: (1) `is_supported('rate-limit', dict)` returns `{'enable': False}` (a truthy dict) for unconfigured clients, causing rate-limiting to be enabled unintentionally; (2) crash (`TypeError: '>' not supported between 'int' and 'NoneType'`) when `rate_limit_max_sleep` is `None` and a 429 is received. +* Fixed `AsyncDAVClient` rate-limit handling to be fully symmetric with the sync client: same `Optional[bool]` default, same features-based auto-detect, same adaptive backoff accumulating sleep time across retries. +* Fixed `Principal._async_get_property()` override having an incompatible signature (missing `use_cached` and `**passthrough`) and reimplementing PROPFIND logic already handled correctly by the parent `DAVObject._async_get_property()`. The override has been removed. +* Fixed inconsistent URL quoting for calendar object UIDs containing slashes -- both `_generate_url()` and `_find_id_and_path()` in `calendarobject_ops.py` now share a single `_quote_uid()` helper (related to https://github.com/python-caldav/caldav/issues/143). +* Fixed `expand_simple_props()` return value handling. + +### Test Framework + +* Added async rate-limit unit tests matching the sync test suite. + ## [3.0.0a2] - 2026-02-25 (Alpha Release) **This is an alpha release for testing purposes.** Please report issues at https://github.com/python-caldav/caldav/issues diff --git a/pyproject.toml b/pyproject.toml index 65949c7b..1cbf742b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -56,7 +56,7 @@ dependencies = [ "niquests", "recurring-ical-events>=2.0.0", "typing_extensions;python_version<'3.11'", - "icalendar>6.0.0", + "icalendar>7.0.0", "icalendar-searcher>=1.0.5,<2", "dnspython", "python-dateutil", From 5d243c8aaab17d7fc5ebcb6a6a95030e09cc7970 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Fri, 27 Feb 2026 22:14:26 +0100 Subject: [PATCH 21/23] ecloud --- caldav/compatibility_hints.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/caldav/compatibility_hints.py b/caldav/compatibility_hints.py index 8e045dd2..3eae4521 100644 --- a/caldav/compatibility_hints.py +++ b/caldav/compatibility_hints.py @@ -940,8 +940,8 @@ def dotted_feature_set_list(self, compact=False): 'enable': True, 'interval': 2, 'count': 1, - 'default_sleep': 2, - 'max_sleep': 30, + 'default_sleep': 4, + 'max_sleep': 120, 'description': "It's needed to manually empty trashbin frequently when running tests. Since this operation takes some time and/or there are some caches, it's needed to run tests slowly, even when hammering the 'empty thrashbin' frequently", }, 'auto-connect.url': { From 95f85ccec8aa71932500adbdddd89c40807c9268 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sat, 28 Feb 2026 10:03:58 +0100 Subject: [PATCH 22/23] =?UTF-8?q?Fix=20Xandikos=20test=20server:=20Xandiko?= =?UTF-8?q?sBackend=20=E2=86=92=20SingleUserFilesystemBackend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit XandikosBackend was removed in xandikos master; the replacement is SingleUserFilesystemBackend (same API, positional path argument). The dulwich do_commit() incompatibility is fixed in xandikos master too. Co-Authored-By: Claude --- tests/test_servers/embedded.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_servers/embedded.py b/tests/test_servers/embedded.py index 2ef62fbf..6e72f679 100644 --- a/tests/test_servers/embedded.py +++ b/tests/test_servers/embedded.py @@ -215,7 +215,7 @@ def start(self) -> None: return try: - from xandikos.web import XandikosApp, XandikosBackend + from xandikos.web import SingleUserFilesystemBackend, XandikosApp except ImportError as e: raise RuntimeError("Xandikos is not installed") from e @@ -228,7 +228,7 @@ def start(self) -> None: self.serverdir.__enter__() # Create backend and configure principal (following conf.py pattern) - backend = XandikosBackend(path=self.serverdir.name) + backend = SingleUserFilesystemBackend(self.serverdir.name) backend._mark_as_principal(f"/{self.username}/") backend.create_principal(f"/{self.username}/", create_defaults=True) From 9e5395f8c75b603b74b73a7863e9f73bc58aefd3 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sat, 28 Feb 2026 14:10:30 +0100 Subject: [PATCH 23/23] Support both old and new xandikos API in embedded test server XandikosBackend was renamed to SingleUserFilesystemBackend in xandikos master (not yet released to PyPI). Try the new name first, fall back to the old name so both the PyPI release and git master work. Co-Authored-By: Claude --- tests/test_servers/embedded.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_servers/embedded.py b/tests/test_servers/embedded.py index 6e72f679..1adf9d3a 100644 --- a/tests/test_servers/embedded.py +++ b/tests/test_servers/embedded.py @@ -215,7 +215,13 @@ def start(self) -> None: return try: - from xandikos.web import SingleUserFilesystemBackend, XandikosApp + from xandikos.web import XandikosApp + + try: + # xandikos master renamed XandikosBackend to SingleUserFilesystemBackend + from xandikos.web import SingleUserFilesystemBackend as XandikosBackend + except ImportError: + from xandikos.web import XandikosBackend # type: ignore[no-redef] except ImportError as e: raise RuntimeError("Xandikos is not installed") from e @@ -228,7 +234,7 @@ def start(self) -> None: self.serverdir.__enter__() # Create backend and configure principal (following conf.py pattern) - backend = SingleUserFilesystemBackend(self.serverdir.name) + backend = XandikosBackend(self.serverdir.name) backend._mark_as_principal(f"/{self.username}/") backend.create_principal(f"/{self.username}/", create_defaults=True)