Skip to content

Commit

Permalink
Merge pull request #8383 from nextcloud/fix/imap/chunk-uids-string-le…
Browse files Browse the repository at this point in the history
…ngth-II

fix(imap): Chunk MessageMapper::findByIds by command length
  • Loading branch information
ChristophWurst authored Apr 21, 2023
2 parents 473e621 + 768d8f8 commit caf1f18
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 11 deletions.
31 changes: 24 additions & 7 deletions lib/IMAP/MessageMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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[]
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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");
}

Expand Down
4 changes: 2 additions & 2 deletions lib/IMAP/Sync/Synchronizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions lib/Service/MailManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
);
Expand Down
33 changes: 33 additions & 0 deletions tests/Unit/IMAP/MessageMapperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit caf1f18

Please sign in to comment.