Skip to content

Commit c395c03

Browse files
tobixenclaude
andcommitted
refactor: consolidate sync/async fixture helpers and trim TODO
- Merge `get_or_create_test_calendar` and `aget_or_create_test_calendar` into a shared async `_get_or_create_impl`, reducing ~130 lines of near-identical duplicated logic - Merge `_filter_calendars_by_component_set` + `_afilter_calendars_by_component_set` and `_find_test_calendar` + `_afind_test_calendar` into single async implementations using `_maybe_await` - Drop dead `get_properties_fn` parameter (was never passed by any caller) - Remove stale TODO comments from `_fixCalendar` in test_caldav.py - Trim docs/design/TODO.md: remove resolved and non-actionable items, condense Nextcloud UID-reuse bug entry Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent ecd243b commit c395c03

3 files changed

Lines changed: 48 additions & 280 deletions

File tree

docs/design/TODO.md

Lines changed: 8 additions & 139 deletions
Original file line numberDiff line numberDiff line change
@@ -1,152 +1,21 @@
11
# Known Issues and TODO Items
22

3-
## Calendar creation/location in integration tests
4-
5-
* Currently there is quite some logic for the sync integration tests for fixing calendars (`test_caldav.py`). All boilerplate should be moved to the `fixture_helper.py` file - or simply removed if it's irrelevant.
6-
* In the `fixture_helpers.py` there is an `aget_or_create_test_calendar` and a `get_or_create_test_calendar`. There is quite much duplicated logic here. Work should be done to consolidate common code.
7-
* If the default calendar already exists, make sure to delete and recreate or wipe it (dependent on relevant features supported/configured) to avoid pollution from old test runs fouling the new test run.
8-
93
## Calendar cleanup in integration tests
104

