From df1e617fe5e979bf578e735ae4f6161a23cd67a2 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sun, 1 Oct 2023 23:36:09 +0200 Subject: [PATCH 1/7] WIP: a check_reverse_relation method --- caldav/objects.py | 46 +++++++++++++++++++++++++++++++++++++++++++- tests/test_caldav.py | 18 +++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/caldav/objects.py b/caldav/objects.py index ff4026ec..7cb58d2e 100755 --- a/caldav/objects.py +++ b/caldav/objects.py @@ -932,7 +932,6 @@ def _handle_relations(self, uid, ical_data) -> None: ## TODO: think more through this - is `save_foo` better than `add_foo`? ## `save_foo` should not be used for updating existing content on the ## calendar! - add_event = save_event add_todo = save_todo add_journal = save_journal @@ -1938,10 +1937,18 @@ class CalendarObjectResource(DAVObject): event, a todo-item, a journal entry, or a free/busy entry """ + ## There is also STARTTOFINISH in RFC9253, it does not have any reverse RELTYPE_REVERSER: ClassVar = { "PARENT": "CHILD", "CHILD": "PARENT", "SIBLING": "SIBLING", + ## this is how Tobias Brox inteprets RFC9253: + "DEPENDENT": "FINISHTOSTART", + "FINISHTOSTART": "DEPENDENT", + ## next/first is a special case, linked list + ## it needs special handling when length of list<>2 + "NEXT": "FIRST", + "FIRST": "NEXT", } _ENDPARAM = None @@ -2072,6 +2079,8 @@ def set_relation( if set_reverse: other = self.parent.object_by_uid(uid) if set_reverse: + ## TODO: special handling of NEXT/FIRST. + ## STARTTOFINISH does not have any equivalent "reverse". reltype_reverse = self.RELTYPE_REVERSER[reltype] other.set_relation(other=self, reltype=reltype_reverse, set_reverse=False) @@ -2119,6 +2128,10 @@ def get_relatives( TODO: this is partially overlapped by plann.lib._relships_by_type in the plann tool. Should consolidate the code. + + TODO: should probably return some kind of object instead of a weird dict structure. + (but due to backward compatibility requirement, such an object should behave like + the current dict) """ ret = defaultdict(set) relations = self.icalendar_component.get("RELATED-TO", []) @@ -2156,6 +2169,37 @@ def get_relatives( return ret + + def check_reverse_relations(self, pdb: bool = False) -> list: + """ + Goes through all relations and verifies that the return relation is set + Returns a list of objects missing a reverse (or an empty list if everything is OK) + """ + ret = [] + relations = self.get_relatives() + for reltype in relations: + for other in relations[reltype]: + revreltype = self.RELTYPE_REVERSER[reltype] + ## TODO: special case FIRST/NEXT needs special handling + other_relations = other.get_relatives( + fetch_objects=False, reltypes={revreltype} + ) + if ( + not str(self.icalendar_component["uid"]) + in other_relations[revreltype] + ): + if pdb: + import pdb + + pdb.set_trace() + ret.append((other, revreltype)) + return ret + + ## TODO: fix this (and consolidate with _handle_relations / set_relation?) + # def ensure_reverse_relations(self): + # missing_relations = self.check_reverse_relations() + # ... + def _get_icalendar_component(self, assert_one=False): """Returns the icalendar subcomponent - which should be an Event, Journal, Todo or FreeBusy from the icalendar class diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 21663620..2953df36 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1758,6 +1758,24 @@ def testCreateChildParent(self): assert len(foo["PARENT"]) == 1 foo = parent_.get_relatives(relfilter=lambda x: x.params.get("GAP")) + ## verify the check_reverse_relations method (TODO: move to a separate test) + assert parent_.check_reverse_relations() == [] + assert child_.check_reverse_relations() == [] + assert grandparent_.check_reverse_relations() == [] + + ## My grandchild is also my child ... that sounds fishy + grandparent_.set_relation(child, reltype='CHILD', set_reverse=False) + + ## The check_reverse should tell that something is amiss + missing_parent = grandparent_.check_reverse_relations() + assert len(missing_parent) == 1 + assert missing_parent[0][0].icalendar_component['uid'] == 'ctuid2' + assert missing_parent[0][1] == 'PARENT' + ## But only when run on the grandparent. The child is blissfully + ## unaware who the second parent is (even if reloading it). + child_.load() + assert child_.check_reverse_relations() == [] + def testSetDue(self): self.skip_on_compatibility_flag("read_only") self.skip_on_compatibility_flag("no_todo") From aea84cf6b6c6ce60742ee7a866fc8702d4eb9eb1 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sun, 1 Oct 2023 23:42:22 +0200 Subject: [PATCH 2/7] black style --- caldav/objects.py | 1 - tests/test_caldav.py | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/caldav/objects.py b/caldav/objects.py index 7cb58d2e..56aef232 100755 --- a/caldav/objects.py +++ b/caldav/objects.py @@ -2169,7 +2169,6 @@ def get_relatives( return ret - def check_reverse_relations(self, pdb: bool = False) -> list: """ Goes through all relations and verifies that the return relation is set diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 2953df36..742305a8 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1764,13 +1764,13 @@ def testCreateChildParent(self): assert grandparent_.check_reverse_relations() == [] ## My grandchild is also my child ... that sounds fishy - grandparent_.set_relation(child, reltype='CHILD', set_reverse=False) + grandparent_.set_relation(child, reltype="CHILD", set_reverse=False) ## The check_reverse should tell that something is amiss missing_parent = grandparent_.check_reverse_relations() assert len(missing_parent) == 1 - assert missing_parent[0][0].icalendar_component['uid'] == 'ctuid2' - assert missing_parent[0][1] == 'PARENT' + assert missing_parent[0][0].icalendar_component["uid"] == "ctuid2" + assert missing_parent[0][1] == "PARENT" ## But only when run on the grandparent. The child is blissfully ## unaware who the second parent is (even if reloading it). child_.load() From fa7a724d76e4cf807829adf8effa156d49322ecd Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Sun, 1 Oct 2023 23:53:48 +0200 Subject: [PATCH 3/7] more comments --- caldav/objects.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/caldav/objects.py b/caldav/objects.py index 56aef232..cc5dc0d4 100755 --- a/caldav/objects.py +++ b/caldav/objects.py @@ -1937,13 +1937,19 @@ class CalendarObjectResource(DAVObject): event, a todo-item, a journal entry, or a free/busy entry """ - ## There is also STARTTOFINISH in RFC9253, it does not have any reverse + ## There is also STARTTOFINISH, STARTTOSTART and FINISHTOFINISH in RFC9253, + ## those do not seem to have any reverse + ## (FINISHTOSTART and STARTTOFINISH may seem like reverse relations, but + ## as I read the RFC, FINISHTOSTART seems like the reverse of DEPENDS-ON) + ## (STARTTOSTART and FINISHTOFINISH may also seem like symmetric relations, + ## meaning they are their own reverse, but as I read the RFC they are + ## asymmetric) RELTYPE_REVERSER: ClassVar = { "PARENT": "CHILD", "CHILD": "PARENT", "SIBLING": "SIBLING", ## this is how Tobias Brox inteprets RFC9253: - "DEPENDENT": "FINISHTOSTART", + "DEPENDS-ON": "FINISHTOSTART", "FINISHTOSTART": "DEPENDENT", ## next/first is a special case, linked list ## it needs special handling when length of list<>2 From 456324dba8baaeb388d4685edd789d40b0674251 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Tue, 13 May 2025 20:18:35 +0200 Subject: [PATCH 4/7] refactoring work --- caldav/objects.py | 181 ++++++++++++++++++++++++++-------------------- 1 file changed, 102 insertions(+), 79 deletions(-) diff --git a/caldav/objects.py b/caldav/objects.py index cc5dc0d4..b7096a54 100755 --- a/caldav/objects.py +++ b/caldav/objects.py @@ -3,9 +3,9 @@ A "DAV object" is anything we get from the caldav server or push into the caldav server, notably principal, calendars and calendar events. -(This file has become huge and will be split up prior to the next -release. I think it makes sense moving the CalendarObjectResource -class hierarchy into a separate file) +(This file has become huge and will be split up when the list of +pull requests have been cleared. I think it makes sense moving the +CalendarObjectResource class hierarchy into a separate file) """ import re import sys @@ -43,7 +43,7 @@ class hierarchy into a separate file) from caldav.lib.python_utilities import to_wire try: - from typing import ClassVar, Optional, Union + from typing import ClassVar, Optional, Union, Type TimeStamp = Optional[Union[date, datetime]] except: @@ -849,14 +849,14 @@ def _use_or_create_ics(self, ical, objtype, **ical_data): return vcal.create_ical(objtype=objtype, **ical_data) return ical - ## TODO: consolidate save_* - too much code duplication here - def save_event( + def save_object( self, + objclass: Type[DAVObject], ical: Optional[str] = None, no_overwrite: bool = False, no_create: bool = False, - **ical_data, - ) -> "Event": + **ical_data + ) -> "CalendarResourceObject": """ Add a new event to the calendar, with the given ical. @@ -866,72 +866,46 @@ def save_event( * no_create - don't create a new object, existing calendar objects should be updated * ical_data - passed to lib.vcal.create_ical """ - e = Event( + o = objclass( self.client, - data=self._use_or_create_ics(ical, objtype="VEVENT", **ical_data), + data=self._use_or_create_ics(ical, objtype=f"V{objclass.__name__.upper()}", **ical_data), parent=self, ) - e.save(no_overwrite=no_overwrite, no_create=no_create, obj_type="event") - self._handle_relations(e.id, ical_data) - return e + o = o.save(no_overwrite=no_overwrite, no_create=no_create) + ## TODO: Saving nothing is currently giving an object with None as URL. + ## This should probably be changed in some future version to raise an error + ## See also CalendarObjectResource.save() + if o.url is not None: + o._handle_reverse_relations(fix=True) + return o - def save_todo( - self, - ical: Optional[str] = None, - no_overwrite: bool = False, - no_create: bool = False, - **ical_data, - ) -> "Todo": + ## It could still be possible to refactor even more, but + ## readability would be harder + def save_event(self, *largs, **kwargs) -> "Event": """ - Add a new task to the calendar, with the given ical. - - Parameters: - * ical - ical object (text) + See save_object """ - t = Todo( - self.client, - data=self._use_or_create_ics(ical, objtype="VTODO", **ical_data), - parent=self, - ) - t.save(no_overwrite=no_overwrite, no_create=no_create, obj_type="todo") - self._handle_relations(t.id, ical_data) - return t + return self.save_object(Event, *largs, **kwargs) - def save_journal( - self, - ical: Optional[str] = None, - no_overwrite: bool = False, - no_create: bool = False, - **ical_data, - ) -> "Journal": + def save_todo(self, *largs, **kwargs) -> "Todo": """ - Add a new journal entry to the calendar, with the given ical. - - Parameters: - * ical - ical object (text) + See save_object """ - j = Journal( - self.client, - data=self._use_or_create_ics(ical, objtype="VJOURNAL", **ical_data), - parent=self, - ) - j.save(no_overwrite=no_overwrite, no_create=no_create, obj_type="journal") - self._handle_relations(j.id, ical_data) - return j - - def _handle_relations(self, uid, ical_data) -> None: - for reverse_reltype, other_uid in [ - ("parent", x) for x in ical_data.get("child", ()) - ] + [("child", x) for x in ical_data.get("parent", ())]: - other = self.object_by_uid(other_uid) - other.set_relation(other=uid, reltype=reverse_reltype, set_reverse=False) + return self.save_object(Todo, *largs, **kwargs) + def save_journal(self, *largs, **kwargs) -> "Journal": + """ + See save_object + """ + return self.save_object(Journal, *largs, **kwargs) + ## legacy aliases ## TODO: should be deprecated ## TODO: think more through this - is `save_foo` better than `add_foo`? ## `save_foo` should not be used for updating existing content on the ## calendar! + add_object = save_object add_event = save_event add_todo = save_todo add_journal = save_journal @@ -1093,7 +1067,7 @@ def _request_report_build_resultlist( """ Takes some input XML, does a report query on a calendar object and returns the resource objects found. - """ + """ matches = [] if props is None: props_ = [cdav.CalendarData()] @@ -1944,7 +1918,7 @@ class CalendarObjectResource(DAVObject): ## (STARTTOSTART and FINISHTOFINISH may also seem like symmetric relations, ## meaning they are their own reverse, but as I read the RFC they are ## asymmetric) - RELTYPE_REVERSER: ClassVar = { + RELTYPE_REVERSE_MAP: ClassVar = { "PARENT": "CHILD", "CHILD": "PARENT", "SIBLING": "SIBLING", @@ -1953,8 +1927,8 @@ class CalendarObjectResource(DAVObject): "FINISHTOSTART": "DEPENDENT", ## next/first is a special case, linked list ## it needs special handling when length of list<>2 - "NEXT": "FIRST", - "FIRST": "NEXT", + #"NEXT": "FIRST", + #"FIRST": "NEXT", } _ENDPARAM = None @@ -2087,7 +2061,7 @@ def set_relation( if set_reverse: ## TODO: special handling of NEXT/FIRST. ## STARTTOFINISH does not have any equivalent "reverse". - reltype_reverse = self.RELTYPE_REVERSER[reltype] + reltype_reverse = self.RELTYPE_REVERSE_MAP[reltype] other.set_relation(other=self, reltype=reltype_reverse, set_reverse=False) existing_relation = self.icalendar_component.get("related-to", None) @@ -2175,31 +2149,76 @@ def get_relatives( return ret - def check_reverse_relations(self, pdb: bool = False) -> list: + def _set_reverse_relation(self, other, reltype): + ## TODO: handle RFC9253 better! Particularly next/first-lists + reverse_reltype = self.RELTYPE_REVERSE_MAP.get(reltype) + if not reverse_reltype: + logging.error("Reltype %s not supported in object uid %s" % (reltype, self.id)) + return + other.set_relation(self, reverse_reltype, other) + + def _verify_reverse_relation(self, other, reltype) -> tuple: + revreltype = self.RELTYPE_REVERSE_MAP[reltype] + ## TODO: special case FIRST/NEXT needs special handling + other_relations = other.get_relatives( + fetch_objects=False, reltypes={revreltype} + ) + if ( + not str(self.icalendar_component["uid"]) + in other_relations[revreltype] + ): + ## I don't remember why we need to return a tuple + ## but it's propagated through the "public" methods, so we'll + ## have to leave it like this. + return (other, revreltype) + return False + + def _handle_reverse_relations(self, verify: bool = False, fix: bool = False, pdb: bool = False) -> list: """ Goes through all relations and verifies that the return relation is set - Returns a list of objects missing a reverse (or an empty list if everything is OK) + if verify is set: + Returns a list of objects missing a reverse. + Use public method check_reverse_relations instead + if verify and fix is set: + Fixup all objects missing a reverse. + Use public method fix_reverse_relations instead. + If fix but not verify is set: + Assume all reverse relations are missing. + Used internally when creating new objects. """ ret = [] + assert verify or fix relations = self.get_relatives() for reltype in relations: for other in relations[reltype]: - revreltype = self.RELTYPE_REVERSER[reltype] - ## TODO: special case FIRST/NEXT needs special handling - other_relations = other.get_relatives( - fetch_objects=False, reltypes={revreltype} - ) - if ( - not str(self.icalendar_component["uid"]) - in other_relations[revreltype] - ): - if pdb: - import pdb - - pdb.set_trace() - ret.append((other, revreltype)) + if verify: + foobar = self._verify_reverse_relation(other, reltype) + if foobar: + ret.append(foobar) + if pdb: + breakpoint() + if fix: + self._set_reverse_relation(other, reltype) + elif fix: + self._set_reverse_relation(other, reltype) return ret + def check_reverse_relations(self, pdb: bool = False) -> list: + """ + Will verify that for all the objects we point at though + the RELATED-TO property, the other object points back to us as + well. + """ + return self._handle_reverse_relations(verify=True, fix=False, pdb=pdb) + + def fix_reverse_relations(self, pdb: bool = False) -> list: + """ + Will ensure that for all the objects we point at though + the RELATED-TO property, the other object points back to us as + well. + """ + return self._handle_reverse_relations(verify=True, fix=True, pdb=pdb) + ## TODO: fix this (and consolidate with _handle_relations / set_relation?) # def ensure_reverse_relations(self): # missing_relations = self.check_reverse_relations() @@ -2575,11 +2594,15 @@ def save( * self """ + if not obj_type: + obj_type = self.__class__.__name__.lower() if ( self._vobject_instance is None and self._data is None and self._icalendar_instance is None ): + ## TODO: This makes no sense. We should probably raise an error. + ## But the behaviour should be officially deprecated first. return self path = self.url.path if self.url else None From 9169f0e12b8a81d4f35683df4c6a985284e85f05 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Tue, 13 May 2025 20:33:12 +0200 Subject: [PATCH 5/7] more work --- caldav/objects.py | 37 +++++++++++++++++++++++-------------- tests/test_caldav.py | 5 +++++ 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/caldav/objects.py b/caldav/objects.py index b7096a54..e5dac613 100755 --- a/caldav/objects.py +++ b/caldav/objects.py @@ -855,7 +855,7 @@ def save_object( ical: Optional[str] = None, no_overwrite: bool = False, no_create: bool = False, - **ical_data + **ical_data, ) -> "CalendarResourceObject": """ Add a new event to the calendar, with the given ical. @@ -868,14 +868,16 @@ def save_object( """ o = objclass( self.client, - data=self._use_or_create_ics(ical, objtype=f"V{objclass.__name__.upper()}", **ical_data), + data=self._use_or_create_ics( + ical, objtype=f"V{objclass.__name__.upper()}", **ical_data + ), parent=self, ) o = o.save(no_overwrite=no_overwrite, no_create=no_create) ## TODO: Saving nothing is currently giving an object with None as URL. ## This should probably be changed in some future version to raise an error ## See also CalendarObjectResource.save() - if o.url is not None: + if o.url is not None: o._handle_reverse_relations(fix=True) return o @@ -898,7 +900,7 @@ def save_journal(self, *largs, **kwargs) -> "Journal": See save_object """ return self.save_object(Journal, *largs, **kwargs) - + ## legacy aliases ## TODO: should be deprecated @@ -1067,7 +1069,7 @@ def _request_report_build_resultlist( """ Takes some input XML, does a report query on a calendar object and returns the resource objects found. - """ + """ matches = [] if props is None: props_ = [cdav.CalendarData()] @@ -1927,8 +1929,8 @@ class CalendarObjectResource(DAVObject): "FINISHTOSTART": "DEPENDENT", ## next/first is a special case, linked list ## it needs special handling when length of list<>2 - #"NEXT": "FIRST", - #"FIRST": "NEXT", + # "NEXT": "FIRST", + # "FIRST": "NEXT", } _ENDPARAM = None @@ -2153,7 +2155,9 @@ def _set_reverse_relation(self, other, reltype): ## TODO: handle RFC9253 better! Particularly next/first-lists reverse_reltype = self.RELTYPE_REVERSE_MAP.get(reltype) if not reverse_reltype: - logging.error("Reltype %s not supported in object uid %s" % (reltype, self.id)) + logging.error( + "Reltype %s not supported in object uid %s" % (reltype, self.id) + ) return other.set_relation(self, reverse_reltype, other) @@ -2163,17 +2167,16 @@ def _verify_reverse_relation(self, other, reltype) -> tuple: other_relations = other.get_relatives( fetch_objects=False, reltypes={revreltype} ) - if ( - not str(self.icalendar_component["uid"]) - in other_relations[revreltype] - ): + if not str(self.icalendar_component["uid"]) in other_relations[revreltype]: ## I don't remember why we need to return a tuple ## but it's propagated through the "public" methods, so we'll ## have to leave it like this. return (other, revreltype) return False - def _handle_reverse_relations(self, verify: bool = False, fix: bool = False, pdb: bool = False) -> list: + def _handle_reverse_relations( + self, verify: bool = False, fix: bool = False, pdb: bool = False + ) -> list: """ Goes through all relations and verifies that the return relation is set if verify is set: @@ -2203,11 +2206,14 @@ def _handle_reverse_relations(self, verify: bool = False, fix: bool = False, pdb self._set_reverse_relation(other, reltype) return ret - def check_reverse_relations(self, pdb: bool = False) -> list: + def check_reverse_relations(self, pdb: bool = False) -> List[tuple]: """ Will verify that for all the objects we point at though the RELATED-TO property, the other object points back to us as well. + + Returns a list of tuples. Each tuple contains an object that + do not point back as expected, and the expected reltype """ return self._handle_reverse_relations(verify=True, fix=False, pdb=pdb) @@ -2216,6 +2222,9 @@ def fix_reverse_relations(self, pdb: bool = False) -> list: Will ensure that for all the objects we point at though the RELATED-TO property, the other object points back to us as well. + + Returns a list of tuples. Each tuple contains an object that + did not point back as expected, and the expected reltype """ return self._handle_reverse_relations(verify=True, fix=True, pdb=pdb) diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 742305a8..ee49c48c 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1776,6 +1776,11 @@ def testCreateChildParent(self): child_.load() assert child_.check_reverse_relations() == [] + child.delete() + + assert parent.check_reverse_relations() + assert not grandparent.check_reverse_relations() + def testSetDue(self): self.skip_on_compatibility_flag("read_only") self.skip_on_compatibility_flag("no_todo") From b44e25a373b9f654ca25a054ac118b6f68524ab4 Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Tue, 13 May 2025 21:24:52 +0200 Subject: [PATCH 6/7] test twekups --- tests/test_caldav.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/test_caldav.py b/tests/test_caldav.py index ee49c48c..358e67bd 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1776,10 +1776,15 @@ def testCreateChildParent(self): child_.load() assert child_.check_reverse_relations() == [] - child.delete() - - assert parent.check_reverse_relations() - assert not grandparent.check_reverse_relations() + ## We should be able to fix the missing parent + grandparent_.fix_reverse_relations() + assert not grandparent_.check_reverse_relations() + + ## TODO: + ## This does not work out. A relation to some object that is not on + ## the calendar is not flagged - but perhaps it shouldn't be flagged? + #child.delete() + #assert parent_.check_reverse_relations() def testSetDue(self): self.skip_on_compatibility_flag("read_only") From 533d1de08bf8748ef5c9718f1790104ec8c3989e Mon Sep 17 00:00:00 2001 From: Tobias Brox Date: Tue, 13 May 2025 21:26:42 +0200 Subject: [PATCH 7/7] style fix --- tests/test_caldav.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 358e67bd..a2b4fa50 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1783,8 +1783,8 @@ def testCreateChildParent(self): ## TODO: ## This does not work out. A relation to some object that is not on ## the calendar is not flagged - but perhaps it shouldn't be flagged? - #child.delete() - #assert parent_.check_reverse_relations() + # child.delete() + # assert parent_.check_reverse_relations() def testSetDue(self): self.skip_on_compatibility_flag("read_only")