Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable2.2] fix(imap): Chunk MessageMapper::findByIds by command length #8399

Closed
wants to merge 1 commit into from
Closed

[stable2.2] fix(imap): Chunk MessageMapper::findByIds by command length #8399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions lib/IMAP/MessageMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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");
}

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