fix(imap): Chunk MessageMapper::findByIds by command length

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
nextcloud · Apr 21, 2023 · 6b105e9 · 6b105e9
1 parent e17b155
commit 6b105e9
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 11 deletions.
diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php
@@ -51,9 +51,12 @@
 use function count;
 use function fclose;
 use function in_array;
+use function is_array;
 use function iterator_to_array;
 use function max;
 use function min;
+use function OCA\Mail\array_flat_map;
+use function OCA\Mail\chunk_uid_sequence;
 use function sprintf;
 
 class MessageMapper {
@@ -239,7 +242,7 @@ static function (int $uid) use ($highestKnownUid) {
 	/**
 	 * @param Horde_Imap_Client_Base $client
 	 * @param string $mailbox
-	 * @param Horde_Imap_Client_Ids $ids
+	 * @param int[]|Horde_Imap_Client_Ids $ids
 	 * @param string $userId
 	 * @param bool $loadBody
 	 * @return IMAPMessage[]
@@ -252,7 +255,7 @@ static function (int $uid) use ($highestKnownUid) {
 	 */
 	public function findByIds(Horde_Imap_Client_Base $client,
 							  string $mailbox,
-							  Horde_Imap_Client_Ids $ids,
+							  $ids,
 							  string $userId,
 							  bool $loadBody = false): array {
 		$query = new Horde_Imap_Client_Fetch_Query();
@@ -267,10 +270,20 @@ public function findByIds(Horde_Imap_Client_Base $client,
 			]
 		);
 
-		/** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */
-		$fetchResults = iterator_to_array($client->fetch($mailbox, $query, [
-			'ids' => $ids,
-		]), false);
+		if (is_array($ids)) {
+			// Chunk to prevent overly long IMAP commands
+			/** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */
+			$fetchResults = array_flat_map(function ($ids) use ($query, $mailbox, $client) {
+				return iterator_to_array($client->fetch($mailbox, $query, [
+					'ids' => $ids,
+				]), false);
+			}, chunk_uid_sequence($ids, 10000));
+		} else {
+			/** @var Horde_Imap_Client_Data_Fetch[] $fetchResults */
+			$fetchResults = iterator_to_array($client->fetch($mailbox, $query, [
+				'ids' => $ids,
+			]), false);
+		}
 
 		$fetchResults = array_values(array_filter($fetchResults, static function (Horde_Imap_Client_Data_Fetch $fetchResult) {
 			return $fetchResult->exists(Horde_Imap_Client::FETCH_ENVELOPE);
@@ -281,7 +294,11 @@ public function findByIds(Horde_Imap_Client_Base $client,
 		} else {
 			$minFetched = $fetchResults[0]->getUid();
 			$maxFetched = $fetchResults[count($fetchResults) - 1]->getUid();
-			$range = $ids->range_string;
+			if ($ids instanceof Horde_Imap_Client_Ids) {
+				$range = $ids->range_string;
+			} else {
+				$range = 'literals';
+			}
 			$this->logger->debug("findByIds in $mailbox got " . count($ids) . " UIDs ($range) and found " . count($fetchResults) . ". minFetched=$minFetched maxFetched=$maxFetched");
 		}
 

diff --git a/lib/IMAP/Sync/Synchronizer.php b/lib/IMAP/Sync/Synchronizer.php
@@ -97,8 +97,8 @@ public function sync(Horde_Imap_Client_Base $imapClient,
 			throw $e;
 		}
 
-		$newMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), new Horde_Imap_Client_Ids($newUids), $userId);
-		$changedMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), new Horde_Imap_Client_Ids($changedUids), $userId);
+		$newMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), $newUids, $userId);
+		$changedMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), $changedUids, $userId);
 		$vanishedMessageUids = $vanishedUids;
 
 		return new Response($newMessages, $changedMessages, $vanishedMessageUids);

diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php
@@ -27,7 +27,6 @@
 use Horde_Imap_Client;
 use Horde_Imap_Client_Exception;
 use Horde_Imap_Client_Exception_NoSupportExtension;
-use Horde_Imap_Client_Ids;
 use Horde_Imap_Client_Socket;
 use Horde_Mime_Exception;
 use OCA\Mail\Account;
@@ -221,7 +220,7 @@ public function getImapMessagesForScheduleProcessing(Account $account,
 			return $this->imapMessageMapper->findByIds(
 				$client,
 				$mailbox->getName(),
-				new Horde_Imap_Client_Ids($uids),
+				$uids,
 				$account->getUserId(),
 				true
 			);

diff --git a/tests/Unit/IMAP/MessageMapperTest.php b/tests/Unit/IMAP/MessageMapperTest.php
@@ -142,6 +142,39 @@ public function testGetByIds(): void {
 		$this->assertEquals($expected, $result);
 	}
 
+	public function testGetByIdsWithManyMessages(): void {
+		/** @var Horde_Imap_Client_Socket|MockObject $imapClient */
+		$imapClient = $this->createMock(Horde_Imap_Client_Socket::class);
+		$mailbox = 'inbox';
+		$ids = range(1, 10000, 2);
+		$userId = 'user';
+		$loadBody = false;
+		$fetchResults = new Horde_Imap_Client_Fetch_Results();
+		$fetchResult1 = $this->createMock(Horde_Imap_Client_Data_Fetch::class);
+		$fetchResult2 = $this->createMock(Horde_Imap_Client_Data_Fetch::class);
+		$imapClient->expects(self::exactly(3))
+			->method('fetch')
+			->willReturnOnConsecutiveCalls(
+				$fetchResults,
+				$fetchResults,
+				$fetchResults
+			);
+		$fetchResults[0] = $fetchResult1;
+		$fetchResults[1] = $fetchResult2;
+		$fetchResult1->method('getUid')
+			->willReturn(1);
+		$fetchResult2->method('getUid')
+			->willReturn(3);
+
+		$this->mapper->findByIds(
+			$imapClient,
+			$mailbox,
+			$ids,
+			$userId,
+			$loadBody
+		);
+	}
+
 	public function testGetByIdsWithEmpty(): void {
 		/** @var Horde_Imap_Client_Socket|MockObject $imapClient */
 		$imapClient = $this->createMock(Horde_Imap_Client_Socket::class);