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

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
nextcloud · Feb 21, 2024 · 4a9eab0 · 4a9eab0
1 parent e9606a1
commit 4a9eab0
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 10 deletions.
diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php
@@ -45,9 +45,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 reset;
 use function sprintf;
 
@@ -222,12 +225,13 @@ function (int $uid) use ($highestKnownUid) {
 	}
 
 	/**
+	 * @param int[]|Horde_Imap_Client_Ids $ids
 	 * @return IMAPMessage[]
 	 * @throws Horde_Imap_Client_Exception
 	 */
 	public function findByIds(Horde_Imap_Client_Base $client,
 							  string $mailbox,
-							  Horde_Imap_Client_Ids $ids,
+							  $ids,
 							  bool $loadBody = false): array {
 		$query = new Horde_Imap_Client_Fetch_Query();
 		$query->envelope();
@@ -241,10 +245,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);
@@ -255,7 +269,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
@@ -95,8 +95,8 @@ public function sync(Horde_Imap_Client_Base $imapClient,
 			throw $e;
 		}
 
-		$newMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), new Horde_Imap_Client_Ids($newUids));
-		$changedMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), new Horde_Imap_Client_Ids($changedUids));
+		$newMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), $newUids);
+		$changedMessages = $this->messageMapper->findByIds($imapClient, $request->getMailbox(), $changedUids);
 		$vanishedMessageUids = $vanishedUids;
 
 		return new Response($newMessages, $changedMessages, $vanishedMessageUids);

diff --git a/lib/Service/MailManager.php b/lib/Service/MailManager.php
@@ -26,7 +26,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 OCA\Mail\Account;
 use OCA\Mail\Contracts\IMailManager;
@@ -208,7 +207,7 @@ public function getImapMessagesForScheduleProcessing(Account $account,
 			return $this->imapMessageMapper->findByIds(
 				$client,
 				$mailbox->getName(),
-				new Horde_Imap_Client_Ids($uids),
+				$uids,
 				true
 			);
 		} catch (Horde_Imap_Client_Exception $e) {

diff --git a/tests/Unit/IMAP/MessageMapperTest.php b/tests/Unit/IMAP/MessageMapperTest.php
@@ -94,6 +94,38 @@ 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,
+			$loadBody
+		);
+	}
+
 	public function testGetByIdsWithEmpty(): void {
 		/** @var Horde_Imap_Client_Socket|MockObject $imapClient */
 		$imapClient = $this->createMock(Horde_Imap_Client_Socket::class);