Skip to content

Use first_instance_start and last_instance_end to filter freebusy queries

Jamie McClymont requested to merge JJJollyjim/davical:refactor-freebusy into master

Background

We run a fairly large DAViCal server with users in many time zones, which is configured (as is the default) to run Postgres queries under SET TIME ZONE 'UTC'.

We often run into issues with events missing from freebusy. I have determined that most (all?) of these issues stem from limitations and bugs in the PL/pgSQL rrule_functions.sql, many of which appear only when the postgres time zone is not the same as the local time zone - this means the test suites never catch them! I had a go at fixing this, but it was going to be some pretty harrowing code, and without tests it seemed like a poor choice.

New approach

Due to the various documented limitations of that code, I determined the best approach was to effectively deprecate rrule_event_overlaps, instead using the first_instance_start and last_instance_end fields on calendar_item to filter the SQL query*, before the PHP RRule code does the final filtering.

* rrule_event_overlaps is still used when those fields have not been populated

The tiny problem with using first_instance_start and last_instance_end is that those columns are never populated! Thus this merge request also populates the columns for new/updated events, and I will at some point write a script to update all existing events. Additionally, these columns were the only remaining non-timezone-aware timestamps in the database, so a database migration was written to resolve this.


For consistency with !57 (merged), getVCalendarRange was modified to support a fallback timezone for floating/all-day events, which is used to populate first_instance_start and last_instance_end. A todo item for the future: recalculate these fields when a collection's fallback timezone changes.

While I was working on this, I found myself really wanting some more traditional unit tests so I could do a TDD approach. Thus I wrote up a few PHPUnit tests which are included in cf7de16e. Hopefully these won't be too controversial an inclusion: I see them as a useful bonus for cases where you want to define the behaviour of a function before you write it, complementing the regression suite. Another thing on my todo list is to run these tests in CI - this should be fairly simple :)

There is one change to regression-suite output (not including the schema version bump): see the commit message of e449529f for details.

Edited by Jamie McClymont

Merge request reports