Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CalDAV fix search with limit and time range #45222

Merged
merged 2 commits into from
May 29, 2024
Merged

CalDAV fix search with limit and time range #45222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4718a7e
fix(caldav): event search with limit and timerange
kesselb May 7, 2024
b769dc4
feat(caldav): order the calendar objects by start date for search
kesselb May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 122 additions & 52 deletions apps/dav/lib/CalDAV/CalDavBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
namespace OCA\DAV\CalDAV;

use DateTime;
use DateTimeImmutable;
use DateTimeInterface;
use OCA\DAV\AppInfo\Application;
use OCA\DAV\CalDAV\Sharing\Backend;
Expand Down Expand Up @@ -1887,15 +1888,34 @@ public function search(
$this->db->escapeLikeParameter($pattern) . '%')));
}

if (isset($options['timerange'])) {
if (isset($options['timerange']['start']) && $options['timerange']['start'] instanceof DateTimeInterface) {
$outerQuery->andWhere($outerQuery->expr()->gt('lastoccurence',
$outerQuery->createNamedParameter($options['timerange']['start']->getTimeStamp())));
}
if (isset($options['timerange']['end']) && $options['timerange']['end'] instanceof DateTimeInterface) {
$outerQuery->andWhere($outerQuery->expr()->lt('firstoccurence',
$outerQuery->createNamedParameter($options['timerange']['end']->getTimeStamp())));
}
$start = null;
$end = null;

$hasLimit = is_int($limit);
$hasTimeRange = false;

if (isset($options['timerange']['start']) && $options['timerange']['start'] instanceof DateTimeInterface) {
/** @var DateTimeInterface $start */
$start = $options['timerange']['start'];
$outerQuery->andWhere(
$outerQuery->expr()->gt(
'lastoccurence',
$outerQuery->createNamedParameter($start->getTimestamp())
)
);
$hasTimeRange = true;
}

if (isset($options['timerange']['end']) && $options['timerange']['end'] instanceof DateTimeInterface) {
/** @var DateTimeInterface $end */
$end = $options['timerange']['end'];
$outerQuery->andWhere(
$outerQuery->expr()->lt(
'firstoccurence',
$outerQuery->createNamedParameter($end->getTimestamp())
)
);
$hasTimeRange = true;
}

if (isset($options['uid'])) {
Expand All @@ -1913,54 +1933,46 @@ public function search(

$outerQuery->andWhere($outerQuery->expr()->in('c.id', $outerQuery->createFunction($innerQuery->getSQL())));

if ($offset) {
$outerQuery->setFirstResult($offset);
}
if ($limit) {
$outerQuery->setMaxResults($limit);
}
// Without explicit order by its undefined in which order the SQL server returns the events.
// For the pagination with hasLimit and hasTimeRange, a stable ordering is helpful.
$outerQuery->addOrderBy('id');

$result = $outerQuery->executeQuery();
$calendarObjects = [];
while (($row = $result->fetch()) !== false) {
$start = $options['timerange']['start'] ?? null;
$end = $options['timerange']['end'] ?? null;
$offset = (int)$offset;
$outerQuery->setFirstResult($offset);

if ($start === null || !($start instanceof DateTimeInterface) || $end === null || !($end instanceof DateTimeInterface)) {
// No filter required
$calendarObjects[] = $row;
continue;
}
$calendarObjects = [];

$isValid = $this->validateFilterForObject($row, [
'name' => 'VCALENDAR',
'comp-filters' => [
[
'name' => 'VEVENT',
'comp-filters' => [],
'prop-filters' => [],
'is-not-defined' => false,
'time-range' => [
'start' => $start,
'end' => $end,
],
],
],
'prop-filters' => [],
'is-not-defined' => false,
'time-range' => null,
]);
if (is_resource($row['calendardata'])) {
// Put the stream back to the beginning so it can be read another time
rewind($row['calendardata']);
}
if ($isValid) {
$calendarObjects[] = $row;
}
if ($hasLimit && $hasTimeRange) {
miaulalala marked this conversation as resolved.
Show resolved Hide resolved
/**
* Event recurrences are evaluated at runtime because the database only knows the first and last occurrence.
*
* Given, a user created 8 events with a yearly reoccurrence and two for events tomorrow.
* The upcoming event widget asks the CalDAV backend for 7 events within the next 14 days.
*
* If limit 7 is applied to the SQL query, we find the 7 events with a yearly reoccurrence
* and discard the events after evaluating the reoccurrence rules because they are not due within
* the next 14 days and end up with an empty result even if there are two events to show.
*
* The workaround for search requests with a limit and time range is asking for more row than requested
* and retrying if we have not reached the limit.
*
* 25 rows and 3 retries is entirely arbitrary.
*/
$maxResults = (int)max($limit, 25);
$outerQuery->setMaxResults($maxResults);

for ($attempt = $objectsCount = 0; $attempt < 3 && $objectsCount < $limit; $attempt++) {
$objectsCount = array_push($calendarObjects, ...$this->searchCalendarObjects($outerQuery, $start, $end));
$outerQuery->setFirstResult($offset += $maxResults);
}
kesselb marked this conversation as resolved.
Show resolved Hide resolved

$calendarObjects = array_slice($calendarObjects, 0, $limit, false);
} else {
$outerQuery->setMaxResults($limit);
$calendarObjects = $this->searchCalendarObjects($outerQuery, $start, $end);
}
$result->closeCursor();

return array_map(function ($o) use ($options) {
$calendarObjects = array_map(function ($o) use ($options) {
$calendarData = Reader::read($o['calendardata']);

// Expand recurrences if an explicit time range is requested
Expand Down Expand Up @@ -1996,6 +2008,64 @@ public function search(
}, $timezones),
];
}, $calendarObjects);

usort($calendarObjects, function (array $a, array $b) {
/** @var DateTimeImmutable $startA */
$startA = $a['objects'][0]['DTSTART'][0] ?? new DateTimeImmutable(self::MAX_DATE);
/** @var DateTimeImmutable $startB */
$startB = $b['objects'][0]['DTSTART'][0] ?? new DateTimeImmutable(self::MAX_DATE);

return $startA->getTimestamp() <=> $startB->getTimestamp();
});

return $calendarObjects;
}

private function searchCalendarObjects(IQueryBuilder $query, DateTimeInterface|null $start, DateTimeInterface|null $end): array {
$calendarObjects = [];
$filterByTimeRange = ($start instanceof DateTimeInterface) || ($end instanceof DateTimeInterface);

$result = $query->executeQuery();

while (($row = $result->fetch()) !== false) {
if ($filterByTimeRange === false) {
// No filter required
$calendarObjects[] = $row;
continue;
}

$isValid = $this->validateFilterForObject($row, [
'name' => 'VCALENDAR',
'comp-filters' => [
[
'name' => 'VEVENT',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit to VEVENTs ?
https://icalendar.org/CalDAV-Access-RFC-4791/9-9-caldav-time-range-xml-element.html

Copy link
Contributor Author

@kesselb kesselb May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted, thank you 🙏

A user mentioned that the widget is not showing items from the tasks app anymore.
I assume that could be the reason.

What do you suggest, extending to VEVENT, VTODO?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, yes. However, from what I can see in the validateCompFilters implementation, you would need to call the method again with the same payload, only changing the name property, you can't add a second comp-filter directly, which sounds…bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the missing tasks / the VTODO handling and wrote down my findings at #45333.
It's more work than I imagined, and therefore I need to discuss it with Christoph.
Happy to have your feedback / pointers on that one ;)

'comp-filters' => [],
'prop-filters' => [],
'is-not-defined' => false,
'time-range' => [
'start' => $start,
'end' => $end,
],
],
],
'prop-filters' => [],
'is-not-defined' => false,
'time-range' => null,
]);

if (is_resource($row['calendardata'])) {
// Put the stream back to the beginning so it can be read another time
rewind($row['calendardata']);
}

if ($isValid) {
$calendarObjects[] = $row;
}
}

$result->closeCursor();

return $calendarObjects;
}

/**
Expand Down
17 changes: 17 additions & 0 deletions apps/dav/tests/misc/caldav-search-limit-timerange-1.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T105946Z
LAST-MODIFIED:20240507T121113Z
DTSTAMP:20240507T121113Z
UID:07514c7b-1014-425c-b1b8-2c35ab0eea1d
SUMMARY:Event A
RRULE:FREQ=YEARLY
DTSTART;TZID=Europe/Berlin:20240101T101500
DTEND;TZID=Europe/Berlin:20240101T111500
TRANSP:OPAQUE
X-MOZ-GENERATION:4
SEQUENCE:2
END:VEVENT
END:VCALENDAR
17 changes: 17 additions & 0 deletions apps/dav/tests/misc/caldav-search-limit-timerange-2.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T110122Z
LAST-MODIFIED:20240507T121120Z
DTSTAMP:20240507T121120Z
UID:67cf8134-ff10-49a7-913d-acfeda463db6
SUMMARY:Event B
RRULE:FREQ=YEARLY
DTSTART;TZID=Europe/Berlin:20240101T123000
DTEND;TZID=Europe/Berlin:20240101T133000
TRANSP:OPAQUE
X-MOZ-GENERATION:4
SEQUENCE:2
END:VEVENT
END:VCALENDAR
17 changes: 17 additions & 0 deletions apps/dav/tests/misc/caldav-search-limit-timerange-3.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T120352Z
LAST-MODIFIED:20240507T121128Z
DTSTAMP:20240507T121128Z
UID:59090ca1-e52b-447f-8e08-491d1da729fa
SUMMARY:Event C
RRULE:FREQ=YEARLY
DTSTART;TZID=Europe/Berlin:20240101T151000
DTEND;TZID=Europe/Berlin:20240101T161000
TRANSP:OPAQUE
X-MOZ-GENERATION:2
SEQUENCE:1
END:VEVENT
END:VCALENDAR
17 changes: 17 additions & 0 deletions apps/dav/tests/misc/caldav-search-limit-timerange-4.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T120414Z
LAST-MODIFIED:20240507T121134Z
DTSTAMP:20240507T121134Z
UID:b1814d32-9adf-4518-8535-37f2c037f423
SUMMARY:Event D
RRULE:FREQ=YEARLY
DTSTART;TZID=Europe/Berlin:20240101T164500
DTEND;TZID=Europe/Berlin:20240101T171500
TRANSP:OPAQUE
SEQUENCE:2
X-MOZ-GENERATION:3
END:VEVENT
END:VCALENDAR
14 changes: 14 additions & 0 deletions apps/dav/tests/misc/caldav-search-limit-timerange-5.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T122221Z
LAST-MODIFIED:20240507T122237Z
DTSTAMP:20240507T122237Z
UID:19c4e049-0b09-4101-a2ad-061a837e6a5e
SUMMARY:Cake Tasting
DTSTART;TZID=Europe/Berlin:20240509T151500
DTEND;TZID=Europe/Berlin:20240509T171500
TRANSP:OPAQUE
END:VEVENT
END:VCALENDAR
15 changes: 15 additions & 0 deletions apps/dav/tests/misc/caldav-search-limit-timerange-6.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T122246Z
LAST-MODIFIED:20240507T175258Z
DTSTAMP:20240507T175258Z
UID:60a7d310-aa7b-4974-8a8a-ff9339367e1d
SUMMARY:Pasta Day
DTSTART;TZID=Europe/Berlin:20240514T123000
DTEND;TZID=Europe/Berlin:20240514T133000
TRANSP:OPAQUE
X-MOZ-GENERATION:2
END:VEVENT
END:VCALENDAR
14 changes: 14 additions & 0 deletions apps/dav/tests/misc/caldav-search-missing-start-1.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T122246Z
LAST-MODIFIED:20240507T175258Z
DTSTAMP:20240507T175258Z
UID:39e1b04f-d1cc-4622-bf97-11c38e070f43
SUMMARY:Missing DTSTART 1
DTEND;TZID=Europe/Berlin:20240514T133000
TRANSP:OPAQUE
X-MOZ-GENERATION:2
END:VEVENT
END:VCALENDAR
14 changes: 14 additions & 0 deletions apps/dav/tests/misc/caldav-search-missing-start-2.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
BEGIN:VCALENDAR
PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN
VERSION:2.0
BEGIN:VEVENT
CREATED:20240507T122246Z
LAST-MODIFIED:20240507T175258Z
DTSTAMP:20240507T175258Z
UID:12413feb-4b8c-4e95-ae7f-9ec4f42f3348
SUMMARY:Missing DTSTART 2
DTEND;TZID=Europe/Berlin:20240514T133000
TRANSP:OPAQUE
X-MOZ-GENERATION:2
END:VEVENT
END:VCALENDAR
Loading
Loading