From a4b47f646e8576ee0bf073759e1c23f20ed39f39 Mon Sep 17 00:00:00 2001 From: Richard Steinmetz Date: Wed, 21 Aug 2024 14:15:33 +0200 Subject: [PATCH] fix(imap): do a single full sync when QRESYNC is enabled Signed-off-by: Richard Steinmetz --- lib/IMAP/Sync/Request.php | 12 ++- lib/IMAP/Sync/Synchronizer.php | 90 +++++++++++++++-------- lib/Service/Sync/ImapToDbSynchronizer.php | 13 +++- tests/Unit/IMAP/Sync/RequestTest.php | 7 +- tests/Unit/IMAP/Sync/SynchronizerTest.php | 53 +++++++------ 5 files changed, 118 insertions(+), 57 deletions(-) diff --git a/lib/IMAP/Sync/Request.php b/lib/IMAP/Sync/Request.php index 67087c6f17..0ebfa432d0 100644 --- a/lib/IMAP/Sync/Request.php +++ b/lib/IMAP/Sync/Request.php @@ -10,6 +10,8 @@ namespace OCA\Mail\IMAP\Sync; class Request { + private string $id; + /** @var string */ private $mailbox; @@ -24,12 +26,20 @@ class Request { * @param string $syncToken * @param int[] $uids */ - public function __construct(string $mailbox, string $syncToken, array $uids) { + public function __construct(string $id, string $mailbox, string $syncToken, array $uids) { + $this->id = $id; $this->mailbox = $mailbox; $this->syncToken = $syncToken; $this->uids = $uids; } + /** + * Get the id of this request which stays constant for all requests while syncing a single mailbox + */ + public function getId(): string { + return $this->id; + } + /** * Get the mailbox name */ diff --git a/lib/IMAP/Sync/Synchronizer.php b/lib/IMAP/Sync/Synchronizer.php index 8572082156..c9b7a3fda8 100644 --- a/lib/IMAP/Sync/Synchronizer.php +++ b/lib/IMAP/Sync/Synchronizer.php @@ -34,6 +34,9 @@ class Synchronizer { /** @var MessageMapper */ private $messageMapper; + private ?string $requestId = null; + private ?Response $response = null; + public function __construct(MessageMapper $messageMapper) { $this->messageMapper = $messageMapper; } @@ -52,24 +55,20 @@ public function __construct(MessageMapper $messageMapper) { public function sync(Horde_Imap_Client_Base $imapClient, Request $request, string $userId, + bool $hasQresync, // TODO: query client directly, but could be unsafe because login has to happen prior int $criteria = Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS): Response { + // Return cached response from last full sync when QRESYNC is enabled + if ($hasQresync && $this->response !== null && $request->getId() === $this->requestId) { + return $this->response; + } + $mailbox = new Horde_Imap_Client_Mailbox($request->getMailbox()); try { - if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { - $newUids = $this->getNewMessageUids($imapClient, $mailbox, $request); - } else { - $newUids = []; - } - if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) { - $changedUids = $this->getChangedMessageUids($imapClient, $mailbox, $request); - } else { - $changedUids = []; - } - if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) { - $vanishedUids = $this->getVanishedMessageUids($imapClient, $mailbox, $request); - } else { - $vanishedUids = []; - } + // Do a full sync and cache the response when QRESYNC is enabled + [$newUids, $changedUids, $vanishedUids] = match ($hasQresync) { + true => $this->doCombinedSync($imapClient, $mailbox, $request), + false => $this->doSplitSync($imapClient, $mailbox, $request, $criteria), + }; } catch (Horde_Imap_Client_Exception_Sync $e) { if ($e->getCode() === Horde_Imap_Client_Exception_Sync::UIDVALIDITY_CHANGED) { throw new UidValidityChangedException(); @@ -86,7 +85,51 @@ public function sync(Horde_Imap_Client_Base $imapClient, $changedMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), $changedUids, $userId); $vanishedMessageUids = $vanishedUids; - return new Response($newMessages, $changedMessages, $vanishedMessageUids, null); + $this->requestId = $request->getId(); + $this->response = new Response($newMessages, $changedMessages, $vanishedMessageUids, null); + return $this->response; + } + + /** + * @psalm-return list{int[], int[], int[]} [$newUids, $changedUids, $vanishedUids] + * @throws Horde_Imap_Client_Exception + * @throws Horde_Imap_Client_Exception_Sync + */ + private function doCombinedSync(Horde_Imap_Client_Base $imapClient, Horde_Imap_Client_Mailbox $mailbox, Request $request): array { + $syncData = $imapClient->sync($mailbox, $request->getToken(), [ + 'criteria' => Horde_Imap_Client::SYNC_ALL, + ]); + + return [ + $syncData->newmsgsuids->ids, + $syncData->flagsuids->ids, + $syncData->vanisheduids->ids, + ]; + } + + /** + * @psalm-return list{int[], int[], int[]} [$newUids, $changedUids, $vanishedUids] + * @throws Horde_Imap_Client_Exception + * @throws Horde_Imap_Client_Exception_Sync + */ + private function doSplitSync(Horde_Imap_Client_Base $imapClient, Horde_Imap_Client_Mailbox $mailbox, Request $request, int $criteria): array { + if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { + $newUids = $this->getNewMessageUids($imapClient, $mailbox, $request); + } else { + $newUids = []; + } + if ($criteria & Horde_Imap_Client::SYNC_FLAGSUIDS) { + $changedUids = $this->getChangedMessageUids($imapClient, $mailbox, $request); + } else { + $changedUids = []; + } + if ($criteria & Horde_Imap_Client::SYNC_VANISHEDUIDS) { + $vanishedUids = $this->getVanishedMessageUids($imapClient, $mailbox, $request); + } else { + $vanishedUids = []; + } + + return [$newUids, $changedUids, $vanishedUids]; } /** @@ -113,12 +156,6 @@ private function getNewMessageUids(Horde_Imap_Client_Base $imapClient, Horde_Ima * @return array */ private function getChangedMessageUids(Horde_Imap_Client_Base $imapClient, Horde_Imap_Client_Mailbox $mailbox, Request $request): array { - if ($imapClient->capability->isEnabled('QRESYNC')) { - return $imapClient->sync($mailbox, $request->getToken(), [ - 'criteria' => Horde_Imap_Client::SYNC_FLAGSUIDS, - ])->flagsuids->ids; - } - // Without QRESYNC we need to specify the known ids and in oder to avoid // overly long IMAP commands they have to be chunked. return array_merge( @@ -143,15 +180,9 @@ static function (Horde_Imap_Client_Ids $uids) use ($imapClient, $mailbox, $reque * @return array */ private function getVanishedMessageUids(Horde_Imap_Client_Base $imapClient, Horde_Imap_Client_Mailbox $mailbox, Request $request): array { - if ($imapClient->capability->isEnabled('QRESYNC')) { - return $imapClient->sync($mailbox, $request->getToken(), [ - 'criteria' => Horde_Imap_Client::SYNC_VANISHEDUIDS, - ])->vanisheduids->ids; - } - // Without QRESYNC we need to specify the known ids and in oder to avoid // overly long IMAP commands they have to be chunked. - $vanishedUids = array_merge( + return array_merge( [], // for php<7.4 https://www.php.net/manual/en/function.array-merge.php ...array_map( static function (Horde_Imap_Client_Ids $uids) use ($imapClient, $mailbox, $request) { @@ -163,6 +194,5 @@ static function (Horde_Imap_Client_Ids $uids) use ($imapClient, $mailbox, $reque chunk_uid_sequence($request->getUids(), self::UID_CHUNK_MAX_BYTES) ) ); - return $vanishedUids; } } diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index 68f35c5d49..bb4b6c7040 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -212,8 +212,10 @@ public function sync(Account $account, // call it a day because Horde caches unrelated/unrequested changes until the next // operation. However, our cache is not reliable as some instance might use APCu which // isn't shared between cron and web requests. + $hasQresync = false; if ($client->capability->isEnabled('QRESYNC')) { $this->logger->debug('Forcing full sync due to QRESYNC'); + $hasQresync = true; $criteria |= Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS; @@ -243,7 +245,7 @@ public function sync(Account $account, try { $logger->debug('Running partial sync for ' . $mailbox->getId()); // Only rebuild threads if there were new or vanished messages - $rebuildThreads = $this->runPartialSync($client, $account, $mailbox, $logger, $criteria, $knownUids); + $rebuildThreads = $this->runPartialSync($client, $account, $mailbox, $logger, $hasQresync, $criteria, $knownUids); } catch (UidValidityChangedException $e) { $logger->warning('Mailbox UID validity changed. Wiping cache and performing full sync for ' . $mailbox->getId()); $this->resetCache($account, $mailbox); @@ -368,6 +370,7 @@ private function runPartialSync( Account $account, Mailbox $mailbox, LoggerInterface $logger, + bool $hasQresync, int $criteria, ?array $knownUids = null): bool { $newOrVanished = false; @@ -379,15 +382,19 @@ private function runPartialSync( $uids = $knownUids ?? $this->dbMapper->findAllUids($mailbox); $perf->step('get all known UIDs'); + $requestId = base64_encode(random_bytes(16)); + if ($criteria & Horde_Imap_Client::SYNC_NEWMSGSUIDS) { $response = $this->synchronizer->sync( $client, new Request( + $requestId, $mailbox->getName(), $mailbox->getSyncNewToken(), $uids ), $account->getUserId(), + $hasQresync, Horde_Imap_Client::SYNC_NEWMSGSUIDS ); $perf->step('get new messages via Horde'); @@ -427,11 +434,13 @@ private function runPartialSync( $response = $this->synchronizer->sync( $client, new Request( + $requestId, $mailbox->getName(), $mailbox->getSyncChangedToken(), $uids ), $account->getUserId(), + $hasQresync, Horde_Imap_Client::SYNC_FLAGSUIDS ); $perf->step('get changed messages via Horde'); @@ -457,11 +466,13 @@ private function runPartialSync( $response = $this->synchronizer->sync( $client, new Request( + $requestId, $mailbox->getName(), $mailbox->getSyncVanishedToken(), $uids ), $account->getUserId(), + $hasQresync, Horde_Imap_Client::SYNC_VANISHEDUIDS ); $perf->step('get vanished messages via Horde'); diff --git a/tests/Unit/IMAP/Sync/RequestTest.php b/tests/Unit/IMAP/Sync/RequestTest.php index 1c91372383..3efba7365c 100644 --- a/tests/Unit/IMAP/Sync/RequestTest.php +++ b/tests/Unit/IMAP/Sync/RequestTest.php @@ -25,8 +25,13 @@ protected function setUp(): void { $this->mailbox = 'inbox'; $this->syncToken = 'ab123'; + $this->requestId = 'abcdef'; - $this->request = new Request($this->mailbox, $this->syncToken, []); + $this->request = new Request($this->requestId, $this->mailbox, $this->syncToken, []); + } + + public function testGetId() { + $this->assertEquals($this->requestId, $this->request->getId()); } public function testGetMailbox() { diff --git a/tests/Unit/IMAP/Sync/SynchronizerTest.php b/tests/Unit/IMAP/Sync/SynchronizerTest.php index 88e50d3b7e..05eb08940f 100644 --- a/tests/Unit/IMAP/Sync/SynchronizerTest.php +++ b/tests/Unit/IMAP/Sync/SynchronizerTest.php @@ -12,7 +12,6 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Horde_Imap_Client; use Horde_Imap_Client_Base; -use Horde_Imap_Client_Data_Capability; use Horde_Imap_Client_Data_Sync; use Horde_Imap_Client_Ids; use Horde_Imap_Client_Mailbox; @@ -47,16 +46,10 @@ public function testSyncWithQresync(): void { $request->expects($this->once()) ->method('getToken') ->willReturn('123456'); + $request->expects($this->exactly(3)) + ->method('getId') + ->willReturn('abcdef'); $hordeSync = $this->createMock(Horde_Imap_Client_Data_Sync::class); - $capabilities = $this->createMock(Horde_Imap_Client_Data_Capability::class); - $imapClient->expects(self::once()) - ->method('__get') - ->with('capability') - ->willReturn($capabilities); - $capabilities->expects(self::once()) - ->method('isEnabled') - ->with('QRESYNC') - ->willReturn(true); $imapClient->expects($this->once()) ->method('sync') ->with($this->equalTo(new Horde_Imap_Client_Mailbox('inbox')), $this->equalTo('123456')) @@ -64,20 +57,40 @@ public function testSyncWithQresync(): void { $newMessages = []; $changedMessages = []; $vanishedMessageUids = [4, 5]; - $hordeSync->expects($this->once()) + $hordeSync->expects($this->exactly(3)) ->method('__get') - ->with('vanisheduids') - ->willReturn(new Horde_Imap_Client_Ids($vanishedMessageUids)); + ->willReturnMap([ + ['newmsgsuids', new Horde_Imap_Client_Ids($newMessages)], + ['flagsuids', new Horde_Imap_Client_Ids($changedMessages)], + ['vanisheduids', new Horde_Imap_Client_Ids($vanishedMessageUids)], + ]); $expected = new Response($newMessages, $changedMessages, $vanishedMessageUids); - $response = $this->synchronizer->sync( + $newResponse = $this->synchronizer->sync( + $imapClient, + $request, + 'user', + true, + Horde_Imap_Client::SYNC_NEWMSGSUIDS, + ); + $changedResponse = $this->synchronizer->sync( $imapClient, $request, 'user', + true, + Horde_Imap_Client::SYNC_FLAGSUIDS, + ); + $vanishedResponse = $this->synchronizer->sync( + $imapClient, + $request, + 'user', + true, Horde_Imap_Client::SYNC_VANISHEDUIDS ); - $this->assertEquals($expected, $response); + $this->assertEquals($expected, $newResponse); + $this->assertEquals($expected, $changedResponse); + $this->assertEquals($expected, $vanishedResponse); } public function testSyncChunked(): void { @@ -89,15 +102,6 @@ public function testSyncChunked(): void { ->willReturn('123456'); $request->method('getUids') ->willReturn(range(1, 8000, 2)); // 19444 bytes - $capabilities = $this->createMock(Horde_Imap_Client_Data_Capability::class); - $imapClient->expects(self::once()) - ->method('__get') - ->with('capability') - ->willReturn($capabilities); - $capabilities->expects(self::once()) - ->method('isEnabled') - ->with('QRESYNC') - ->willReturn(false); $hordeSync = $this->createMock(Horde_Imap_Client_Data_Sync::class); $imapClient->expects($this->exactly(3)) ->method('sync') @@ -113,6 +117,7 @@ public function testSyncChunked(): void { $imapClient, $request, 'user', + false, Horde_Imap_Client::SYNC_VANISHEDUIDS );