11-
Calendar cleanup in the integration tests is a bit of a mess and should be cleaned up. Also, the sync and async integration code now have different code paths, that's not acceptable, that should be cleaned up.
12-
13-
Currently unsupported scenarios:
14-
15-
* Test run towards read-only calendars (there exists service providers that have a partial caldav interface for read-only access to a calendar) ... maybe a future feature, but as for now it doesn't really work out, so let's ignore this possibility as for now.
16-
* Test runs towards a calendar that already has data in it, and without deleting the data in the calendar. I had the idea to allow this, but ... no, it doesn't work out, and let's keep it that way. BUT: currently, for servers where calendars cannot be created, the test will take the first available calendar it finds and wipe it. That's also not acceptable, not without the user explicitly confguring that this is OK.
17-
18-
Server compatibility to consider:
19-
20-
* Some servers disallows calendar creation.
21-
* On some servers calendar creation is not reliable (i.e. quota for how many calendars one can have. Workaround: manual creation of test calendar and explicitly specifying what calendar to use in the configuration).
22-
* On some servers calendar deletion is not possible or not reliable. (i.e. calendar "moved" to thrash bin rather than deleted).
23-
* On some servers, objects on the calendar are "sticky", not fully deleted (and is polluting the UID name space) even if the calendar is deleted. (perhaps due to the thrash-bin-issue above)
24-
* Sometimes separate calendars has to be used for tasks, events and journals.
25-
* In some situations the user may want to point testing towards one (or several) specific calendars - either due to the issues above or for other reasons
26-
27-
There are also different modes of cleanup:
28-
29-
* Wipe all "known objects", objects created by the tests (old "through" mode, it was needed for running tests towards existing production calendars without wiping the original content - but now I've decided not to support this. It may be needed if having problems with "sticky" events, but probably better to just "wipe" the calendar).
30-
* Wiping the calendar after every test (like, take out the list of objects, and delete every single event - `[x.delete() for x in calendar.search()]`)
31-
* Deleting the whole calendar after every test.
32-
* Do the same before every test
33-
34-
The first option is needed if one wants to run the tests towards a "live" calendar without deleting content on the live calendar. Since this is anyway not supported, I think this code may as well be yanked out.
35-
36-
Deleting a calendar is much faster than deleting every item on the calendar. However, for "sticky" objects wiping is necessary. For calendars not supporting calendar deletion, a "delete"-operation towards a calendar will automatically wipe it.
37-
38-
Probably test cleanups should be done after the server run, but also before if needed.
5+
For servers where calendars cannot be created, the test will take the first available calendar it finds and wipe it. That is not acceptable without the user explicitly configuring that this is OK.
396

407
## Nextcloud UNIQUE Constraint Violations
418

429
**Status**: Known issue, needs upstream investigation
4310
**Priority**: Low (doesn't block caldav work)
44-
**Estimated research time**: 6-12 hours
4511

4612
### Problem
47-
Nextcloud occasionally gets into an inconsistent internal state where it reports UNIQUE constraint violations when trying to save calendar objects:
48-
49-
```
50-
SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed:
51-
oc_calendarobjects.calendarid, oc_calendarobjects.calendartype, oc_calendarobjects.uid
52-
```
53-
54-
### Observations
55-
- **Server-specific**: Only affects Nextcloud, not Radicale, Baikal, Xandikos, etc.
56-
- **Intermittent**: Happens during `caldav_server_tester.ServerQuirkChecker.check_all()`
57-
- **Workaround**: Taking down and restarting the ephemeral Docker container resolves it
58-
- **Hypothesis**: Internal state corruption in Nextcloud, not a caldav library issue
59-
- **Pre-existing**: Test was already failing before starting to work on the async support
60-
61-
### Example Failure
62-
```
63-
tests/test_caldav.py::TestForServerNextcloud::testCheckCompatibility
64-
E caldav.lib.error.PutError: PutError at '500 Internal Server Error
65-
E <s:message>An exception occurred while executing a query: SQLSTATE[23000]:
66-
Integrity constraint violation: 19 UNIQUE constraint failed:
67-
oc_calendarobjects.calendarid, oc_calendarobjects.calendartype,
68-
oc_calendarobjects.uid</s:message>
69-
```
70-
71-
### Test Results: Hypothesis CONFIRMED ✓
72-
73-
**Date**: 2025-12-17
74-
**Test script**: `/tmp/test_nextcloud_uid_reuse.py`
75-
76-
**Finding**: Nextcloud does NOT allow reusing a UID after deletion. This is a **Nextcloud bug**.
77-
78-
**Test steps**:
79-
1. Created event with UID `test-uid-reuse-hypothesis-12345`
80-
2. Deleted the event ✓
81-
3. Confirmed deletion with `get_event_by_uid()` (throws NotFoundError) ✓
82-
4. Attempted to create new event with same UID → **FAILED with UNIQUE constraint**
83-
84-
**Error received**:
85-
```
86-
500 Internal Server Error
87-
SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed:
88-
oc_calendarobjects.calendarid, oc_calendarobjects.calendartype, oc_calendarobjects.uid
89-
```
90-
91-
**Conclusion**:
92-
- This violates CalDAV RFC expectations - UIDs should be reusable after deletion
93-
- Nextcloud's internal database retains constraint even after CalDAV object is deleted
94-
- This explains why `ServerQuirkChecker.check_all()` fails - it likely deletes and recreates test objects
95-
- Container restart fixes it because it clears the internal state
96-
97-
### Next Steps (when prioritized)
98-
1.~~Test the UID reuse hypothesis~~ - **CONFIRMED**
99-
2. Search Nextcloud issue tracker for similar reports
100-
3. Create minimal bug report with reproduction steps
101-
4. File upstream bug report with Nextcloud
102-
5. Consider adding server quirk detection in caldav_server_tester
103-
6. Document workaround: avoid UID reuse with Nextcloud, or restart container between test runs
104-
105-
### References
106-
- Test: `tests/test_caldav.py::TestForServerNextcloud::testCheckCompatibility`
107-
- Discussion: Session on 2025-12-17
108-
109-
---
110-
111-
## Phase 2 Remaining Work
112-
113-
### Test Suite Status
114-
- **Radicale**: 42 passed, 13 skipped ✓
115-
- **Baikal**: Some tests passing after path/auth fixes
116-
- **Nextcloud**: testCheckCompatibility failing (see above)
117-
- **Other servers**: Status unknown
118-
119-
### Known Limitations (to be addressed in Phase 3)
120-
- AsyncPrincipal not implemented → path matching warnings for Principal objects
121-
- Async collection methods (get_event_by_uid, etc.) not implemented → no_create/no_overwrite validation done in sync wrapper
122-
- Recurrence handling done in sync wrapper → will move to async in Phase 3
123-
124-
### Known Test Limitations
125-
126-
#### MockedDAVClient doesn't work with async delegation
127-
**Status**: Known limitation in Phase 2
128-
**Affected test**: `tests/test_caldav_unit.py::TestCalDAV::testPathWithEscapedCharacters`
129-
130-
MockedDAVClient overrides `request()` to return mocked responses without network calls.
131-
However, with async delegation, `_run_async()` creates a new async client that makes
132-
real HTTP connections, bypassing the mock.
133-
134-
**Options to fix**:
135-
1. Make MockedDAVClient override `_get_async_client()` to return a mocked async client
136-
2. Update tests to use `@mock.patch` on async client methods
137-
3. Implement a fallback sync path for mocked clients
13+
Nextcloud does not allow reusing a UID after deletion — the internal database retains the constraint even after the CalDAV object is deleted. This causes intermittent UNIQUE constraint violation errors (500) when `ServerQuirkChecker.check_all()` deletes and recreates test objects with the same UID.
13814

139-
**Current approach**: Raise clear NotImplementedError when mocked client tries to use
140-
async delegation, documenting that mocking needs to be updated for async support.
15+
This violates CalDAV RFC expectations and is a Nextcloud bug. Restarting the Docker container resolves it by clearing internal state.
14116

142-
### Recently Fixed
143-
- ✓ Infinite redirect loop in multiplexing retry
144-
- ✓ Path matching assertion failures
145-
- ✓ HTTPDigestAuth sync→async conversion
146-
- ✓ UID generation issues
147-
- ✓ Async class type mapping (Event→AsyncEvent, etc.)
148-
- ✓ no_create/no_overwrite validation moved to sync wrapper
149-
- ✓ Recurrence handling moved to sync wrapper
150-
- ✓ Unit tests without client (load with only_if_unloaded)
151-
- ✓ Mocked client detection for unit tests (testAbsoluteURL)
152-
- ✓ Sync fallback in get_properties() for mocked clients
17+
### Next Steps
18+
1. Search Nextcloud issue tracker for similar reports
19+
2. File upstream bug report with reproduction steps
20+
3. Consider adding server quirk detection in caldav_server_tester
21+
4. Document workaround: avoid UID reuse with Nextcloud, or restart container between test runs

0 commit comments

Comments
 (0)