Skip to content

Commit ce4ebe9

Browse files
tobixenclaude
andcommitted
fix: check_component() must not mutate self
check_component() resolved None fields (include_completed, todo, event, journal) and normalised date start/end/alarm_start/alarm_end values by writing back to self. This made a Searcher instance stateful: calling check_component() twice could yield different results, and reusing a Searcher across multiple operations changed behaviour after the first call. Fix: compute all resolved/normalised values as local variables inside check_component(). Thread them to the internal filter helpers via new keyword parameters (_start, _end, _alarm_start, _alarm_end for _check_range/_check_alarm_range; _include_completed for _check_completed_filter). The parameters default to None which falls back to self.* for existing callers that do not pass them. Add tests/test_no_mutation.py to guard against regression. Triggered by python-caldav/caldav#650 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 2c1a6bc commit ce4ebe9

4 files changed

Lines changed: 203 additions & 49 deletions

File tree

CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
55
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
66
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
77

8+
## [Unreleased]
9+
10+
### Fixed
11+
12+
- **`check_component()` mutated `self` on every call**: The method resolved `None` fields (`include_completed`, `todo`, `event`, `journal`) and normalised `start`/`end`/`alarm_start`/`alarm_end` date objects to datetimes by writing back to `self`. This made a `Searcher` instance stateful: calling `check_component()` twice could produce different results, and reusing a `Searcher` across multiple search operations (e.g. in the python-caldav library) could silently change behaviour after the first call. All these values are now computed as local variables inside `check_component()` and threaded to the internal filter methods via new keyword parameters (`_start`, `_end`, `_alarm_start`, `_alarm_end`, `_include_completed`), keeping `self` immutable throughout. (Triggered by https://github.com/python-caldav/caldav/issues/650)
13+
814
## [1.0.5] - 2026-02-19
915

1016
### Changes

src/icalendar_searcher/filters.py

Lines changed: 54 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,26 @@ class FilterMixin:
2525
- _property_locale: dict of property locales
2626
"""
2727

28-
def _check_range(self, component: Component) -> bool:
28+
def _check_range(
29+
self,
30+
component: Component,
31+
_start: datetime | None = None,
32+
_end: datetime | None = None,
33+
) -> bool:
2934
"""Check if a component falls within the time range specified by self.start and self.end.
3035
3136
Implements RFC4791 section 9.9 time-range filtering logic for VEVENT, VTODO, and VJOURNAL.
3237
3338
:param component: A single calendar component (VEVENT, VTODO, or VJOURNAL)
39+
:param _start: Effective start datetime (defaults to self.start). Pass a pre-normalised
40+
value from check_component() to avoid mutating self.
41+
:param _end: Effective end datetime (defaults to self.end). See _start.
3442
:return: True if the component matches the time range, False otherwise
3543
"""
44+
## Resolve effective values — callers may pass pre-computed normalised datetimes
45+
## so that self is not mutated during filtering.
46+
start = _start if _start is not None else self.start
47+
end = _end if _end is not None else self.end
3648
comp_name = component.name
3749

3850
## The logic below should correspond neatly with RFC4791 section 9.9
@@ -126,22 +138,29 @@ def _check_range(self, component: Component) -> bool:
126138

127139
## After the logic above, all rows in the matrix boils down to
128140
## this: (we could reduce it even more by defaulting
129-
## self.start and self.end to DATE_MIN_DT etc)
130-
if self.start and self.end and comp_end:
131-
return self.start < comp_end and self.end > comp_start
132-
elif self.end:
133-
return self.end > comp_start
134-
elif self.start and comp_end:
135-
return self.start < comp_end
141+
## start and end to DATE_MIN_DT etc)
142+
if start and end and comp_end:
143+
return start < comp_end and end > comp_start
144+
elif end:
145+
return end > comp_start
146+
elif start and comp_end:
147+
return start < comp_end
136148
return True
137149

138-
def _check_completed_filter(self, component: Component) -> bool:
150+
def _check_completed_filter(
151+
self,
152+
component: Component,
153+
_include_completed: bool | None = None,
154+
) -> bool:
139155
"""Check if a component should be included based on the include_completed filter.
140156
141157
:param component: A single calendar component
158+
:param _include_completed: Effective value (defaults to self.include_completed). Pass a
159+
pre-resolved value from check_component() to avoid mutating self.
142160
:return: True if the component should be included, False if it should be filtered out
143161
"""
144-
if self.include_completed:
162+
include_completed = _include_completed if _include_completed is not None else self.include_completed
163+
if include_completed:
145164
return True
146165

147166
## If include_completed is False, exclude completed/cancelled VTODOs
@@ -372,16 +391,28 @@ def _check_property_filters(self, component: Component, skip_undef: bool = False
372391

373392
## DISCLAIMER: Mostly AI-generated code, with a touch of human polishing
374393
## and bugfixing. Alarms are a bit complex.
375-
def _check_alarm_range(self, component: Component) -> bool:
394+
def _check_alarm_range(
395+
self,
396+
component: Component,
397+
_alarm_start: datetime | None = None,
398+
_alarm_end: datetime | None = None,
399+
) -> bool:
376400
"""Check if a component has alarms that fire within the alarm time range.
377401
378402
Implements RFC 4791 section 9.9 alarm time-range filtering.
379403
380404
:param component: A single calendar component (VEVENT, VTODO, or VJOURNAL)
405+
:param _alarm_start: Effective alarm start (defaults to self.alarm_start). Pass a
406+
pre-normalised value from check_component() to avoid mutating self.
407+
:param _alarm_end: Effective alarm end (defaults to self.alarm_end). See _alarm_start.
381408
:return: True if any alarm fires within the alarm range, False otherwise
382409
"""
383410
from datetime import timedelta
384411

412+
## Resolve effective values — callers may pass pre-computed normalised datetimes
413+
alarm_start = _alarm_start if _alarm_start is not None else self.alarm_start
414+
alarm_end = _alarm_end if _alarm_end is not None else self.alarm_end
415+
385416
## Get all VALARM subcomponents
386417
alarms = [x for x in component.subcomponents if x.name == "VALARM"]
387418

@@ -451,27 +482,27 @@ def _check_alarm_range(self, component: Component) -> bool:
451482
for i in range(int(repeat_count) + 1):
452483
repeat_time = alarm_time + (duration * i)
453484
## Check if this repetition fires within the alarm range
454-
if self.alarm_start and self.alarm_end:
455-
if self.alarm_start <= repeat_time < self.alarm_end:
485+
if alarm_start and alarm_end:
486+
if alarm_start <= repeat_time < alarm_end:
456487
return True
457-
elif self.alarm_start:
458-
if repeat_time >= self.alarm_start:
488+
elif alarm_start:
489+
if repeat_time >= alarm_start:
459490
return True
460-
elif self.alarm_end:
461-
if repeat_time < self.alarm_end:
491+
elif alarm_end:
492+
if repeat_time < alarm_end:
462493
return True
463494
## None of the repetitions matched
464495
continue
465496

466497
## Check if this alarm (first occurrence) fires within the alarm range
467-
if self.alarm_start and self.alarm_end:
468-
if self.alarm_start <= alarm_time < self.alarm_end:
498+
if alarm_start and alarm_end:
499+
if alarm_start <= alarm_time < alarm_end:
469500
return True
470-
elif self.alarm_start:
471-
if alarm_time >= self.alarm_start:
501+
elif alarm_start:
502+
if alarm_time >= alarm_start:
472503
return True
473-
elif self.alarm_end:
474-
if alarm_time < self.alarm_end:
504+
elif alarm_end:
505+
if alarm_time < alarm_end:
475506
return True
476507

477508
return False

src/icalendar_searcher/searcher.py

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -348,14 +348,21 @@ def check_component(
348348
return orig_recurrence_set
349349

350350
## Ensure timezone is set. Ensure start and end are datetime objects.
351-
for attr in ("start", "end", "alarm_start", "alarm_end"):
352-
value = getattr(self, attr)
353-
if value:
354-
if not isinstance(value, datetime):
355-
logging.warning(
356-
"Date-range searches not well supported yet; use datetime rather than dates"
357-
)
358-
setattr(self, attr, _normalize_dt(value))
351+
## Compute normalised copies locally — do NOT mutate self, as check_component()
352+
## must be idempotent and safe to call multiple times on the same Searcher.
353+
def _normalise_dt_local(value: datetime | None) -> datetime | None:
354+
if not value:
355+
return value
356+
if not isinstance(value, datetime):
357+
logging.warning(
358+
"Date-range searches not well supported yet; use datetime rather than dates"
359+
)
360+
return _normalize_dt(value)
361+
362+
_start = _normalise_dt_local(self.start)
363+
_end = _normalise_dt_local(self.end)
364+
_alarm_start = _normalise_dt_local(self.alarm_start)
365+
_alarm_end = _normalise_dt_local(self.alarm_end)
359366

360367
## recurrence_set is our internal generator/iterator containing
361368
## everything that hasn't been filtered out yet (in most
@@ -407,10 +414,11 @@ def check_component(
407414
## filtering out occurrences with properties added by expansion.
408415
skip_undef_for_expanded = True
409416

410-
## self.include_completed should default to False if todo is explicity set,
411-
## otherwise True
412-
if self.include_completed is None:
413-
self.include_completed = not self.todo
417+
## include_completed should default to False if todo is explicitly set, otherwise True.
418+
## Compute locally — do NOT mutate self.include_completed so the Searcher stays reusable.
419+
_include_completed = self.include_completed
420+
if _include_completed is None:
421+
_include_completed = not self.todo
414422

415423
## Component type flags are a bit difficult. In the CalDAV library,
416424
## if all of them are None, everything should be returned. If only
@@ -421,19 +429,18 @@ def check_component(
421429
## correct solution from the start: 1) if any flags are True,
422430
## then consider flags set as None as False. 2) if any flags
423431
## are still None, then consider those to be True. 3) List
424-
## the flags that are True as acceptable component types:
432+
## the flags that are True as acceptable component types.
433+
## Computed locally — do NOT mutate self.todo / self.event / self.journal.
425434

426435
comptypesl = ("todo", "event", "journal")
427436
if any(getattr(self, x) for x in comptypesl):
428-
for x in comptypesl:
429-
if getattr(self, x) is None:
430-
setattr(self, x, False)
437+
## At least one type is explicitly True: treat remaining Nones as False (opt-in mode)
438+
_compflags = {x: bool(getattr(self, x)) for x in comptypesl}
431439
else:
432-
for x in comptypesl:
433-
if getattr(self, x) is None:
434-
setattr(self, x, True)
440+
## No type is explicitly True: treat Nones as True, but respect explicit Falses (opt-out mode)
441+
_compflags = {x: getattr(self, x) is not False for x in comptypesl}
435442

436-
comptypesu = set([f"V{x.upper()}" for x in comptypesl if getattr(self, x)])
443+
comptypesu = set([f"V{x.upper()}" for x in comptypesl if _compflags[x]])
437444

438445
## if expand_only, expand all comptypes, otherwise only the comptypes specified in the filters
439446
comptypes_for_expansion = ["VTODO", "VEVENT", "VJOURNAL"] if expand_only else comptypesu
@@ -444,15 +451,15 @@ def check_component(
444451
if not expand_only:
445452
## OPTIMIZATION TODO: If the object was recurring, we should
446453
## probably trust recur.between to do the right thing?
447-
if not _ignore_rrule_and_time and (self.start or self.end):
448-
recurrence_set = (x for x in recurrence_set if self._check_range(x))
454+
if not _ignore_rrule_and_time and (_start or _end):
455+
recurrence_set = (x for x in recurrence_set if self._check_range(x, _start=_start, _end=_end))
449456

450457
## This if is just to save some few CPU cycles - skip filtering if it's not needed
451-
if not all(getattr(self, x) for x in comptypesl):
458+
if not all(_compflags[x] for x in comptypesl):
452459
recurrence_set = (x for x in recurrence_set if x.name in comptypesu)
453460

454461
## Filter based on include_completed setting
455-
recurrence_set = (x for x in recurrence_set if self._check_completed_filter(x))
462+
recurrence_set = (x for x in recurrence_set if self._check_completed_filter(x, _include_completed=_include_completed))
456463

457464
## Apply property filters
458465
if self._property_filters or self._property_operator:
@@ -463,8 +470,8 @@ def check_component(
463470
)
464471

465472
## Apply alarm filters
466-
if not _ignore_rrule_and_time and (self.alarm_start or self.alarm_end):
467-
recurrence_set = (x for x in recurrence_set if self._check_alarm_range(x))
473+
if not _ignore_rrule_and_time and (_alarm_start or _alarm_end):
474+
recurrence_set = (x for x in recurrence_set if self._check_alarm_range(x, _alarm_start=_alarm_start, _alarm_end=_alarm_end))
468475

469476
if self.expand:
470477
## TODO: fix wrapping, if needed

tests/test_no_mutation.py

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
"""
2+
Tests that check_component() does not mutate the Searcher object.
3+
4+
A Searcher must be reusable: calling check_component() (or filter()) multiple
5+
times must produce consistent results and must not change the Searcher's own
6+
fields as a side effect.
7+
8+
Ref: https://github.com/python-caldav/caldav/issues/650
9+
"""
10+
11+
from datetime import date, datetime, timezone
12+
13+
from icalendar import Calendar, Todo
14+
15+
from icalendar_searcher import Searcher
16+
17+
18+
def _pending_todo(uid: str = "pending") -> Todo:
19+
task = Todo()
20+
task.add("uid", uid)
21+
task.add("summary", "Pending task")
22+
task.add("status", "NEEDS-ACTION")
23+
return task
24+
25+
26+
def _completed_todo(uid: str = "done") -> Todo:
27+
task = Todo()
28+
task.add("uid", uid)
29+
task.add("summary", "Done task")
30+
task.add("status", "COMPLETED")
31+
task.add("completed", datetime(2000, 1, 2, tzinfo=timezone.utc))
32+
return task
33+
34+
35+
def test_include_completed_none_not_mutated() -> None:
36+
"""include_completed=None must remain None after check_component()."""
37+
searcher = Searcher(todo=True)
38+
assert searcher.include_completed is None
39+
40+
searcher.check_component(_pending_todo())
41+
42+
assert searcher.include_completed is None, (
43+
"check_component() mutated include_completed from None — "
44+
"this breaks reuse of the Searcher object"
45+
)
46+
47+
48+
def test_include_completed_none_not_mutated_on_completed_todo() -> None:
49+
"""include_completed=None must remain None even after filtering a completed todo."""
50+
searcher = Searcher(todo=True)
51+
assert searcher.include_completed is None
52+
53+
searcher.check_component(_completed_todo())
54+
55+
assert searcher.include_completed is None, (
56+
"check_component() mutated include_completed from None"
57+
)
58+
59+
60+
def test_component_type_flags_not_mutated() -> None:
61+
"""todo/event/journal flags that are None must remain None after check_component()."""
62+
searcher = Searcher(todo=True)
63+
assert searcher.event is None
64+
assert searcher.journal is None
65+
66+
searcher.check_component(_pending_todo())
67+
68+
assert searcher.event is None, (
69+
"check_component() mutated event flag from None"
70+
)
71+
assert searcher.journal is None, (
72+
"check_component() mutated journal flag from None"
73+
)
74+
75+
76+
def test_all_none_flags_not_mutated() -> None:
77+
"""When all of todo/event/journal are None, they must remain None after check_component()."""
78+
searcher = Searcher() # all type flags None
79+
assert searcher.todo is None
80+
assert searcher.event is None
81+
assert searcher.journal is None
82+
83+
cal = Calendar()
84+
cal.add_component(_pending_todo())
85+
searcher.check_component(cal)
86+
87+
assert searcher.todo is None, "check_component() mutated todo flag from None"
88+
assert searcher.event is None, "check_component() mutated event flag from None"
89+
assert searcher.journal is None, "check_component() mutated journal flag from None"
90+
91+
92+
def test_start_date_not_mutated_to_datetime() -> None:
93+
"""start/end given as date objects must not be replaced with datetime after check_component()."""
94+
start = date(2020, 1, 1)
95+
end = date(2030, 1, 1)
96+
searcher = Searcher(todo=True, start=start, end=end)
97+
assert type(searcher.start) is date
98+
assert type(searcher.end) is date
99+
100+
task = _pending_todo()
101+
task.add("dtstart", datetime(2025, 6, 1, tzinfo=timezone.utc))
102+
task.add("due", datetime(2025, 7, 1, tzinfo=timezone.utc))
103+
searcher.check_component(task)
104+
105+
assert type(searcher.start) is date, (
106+
f"check_component() replaced start with {searcher.start!r} (type {type(searcher.start).__name__})"
107+
)
108+
assert type(searcher.end) is date, (
109+
f"check_component() replaced end with {searcher.end!r} (type {type(searcher.end).__name__})"
110+
)

0 commit comments

Comments
 (0)