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

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

Merged
merged 1 commit into from
Apr 21, 2023
Merged

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

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
31 changes: 24 additions & 7 deletions lib/IMAP/MessageMapper.php
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@st3iny this is the problematic call. You can mod getChangedMessageUids to just return $request->getUids(). It will fake that all messages changed and therefore trigger a re-fetch.

$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 @@ -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
View file
Open in desktop
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