CalDAV fix search with limit and time range #45222
Conversation
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
from
3fdcf9f
to
d335b1b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
2 times, most recently
from
b5856fd
to
7d2eee6
Compare
kesselb
changed the title
kesselb
added
3. to review
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
from
7d2eee6
to
d2dd88b
Compare
kesselb
requested review from
ChristophWurst,
miaulalala and
tcitworld
as code owners
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
from
d2dd88b
to
b7c3704
Compare
|
Reminder for myself: The search query does not have an "order by" command. |
Databases will probably use an index for the query, or traverse the data according to the primary id. So the sorting will be unpredictable across instances but for the same instance it should be stable. |
| 'name' => 'VCALENDAR', | ||
| 'comp-filters' => [ | ||
| [ | ||
| 'name' => 'VEVENT', |
There was a problem hiding this comment.
Should we limit to VEVENTs ?
https://icalendar.org/CalDAV-Access-RFC-4791/9-9-caldav-time-range-xml-element.html
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ;)
|
Added the test case "testSearchWithLimitAndTimeRangeShouldReturnObjectsInTheSameOrder". Cake Tasting (2024-05-09) and Pasta Day (2024-05-14) match the criteria. Pasta Day was added to the calendar first and therefore is the first result returned by the SQL server. Likely, the SQL server(s) sorted the results by the primary id. I observed locally that the records are not always sorted by the primary id. Especially if you move around event's a lot, it happens that suddenly the order changes. The calendar search api does not document how/if the results are ordered.
|
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
from
9410085
to
9fa82f2
Compare
Then we are free to add sorting :) Pick a column. Ideally it's an indexed one. |
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
from
9fa82f2
to
d8189bf
Compare
|
|
||
| $hasLimit = is_int($limit); | ||
| $hasTimeRange = false; | ||
|
|
||
| if (isset($options['timerange'])) { |
There was a problem hiding this comment.
TIL! You can chuck the initial isset check on $options['timerange'] as isset can handle deep checks with possible non-existing values for the upper arrays
There was a problem hiding this comment.
Good idea, indeed easier to read now.
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
2 times, most recently
from
458b1cb
to
dea1906
Compare
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. Signed-off-by: Daniel Kesselberg <[email protected]>
Sorting the events by the start date leads to more predictable results for the search API consumers. Signed-off-by: Daniel Kesselberg <[email protected]>
kesselb
force-pushed
the
bug/noid/caldav-search-limit-and-timerange
branch
from
dea1906
to
b769dc4
Compare
kesselb
added
4. to release
|
/backport to stable29 |
|
/backport to stable28 |
|
/backport to stable27 |
Summary
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.
TODO
Test cases
search_with_limit_and_timerange.txt
A sample calendar with 7 reoccurring events (in January 2024) and two events happening this month.
GitHub does now allow ICS attachments, thus don't forget to rename.
On master/main the upcoming events widget should be empty.
On this branch Cake Tasting and Pasta Day should appear in the widget.
Checklist