Skip to content

Commit 4265746

Browse files
fix(dav): pair changed iTip instances by UID/RECURRENCE-ID instead of array position
Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com> [skip ci]
1 parent a98eb47 commit 4265746

11 files changed

Lines changed: 550 additions & 377 deletions

File tree

‎apps/dav/composer/composer/autoload_classmap.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@
6363
'OCA\\DAV\\CalDAV\\CalendarRoot' => $baseDir . '/../lib/CalDAV/CalendarRoot.php',
6464
'OCA\\DAV\\CalDAV\\DefaultCalendarValidator' => $baseDir . '/../lib/CalDAV/DefaultCalendarValidator.php',
6565
'OCA\\DAV\\CalDAV\\EmbeddedCalDavServer' => $baseDir . '/../lib/CalDAV/EmbeddedCalDavServer.php',
66-
'OCA\\DAV\\CalDAV\\EventComparisonService' => $baseDir . '/../lib/CalDAV/EventComparisonService.php',
6766
'OCA\\DAV\\CalDAV\\EventReader' => $baseDir . '/../lib/CalDAV/EventReader.php',
6867
'OCA\\DAV\\CalDAV\\EventReaderRDate' => $baseDir . '/../lib/CalDAV/EventReaderRDate.php',
6968
'OCA\\DAV\\CalDAV\\EventReaderRRule' => $baseDir . '/../lib/CalDAV/EventReaderRRule.php',

‎apps/dav/composer/composer/autoload_static.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@ class ComposerStaticInitDAV
7878
'OCA\\DAV\\CalDAV\\CalendarRoot' => __DIR__ . '/..' . '/../lib/CalDAV/CalendarRoot.php',
7979
'OCA\\DAV\\CalDAV\\DefaultCalendarValidator' => __DIR__ . '/..' . '/../lib/CalDAV/DefaultCalendarValidator.php',
8080
'OCA\\DAV\\CalDAV\\EmbeddedCalDavServer' => __DIR__ . '/..' . '/../lib/CalDAV/EmbeddedCalDavServer.php',
81-
'OCA\\DAV\\CalDAV\\EventComparisonService' => __DIR__ . '/..' . '/../lib/CalDAV/EventComparisonService.php',
8281
'OCA\\DAV\\CalDAV\\EventReader' => __DIR__ . '/..' . '/../lib/CalDAV/EventReader.php',
8382
'OCA\\DAV\\CalDAV\\EventReaderRDate' => __DIR__ . '/..' . '/../lib/CalDAV/EventReaderRDate.php',
8483
'OCA\\DAV\\CalDAV\\EventReaderRRule' => __DIR__ . '/..' . '/../lib/CalDAV/EventReaderRRule.php',

‎apps/dav/lib/CalDAV/EventComparisonService.php‎

Lines changed: 0 additions & 100 deletions
This file was deleted.

‎apps/dav/lib/CalDAV/Schedule/IMipPlugin.php‎

Lines changed: 70 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
namespace OCA\DAV\CalDAV\Schedule;
1111

