From 4a9eab08b37166df1c507e4a153847a563daa786 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 19 Apr 2023 17:43:38 +0200 Subject: [PATCH] fix(imap): Chunk MessageMapper::findByIds by command length Signed-off-by: Christoph Wurst --- lib/IMAP/MessageMapper.php | 30 ++++++++++++++++++++----- lib/IMAP/Sync/Synchronizer.php | 4 ++-- lib/Service/MailManager.php | 3 +-- tests/Unit/IMAP/MessageMapperTest.php | 32 +++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 10 deletions(-) diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php index 6e5e2bc8ca..41b6e714f8 100644 --- 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 index 034b141f19..3d8c90a413 100644 --- 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 index dcce23d4e0..a562f3a4f6 100644 --- 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 index cbcc52b660..b370c03d04 100644 --- 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);