From 2790c13a8242ac48ab03cdc604167cef6a1e1650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Mon, 31 Aug 2026 22:16:15 +0200 Subject: [PATCH] feat(calendar): a client can ask what changed, and be told the truth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CalendarState was the string "fixed", and its docblock explained that this was a confession rather than a design: events are written from the sync engine, from extraction, from the web editor and from CalendarEvent/set, and a log recording a quarter of the writes "is not a weaker version of it, it is a lie with a number on it". So canCalculateChanges was false, there was no CalendarEvent/changes, and every client re-ran its query. calendar_change_log closes that. The rows are written by a Doctrine listener rather than by a recorder the writers call, and that is the whole point — CalendarEvent exposes public mutable properties, so fields move wherever somebody assigns one and lets the flush carry it. An RSVP in InviteResponder, a SEQUENCE bump in EventReconciler, the recurrence flags in RecurrenceMaterialiser: none pass through CalendarEventWriter, so a recorder wired into write() would have missed all three and every deletion besides. Doctrine already knows what changed; asking it is the only way to be complete without asking every future caller to remember. CalendarChangeLog the log. Its own table rather than new JmapObjectType cases on jmap_change_log, which CalendarState suggested: that table is keyed by account_id, and CalendarAccountResolver says a user can delete their last account and keep a calendar. CalDAV also counts per collection, and there is nowhere to put the collection. the listener onFlush collects, postFlush writes. An ignore list, not an allow list, so a field added later defaults to being logged — over-recording costs a wasted conditional GET, under-recording is the silent failure. CalendarChangeReader tokens and deltas, protocol-neutral, in both scopes: per user for JMAP, per calendar for the CalDAV server this is the groundwork for. Two bugs fell out while building it. The first was mine and is why postFlush writes SQL rather than persisting entities. Flushing from inside postFlush re-runs the whole unit of work, and the second pass sees what the first left behind: EventDismisser removes an event while an EventSourceLink still points at it, which is legal once and throws "a new entity was found through the relationship" twice. It broke every calendar deletion in the suite and five demo-flow tests. A plain INSERT stays out of the UnitOfWork entirely, so this listener cannot make somebody else's flush fail. The second is subtler: a flush that throws never reaches postFlush, so the queued rows survived to be written by the next flush that succeeded — logging changes the database had rolled back. onFlush clears the queue as it starts. A worker whose EntityManager is reset and carries on is exactly where that would have bitten. CalendarEvent/get, /query and /set now return real sequences. Calendar/get still does not: the log records events, not the collections holding them, so a rename or a recolour moves nothing. That is a smaller gap than it sounds — a user has a handful of calendars, Calendar/get returns all of them by default, and CalDAV discovers the collection set with a PROPFIND that holds no token. CalendarState now says so instead of describing a world that no longer exists. Also marks config/reference.php optional in phpstan.dist.neon. It is gitignored and generated, every sibling entry in that exclude list already carries (?), and without it PHPStan refuses to start on a clean checkout. No deployment change. One new table, created on boot. Co-Authored-By: Claude Opus 5 --- migrations/Version20260903130000.php | 96 +++++++ phpstan.dist.neon | 2 +- src/Domain/DTO/Calendar/CalendarDelta.php | 58 ++++ .../Enum/Calendar/CalendarChangeKind.php | 27 ++ .../Exception/CalendarStateTokenException.php | 31 +++ src/Entity/Calendar/CalendarChangeLog.php | 119 ++++++++ .../Listener/CalendarChangeLogListener.php | 232 ++++++++++++++++ src/Jmap/Calendar/CalendarState.php | 57 ++-- .../Calendar/CalendarEventChangesMethod.php | 80 ++++++ .../Calendar/CalendarEventGetMethod.php | 5 +- .../Calendar/CalendarEventQueryMethod.php | 15 +- .../Calendar/CalendarEventSetMethod.php | 14 +- .../Method/Calendar/CalendarGetMethod.php | 8 +- .../Calendar/CalendarChangeLogRepository.php | 143 ++++++++++ .../Calendar/Change/CalendarChangeReader.php | 203 ++++++++++++++ .../CalendarChangeLogListenerTest.php | 254 ++++++++++++++++++ .../CalendarEventChangesMethodTest.php | 146 ++++++++++ 17 files changed, 1449 insertions(+), 41 deletions(-) create mode 100644 migrations/Version20260903130000.php create mode 100644 src/Domain/DTO/Calendar/CalendarDelta.php create mode 100644 src/Domain/Enum/Calendar/CalendarChangeKind.php create mode 100644 src/Domain/Exception/CalendarStateTokenException.php create mode 100644 src/Entity/Calendar/CalendarChangeLog.php create mode 100644 src/Infrastructure/Event/Listener/CalendarChangeLogListener.php create mode 100644 src/Jmap/Method/Calendar/CalendarEventChangesMethod.php create mode 100644 src/Repository/Calendar/CalendarChangeLogRepository.php create mode 100644 src/Service/Calendar/Change/CalendarChangeReader.php create mode 100644 tests/Infrastructure/Event/Listener/CalendarChangeLogListenerTest.php create mode 100644 tests/Jmap/Method/Calendar/CalendarEventChangesMethodTest.php diff --git a/migrations/Version20260903130000.php b/migrations/Version20260903130000.php new file mode 100644 index 00000000..7260c034 --- /dev/null +++ b/migrations/Version20260903130000.php @@ -0,0 +1,96 @@ +addSql(<<<'SQL' + CREATE TABLE calendar_change_log ( + sequence INT GENERATED BY DEFAULT AS IDENTITY NOT NULL, + user_id INT NOT NULL, + calendar_id INT NOT NULL, + event_id INT NOT NULL, + event_uid VARCHAR(255) NOT NULL, + change_kind VARCHAR(16) NOT NULL, + created_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, + updated_at TIMESTAMP(0) WITHOUT TIME ZONE NOT NULL, + PRIMARY KEY (sequence) + ) + SQL); + + $this->addSql('CREATE INDEX idx_calendar_change_log_calendar_sequence ON calendar_change_log (calendar_id, sequence)'); + $this->addSql('CREATE INDEX idx_calendar_change_log_user_sequence ON calendar_change_log (user_id, sequence)'); + } + + public function down(Schema $schema): void + { + // Nothing an operator typed lives here, and a client that loses the log + // resyncs from scratch — which is what it did before the table existed. + $this->addSql('DROP TABLE calendar_change_log'); + } +} diff --git a/phpstan.dist.neon b/phpstan.dist.neon index 6c4d3292..6cce8418 100644 --- a/phpstan.dist.neon +++ b/phpstan.dist.neon @@ -73,7 +73,7 @@ parameters: # Generated: 54 migrations nobody edits by hand, and a ~1000-line # dump of the framework config tree that exists to be read. - migrations - - config/reference.php + - config/reference.php (?) # Twig extensions lifted from upstream rather than written here. # NOTE: only Vendor/ — the rest of src/Twig is application code # (SidebarCounts and friends) and must stay analysed. diff --git a/src/Domain/DTO/Calendar/CalendarDelta.php b/src/Domain/DTO/Calendar/CalendarDelta.php new file mode 100644 index 00000000..cf981bcc --- /dev/null +++ b/src/Domain/DTO/Calendar/CalendarDelta.php @@ -0,0 +1,58 @@ + $created event id => RFC 5545 UID + * @param array $updated event id => RFC 5545 UID + * @param array $destroyed event id => RFC 5545 UID + */ + public function __construct( + public readonly string $oldState, + public readonly string $newState, + public readonly bool $hasMoreChanges, + public readonly array $created, + public readonly array $updated, + public readonly array $destroyed, + ) { + } + + /** @return list */ + public function createdIds(): array + { + return array_map(strval(...), array_keys($this->created)); + } + + /** @return list */ + public function updatedIds(): array + { + return array_map(strval(...), array_keys($this->updated)); + } + + /** @return list */ + public function destroyedIds(): array + { + return array_map(strval(...), array_keys($this->destroyed)); + } +} diff --git a/src/Domain/Enum/Calendar/CalendarChangeKind.php b/src/Domain/Enum/Calendar/CalendarChangeKind.php new file mode 100644 index 00000000..fd4330dd --- /dev/null +++ b/src/Domain/Enum/Calendar/CalendarChangeKind.php @@ -0,0 +1,27 @@ + since`. CalendarState's docblock asks for this table in so many + * words — it explains that calendars have no recorder, that a log covering some + * of the writers "is a lie with a number on it", and that the fixed state string + * exists only until one arrives. + * + * ── Why not jmap_change_log, which CalendarState suggested ──────────────── + * + * That table is keyed `(account_id, object_type, sequence)`, and neither half of + * the key fits a calendar. + * + * **A calendar has no account.** CalendarAccountResolver says it outright: a + * plMail Calendar is the user's, and the mail account is only ever an optional + * owner of the one calendar extraction files into. It also names the state that + * breaks the reuse — "a user can delete their last account and keep a calendar". + * Keyed by account_id, that user's calendar becomes untrackable, and CalDAV, + * which has no notion of a mail account at all, would stop working for them. + * + * **CalDAV counts per collection, not per account.** RFC 6578 runs + * sync-collection against one collection, so the log has to answer "what changed + * in calendar 4 since token 91". jmap_change_log has nowhere to put the 4. + * + * One table answers both readers because they filter the same rows differently: + * CalDAV by calendar_id, JMAP by user_id across all of them. Two tables would + * have to agree about ordering, and two sequences cannot. + * + * ── Why a tombstone carries the uid ─────────────────────────────────────── + * + * A CalDAV client names a resource by a URL built from the event's UID, so a + * deletion has to report the UID of a row that no longer exists to be read. The + * same is true of calendar_id: after the event is gone there is nothing left to + * join to. Both are therefore copied here at write time rather than resolved at + * read time — this row is the only surviving record of a deleted event, which is + * the whole reason it is written. + * + * Ids are scalars, not relations, for the reason jmap_change_log gives: these + * rows are written from a Doctrine listener during flush, where holding entity + * references is the documented footgun. A plain id sidesteps it, and a + * ManyToOne to an event being deleted in the same flush could not be held at + * all. + */ +#[ORM\Entity(repositoryClass: CalendarChangeLogRepository::class)] +#[ORM\Table(name: 'calendar_change_log')] +// The CalDAV read: one collection, everything after a token, in order. Calendar +// leads because sync-collection always names one and never scans across them. +#[ORM\Index(name: 'idx_calendar_change_log_calendar_sequence', columns: ['calendar_id', 'sequence'])] +// The JMAP read: CalendarEvent/changes spans every calendar the user has, so it +// filters on the user and takes the same ordering from the primary key. +#[ORM\Index(name: 'idx_calendar_change_log_user_sequence', columns: ['user_id', 'sequence'])] +#[ORM\HasLifecycleCallbacks] +class CalendarChangeLog +{ + use TimestampableTrait; + + /** + * The token. Integer and unpruned, matching ChangeLog — see its note on the + * 2.1-billion ceiling and on what switching to bigint would cost. Calendars + * write far fewer rows than mail does, so whichever limit mail meets first + * will be met here long after. + */ + #[ORM\Id] + #[ORM\GeneratedValue] + #[ORM\Column(type: 'integer')] + public private(set) ?int $sequence = null; + + #[ORM\Column(name: 'user_id')] + public private(set) int $userId; + + /** + * The collection the change happened in. An event moved between calendars + * writes two rows — destroyed in the old, created in the new — because that + * is what each collection's readers have to be told, and a single "updated" + * row would leave the old collection claiming a resource it no longer has. + */ + #[ORM\Column(name: 'calendar_id')] + public private(set) int $calendarId; + + #[ORM\Column(name: 'event_id')] + public private(set) int $eventId; + + /** RFC 5545 UID, copied so a tombstone still names its resource. */ + #[ORM\Column(name: 'event_uid', length: 255)] + public private(set) string $eventUid; + + #[ORM\Column(name: 'change_kind', length: 16, enumType: CalendarChangeKind::class)] + public private(set) CalendarChangeKind $changeKind; + + public function __construct( + int $userId, + int $calendarId, + int $eventId, + string $eventUid, + CalendarChangeKind $changeKind, + ) { + $this->userId = $userId; + $this->calendarId = $calendarId; + $this->eventId = $eventId; + $this->eventUid = $eventUid; + $this->changeKind = $changeKind; + $this->createdAt = new DateTimeImmutable(); + } +} diff --git a/src/Infrastructure/Event/Listener/CalendarChangeLogListener.php b/src/Infrastructure/Event/Listener/CalendarChangeLogListener.php new file mode 100644 index 00000000..6a2a34c4 --- /dev/null +++ b/src/Infrastructure/Event/Listener/CalendarChangeLogListener.php @@ -0,0 +1,232 @@ + + */ + private array $pending = []; + + public function onFlush(OnFlushEventArgs $args): void + { + // Anything still queued belongs to a flush that never reached + // postFlush — it threw. Those rows describe changes the database + // rolled back, and writing them on the next successful flush would put + // events into the log that no longer changed, which a client would + // fetch and find unaltered. A worker whose EntityManager is reset and + // carries on is exactly where that happens. + $this->pending = []; + + $uow = $args->getObjectManager()->getUnitOfWork(); + + foreach ($uow->getScheduledEntityInsertions() as $entity) { + if ($entity instanceof CalendarEvent) { + $this->queue($entity, CalendarChangeKind::Created, deferId: true); + } + } + + foreach ($uow->getScheduledEntityUpdates() as $entity) { + if (false === $entity instanceof CalendarEvent) { + continue; + } + + $this->queueUpdate($entity, $uow->getEntityChangeSet($entity)); + } + + foreach ($uow->getScheduledEntityDeletions() as $entity) { + if ($entity instanceof CalendarEvent) { + $this->queue($entity, CalendarChangeKind::Destroyed); + } + } + } + + public function postFlush(PostFlushEventArgs $args): void + { + if ([] === $this->pending) { + return; + } + + $rows = $this->pending; + $this->pending = []; + + $connection = $args->getObjectManager()->getConnection(); + $now = (new DateTimeImmutable())->format('Y-m-d H:i:s'); + + foreach ($rows as $row) { + // An insert's id was assigned by the flush that just finished. + $eventId = $row['event'] ?? $row['entity']?->id; + + if (null === $eventId) { + continue; + } + + $connection->insert('calendar_change_log', [ + 'user_id' => $row['user'], + 'calendar_id' => $row['calendar'], + 'event_id' => $eventId, + 'event_uid' => $row['uid'], + 'change_kind' => $row['kind']->value, + 'created_at' => $now, + 'updated_at' => $now, + ]); + } + } + + /** + * An update, unless nothing a client could see actually moved. + * + * A calendar change is two rows rather than one. Each collection has its own + * readers and its own token, and the collection an event left has to be told + * the resource is gone — an "updated" row filed under the new calendar would + * leave the old one advertising an href that now answers 404. + * + * @param array $changeSet + */ + private function queueUpdate(CalendarEvent $event, array $changeSet): void + { + $visible = array_diff(array_keys($changeSet), self::IGNORED); + + if ([] === $visible) { + return; + } + + if (true === in_array('calendar', $visible, true)) { + /** @var array{0:mixed,1:mixed} $move */ + $move = $changeSet['calendar']; + + if ($move[0] instanceof Calendar) { + $this->push($event, (int) $move[0]->id, CalendarChangeKind::Destroyed); + } + + $this->queue($event, CalendarChangeKind::Created); + + return; + } + + $this->queue($event, CalendarChangeKind::Updated); + } + + /** + * Queue a row for the event's current calendar. + * + * An event with no calendar sits in no collection, so no reader can see it + * and there is nothing to tell them. Same for one with no owner: the log is + * scoped by user, and a row that named nobody could never be read back. + */ + private function queue(CalendarEvent $event, CalendarChangeKind $kind, bool $deferId = false): void + { + if (null === $event->calendar?->id) { + return; + } + + $this->push($event, (int) $event->calendar->id, $kind, $deferId); + } + + private function push(CalendarEvent $event, int $calendarId, CalendarChangeKind $kind, bool $deferId = false): void + { + $userId = $event->usr?->id; + + if (null === $userId) { + return; + } + + $this->pending[] = [ + 'user' => $userId, + 'calendar' => $calendarId, + 'event' => true === $deferId ? null : $event->id, + 'entity' => true === $deferId ? $event : null, + 'uid' => $event->uid, + 'kind' => $kind, + ]; + } +} diff --git a/src/Jmap/Calendar/CalendarState.php b/src/Jmap/Calendar/CalendarState.php index b12bb167..2beafc5b 100644 --- a/src/Jmap/Calendar/CalendarState.php +++ b/src/Jmap/Calendar/CalendarState.php @@ -5,36 +5,41 @@ declare(strict_types=1); namespace App\Jmap\Calendar; /** - * The state string every calendar method returns, and why it is a constant. + * The state string Calendar/get returns, and why it alone is still a constant. * - * Mail's state is a real token: `StateManager` writes one `jmap_change_log` row - * per mutation and the autoincrement PK *is* the token, so `Email/get` can hand - * a client a number that `Email/changes` will honour. That works because every - * path that changes a JMAP-visible mail property calls `StateManager::record*` - * — through `MailChangeRecorder`, which exists exactly so five callers cannot - * each forget the same two things. + * This class used to explain why *every* calendar method answered with a fixed + * string: events are written from the sync engine, from extraction, from the + * web editor and from CalendarEvent/set, only the last of which lives in + * src/Jmap/, and a log recording a quarter of the writes "is not a weaker + * version of it, it is a lie with a number on it". * - * Calendars have no such recorder, and could not be given one from inside this - * directory. An event changes from four places: the sync engine pulling a - * remote calendar, extraction reading a message, the web editor, and - * `CalendarEvent/set` here. Only the last is in `src/Jmap/`. **A log that - * recorded a quarter of the writes would be worse than none**: the token would - * sit still while a pull replaced the whole day, and a client comparing states - * would conclude nothing had changed and never refetch. Mail's log is - * trustworthy because it is complete; a partial one is not a weaker version of - * it, it is a lie with a number on it. + * That is fixed. `calendar_change_log` records every event write, and it is + * written by a Doctrine listener rather than by the writers — completeness by + * construction rather than by asking four callers to remember, which is what + * makes it trustworthy where a recorder would not have been. CalendarEvent/get, + * /query and /set now return real sequences, and CalendarEvent/changes exists. * - * So the state is fixed and the methods say so in the only other way the - * protocol offers: `canCalculateChanges` is false, there is no - * `Calendar/changes` or `CalendarEvent/changes`, and a client re-runs its query - * — which is what `Email/query` already asks for and is spec-legal. + * ── What is still fixed, and why that is honest ─────────────────────────── * - * The value is deliberately not a number. Should calendars later join the - * change log — a `CalendarChangeRecorder` beside `MailChangeRecorder`, called by - * all four writers, and `JmapObjectType` cases to match — tokens become - * sequences, and a client still holding this one fails `ctype_digit` in - * `StateManager::changesSince()` and is told to resync. That is the correct - * degradation, and it is free. + * Calendars themselves. The log records events, keyed by the collection they + * are in; a calendar being renamed, recoloured, reordered or removed writes + * nothing. So Calendar/get keeps a constant state and there is no + * Calendar/changes — the same refusal as before, now covering one object type + * instead of two. + * + * That is a smaller gap than it sounds. A user has a handful of calendars and + * Calendar/get returns all of them by default, so re-running it is cheap and is + * what a client does anyway; there is no /query to page through. CalDAV, the + * other reader of this data, does not need it at all — a client discovers the + * collection set with a PROPFIND on the calendar-home-set, which is a live read + * and holds no token. + * + * ── If calendars later join the log ─────────────────────────────────────── + * + * The shape is already there: give CalendarChangeLog a nullable event_id, or a + * sibling table, and record collection writes from the same listener. Clients + * holding this string then fail ctype_digit in CalendarChangeReader and are + * told to resync, which is the correct degradation and is free. */ final class CalendarState { diff --git a/src/Jmap/Method/Calendar/CalendarEventChangesMethod.php b/src/Jmap/Method/Calendar/CalendarEventChangesMethod.php new file mode 100644 index 00000000..a1d00e2c --- /dev/null +++ b/src/Jmap/Method/Calendar/CalendarEventChangesMethod.php @@ -0,0 +1,80 @@ +accountResolver->resolve($context->user, $arguments['accountId'] ?? null); + + $sinceState = $arguments['sinceState'] ?? null; + + if (false === is_string($sinceState)) { + throw new MethodException('invalidArguments', '"sinceState" is required.'); + } + + $maxChanges = $arguments['maxChanges'] ?? null; + + if (null !== $maxChanges && false === is_int($maxChanges)) { + throw new MethodException('invalidArguments', '"maxChanges" must be a number.'); + } + + try { + $delta = $this->changes->sinceForUser((int) $context->user->id, $sinceState, $maxChanges); + } catch (CalendarStateTokenException $e) { + throw new MethodException('cannotCalculateChanges', $e->getMessage()); + } + + return [ + 'accountId' => (string) $account->id, + 'oldState' => $delta->oldState, + 'newState' => $delta->newState, + 'hasMoreChanges' => $delta->hasMoreChanges, + 'created' => $delta->createdIds(), + 'updated' => $delta->updatedIds(), + 'destroyed' => $delta->destroyedIds(), + ]; + } +} diff --git a/src/Jmap/Method/Calendar/CalendarEventGetMethod.php b/src/Jmap/Method/Calendar/CalendarEventGetMethod.php index 22f90ef1..1dbf6de7 100644 --- a/src/Jmap/Method/Calendar/CalendarEventGetMethod.php +++ b/src/Jmap/Method/Calendar/CalendarEventGetMethod.php @@ -5,12 +5,12 @@ declare(strict_types=1); namespace App\Jmap\Method\Calendar; use App\Jmap\Account\CalendarAccountResolver; -use App\Jmap\Calendar\CalendarState; use App\Jmap\Calendar\OccurrenceId; use App\Jmap\Mapper\CalendarEventMapper; use App\Jmap\Method\JmapMethod; use App\Jmap\Protocol\Exception\MethodException; use App\Jmap\Protocol\JmapContext; +use App\Service\Calendar\Change\CalendarChangeReader; use App\Repository\Calendar\CalendarEventOccurrenceRepository; use App\Repository\Calendar\CalendarEventRepository; @@ -55,6 +55,7 @@ final class CalendarEventGetMethod implements JmapMethod private readonly CalendarEventRepository $events, private readonly CalendarEventOccurrenceRepository $occurrences, private readonly CalendarEventMapper $mapper, + private readonly CalendarChangeReader $changes, ) { } @@ -126,7 +127,7 @@ final class CalendarEventGetMethod implements JmapMethod return [ 'accountId' => (string) $account->id, - 'state' => CalendarState::FIXED, + 'state' => $this->changes->stateForUser((int) $context->user->id), 'list' => $list, 'notFound' => $notFound, ]; diff --git a/src/Jmap/Method/Calendar/CalendarEventQueryMethod.php b/src/Jmap/Method/Calendar/CalendarEventQueryMethod.php index 26dcbb19..0f66e761 100644 --- a/src/Jmap/Method/Calendar/CalendarEventQueryMethod.php +++ b/src/Jmap/Method/Calendar/CalendarEventQueryMethod.php @@ -5,10 +5,10 @@ declare(strict_types=1); namespace App\Jmap\Method\Calendar; use App\Jmap\Account\CalendarAccountResolver; -use App\Jmap\Calendar\CalendarState; use App\Jmap\Method\JmapMethod; use App\Jmap\Protocol\Exception\MethodException; use App\Jmap\Protocol\JmapContext; +use App\Service\Calendar\Change\CalendarChangeReader; use App\Jmap\Query\CalendarEventQueryRunner; /** @@ -43,6 +43,7 @@ final class CalendarEventQueryMethod implements JmapMethod public function __construct( private readonly CalendarAccountResolver $accountResolver, private readonly CalendarEventQueryRunner $runner, + private readonly CalendarChangeReader $changes, ) { } @@ -106,11 +107,13 @@ final class CalendarEventQueryMethod implements JmapMethod return [ 'accountId' => (string) $account->id, - 'queryState' => CalendarState::FIXED, - // No CalendarEvent/queryChanges, and no /changes for it to diff - // against — CalendarState explains why there is no state to hold a - // delta from. Clients re-run the query, which is spec-legal and is - // what Email/query already asks for. + 'queryState' => $this->changes->stateForUser((int) $context->user->id), + // A real token now — calendar_change_log moves it — but still no + // CalendarEvent/queryChanges, which is a different promise: a query + // delta has to account for rows entering and leaving the *filter*, + // and a log of what changed cannot say whether a change moved an + // event across the window a filter describes. Clients re-run the + // query, which is spec-legal and what Email/query already asks for. 'canCalculateChanges' => false, 'position' => $result->position, 'ids' => $result->ids, diff --git a/src/Jmap/Method/Calendar/CalendarEventSetMethod.php b/src/Jmap/Method/Calendar/CalendarEventSetMethod.php index d6848309..f3b472a2 100644 --- a/src/Jmap/Method/Calendar/CalendarEventSetMethod.php +++ b/src/Jmap/Method/Calendar/CalendarEventSetMethod.php @@ -8,12 +8,12 @@ use App\Entity\Calendar\Calendar; use App\Entity\Calendar\CalendarEvent; use App\Infrastructure\Messaging\Message\SyncCalendarMessage; use App\Jmap\Account\CalendarAccountResolver; -use App\Jmap\Calendar\CalendarState; use App\Jmap\Calendar\JmapEventWriter; use App\Jmap\Calendar\OccurrenceId; use App\Jmap\Method\JmapMethod; use App\Jmap\Protocol\Exception\MethodException; use App\Jmap\Protocol\JmapContext; +use App\Service\Calendar\Change\CalendarChangeReader; use App\Repository\Calendar\CalendarEventRepository; use App\Service\Calendar\CalendarEventWriter; use Doctrine\ORM\EntityManagerInterface; @@ -60,6 +60,7 @@ final class CalendarEventSetMethod implements JmapMethod private readonly CalendarEventWriter $writer, private readonly MessageBusInterface $bus, private readonly EntityManagerInterface $entityManager, + private readonly CalendarChangeReader $changes, ) { } @@ -90,14 +91,21 @@ final class CalendarEventSetMethod implements JmapMethod $this->applyUpdates($context, $arguments['update'] ?? null, $updated, $notUpdated, $touched); $this->applyDestroys($context, $arguments['destroy'] ?? null, $destroyed, $notDestroyed, $touched); + // Read before the flush and again after it: the listener writes the + // log during that flush, so these two calls are what oldState and + // newState actually mean. + $oldState = $this->changes->stateForUser((int) $context->user->id); + $this->entityManager->flush(); + $newState = $this->changes->stateForUser((int) $context->user->id); + $this->dispatchSync($touched); return [ 'accountId' => (string) $account->id, - 'oldState' => CalendarState::FIXED, - 'newState' => CalendarState::FIXED, + 'oldState' => $oldState, + 'newState' => $newState, 'created' => 0 === count($created) ? new \stdClass() : $created, 'notCreated' => 0 === count($notCreated) ? new \stdClass() : $notCreated, 'updated' => 0 === count($updated) ? new \stdClass() : $updated, diff --git a/src/Jmap/Method/Calendar/CalendarGetMethod.php b/src/Jmap/Method/Calendar/CalendarGetMethod.php index f260c8cd..357adb3f 100644 --- a/src/Jmap/Method/Calendar/CalendarGetMethod.php +++ b/src/Jmap/Method/Calendar/CalendarGetMethod.php @@ -86,9 +86,11 @@ final class CalendarGetMethod implements JmapMethod return [ 'accountId' => (string) $account->id, - // Fixed, and CalendarState says why at length: calendars are - // written from four places and only this one could record a change, - // so a token here would sit still while a sync replaced the day. + // Still fixed, and now the only method that is: the change log + // records events, not the collections holding them, so a rename or + // a recolour moves nothing. CalendarState says what that costs — + // little, because a user has a handful of calendars and this method + // returns all of them by default. 'state' => CalendarState::FIXED, 'list' => $list, 'notFound' => $notFound, diff --git a/src/Repository/Calendar/CalendarChangeLogRepository.php b/src/Repository/Calendar/CalendarChangeLogRepository.php new file mode 100644 index 00000000..b0307751 --- /dev/null +++ b/src/Repository/Calendar/CalendarChangeLogRepository.php @@ -0,0 +1,143 @@ + + */ +final class CalendarChangeLogRepository extends ServiceEntityRepository +{ + public function __construct(ManagerRegistry $registry) + { + parent::__construct($registry, CalendarChangeLog::class); + } + + /** The CalDAV sync-token for one collection: 0 when nothing is logged. */ + public function latestSequenceForCalendar(int $calendarId): int + { + return $this->boundary('calendarId', $calendarId, 'MAX'); + } + + /** The JMAP state token for a user's calendars: 0 when nothing is logged. */ + public function latestSequenceForUser(int $userId): int + { + return $this->boundary('userId', $userId, 'MAX'); + } + + /** + * The lowest sequence still retained, or 0 when none remain. + * + * A reader holding a token below this has fallen off the back of pruned + * history and must be told to start over — `cannotCalculateChanges` in JMAP, + * an invalid sync-token in CalDAV, which RFC 6578 answers with a `403` and + * `valid-sync-token`, and the client then does a full REPORT. + */ + public function oldestSequenceForCalendar(int $calendarId): int + { + return $this->boundary('calendarId', $calendarId, 'MIN'); + } + + public function oldestSequenceForUser(int $userId): int + { + return $this->boundary('userId', $userId, 'MIN'); + } + + /** + * Rows strictly newer than $since for one collection, oldest first. + * + * Fetches one more than $limit so the caller can tell whether more remain + * without counting the whole tail. + * + * @return list + */ + public function changesSinceForCalendar(int $calendarId, int $since, int $limit): array + { + return $this->delta('calendarId', $calendarId, $since, $limit); + } + + /** @return list */ + public function changesSinceForUser(int $userId, int $since, int $limit): array + { + return $this->delta('userId', $userId, $since, $limit); + } + + /** + * Prune rows older than $before for one collection. + * + * A bulk DELETE, like ChangeLogRepository::pruneOlderThan(): retention that + * hydrated its victims would scale with the history it exists to remove. + * Nothing in src/ calls this yet — it is here so the ceiling in + * CalendarChangeLog's docblock has an answer when it is reached. + */ + public function pruneOlderThan(int $calendarId, DateTimeImmutable $before): int + { + return (int) $this->createQueryBuilder('c') + ->delete() + ->where('c.calendarId = :id') + ->andWhere('c.createdAt < :before') + ->setParameter('id', $calendarId) + ->setParameter('before', $before) + ->getQuery() + ->execute(); + } + + /** + * MAX or MIN over the sequence within one scope. + * + * Doctrine's API has no form of an aggregate over one column, and the + * alternative is loading an entire change history to read the largest of one + * integer — on every request, since a state token is on every reply. + */ + private function boundary(string $field, int $id, string $aggregate): int + { + $result = $this->scoped($field, $id) + ->select(sprintf('%s(c.sequence)', $aggregate)) + ->getQuery() + ->getSingleScalarResult(); + + if (null === $result) { + return 0; + } + + return (int) $result; + } + + /** + * @return list + */ + private function delta(string $field, int $id, int $since, int $limit): array + { + return $this->scoped($field, $id) + ->andWhere('c.sequence > :since') + ->setParameter('since', $since) + ->orderBy('c.sequence', 'ASC') + ->setMaxResults($limit + 1) + ->getQuery() + ->getResult(); + } + + private function scoped(string $field, int $id): QueryBuilder + { + return $this->createQueryBuilder('c') + ->where(sprintf('c.%s = :id', $field)) + ->setParameter('id', $id); + } +} diff --git a/src/Service/Calendar/Change/CalendarChangeReader.php b/src/Service/Calendar/Change/CalendarChangeReader.php new file mode 100644 index 00000000..1e23fb34 --- /dev/null +++ b/src/Service/Calendar/Change/CalendarChangeReader.php @@ -0,0 +1,203 @@ +log->latestSequenceForCalendar($calendarId); + } + + /** The JMAP state string for a user's calendars. */ + public function stateForUser(int $userId): string + { + return (string) $this->log->latestSequenceForUser($userId); + } + + public function sinceForCalendar(int $calendarId, ?string $sinceState, ?int $maxChanges = null): CalendarDelta + { + return $this->delta( + $sinceState, + $maxChanges, + $this->log->latestSequenceForCalendar($calendarId), + $this->log->oldestSequenceForCalendar($calendarId), + fn (int $since, int $limit): array => $this->log->changesSinceForCalendar($calendarId, $since, $limit), + ); + } + + public function sinceForUser(int $userId, ?string $sinceState, ?int $maxChanges = null): CalendarDelta + { + return $this->delta( + $sinceState, + $maxChanges, + $this->log->latestSequenceForUser($userId), + $this->log->oldestSequenceForUser($userId), + fn (int $since, int $limit): array => $this->log->changesSinceForUser($userId, $since, $limit), + ); + } + + /** + * @param callable(int,int):list $fetch + */ + private function delta( + ?string $sinceState, + ?int $maxChanges, + int $latest, + int $oldest, + callable $fetch, + ): CalendarDelta { + if (null === $sinceState) { + throw new CalendarStateTokenException('A state token is required.'); + } + + if (false === ctype_digit($sinceState)) { + throw new CalendarStateTokenException('Unrecognised state token.'); + } + + $limit = $maxChanges ?? self::DEFAULT_MAX_CHANGES; + + if ($limit < 1) { + $limit = self::DEFAULT_MAX_CHANGES; + } + + $since = (int) $sinceState; + + if ($since === $latest) { + return new CalendarDelta((string) $since, (string) $latest, false, [], [], []); + } + + // A token ahead of the log cannot have come from this server — a + // restored backup, a client bug, a token copied between collections. + // Answering "nothing changed" would leave that client stale forever. + if ($since > $latest) { + throw new CalendarStateTokenException('State token is ahead of the change log.'); + } + + // "0" is every client's first call and is always answerable — it is not + // a token that could have predated a prune. + // + // The guard past that is sound only because the sequence is global while + // the scopes are not: one calendar's first row can sit at any number, so + // "below the oldest row in this scope" would wrongly accuse a fresh + // client. A non-zero token, though, was minted by stateFor*() and so is + // a sequence that belonged to this scope — below the oldest retained one + // means the history really was pruned. + if (0 !== $since && 0 !== $oldest && $since < $oldest - 1) { + throw new CalendarStateTokenException('State token predates retained history.'); + } + + $rows = $fetch($since, $limit); + $hasMore = count($rows) > $limit; + + if (true === $hasMore) { + $rows = array_slice($rows, 0, $limit); + } + + return $this->collapse($since, $rows, $hasMore); + } + + /** + * Many rows about one event become one verdict about it. + * + * An event created and destroyed inside the same window is dropped entirely: + * a client that never saw it has nothing to forget, and reporting a + * destruction for an id it does not hold is noise at best. Anything else + * that was created reads as created however many times it was then edited, + * and everything remaining is an update. + * + * @param list $rows + */ + private function collapse(int $since, array $rows, bool $hasMore): CalendarDelta + { + $newState = $since; + + /** @var array $seen */ + $seen = []; + + foreach ($rows as $row) { + $newState = (int) $row->sequence; + $id = (string) $row->eventId; + + if (false === array_key_exists($id, $seen)) { + $seen[$id] = ['uid' => $row->eventUid, 'created' => false, 'destroyed' => false]; + } + + // The uid of the newest row wins: an event whose uid was rewritten + // is, to a CalDAV client, a different resource, and the later name + // is the one the href has to carry. + $seen[$id]['uid'] = $row->eventUid; + + if (CalendarChangeKind::Created === $row->changeKind) { + $seen[$id]['created'] = true; + } + + if (CalendarChangeKind::Destroyed === $row->changeKind) { + $seen[$id]['destroyed'] = true; + } + } + + $created = []; + $updated = []; + $destroyed = []; + + foreach ($seen as $id => $flags) { + if (true === $flags['created'] && true === $flags['destroyed']) { + continue; + } + + if (true === $flags['created']) { + $created[$id] = $flags['uid']; + continue; + } + + if (true === $flags['destroyed']) { + $destroyed[$id] = $flags['uid']; + continue; + } + + $updated[$id] = $flags['uid']; + } + + return new CalendarDelta( + (string) $since, + (string) $newState, + $hasMore, + $created, + $updated, + $destroyed, + ); + } +} diff --git a/tests/Infrastructure/Event/Listener/CalendarChangeLogListenerTest.php b/tests/Infrastructure/Event/Listener/CalendarChangeLogListenerTest.php new file mode 100644 index 00000000..d24d283e --- /dev/null +++ b/tests/Infrastructure/Event/Listener/CalendarChangeLogListenerTest.php @@ -0,0 +1,254 @@ +em = $container->get(EntityManagerInterface::class); + $this->connection = $container->get(Connection::class); + $this->writer = $container->get(CalendarEventWriter::class); + $this->log = $container->get(CalendarChangeLogRepository::class); + + $this->connection->beginTransaction(); + $this->seed(); + } + + protected function tearDown(): void + { + if (true === $this->connection->isTransactionActive()) { + $this->connection->rollBack(); + } + + parent::tearDown(); + } + + public function testCreatingAnEventIsRecordedAgainstItsCalendar(): void + { + $event = $this->event('Kickoff'); + + $rows = $this->rowsForCalendar(); + + self::assertCount(1, $rows, 'a created event should leave exactly one row'); + self::assertSame(CalendarChangeKind::Created, $rows[0]->changeKind); + self::assertSame($event->id, $rows[0]->eventId); + self::assertSame($event->uid, $rows[0]->eventUid); + } + + /** + * The RSVP path. InviteResponder assigns jscalendar and flushes; nothing + * about that write goes near CalendarEventWriter. + */ + public function testAnRsvpIsRecordedEvenThoughItSkipsTheWriter(): void + { + $event = $this->event('Quarterly review'); + $since = $this->latest(); + + $event->jscalendar = ['participants' => ['me@example.test' => ['participationStatus' => 'accepted']]]; + $this->em->flush(); + + $rows = $this->rowsForCalendar($since); + + self::assertCount(1, $rows, 'an answer to an invitation is a change a client must see'); + self::assertSame(CalendarChangeKind::Updated, $rows[0]->changeKind); + } + + /** The EventReconciler path: SEQUENCE is an iCalendar field, so it counts. */ + public function testASequenceBumpIsRecorded(): void + { + $event = $this->event('Standup'); + $since = $this->latest(); + + ++$event->sequence; + $this->em->flush(); + + self::assertCount(1, $this->rowsForCalendar($since)); + } + + /** + * Bookkeeping is not a change. Recording it would wake every client each + * time a sync pass stamped a row it had not altered. + */ + public function testSyncBookkeepingDoesNotMoveTheTokenRatherThanWakingEveryClient(): void + { + $event = $this->event('Retro'); + $since = $this->latest(); + + $event->syncedAt = new DateTimeImmutable(); + $event->remoteId = 'remote-42'; + $this->em->flush(); + + self::assertSame([], $this->rowsForCalendar($since), 'no client can observe these fields'); + self::assertSame($since, $this->latest(), 'the token must not move'); + } + + /** + * The tombstone is the only surviving record of a deleted event, so it has + * to carry what a CalDAV client names the resource by. + */ + public function testDeletingAnEventLeavesATombstoneThatStillNamesIt(): void + { + $event = $this->event('Cancelled thing'); + $uid = $event->uid; + $id = $event->id; + $since = $this->latest(); + + $this->em->remove($event); + $this->em->flush(); + + $rows = $this->rowsForCalendar($since); + + self::assertCount(1, $rows); + self::assertSame(CalendarChangeKind::Destroyed, $rows[0]->changeKind); + self::assertSame($uid, $rows[0]->eventUid, 'the uid must outlive the row it came from'); + self::assertSame($id, $rows[0]->eventId); + } + + /** + * A move is two facts, not one: the collection it left has to be told the + * resource is gone, or it goes on advertising an href that answers 404. + */ + public function testMovingAnEventBetweenCalendarsTellsBothCollectionsRatherThanOnlyTheNewOne(): void + { + $event = $this->event('Moving day'); + $since = $this->latest(); + + $destination = $this->calendarNamed('Second'); + + $event->calendar = $destination; + $this->em->flush(); + + $left = $this->rowsForCalendar($since); + $arrived = $this->log->changesSinceForCalendar((int) $destination->id, 0, 50); + + self::assertCount(1, $left); + self::assertSame(CalendarChangeKind::Destroyed, $left[0]->changeKind, 'the old collection loses it'); + + self::assertCount(1, $arrived); + self::assertSame(CalendarChangeKind::Created, $arrived[0]->changeKind, 'the new collection gains it'); + } + + /** The per-user read spans calendars; the per-calendar read does not. */ + public function testTheUserScopedReadSpansEveryCalendar(): void + { + $second = $this->calendarNamed('Second'); + + $this->event('On the first'); + $this->event('On the second', $second); + + self::assertCount(2, $this->log->changesSinceForUser((int) $this->user->id, 0, 50)); + self::assertCount(1, $this->log->changesSinceForCalendar((int) $second->id, 0, 50)); + } + + // ---------------------------------------------------------------- helpers + + /** @return list<\App\Entity\Calendar\CalendarChangeLog> */ + private function rowsForCalendar(int $since = 0, ?Calendar $calendar = null): array + { + return $this->log->changesSinceForCalendar( + (int) ($calendar ?? $this->calendar)->id, + $since, + 50, + ); + } + + private function latest(): int + { + return $this->log->latestSequenceForCalendar((int) $this->calendar->id); + } + + private function event(string $title, ?Calendar $calendar = null): CalendarEvent + { + $event = $this->writer->write( + new CalendarEvent(), + $calendar ?? $this->calendar, + $this->user, + $title, + new DateTimeImmutable('2026-09-10 09:00:00'), + new DateTimeImmutable('2026-09-10 10:00:00'), + 'UTC', + ); + + $this->em->flush(); + + return $event; + } + + private function calendarNamed(string $name): Calendar + { + $calendar = new Calendar(); + $calendar->usr = $this->user; + $calendar->name = $name; + $calendar->role = CalendarRole::Custom; + $calendar->timeZone = 'UTC'; + + $this->em->persist($calendar); + $this->em->flush(); + + return $calendar; + } + + private function seed(): void + { + $user = new User(); + $user->email = 'changelog-' . uniqid('', true) . '@example.test'; + $user->nameFirst = 'Change'; + $user->nameLast = 'Log'; + $user->roles = ['ROLE_USER']; + $user->password = 'x'; + $this->em->persist($user); + + $calendar = new Calendar(); + $calendar->usr = $user; + $calendar->name = 'Change log fixture'; + $calendar->role = CalendarRole::Custom; + $calendar->timeZone = 'UTC'; + $this->em->persist($calendar); + + $this->em->flush(); + + $this->user = $user; + $this->calendar = $calendar; + } +} diff --git a/tests/Jmap/Method/Calendar/CalendarEventChangesMethodTest.php b/tests/Jmap/Method/Calendar/CalendarEventChangesMethodTest.php new file mode 100644 index 00000000..347b168b --- /dev/null +++ b/tests/Jmap/Method/Calendar/CalendarEventChangesMethodTest.php @@ -0,0 +1,146 @@ +method = self::getContainer()->get(CalendarEventChangesMethod::class); + } + + public function testAFreshClientIsToldAboutEverythingItHasNotSeen(): void + { + $calendar = $this->seedCalendar('Work'); + $event = $this->seedEvent($calendar, 'Kickoff', $this->baseDay()); + + $result = $this->changes('0'); + + self::assertSame([(string) $event->id], $result['created']); + self::assertSame([], $result['updated']); + self::assertSame([], $result['destroyed']); + self::assertFalse($result['hasMoreChanges']); + } + + /** A token is only worth anything if using it settles. */ + public function testAskingAgainWithTheNewStateReportsNothing(): void + { + $calendar = $this->seedCalendar('Work'); + $this->seedEvent($calendar, 'Kickoff', $this->baseDay()); + + $first = $this->changes('0'); + $second = $this->changes($first['newState']); + + self::assertSame([], $second['created']); + self::assertSame([], $second['updated']); + self::assertSame([], $second['destroyed']); + self::assertSame($first['newState'], $second['newState'], 'a settled state must not drift'); + } + + public function testAnEditArrivesAsUpdatedRatherThanCreated(): void + { + $calendar = $this->seedCalendar('Work'); + $event = $this->seedEvent($calendar, 'Kickoff', $this->baseDay()); + + $since = $this->changes('0')['newState']; + + $event->title = 'Kickoff (moved)'; + $this->em->flush(); + + $result = $this->changes($since); + + self::assertSame([], $result['created']); + self::assertSame([(string) $event->id], $result['updated']); + } + + public function testADeletionArrivesAsDestroyed(): void + { + $calendar = $this->seedCalendar('Work'); + $event = $this->seedEvent($calendar, 'Doomed', $this->baseDay()); + $id = (string) $event->id; + + $since = $this->changes('0')['newState']; + + $this->em->remove($event); + $this->em->flush(); + + self::assertSame([$id], $this->changes($since)['destroyed']); + } + + /** + * A client that never saw the event has nothing to forget, and a + * destruction for an id it does not hold is noise. + */ + public function testAnEventCreatedAndDeletedInsideOneWindowIsNotReportedAtAll(): void + { + $calendar = $this->seedCalendar('Work'); + + $since = $this->changes('0')['newState']; + + $event = $this->seedEvent($calendar, 'Brief life', $this->baseDay()); + $this->em->remove($event); + $this->em->flush(); + + $result = $this->changes($since); + + self::assertSame([], $result['created']); + self::assertSame([], $result['updated']); + self::assertSame([], $result['destroyed']); + } + + /** + * The protective half. Answering "nothing changed" to a token we cannot + * place would leave the client stale with no way to notice. + */ + public function testAnUnreadableTokenAsksTheClientToStartOverRatherThanAnsweringNothing(): void + { + $this->expectException(MethodException::class); + $this->expectExceptionMessage('Unrecognised state token.'); + + $this->changes('not-a-sequence'); + } + + public function testATokenAheadOfTheLogIsRefused(): void + { + $calendar = $this->seedCalendar('Work'); + $this->seedEvent($calendar, 'Kickoff', $this->baseDay()); + + $ahead = (string) (((int) $this->changes('0')['newState']) + 1000); + + $this->expectException(MethodException::class); + + $this->changes($ahead); + } + + /** + * @return array + */ + private function changes(string $sinceState): array + { + return $this->method->handle( + ['accountId' => $this->accountId(), 'sinceState' => $sinceState], + $this->context(), + ); + } +}