1212
use OCA\DAV\CalDAV\CalendarObject;
13-
use OCA\DAV\CalDAV\EventComparisonService;
1413
use OCP\Accounts\IAccountManager;
1514
use OCP\AppFramework\Utility\ITimeFactory;
1615
use OCP\Defaults;
@@ -65,7 +64,6 @@ public function __construct(
6564
private Defaults $defaults,
6665
private IUserSession $userSession,
6766
private IMipService $imipService,
68-
private EventComparisonService $eventComparisonService,
6967
private IMailManager $mailManager,
7068
private IEmailValidator $emailValidator,
7169
private IAccountManager $accountManager,
@@ -146,29 +144,82 @@ public function schedule(Message $iTipMessage) {
146144

147145
$recipientName = $iTipMessage->recipientName ? (string)$iTipMessage->recipientName : null;
148146

149-
$newEvents = $iTipMessage->message;
150-
$oldEvents = $this->getVCalendar();
147+
$newObjects = $iTipMessage->message;
148+
$oldObjects = $this->getVCalendar();
149+
150+
$method = match (strtolower($iTipMessage->method)) {
151+
'reply' => self::METHOD_REPLY,
152+
'cancel' => self::METHOD_CANCEL,
153+
default => self::METHOD_REQUEST,
154+
};
155+
156+
// For REQUEST method, we need to determine which instances have changed by comparing new and old events
157+
if ($method === self::METHOD_REQUEST) {
158+
$newEvents = $this->imipService->eventInstances($newObjects);
159+
$oldEventsByInstance = [];
160+
if ($oldObjects !== null) {
161+
foreach ($this->imipService->eventInstances($oldObjects) as $oldEvent) {
162+
$oldEventsByInstance[$this->imipService->instanceKey($oldEvent)] = $oldEvent;
163+
}
164+
}
151165

152-
$modified = $this->eventComparisonService->findModified($newEvents, $oldEvents);
153-
/** @var VEvent $vEvent */
154-
$vEvent = array_pop($modified['new']);
155-
/** @var VEvent $oldVevent */
156-
$oldVevent = !empty($modified['old']) && is_array($modified['old']) ? array_pop($modified['old']) : null;
157-
$isModified = isset($oldVevent);
166+
$allInstances = array_map(
167+
fn (VEvent $newEvent) => ['new' => $newEvent, 'old' => $oldEventsByInstance[$this->imipService->instanceKey($newEvent)] ?? null],
168+
$newEvents,
169+
);
158170

159-
// No changed events after all - this shouldn't happen if there is significant change yet here we are
171+
$changedInstances = array_values(array_filter(
172+
$allInstances,
173+
fn (array $pair) => $pair['old'] === null || $this->imipService->diffInstance($pair['new'], $pair['old']) !== [],
174+
));
175+
176+
$modifiedInstances = $changedInstances !== [] ? $changedInstances : $allInstances;
177+
}
178+
// For CANCEL/REPLY only ever consume the new instance, so there's no old instance to diff against or compare
179+
else {
180+
$modifiedInstances = array_map(
181+
static fn (VEvent $event) => ['new' => $event, 'old' => null],
182+
$this->imipService->eventInstances($newObjects),
183+
);
184+
}
185+
186+
// No VEvents in the message at all - this shouldn't happen if there is significant change yet here we are
160187
// The scheduling status is debatable
161-
if (empty($vEvent)) {
162-
$this->logger->warning('iTip message said the change was significant but comparison did not detect any updated VEvents');
188+
if (empty($modifiedInstances)) {
189+
$this->logger->warning('iTip message said the change was significant but the message contained no VEvents');
163190
$iTipMessage->scheduleStatus = '1.0;We got the message, but it\'s not significant enough to warrant an email';
164191
return;
165192
}
166193

167-
// we (should) have one event component left
168-
// as the ITip\Broker creates one iTip message per change
169-
// and triggers the "schedule" event once per message
170-
// we also might not have an old event as this could be a new
171-
// invitation, or a new recurrence exception
194+
if (count($modifiedInstances) > 1) {
195+
$this->logger->debug('iTip message contains multiple instances; only the primary instance is reflected in the invitation email', [
196+
'uid' => $iTipMessage->uid,
197+
'instanceCount' => count($modifiedInstances),
198+
]);
199+
}
200+
201+
// we (should) have one instance per message, as the ITip\Broker creates
202+
// one iTip message per attendee and triggers the "schedule" event once
203+
// per message; a message can still bundle several instances (e.g.
204+
// primary + overrides edited together), in which case only the primary
205+
// instance is reflected in the email for now
206+
$primaryInstance = null;
207+
foreach ($modifiedInstances as $instance) {
208+
if (!isset($instance['new']->{'RECURRENCE-ID'})) {
209+
$primaryInstance = $instance;
210+
break;
211+
}
212+
}
213+
$primaryInstance ??= $modifiedInstances[0];
214+
215+
/** @var VEvent $vEvent */
216+
$vEvent = $primaryInstance['new'];
217+
/** @var VEvent|null $oldVevent */
218+
$oldVevent = $primaryInstance['old'];
219+
$isModified = $oldVevent !== null;
220+
221+
// we might not have an old event as this could be a new invitation,
222+
// or a new recurrence exception
172223
$attendee = $this->imipService->getCurrentAttendee($iTipMessage);
173224
if ($attendee === null) {
174225
$uid = $vEvent->UID ?? 'no UID found';
@@ -199,18 +250,15 @@ public function schedule(Message $iTipMessage) {
199250
}
200251

201252
$replyingAttendee = null;
202-
switch (strtolower($iTipMessage->method)) {
253+
switch ($method) {
203254
case self::METHOD_REPLY:
204-
$method = self::METHOD_REPLY;
205255
$data = $this->imipService->buildReplyBodyData($vEvent);
206256
$replyingAttendee = $this->imipService->getReplyingAttendee($iTipMessage);
207257
break;
208258
case self::METHOD_CANCEL:
209-
$method = self::METHOD_CANCEL;
210259
$data = $this->imipService->buildCancelledBodyData($vEvent);
211260
break;
212261
default:
213-
$method = self::METHOD_REQUEST;
214262
$data = $this->imipService->buildBodyData($vEvent, $oldVevent);
215263
break;
216264
}

‎apps/dav/lib/CalDAV/Schedule/IMipService.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,32 @@ class IMipService {
4040
'meeting_location' => 'LOCATION'
4141
];
4242

43+
/**
44+
* Properties compared to build an instance's changed-property delta:
45+
* everything the invitation email can show a diff for
46+
* (SUMMARY/DESCRIPTION/LOCATION/URL via STRING_DIFF, DTSTART/DTEND/DURATION
47+
* for the "when" string), plus SEQUENCE/LAST-MODIFIED and the rest of
48+
* iTip's own significantChangeProperties (RRULE/RDATE/EXDATE/STATUS;
49+
* DUE is VTODO-only and not relevant to VEVENT invites).
50+
*
51+
* @var string[]
52+
*/
53+
private const array INSTANCE_DIFF_PROPERTIES = [
54+
'SUMMARY',
55+
'DESCRIPTION',
56+
'LOCATION',
57+
'URL',
58+
'DTSTART',
59+
'DTEND',
60+
'DURATION',
61+
'STATUS',
62+
'RRULE',
63+
'RDATE',
64+
'EXDATE',
65+
'SEQUENCE',
66+
'LAST-MODIFIED',
67+
];
68+
4369
public function __construct(
4470
private URLGenerator $urlGenerator,
4571
private IDBConnection $db,
@@ -149,6 +175,38 @@ private function linkify(?string $url): ?string {
149175
return sprintf('<a href="%1$s">%1$s</a>', htmlspecialchars($url));
150176
}
151177

178+
public function instanceKey(VEvent $event): string {
179+
return self::readPropertyWithDefault($event, 'UID', '') . '#' . self::readPropertyWithDefault($event, 'RECURRENCE-ID', '');
180+
}
181+
182+
/**
183+
* Every VEVENT component directly in $calendar, in document order.
184+
*
185+
* @return list<VEvent>
186+
*/
187+
public function eventInstances(VCalendar $calendar): array {
188+
return array_values(array_filter(
189+
$calendar->getComponents(),
190+
static fn ($component) => $component instanceof VEvent,
191+
));
192+
}
193+
194+
/**
195+
* Names of the RFC5545 properties that differ between $newEvent and
196+
* $oldEvent.
197+
*
198+
* @return string[]
199+
*/
200+
public function diffInstance(VEvent $newEvent, VEvent $oldEvent): array {
201+
$changed = [];
202+
foreach (self::INSTANCE_DIFF_PROPERTIES as $property) {
203+
if (self::readPropertyWithDefault($newEvent, $property, '') !== self::readPropertyWithDefault($oldEvent, $property, '')) {
204+
$changed[] = $property;
205+
}
206+
}
207+
return $changed;
208+
}
209+
152210
/**
153211
* @param VEvent $vEvent
154212
* @param VEvent|null $oldVEvent

‎apps/dav/lib/Server.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
use OCA\DAV\CalDAV\BirthdayCalendar\EnablePlugin;
1515
use OCA\DAV\CalDAV\BirthdayService;
1616
use OCA\DAV\CalDAV\DefaultCalendarValidator;
17-
use OCA\DAV\CalDAV\EventComparisonService;
1817
use OCA\DAV\CalDAV\ICSExportPlugin\ICSExportPlugin;
1918
use OCA\DAV\CalDAV\Publishing\PublishPlugin;
2019
use OCA\DAV\CalDAV\Schedule\IMipPlugin;
@@ -357,7 +356,6 @@ public function __construct(
357356
\OCP\Server::get(Defaults::class),
358357
$userSession,
359358
\OCP\Server::get(IMipService::class),
360-
\OCP\Server::get(EventComparisonService::class),
361359
\OCP\Server::get(\OCP\Mail\Provider\IManager::class),
362360
\OCP\Server::get(IEmailValidator::class),
363361
\OCP\Server::get(IAccountManager::class),

0 commit comments

Comments
 (0)