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

Share Horde_Client between sync and permflags query #6323

Merged
merged 1 commit into from
May 11, 2022
Merged

Share Horde_Client between sync and permflags query #6323

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
40 changes: 26 additions & 14 deletions lib/BackgroundJob/MigrateImportantJob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
use OCA\Mail\Db\MailboxMapper;

use OCA\Mail\Exception\ServiceException;
use OCA\Mail\IMAP\IMAPClientFactory;
use OCA\Mail\Migration\MigrateImportantFromImapAndDb;
use OCA\Mail\Service\MailManager;
use OCP\AppFramework\Db\DoesNotExistException;
Expand All @@ -57,19 +58,24 @@ class MigrateImportantJob extends QueuedJob {
/** @var LoggerInterface */
private $logger;

/** @var IMAPClientFactory */
private $imapClientFactory;

public function __construct(MailboxMapper $mailboxMapper,
MailAccountMapper $mailAccountMapper,
MailManager $mailManager,
MigrateImportantFromImapAndDb $migration,
LoggerInterface $logger,
ITimeFactory $timeFactory
ITimeFactory $timeFactory,
IMAPClientFactory $imapClientFactory
) {
parent::__construct($timeFactory);
$this->mailboxMapper = $mailboxMapper;
$this->mailAccountMapper = $mailAccountMapper;
$this->mailManager = $mailManager;
$this->migration = $migration;
$this->logger = $logger;
$this->imapClientFactory = $imapClientFactory;
}

/**
Expand All @@ -96,21 +102,27 @@ public function run($argument) {
}

$account = new Account($mailAccount);
if ($this->mailManager->isPermflagsEnabled($account, $mailbox->getName()) === false) {
$this->logger->debug('Permflags not enabled for <' . $accountId . '>');
return;
}

try {
$this->migration->migrateImportantOnImap($account, $mailbox);
} catch (ServiceException $e) {
$this->logger->debug('Could not flag messages on IMAP for mailbox <' . $mailboxId . '>.');
}
$client = $this->imapClientFactory->getClient($account);

try {
$this->migration->migrateImportantFromDb($account, $mailbox);
} catch (ServiceException $e) {
$this->logger->debug('Could not flag messages from DB on IMAP for mailbox <' . $mailboxId . '>.');
if ($this->mailManager->isPermflagsEnabled($client, $account, $mailbox->getName()) === false) {
$this->logger->debug('Permflags not enabled for <' . $accountId . '>');
return;
}

try {
$this->migration->migrateImportantOnImap($client, $account, $mailbox);
} catch (ServiceException $e) {
$this->logger->debug('Could not flag messages on IMAP for mailbox <' . $mailboxId . '>.');
}

try {
$this->migration->migrateImportantFromDb($client, $account, $mailbox);
} catch (ServiceException $e) {
$this->logger->debug('Could not flag messages from DB on IMAP for mailbox <' . $mailboxId . '>.');
}
} finally {
$client->logout();
}
}
}
3 changes: 2 additions & 1 deletion lib/Contracts/IMailManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

namespace OCA\Mail\Contracts;

use Horde_Imap_Client_Socket;
use OCA\Mail\Account;
use OCA\Mail\Db\Mailbox;
use OCA\Mail\Db\Message;
Expand Down Expand Up @@ -248,7 +249,7 @@ public function getTagByImapLabel(string $imapLabel, string $userId): Tag;
* @param string $mailbox
* @return boolean
*/
public function isPermflagsEnabled(Account $account, string $mailbox): bool;
public function isPermflagsEnabled(Horde_Imap_Client_Socket $client, Account $account, string $mailbox): bool;

/**
* Create a mail tag
Expand Down
54 changes: 21 additions & 33 deletions lib/Migration/MigrateImportantFromImapAndDb.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
namespace OCA\Mail\Migration;

use Horde_Imap_Client_Exception;
use Horde_Imap_Client_Socket;
use OCA\Mail\Account;
use OCA\Mail\Db\Mailbox;
use OCA\Mail\Db\MailboxMapper;
Expand All @@ -52,55 +53,42 @@ class MigrateImportantFromImapAndDb {
/** @var LoggerInterface */
private $logger;

public function __construct(IMAPClientFactory $clientFactory,
MessageMapper $messageMapper,
public function __construct(MessageMapper $messageMapper,
MailboxMapper $mailboxMapper,
LoggerInterface $logger
) {
$this->clientFactory = $clientFactory;
$this->messageMapper = $messageMapper;
$this->mailboxMapper = $mailboxMapper;
$this->logger = $logger;
}

public function migrateImportantOnImap(Account $account, Mailbox $mailbox): void {
$client = $this->clientFactory->getClient($account);
public function migrateImportantOnImap(Horde_Imap_Client_Socket $client, Account $account, Mailbox $mailbox): void {
try {
//get all messages that have an $important label from IMAP
$uids = $this->messageMapper->getFlagged($client, $mailbox, '$important');
} catch (Horde_Imap_Client_Exception $e) {
throw new ServiceException("Could not fetch UIDs of important messages: " . $e->getMessage(), 0, $e);
}
// add $label1 for all that are tagged on IMAP
if (!empty($uids)) {
try {
$uids = $this->messageMapper->getFlagged($client, $mailbox, '$important');
$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
} catch (Horde_Imap_Client_Exception $e) {
throw new ServiceException("Could not fetch UIDs of important messages: " . $e->getMessage(), 0, $e);
}
// add $label1 for all that are tagged on IMAP
if (!empty($uids)) {
try {
$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
} catch (Horde_Imap_Client_Exception $e) {
$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
throw new ServiceException($e->getMessage(), 0, $e);
}
$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
throw new ServiceException($e->getMessage(), 0, $e);
}
} finally {
$client->logout();
}
}

public function migrateImportantFromDb(Account $account, Mailbox $mailbox): void {
$client = $this->clientFactory->getClient($account);
try {
$uids = $this->mailboxMapper->findFlaggedImportantUids($mailbox->getId());
// store our data on imap
if (!empty($uids)) {
try {
$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
} catch (Horde_Imap_Client_Exception $e) {
$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
throw new ServiceException($e->getMessage(), 0, $e);
}
public function migrateImportantFromDb(Horde_Imap_Client_Socket $client, Account $account, Mailbox $mailbox): void {
$uids = $this->mailboxMapper->findFlaggedImportantUids($mailbox->getId());
// store our data on imap
if (!empty($uids)) {
try {
$this->messageMapper->addFlag($client, $mailbox, $uids, Tag::LABEL_IMPORTANT);
} catch (Horde_Imap_Client_Exception $e) {
$this->logger->debug('Could not flag messages in mailbox <' . $mailbox->getId() . '>');
throw new ServiceException($e->getMessage(), 0, $e);
}
} finally {
$client->logout();
}
}
}
19 changes: 10 additions & 9 deletions lib/Service/MailManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use Horde_Imap_Client;
use Horde_Imap_Client_Exception;
use Horde_Imap_Client_Exception_NoSupportExtension;
use Horde_Imap_Client_Socket;
use OCA\Mail\Account;
use OCA\Mail\Contracts\IMailManager;
use OCA\Mail\Db\Mailbox;
Expand Down Expand Up @@ -401,7 +402,7 @@ public function flagMessage(Account $account, string $mailbox, int $uid, string
$client = $this->imapClientFactory->getClient($account);
try {
// Only send system flags to the IMAP server as other flags might not be supported
$imapFlags = $this->filterFlags($account, $flag, $mailbox);
$imapFlags = $this->filterFlags($client, $account, $flag, $mailbox);
foreach ($imapFlags as $imapFlag) {
if (empty($imapFlag) === true) {
continue;
Expand Down Expand Up @@ -456,8 +457,8 @@ public function tagMessage(Account $account, string $mailbox, Message $message,
} catch (DoesNotExistException $e) {
throw new ClientException("Mailbox $mailbox does not exist", 0, $e);
}
if ($this->isPermflagsEnabled($account, $mailbox) === true) {
$client = $this->imapClientFactory->getClient($account);
$client = $this->imapClientFactory->getClient($account);
if ($this->isPermflagsEnabled($client, $account, $mailbox) === true) {
try {
if ($value) {
// imap keywords and flags work the same way
Expand All @@ -474,7 +475,10 @@ public function tagMessage(Account $account, string $mailbox, Message $message,
} finally {
$client->logout();
}
} else {
$client->logout();
}

if ($value) {
$this->tagMapper->tagMessage($tag, $message->getMessageId(), $account->getUserId());
} else {
Expand Down Expand Up @@ -617,15 +621,15 @@ public function getTagByImapLabel(string $imapLabel, string $userId): Tag {
* @param string $mailbox
* @return array
*/
public function filterFlags(Account $account, string $flag, string $mailbox): array {
public function filterFlags(Horde_Imap_Client_Socket $client, Account $account, string $flag, string $mailbox): array {
// check for RFC server flags
if (array_key_exists($flag, self::ALLOWED_FLAGS) === true) {
return self::ALLOWED_FLAGS[$flag];
}

// Only allow flag setting if IMAP supports Permaflags
// @TODO check if there are length & char limits on permflags
if ($this->isPermflagsEnabled($account, $mailbox) === true) {
if ($this->isPermflagsEnabled($client, $account, $mailbox) === true) {
return [$flag];
}
return [];
Expand All @@ -638,8 +642,7 @@ public function filterFlags(Account $account, string $flag, string $mailbox): ar
* @param string $mailbox
* @return boolean
*/
public function isPermflagsEnabled(Account $account, string $mailbox): bool {
$client = $this->imapClientFactory->getClient($account);
public function isPermflagsEnabled(Horde_Imap_Client_Socket $client, Account $account, string $mailbox): bool {
try {
$capabilities = $client->status($mailbox, Horde_Imap_Client::STATUS_PERMFLAGS);
} catch (Horde_Imap_Client_Exception $e) {
Expand All @@ -648,8 +651,6 @@ public function isPermflagsEnabled(Account $account, string $mailbox): bool {
(int)$e->getCode(),
$e
);
} finally {
$client->logout();
}
return (is_array($capabilities) === true && array_key_exists('permflags', $capabilities) === true && in_array("\*", $capabilities['permflags'], true) === true);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Service/Sync/ImapToDbSynchronizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ private function runPartialSync(Account $account,
);
$perf->step('get changed messages via Horde');

$permflagsEnabled = $this->mailManager->isPermflagsEnabled($account, $mailbox->getName());
$permflagsEnabled = $this->mailManager->isPermflagsEnabled($client, $account, $mailbox->getName());

foreach (array_chunk($response->getChangedMessages(), 500) as $chunk) {
$this->dbMapper->updateBulk($account, $permflagsEnabled, ...array_map(static function (IMAPMessage $imapMessage) use ($mailbox, $account) {
Expand Down
Loading