Fix initial sync stopping prematurely

The initial sync is broken up into chunks. These chunks are estimated
because we only know the lowest and highest UID and there are likely
gaps in this range. One of the last chunks might query a range that has
more messages than the allowed chunk size. We slice it down. This means
the actually feched max UID is lower than the theorical max UID for this
chunk.

If the stop condition is to stop at the theoretical max UID then we
occasionally stop too early. Where we actually have to stop is the
actually fetched highest UID.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

1 files changed, 96 insertions, 10 deletions

diff --git a/tests/Unit/IMAP/MessageMapperTest.php b/tests/Unit/IMAP/MessageMapperTest.php
index 80011fe20..88dbd598d 100644
--- a/tests/Unit/IMAP/MessageMapperTest.php
+++ b/tests/Unit/IMAP/MessageMapperTest.php
@@ -35,6 +35,7 @@ use OCA\Mail\IMAP\MessageMapper;
 use OCA\Mail\Model\IMAPMessage;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
+use function range;
 
 class MessageMapperTest extends TestCase {
 
@@ -132,7 +133,7 @@ class MessageMapperTest extends TestCase {
 		/** @var Horde_Imap_Client_Socket|MockObject $client */
 		$client = $this->createMock(Horde_Imap_Client_Socket::class);
 		$mailbox = 'inbox';
-		$client->expects($this->once())
+		$client->expects(self::once())
 			->method('search')
 			->with(
 				$mailbox,
@@ -153,8 +154,13 @@ class MessageMapperTest extends TestCase {
 		$query = new Horde_Imap_Client_Fetch_Query();
 		$query->uid();
 		$uidResults = new Horde_Imap_Client_Fetch_Results();
+		foreach (range(123, 321) as $i) {
+			$uid = new Horde_Imap_Client_Data_Fetch();
+			$uid->setUid($i);
+			$uidResults[$i] = $uid;
+		}
 		$bodyResults = new Horde_Imap_Client_Fetch_Results();
-		$client->expects($this->exactly(2))
+		$client->expects(self::exactly(2))
 			->method('fetch')
 			->withConsecutive(
 				[
@@ -166,8 +172,8 @@ class MessageMapperTest extends TestCase {
 				],
 				[
 					$mailbox,
-					$this->anything(),
-					$this->anything()
+					self::anything(),
+					self::anything()
 				]
 			)
 			->willReturnOnConsecutiveCalls(
@@ -175,19 +181,21 @@ class MessageMapperTest extends TestCase {
 				$bodyResults
 			);
 
-		$this->mapper->findAll(
+		$result = $this->mapper->findAll(
 			$client,
 			$mailbox,
 			5000,
 			0
 		);
+
+		self::assertTrue($result['all']);
 	}
 
 	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())
+		$client->expects(self::once())
 			->method('search')
 			->with(
 				$mailbox,
@@ -208,8 +216,13 @@ class MessageMapperTest extends TestCase {
 		$query = new Horde_Imap_Client_Fetch_Query();
 		$query->uid();
 		$uidResults = new Horde_Imap_Client_Fetch_Results();
+		foreach (range(123, 321) as $i) {
+			$uid = new Horde_Imap_Client_Data_Fetch();
+			$uid->setUid($i);
+			$uidResults[$i] = $uid;
+		}
 		$bodyResults = new Horde_Imap_Client_Fetch_Results();
-		$client->expects($this->exactly(2))
+		$client->expects(self::exactly(2))
 			->method('fetch')
 			->withConsecutive(
 				[
@@ -221,8 +234,8 @@ class MessageMapperTest extends TestCase {
 				],
 				[
 					$mailbox,
-					$this->anything(),
-					$this->anything()
+					self::anything(),
+					self::anything()
 				]
 			)
 			->willReturnOnConsecutiveCalls(
@@ -230,12 +243,85 @@ class MessageMapperTest extends TestCase {
 				$bodyResults
 			);
 
-		$this->mapper->findAll(
+		$result = $this->mapper->findAll(
 			$client,
 			$mailbox,
 			5000,
 			300
 		);
+
+		self::assertTrue($result['all']);
+	}
+
+	/**
+	 * Assume we have a large mailbox with many messages spread across a wide
+	 * range of UIDs. This inbox is fetched in chunks and the chunks might be
+	 * fragmented in many way. One edge case is that the last estimated chunk
+	 * already reached the upper UID but there are too many messages to fetch,
+	 * so the process will stop before that.
+	 */
+	public function testFindAllPackedLastChunk(): void {
+		/** @var Horde_Imap_Client_Socket|MockObject $client */
+		$client = $this->createMock(Horde_Imap_Client_Socket::class);
+		$mailbox = 'inbox';
+		$client->expects(self::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' => 10000,
+				'max' => 99999,
+				'count' => 50000,
+			]);
+		$query = new Horde_Imap_Client_Fetch_Query();
+		$query->uid();
+		$uidResults = new Horde_Imap_Client_Fetch_Results();
+		foreach (range(92000, 98000) as $i) {
+			$uid = new Horde_Imap_Client_Data_Fetch();
+			$uid->setUid($i);
+			$uidResults[$i] = $uid;
+		}
+		$bodyResults = new Horde_Imap_Client_Fetch_Results();
+		$client->expects(self::exactly(2))
+			->method('fetch')
+			->withConsecutive(
+				[
+					$mailbox,
+					$query,
+					[
+						'ids' => new Horde_Imap_Client_Ids('92001:99999'),
+					]
+				],
+				[
+					$mailbox,
+					self::anything(),
+					self::anything()
+				]
+			)
+			->willReturnOnConsecutiveCalls(
+				$uidResults,
+				$bodyResults
+			);
+
+		$result = $this->mapper->findAll(
+			$client,
+			$mailbox,
+			5000,
+			92000
+		);
+
+		// This chunk returns 8k messages, when we only expected 5k. So the process
+		// isn't done and the client has to fetch again.
+		self::assertFalse($result['all']);
 	}
 
 	public function testGetFlagged() {