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.

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

fix: (CalDav) Delete invitation link when deleting Calendars or Events #47832

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

SebastianKrupinski
Copy link
Contributor

Summary

When deleting a calendar or event also delete event invitation links

*
* @param int $calendarId
*/
protected function purgeCalendarInvitations(int $calendarId) {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\DAV\CalDAV\CalDavBackend::purgeCalendarInvitations does not have a return type, expecting void
*
* @param string $eventId UID of the event
*/
protected function purgeObjectInvitations(string $eventId) {

Check notice

Code scanning / Psalm

MissingReturnType

Method OCA\DAV\CalDAV\CalDavBackend::purgeObjectInvitations does not have a return type, expecting void
Copy link
Member

@tcitworld tcitworld left a comment

Choose a reason for hiding this comment

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

We also need a repair step to purge all entries in calendar_invitations which don't match existing calendar objects.

apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
@miaulalala
Copy link
Contributor

We also need a repair step to purge all entries in calendar_invitations which don't match existing calendar objects.

Set the expiration date for those outdated links to in the past, and the CleanupInvitationTokenJob will take care of it.

https://github.com/nextcloud/server/blob/af6de04e9e141466dc229e444ff3f146f4a34765/apps/dav/lib/BackgroundJob/CleanupInvitationTokenJob.php

It will probably need modifications like

protected function run($argument): void {
$time = $this->time->getTime() - (60 * 60);
$this->calDavBackend->deleteOutdatedSchedulingObjects($time, 50000);
$this->logger->info('Removed outdated scheduling objects');
}

as to not completely crash an instance until all outdated links are deleted.

Would be a good idea to get some data from our instance too to see how many of these links exist in our database. Please ask the sysadmins to run a query for you.

@tcitworld
Copy link
Member

In my production instance, with ~22k rows in oc_calendar_invitations and 22M rows in oc_calendarobjects, I get 2162 entries for

select count(i.id) from oc_calendar_invitations i left join oc_calendarobjects o on i.uid = o.uid where o.id is null;

@SebastianKrupinski
Copy link
Contributor Author

In my production instance, with ~22k rows in oc_calendar_invitations and 22M rows in oc_calendarobjects, I get 2162 entries for

22M rows! wow. Can you tell how many calendar object the biggest calendar has?

@tcitworld
Copy link
Member

Can you tell how many calendar object the biggest calendar has?

51k objects

@SebastianKrupinski SebastianKrupinski force-pushed the fix/issue-12387-delete-invitations branch from dec06cc to 692d0e9 Compare October 16, 2024 22:56
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Code looks good, too.

@st3iny
Copy link
Member

st3iny commented Nov 21, 2024

/backport to stable30

@st3iny
Copy link
Member

st3iny commented Nov 21, 2024

/backport to stable29

@st3iny
Copy link
Member

st3iny commented Nov 21, 2024

/backport to stable28

@st3iny st3iny added bug feature: caldav Related to CalDAV internals labels Nov 21, 2024
@st3iny st3iny added this to the Nextcloud 31 milestone Nov 21, 2024
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Looks good!

Performance wise, we can optimize the query loop a bit

apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
@SebastianKrupinski SebastianKrupinski force-pushed the fix/issue-12387-delete-invitations branch from 49d8762 to 4740c18 Compare November 21, 2024 14:08
@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 21, 2024
@SebastianKrupinski SebastianKrupinski merged commit 1681283 into master Nov 21, 2024
186 checks passed
@SebastianKrupinski SebastianKrupinski deleted the fix/issue-12387-delete-invitations branch November 21, 2024 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: caldav Related to CalDAV internals
Projects
Development

Successfully merging this pull request may close these issues.

When a calendar event is deleted, corresponding calendar_invitation entries are not deleted
5 participants