diff --git a/caldav/objects.py b/caldav/objects.py index ff4026ec..e5dac613 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": + ) -> "CalendarResourceObject": """ Add a new event to the calendar, with the given ical. @@ -866,65 +866,40 @@ 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 + return self.save_object(Todo, *largs, **kwargs) - 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) + def save_journal(self, *largs, **kwargs) -> "Journal": + """ + See save_object + """ + return self.save_object(Journal, *largs, **kwargs) ## legacy aliases ## TODO: should be deprecated @@ -932,7 +907,7 @@ 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_object = save_object add_event = save_event add_todo = save_todo add_journal = save_journal @@ -1938,10 +1913,24 @@ class CalendarObjectResource(DAVObject): event, a todo-item, a journal entry, or a free/busy entry """ - RELTYPE_REVERSER: ClassVar = { + ## 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_REVERSE_MAP: ClassVar = { "PARENT": "CHILD", "CHILD": "PARENT", "SIBLING": "SIBLING", + ## this is how Tobias Brox inteprets RFC9253: + "DEPENDS-ON": "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,7 +2061,9 @@ def set_relation( if set_reverse: other = self.parent.object_by_uid(uid) if set_reverse: - reltype_reverse = self.RELTYPE_REVERSER[reltype] + ## TODO: special handling of NEXT/FIRST. + ## STARTTOFINISH does not have any equivalent "reverse". + 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) @@ -2119,6 +2110,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 +2151,88 @@ def get_relatives( return ret + 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 + 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]: + 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[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) + + 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) + + ## 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 @@ -2526,11 +2603,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 diff --git a/tests/test_caldav.py b/tests/test_caldav.py index 21663620..a2b4fa50 100644 --- a/tests/test_caldav.py +++ b/tests/test_caldav.py @@ -1758,6 +1758,34 @@ 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() == [] + + ## 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") self.skip_on_compatibility_flag("no_todo")