diff --git a/lib/IMAP/MessageMapper.php b/lib/IMAP/MessageMapper.php index be8240410..7df82e9b9 100644 --- a/lib/IMAP/MessageMapper.php +++ b/lib/IMAP/MessageMapper.php @@ -71,18 +71,18 @@ class MessageMapper { /** * @param Horde_Imap_Client_Socket $client - * @param Mailbox $mailbox + * @param string $mailbox * * @param int $maxResults - * @param int|null $highestKnownUid + * @param int $highestKnownUid * * @return array * @throws Horde_Imap_Client_Exception */ public function findAll(Horde_Imap_Client_Socket $client, - Mailbox $mailbox, + string $mailbox, int $maxResults, - ?int $highestKnownUid = 0): array { + int $highestKnownUid): array { /** * To prevent memory exhaustion, we don't want to just ask for a list of * all UIDs and limit them client-side. Instead we can (hopefully @@ -94,7 +94,7 @@ class MessageMapper { */ $metaResults = $client->search( - $mailbox->getName(), + $mailbox, null, [ 'results' => [ @@ -125,11 +125,17 @@ class MessageMapper { // +1 is added to fetch all messages with the rare case of strictly // continuous UIDs and fractions $estimatedPageSize = (int)(($totalRange / $total) * $maxResults) + 1; + // Determine min UID to fetch, but don't exceed the known maximum + $lower = max( + $min, + ($highestKnownUid ?? 0) + 1 + ); // Determine max UID to fetch, but don't exceed the known maximum $upper = min( $max, - $highestKnownUid + $estimatedPageSize + $lower + $estimatedPageSize ); + $this->logger->debug("Built range for findAll: min=$min max=$max total=$total totalRange=$totalRange estimatedPageSize=$estimatedPageSize lower=$lower upper=$upper highestKnownUid=$highestKnownUid"); $query = new Horde_Imap_Client_Fetch_Query(); $query->uid(); @@ -140,10 +146,10 @@ class MessageMapper { return $data->getUid(); }, iterator_to_array($client->fetch( - $mailbox->getName(), + $mailbox, $query, [ - 'ids' => new Horde_Imap_Client_Ids(($highestKnownUid + 1) . ':' . $upper) + 'ids' => new Horde_Imap_Client_Ids($lower . ':' . $upper) ] )) ), @@ -159,7 +165,7 @@ class MessageMapper { return [ 'messages' => $this->findByIds( $client, - $mailbox->getName(), + $mailbox, $uidsToFetch ), 'all' => $upper === $max, diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index 8a84b9087..7709d692b 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -175,7 +175,12 @@ class ImapToDbSynchronizer { $highestKnownUid = $this->dbMapper->findHighestUid($mailbox); $client = $this->clientFactory->getClient($account); try { - $imapMessages = $this->imapMapper->findAll($client, $mailbox, self::MAX_NEW_MESSAGES, $highestKnownUid); + $imapMessages = $this->imapMapper->findAll( + $client, + $mailbox->getName(), + self::MAX_NEW_MESSAGES, + $highestKnownUid ?? 0 + ); $perf->step('fetch all messages from IMAP'); } catch (Horde_Imap_Client_Exception $e) { throw new ServiceException('Can not get messages from mailbox ' . $mailbox->getName() . ': ' . $e->getMessage(), 0, $e); diff --git a/tests/Unit/IMAP/MessageMapperTest.php b/tests/Unit/IMAP/MessageMapperTest.php index 666b97219..58c52a6eb 100644 --- a/tests/Unit/IMAP/MessageMapperTest.php +++ b/tests/Unit/IMAP/MessageMapperTest.php @@ -1,5 +1,7 @@ * @@ -22,9 +24,12 @@ namespace OCA\Mail\Tests\Unit\IMAP; use ChristophWurst\Nextcloud\Testing\TestCase; -use Horde_Imap_Client_Base; +use Horde_Imap_Client; use Horde_Imap_Client_Data_Fetch; +use Horde_Imap_Client_Fetch_Query; use Horde_Imap_Client_Fetch_Results; +use Horde_Imap_Client_Ids; +use Horde_Imap_Client_Socket; use OCA\Mail\IMAP\MessageMapper; use OCA\Mail\Model\IMAPMessage; use OCP\ILogger; @@ -49,7 +54,8 @@ class MessageMapperTest extends TestCase { } public function testGetByIds() { - $imapClient = $this->createMock(Horde_Imap_Client_Base::class); + /** @var Horde_Imap_Client_Socket|MockObject $imapClient */ + $imapClient = $this->createMock(Horde_Imap_Client_Socket::class); $mailbox = 'inbox'; $ids = [1, 3]; @@ -78,4 +84,139 @@ class MessageMapperTest extends TestCase { $this->assertEquals($expected, $result); } + + public function testFindAllEmptyMailbox(): void { + /** @var Horde_Imap_Client_Socket|MockObject $client */ + $client = $this->createMock(Horde_Imap_Client_Socket::class); + $mailbox = 'inbox'; + $client->expects($this->once()) + ->method('search') + ->with( + $mailbox, + null, + [ + 'results' => [ + Horde_Imap_Client::SEARCH_RESULTS_MIN, + Horde_Imap_Client::SEARCH_RESULTS_MAX, + Horde_Imap_Client::SEARCH_RESULTS_COUNT, + ] + ] + ) + ->willReturn([ + 'min' => 0, + 'max' => 0, + 'count' => 0, + ]); + $client->expects($this->never()) + ->method('fetch'); + + $result = $this->mapper->findAll( + $client, + $mailbox, + 5000, + 0 + ); + + $this->assertSame( + [ + 'messages' => [], + 'all' => true, + ], + $result + ); + } + + public function testFindAllNoKnownUid(): void { + /** @var Horde_Imap_Client_Socket|MockObject $client */ + $client = $this->createMock(Horde_Imap_Client_Socket::class); + $mailbox = 'inbox'; + $client->expects($this->once()) + ->method('search') + ->with( + $mailbox, + null, + [ + 'results' => [ + Horde_Imap_Client::SEARCH_RESULTS_MIN, + Horde_Imap_Client::SEARCH_RESULTS_MAX, + Horde_Imap_Client::SEARCH_RESULTS_COUNT, + ] + ] + ) + ->willReturn([ + 'min' => 123, + 'max' => 321, + 'count' => 50, + ]); + $query = new Horde_Imap_Client_Fetch_Query(); + $query->uid(); + $uidResults = new Horde_Imap_Client_Fetch_Results(); + $client->expects($this->at(1)) + ->method('fetch') + ->with( + $mailbox, + $query, + [ + 'ids' => new Horde_Imap_Client_Ids('123:321'), + ] + )->willReturn($uidResults); + $bodyResults = new Horde_Imap_Client_Fetch_Results(); + $client->expects($this->at(2)) + ->method('fetch') + ->willReturn($bodyResults); + + $this->mapper->findAll( + $client, + $mailbox, + 5000, + 0 + ); + } + + public function testFindAllWithKnownUid(): void { + /** @var Horde_Imap_Client_Socket|MockObject $client */ + $client = $this->createMock(Horde_Imap_Client_Socket::class); + $mailbox = 'inbox'; + $client->expects($this->once()) + ->method('search') + ->with( + $mailbox, + null, + [ + 'results' => [ + Horde_Imap_Client::SEARCH_RESULTS_MIN, + Horde_Imap_Client::SEARCH_RESULTS_MAX, + Horde_Imap_Client::SEARCH_RESULTS_COUNT, + ] + ] + ) + ->willReturn([ + 'min' => 123, + 'max' => 321, + 'count' => 50, + ]); + $query = new Horde_Imap_Client_Fetch_Query(); + $query->uid(); + $uidResults = new Horde_Imap_Client_Fetch_Results(); + $client->expects($this->at(1)) + ->method('fetch') + ->with( + $mailbox, + $query, + [ + 'ids' => new Horde_Imap_Client_Ids('301:321'), + ] + )->willReturn($uidResults); + $bodyResults = new Horde_Imap_Client_Fetch_Results(); + $client->expects($this->at(2)) + ->method('fetch') + ->willReturn($bodyResults); + + $this->mapper->findAll( + $client, + $mailbox, + 5000, + 300 + ); + } }