Expand recurring events client-side

[daniele-athome]

Hi there, I've been trying to implement a fix for #157. I've been testing this with my Home Assistant installation (ref. home-assistant/core#40127) and it seems to be working, but I don't know if I'm doing this right (I've worked on this for like 3 hours). A few challenges I had:

• I'm using a 3rd party library (recurring_ical_events) to expand events, which uses icalendar
• recurring_ical_events returns icalendar.Event objects which need to be converted to caldav.Event
• To do that I needed to wrap those icalendar.Events into icalendar.Calendar
• I then added the other (non-VEVENT) components from the original caldav.Event into all those expanded icalendar.Calendar
• Finally I created caldav.Event like this: caldav.Event(data=icalendar_Calendar_object.to_ical()

Please be aware that this is PoC-quality code: it's meant as a way to address this issue and start having feedback. I will rewrite it from scratch when everyone is confident that it could work - I will leave the draft status until that.

Before going deeper into this (e.g. writing tests) I'd like to have some feedback please.

This patch needs the following packages installed:

• icalendar
• recurring_ical_events

[tobixen]

I've added some "critical" comments as direct comments towards the code, but don't get me wrong - this is cool, and I want to merge it.

Some general comments:

• I've started doing some refactoring work and most of the logic that used to be in the date_search method is now in the search-method. The date_search-method will be deprecated at some point, as one can as well use search instead. This means this client-side logic should be moved there.
• If this code works, why bother with server side expansion at all? Perhaps client-side CPU is cheaper than bandwidth? I do have an idea of adding attributes (or "compatibility flags" - based on tests/compatibility_issues.py) to the calendar connection object for deciding such behaviour.
• I think I saw some long lines there. Personally I loath overly wrapped lines and I don't have problems with long lines ... but there are code style standards for Python, probably it's a nice idea to adhere to the standards. It's not so difficult, just do a tox -e style and the code will be autoformatted.

[daniele-athome]

Thanks for your review!

I've started doing some refactoring work and most of the logic that used to be in the date_search method is now in the search-method. The date_search-method will be deprecated at some point, as one can as well use search instead. This means this client-side logic should be moved there.

Ok I'll look into it.

If this code works, why bother with server side expansion at all? Perhaps client-side CPU is cheaper than bandwidth? I do have an idea of adding attributes (or "compatibility flags" - based on tests/compatibility_issues.py) to the calendar connection object for deciding such behaviour.

I'm not sure... what do the various RFCs have to say about this? I didn't look honestly.
One thing I can say (which I'm sure you already know) is that some clients already do client-side expansion (e.g. DAVx5 for sure since I use it with a SOGo server).

I think I saw some long lines there. Personally I loath overly wrapped lines and I don't have problems with long lines ... but there are code style standards for Python, probably it's a nice idea to adhere to the standards. It's not so difficult, just do a tox -e style and the code will be autoformatted.

Sure! As I said this is really a PoC.

I have a couple of questions for you:

• what about the TODO at this line? Does it make sense to inherit the parent event data? Do the recurring event and the "occurance events" share the same URL or ID for instance? (depending on the answer it might even make sense to let the server do the expansion as a general rule)
• does it make sense to add the other components to the occurance events from the recurring event? As I've done here; honestly I don't even know if just skipping all VEVENTs is right in that situation; also this "copying of the components" will have the RRULE copied too, which I don't think it's right... or is it?

[tobixen]

The expand logic should only return one caldav.Event or caldav.Todo-object, which wraps icalendar data with one VCALENDAR and multiple (expanded) VEVENT or VTODO, hence your question on what attributes to duplicate in the caldav.Event-class is sort of moot.

I'm now working on a split_expanded-method. It will return [ self ] for ordinary objects, and a list of caldav.Event or caldav.Todo if the icalendar data contains multiple recurrences. My idea is that the calendar.search-method by default will do a split_expanded (because this is easier and more expected by end-clients), but that date_search will return one object with multiple recurrences (due to backward compatibility).

[tobixen]

On the icalendar level, recurrence instances should inherit most of the data from the parent, but RRULE should be removed and a RECURRENCE-ID should be added. The RFC is not very clear on those details though, and different implementations may do things a bit differently.

[tobixen]

In the expanded set of recurrences, none of the VEVENTs should have RRULE set. The RFC is very clear on that.

[tobixen]

See https://www.rfc-editor.org/rfc/rfc4791#section-7.8.3 and https://www.rfc-editor.org/rfc/rfc4791#section-9.6.5 for information on how the expanded icalendar data should look.

The returned calendar components MUST NOT use recurrence properties (i.e., EXDATE, EXRULE, RDATE, and RRULE) and MUST NOT have reference to or include VTIMEZONE components. Date and local time with reference to time zone information MUST be converted into date with UTC time.

[daniele-athome]

I wasn't considering the Todo objects by the way...

The expand logic should only return one caldav.Event or caldav.Todo-object, which wraps icalendar data with one VCALENDAR and multiple (expanded) VEVENT or VTODO, hence your question on what attributes to duplicate in the caldav.Event-class is sort of moot.

I'm talking about the non-VCALENDAR attributes such as client, id, URL, parent, coming from DAVObject. Or am I getting it wrong?

I'm now working on a split_expanded-method. It will return [ self ] for ordinary objects, and a list of caldav.Event or caldav.Todo if the icalendar data contains multiple recurrences. My idea is that the calendar.search-method by default will do a split_expanded (because this is easier and more expected by end-clients), but that date_search will return one object with multiple recurrences (due to backward compatibility).

Wait, are we working on the same thing? Should I stop? :-)

[tobixen]

I wasn't considering the Todo objects by the way...

It is not particular normal, and perhaps not even particularly useful to expand recurring TODOs, but it's still important to consider them :-)

I'm talking about the non-VCALENDAR attributes such as client, id, URL, parent, coming from DAVObject. Or am I getting it wrong?

You should not be initiating multiple caldav.Event-objects in your expandation logic - you should only create one big VCALENDAR with multiple VEVENTs (each having the same UID as the parent, but also the RECURRENCE-ID set)

Wait, are we working on the same thing? Should I stop? :-)

I'm not doing any expansion in my code, I am only doing the splitting/copying from one Event-objects with a recurrence set in the data, into multiple Event-objects with one recurrence in each of them. So your code should not generate new Event-objects.

[tobixen]

My code is in #223, though I've managed to mix up my commits a bit, so it's mixed together with some bugfixes now. I'll try to split it up in two changesets.

[daniele-athome]

So far I've addressed the following (almost) successfully:

• I used dateutil.rrule
• I removed the not allowed properties from the newly generated events (exdate, exrule, and so on; also VTIMEZONE is removed if I got the RFC right) and used DTSTART as RECURRENCE-ID (we can change it, it's just a test)
• I created one VCALENDAR with multiple VEVENTS

However, I said almost because dateutil.rrule can't process datetimes with time zone information (which is what Home Assistant uses):

  File "/home/homeassistant/hass/lib/python3.9/site-packages/caldav/objects.py", line 931, in date_search
    expanded_objects.append(_expand_event(o, start, end))
  File "/home/homeassistant/hass/lib/python3.9/site-packages/caldav/objects.py", line 59, in _expand_event
    recurrings = rrulestr(event.instance.vevent.rrule.value).between(start, end, True)
  File "/home/homeassistant/hass/lib/python3.9/site-packages/dateutil/rrule.py", line 284, in between
    if i > before:
TypeError: can't compare offset-naive and offset-aware datetimes

There is some discussion on the matter:

• https://github.com/dateutil/dateutil/issues/979
• https://github.com/dateutil/dateutil/issues/341

I'll look further into it. In the meantime you might want to try your code in #223 against mine.

[tobixen]

If you get it to work with the recurring_ical_events library, but not with the dateutil.rrule, then it's no point spending too much time arguing with dateutil.rrule.

[tobixen]

I have the branch feature_split_expanded now in pull request #225 featuring the new method split_expanded. Can you manage to rebase your work on that branch? (if not, then I'll just pull in your branch).

I will write up some test code before merging #225 to master, though came to think I can use your function for creating test data for testing my function :-)

[tobixen]

I see the test code fails now, because the default value for expand is maybe. This is a backward-compatible thing, if will be switched to False if the date-search is open-ended. Here is the current code:

        if expand == "maybe":
            expand = end

I also think it should be made into a method under the CalendarResourceObject class rather than a function. I can take over if you want, but to get the annotations right it's better you do it :-)

[daniele-athome]

Ok I've rebased my master branch onto #225. Before I follow up on your deprecation notice on date_search and all your other review comments, I have to say I've been struggling with dateutil.rrule. It does work in principle, but:

• I had to manually produce a RECURRENCE-ID (not really a problem though)
• I need to manually check for EXDATEs and EXRULEs (there is a way to do that with dateutil but everything needs to be extracted manually from the event)
• (Who knows what else I need to do manually that I've not taken into account)

So I'm gonna go ahead on your advice and go back to using recurring_ical_events which was already doing all of the above and probably much better.

[daniele-athome]

I'm trying to move the expansion logic in search. How do I access start date and end date when only the query XML is provided? I guess I could just walk through the XML tree but it seems overkill.

[tobixen]

Unfortunately I will be quite busy this week and the start of next week.

Please have a look at eee05e0188b9177674eb64a6c39a1bbe78e2529e ... possibly cherry-pick it into your branch. I didn't get time to verify if all my changes are sane. Tests still break though, so it's needed to work a bit more on it.

[tobixen]

(The style changes in that commit was made by the automated code styling tool ... I probably shouldn't have been running the code through it, in this case it just adds noise ... sorry for that)

[tobixen]

Obviously that eee05e0 commit is quite broken, new one coming up shortly.

[tobixen]

I have the branch 222 now, with one more commit ... baf20c8 ... but tests are still broken, so it's needed to look more into it. I'm not sure when I eventually will have time for it.

Perhaps the best approach now is to write up a fresh unit test covering the expand_rrule method - it should be in tests/test_caldav_unit.py, a new test that first creates an Event object with some icalendar data containing an RRULE, running the self.expand_rrule method, and then do some asserts on self.data. When we know that the expand_rrule method works as intended (and I think it should modify self rather than return a new object) we can go on and hammer the search-method into submission.

[daniele-athome]

Merged, thanks.

(and I think it should modify self rather than return a new object)

It's fine either way for me. I'll modify it in that way if you want.

I'll consolidate all the commits and force push again onto this PR then I'll try looking into the tests.

P.S. I put some NotImplementedError throws in search for the moment.

[tobixen]

P.S. I put some NotImplementedError throws in search for the moment.

I noticed, and I'm ok with that.

-- Tobias Brox Senior System Consultant Redpill Linpro AS - Changing the game Mobil: +47 917 000 50 Kontor: +47 215 441 68

[tobixen]

I may have a small time slot now, if I'm lucky I will be able to make the test code

[tobixen]

Started writing test code. The bad thing with expanding the object in-line is that information is lost, so the object needs to be initiated for each test that is done. Anyway ... the expanded data should contain the RECURRENCE-ID header. I think it should be equal to DTSTART.

[tobixen]

$ pytest --pdb -k TestExpandRRule
================================== test session starts ===================================
platform linux -- Python 3.10.8, pytest-7.1.3, pluggy-1.0.0
rootdir: /home/tobias/caldav
collected 103 items / 101 deselected / 2 selected

tests/test_caldav_unit.py .F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

self = <tests.test_caldav_unit.TestExpandRRule object at 0x7f7d674c9c90>

    def testOne(self):
        self.yearly.expand_rrule(start=datetime(1998,10,10), end=datetime(1998,12,12))
        assert len(self.yearly.icalendar_instance.subcomponents) == 1
        assert not 'RRULE' in self.yearly.icalendar_object()
>       assert 'RECURRENCE-ID' in self.yearly.icalendar_object()
E       AssertionError: assert 'RECURRENCE-ID' in VEVENT({'UID': vText('b'19970901T130000Z-123403@example.com''), 'DTSTART': <icalendar.prop.vDDDTypes object at 0x7f7d6...ssful Anniversary''), 'TRANSP': vText('b'TRANSPARENT''), 'DTEND': <icalendar.prop.vDDDTypes object at 0x7f7d674c75b0>})
E        +  where VEVENT({'UID': vText('b'19970901T130000Z-123403@example.com''), 'DTSTART': <icalendar.prop.vDDDTypes object at 0x7f7d6...ssful Anniversary''), 'TRANSP': vText('b'TRANSPARENT''), 'DTEND': <icalendar.prop.vDDDTypes object at 0x7f7d674c75b0>}) = <bound method CalendarObjectResource.icalendar_object of Event(Event: None)>()
E        +    where <bound method CalendarObjectResource.icalendar_object of Event(Event: None)> = Event(Event: None).icalendar_object
E        +      where Event(Event: None) = <tests.test_caldav_unit.TestExpandRRule object at 0x7f7d674c9c90>.yearly

tests/test_caldav_unit.py:181: AssertionError
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> entering PDB >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

>>>>>>>>>>>>>>>>>>>>>>> PDB post_mortem (IO-capturing turned off) >>>>>>>>>>>>>>>>>>>>>>>>
> /home/tobias/caldav/tests/test_caldav_unit.py(181)testOne()
-> assert 'RECURRENCE-ID' in self.yearly.icalendar_object()
(Pdb) c

>>>>>>>>>>>>>>>>>>>>>>>>>> PDB continue (IO-capturing resumed) >>>>>>>>>>>>>>>>>>>>>>>>>>>
                                                                                   [100%]

==================================== warnings summary ====================================
../../../usr/lib/python3.10/site-packages/nose/importer.py:12
  /usr/lib/python3.10/site-packages/nose/importer.py:12: DeprecationWarning: the imp module is deprecated in favour of importlib and slated for removal in Python 3.12; see the module's documentation for alternative uses
    from imp import find_module, load_module, acquire_lock, release_lock

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================ short test summary info =================================
FAILED tests/test_caldav_unit.py::TestExpandRRule::testOne - AssertionError: assert 'RE...
================= 1 failed, 1 passed, 101 deselected, 1 warning in 4.25s =================

[tobixen]

(the above is from the 222-branch. You may pull in the changes from that branch eventually)

[daniele-athome]

Thanks. I committed some minor mistakes that I see you pulled. Something is not working right though. I'm checking now.

[tobixen]

Thanks. I committed some minor mistakes that I see you pulled. Something is not working right though. I'm checking now.

Except for the recurrence-id, the expand logic seems to work fine.

-- Tobias Brox Senior System Consultant Redpill Linpro AS - Changing the game Mobil: +47 917 000 50 Kontor: +47 215 441 68

[daniele-athome]

Ok sorry it was my fault, it all seems to work now. The tests passed too after applying 506a282f8c45270b2bc1b0a7160be90cbbed0662. What's left (in objects.py):

# TODO remove or downgrade to debug
logging.info(

I'd remove it altogether, little to no added value.

# FIXME too much copying
stripped_event = self.copy(keep_uid=True)

Surely there is a better way to do this.

Then I'll begin squash some of the commits - history is too dirty for merging.

[tobixen]

Yes, everything should be squashed, but it can wait until all the tests pass. Check my last commit in the 222-branch :-)

[tobixen]

Regarding instance vs object vs ...

• event.instance is a property that gives an object from the vobject module. It's deprecated, it's there for backward-compatbility.
• event.vobject_instance is an alias of the one above. It's preferred to be explicit. Since most people seem to prefer the icalendar module rather than the vobject module ... from version 0.10 I'm trying to phase out internal usage of vobject.
• event.icalendar_instance is the preferred one. It will return an object from the icalendar module.
• event.icalendar_object() returns the subcomponent (event or task or journal). I'm a bit unhappy with the naming here, I will probably create an event.icalendar_subcomponent property instead.

Internally the icalendar data is always present as either raw data, an icalendar object or a vobject object. Whenever one of those "magic" properties are used, if other representations exists, it will be converted and the old representation will be thrown away. Now there is a slightly bad thing with this ... such properties are not supposed to have side effects, but details like ordering of the properties and line wrapping may be changed when accessing the properties. I also see that there may be a potential for bugs there.

[tobixen]

recurring_ical_events() seems to ignore tasks.

[tobixen]

Three failing tests now, and they are all related to the module not supporting tasks. I will consider making work-arounds in the test code unless this can be fixed in the module relatively fast.

The logging ... yes, probably better to just remove it. As for the copying - I will think a bit if I can come up with a better idea, but the overhead is not too bad anyway.

[tobixen]

FAILED tests/test_caldav.py::TestLocalRadicale::testTodoDatesearch - assert 0 == 1
FAILED tests/test_caldav.py::TestLocalXandikos::testTodoDatesearch - assert 0 == 1
FAILED tests/test_caldav_unit.py::TestExpandRRule::testThreeTodo - AssertionError: asse...

[daniele-athome]

I was going to open an issue to recurring_ical_events when I found this: niccokunzmann/python-recurring-ical-events#97 :-)

I'm here if you need help with something - I might get busy too in the following days but I can usually act within 24 hours or so.

[tobixen]

I sort of started to write up a test code for tasks in the python-recurring-ical-events project, but got interrupted with it. I'll get it done now

[tobixen]

I'm here if you need help with something - I might get busy too in the following days but I can usually act within 24 hours or so.

As you can see, I wrote up a test for the python-recurring-ical-events project, but I don't have time writing up the code change now. If you beat me at it you may give it a go :-)

[tobixen]

Let's see ... if I remember right the only reason why this isn't merged is that the testTodoDatesearch does not pass, and the reason for that is that the recurring-ical-events library does not support todos. The code is in place in a pull request there now, but in the meanwhile another test on that project broke after the latest release of a new icalendar library. The author of the recurring-ical-events library has written up a pull request for the icalendar library ... but ... it's like a never-ending rabbit hole. I think I'll just make some temporary workaround and skip the broken tests as for now so that v0.10.1 can be released.

[tobixen]

I squashed some of the commits, pulled it into master, polished a bit and put code in place for skipping two broken tests.

Will try to get v0.10.1 released (just need to run tests towards a lot of different servers first)

[daniele-athome]

Thank you! I'll begin working on the Home Assistant side to use this.