Merge pull request #2920 from nextcloud/fix/find-all-messages-lower-bound

Fix lower bound when fetching a range of IMAP messages for the sync

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,

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);

tests/Unit/IMAP/MessageMapperTest.php

 <?php
 
+declare(strict_types=1);
+
 /**
  * @author Christoph Wurst <christoph@winzerhof-wurst.at>
  *
@@ -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
+		);
+	}
 }