Merge pull request #10192 from nextcloud/backport/10035/stable4.0

[stable4.0] fix(imap): do a single full sync when QRESYNC is enabled
nextcloud · Sep 30, 2024 · 0a669a0 · 0a669a0
2 parents 3c09257 + a4b47f6
commit 0a669a0
Show file tree

Hide file tree

Showing 5 changed files with 118 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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,37 +46,51 @@ 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'))
 			->willReturn($hordeSync);
 		$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
 		);