Merge pull request #10117 from nextcloud/fix/issue-10098-mailbox-sync

fix: use single connection to sync all mailboxes

lib/Service/Sync/ImapToDbSynchronizer.php

@@ -104,6 +104,9 @@ public function syncAccount(Account $account,
 		$snoozeMailboxId = $account->getMailAccount()->getSnoozeMailboxId();
 		$sentMailboxId = $account->getMailAccount()->getSentMailboxId();
 		$trashRetentionDays = $account->getMailAccount()->getTrashRetentionDays();
+
+		$client = $this->clientFactory->getClient($account);
+
 		foreach ($this->mailboxMapper->findAll($account) as $mailbox) {
 			$syncTrash = $trashMailboxId === $mailbox->getId() && $trashRetentionDays !== null;
 			$syncSnooze = $snoozeMailboxId === $mailbox->getId();
@@ -116,6 +119,7 @@ public function syncAccount(Account $account,
 			$logger->debug('Syncing ' . $mailbox->getId());
 			if ($this->sync(
 				$account,
+				$client,
 				$mailbox,
 				$logger,
 				$criteria,
@@ -126,6 +130,9 @@ public function syncAccount(Account $account,
 				$rebuildThreads = true;
 			}
 		}
+
+		$client->logout();
+
 		$this->dispatcher->dispatchTyped(
 			new SynchronizationEvent(
 				$account,
@@ -187,6 +194,7 @@ private function resetCache(Account $account, Mailbox $mailbox): void {
 	 * @return bool whether to rebuild threads or not
 	 */
 	public function sync(Account $account,
+		Horde_Imap_Client_Base $client,
 		Mailbox $mailbox,
 		LoggerInterface $logger,
 		int $criteria = Horde_Imap_Client::SYNC_NEWMSGSUIDS | Horde_Imap_Client::SYNC_FLAGSUIDS | Horde_Imap_Client::SYNC_VANISHEDUIDS,
@@ -198,7 +206,6 @@ public function sync(Account $account,
 			return $rebuildThreads;
 		}
 
-		$client = $this->clientFactory->getClient($account);
 		$client->login(); // Need to login before fetching capabilities.
 
 		// There is no partial sync when using QRESYNC. As per RFC the client will always pull
@@ -271,8 +278,6 @@ public function sync(Account $account,
 				$logger->debug('Unlocking mailbox ' . $mailbox->getId() . ' from new messages sync');
 				$this->mailboxMapper->unlockFromNewSync($mailbox);
 			}
-
-			$client->logout();
 		}
 
 		if (!$batchSync) {

lib/Service/Sync/SyncService.php

@@ -19,6 +19,7 @@
 use OCA\Mail\Exception\MailboxLockedException;
 use OCA\Mail\Exception\MailboxNotCachedException;
 use OCA\Mail\Exception\ServiceException;
+use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MailboxSync;
 use OCA\Mail\IMAP\PreviewEnhancer;
 use OCA\Mail\IMAP\Sync\Response;
@@ -29,6 +30,9 @@
 use function array_map;
 
 class SyncService {
+
+	private IMAPClientFactory $clientFactory;
+
 	/** @var ImapToDbSynchronizer */
 	private $synchronizer;
 
@@ -50,13 +54,16 @@ class SyncService {
 	/** @var MailboxSync */
 	private $mailboxSync;
 
-	public function __construct(ImapToDbSynchronizer $synchronizer,
+	public function __construct(
+		IMAPClientFactory $clientFactory,
+		ImapToDbSynchronizer $synchronizer,
 		FilterStringParser $filterStringParser,
 		MailboxMapper $mailboxMapper,
 		MessageMapper $messageMapper,
 		PreviewEnhancer $previewEnhancer,
 		LoggerInterface $logger,
 		MailboxSync $mailboxSync) {
+		$this->clientFactory = $clientFactory;
 		$this->synchronizer = $synchronizer;
 		$this->filterStringParser = $filterStringParser;
 		$this->mailboxMapper = $mailboxMapper;
@@ -103,9 +110,12 @@ public function syncMailbox(Account $account,
 		if ($partialOnly && !$mailbox->isCached()) {
 			throw MailboxNotCachedException::from($mailbox);
 		}
-
+
+		$client = $this->clientFactory->getClient($account);
+
 		$this->synchronizer->sync(
 			$account,
+			$client,
 			$mailbox,
 			$this->logger,
 			$criteria,
@@ -115,6 +125,8 @@ public function syncMailbox(Account $account,
 
 		$this->mailboxSync->syncStats($account, $mailbox);
 
+		$client->logout();
+
 		$query = $filter === null ? null : $this->filterStringParser->parse($filter);
 		return $this->getDatabaseSyncChanges(
 			$account,

tests/Unit/Service/Sync/SyncServiceTest.php

@@ -9,22 +9,29 @@
 
 namespace OCA\Mail\Tests\Unit\Service\Sync;
 
+use Horde_Imap_Client_Socket;
 use OCA\Mail\Account;
 use OCA\Mail\Db\Mailbox;
 use OCA\Mail\Db\MailboxMapper;
 use OCA\Mail\Db\MessageMapper;
 use OCA\Mail\Exception\MailboxNotCachedException;
+use OCA\Mail\IMAP\IMAPClientFactory;
 use OCA\Mail\IMAP\MailboxStats;
 use OCA\Mail\IMAP\MailboxSync;
 use OCA\Mail\IMAP\PreviewEnhancer;
 use OCA\Mail\IMAP\Sync\Response;
 use OCA\Mail\Service\Search\FilterStringParser;
 use OCA\Mail\Service\Sync\ImapToDbSynchronizer;
 use OCA\Mail\Service\Sync\SyncService;
+use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Psr\Log\LoggerInterface;
 
 class SyncServiceTest extends TestCase {
+
+	private IMAPClientFactory&MockObject $clientFactory;
+	private Horde_Imap_Client_Socket&MockObject $client;
+
 	/** @var ImapToDbSynchronizer */
 	private $synchronizer;
 
@@ -52,6 +59,8 @@ class SyncServiceTest extends TestCase {
 	protected function setUp(): void {
 		parent::setUp();
 
+		$this->clientFactory = $this->createMock(IMAPClientFactory::class);
+		$this->client = $this->createMock(Horde_Imap_Client_Socket::class);
 		$this->synchronizer = $this->createMock(ImapToDbSynchronizer::class);
 		$this->filterStringParser = $this->createMock(FilterStringParser::class);
 		$this->mailboxMapper = $this->createMock(MailboxMapper::class);
@@ -61,6 +70,7 @@ protected function setUp(): void {
 		$this->mailboxSync = $this->createMock(MailboxSync::class);
 
 		$this->syncService = new SyncService(
+			$this->clientFactory,
 			$this->synchronizer,
 			$this->filterStringParser,
 			$this->mailboxMapper,
@@ -102,7 +112,10 @@ public function testSyncMailboxReturnsFolderStats() {
 			[],
 			new MailboxStats(42, 10, null)
 		);
-
+		$this->clientFactory
+			->method('getClient')
+			->with($account)
+			->willReturn($this->client);
 		$this->messageMapper
 			->method('findUidsForIds')
 			->with($mailbox, [])
@@ -111,6 +124,7 @@ public function testSyncMailboxReturnsFolderStats() {
 			->method('sync')
 			->with(
 				$account,
+				$this->client,
 				$mailbox,
 				$this->loggerInterface,
 				0,