Skip to content

Commit

Permalink
fix(imap): persist vanished messages immediately on EXAMINE commands
Browse files Browse the repository at this point in the history
Signed-off-by: Richard Steinmetz <[email protected]>
  • Loading branch information
st3iny committed Aug 30, 2024
1 parent 8792c72 commit 44c5315
Show file tree
Hide file tree
Showing 6 changed files with 270 additions and 45 deletions.
93 changes: 53 additions & 40 deletions lib/Cache/Cache.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,17 @@
use Exception;
use Horde_Imap_Client_Cache_Backend;
use Horde_Imap_Client_Exception;
use InvalidArgumentException;
use OCA\Mail\Account;
use OCA\Mail\Db\MailboxMapper;
use OCA\Mail\Db\MessageMapper;
use OCP\ICache;

/**
* This class is inspired by Horde_Imap_Client_Cache_Backend_Cache of the Horde Project
*/
class Cache extends Horde_Imap_Client_Cache_Backend {
/** Cache structure version. */
public const VERSION = 3;

/**
* The cache object.
*/
protected ICache $_cache;
public const VERSION = 4;

/**
* The working data for the current pageload. All changes take place to
Expand All @@ -52,26 +49,24 @@ class Cache extends Horde_Imap_Client_Cache_Backend {
*/
protected array $_update = [];

/**
* Constructor.
*
* @param array $params Configuration parameters:
*/
public function __construct(array $params = []) {
// Default parameters.
$params = array_merge([
'lifetime' => 604800,
'slicesize' => 50
], array_filter($params));

if (!isset($params['cacheob'])) {
throw new InvalidArgumentException('Missing cacheob parameter.');
}

foreach (['lifetime', 'slicesize'] as $val) {
$params[$val] = intval($params[$val]);
}

/** @var int[]|null */
private ?array $cachedUids = null;

private ?int $uidvalid = null;

public function __construct(
private MessageMapper $messageMapper,
private MailboxMapper $mailboxMapper,
private Account $account,
private ICache $_cache,
int $lifetime = 604800,
int $slicesize = 50,
) {
$params = [
'lifetime' => $lifetime,
'slicesize' => $slicesize,
'cacheob' => $this->_cache,
];
parent::__construct($params);
}

Expand Down Expand Up @@ -164,13 +159,23 @@ public function get($mailbox, $uids, $fields, $uidvalid) {
return $ret;
}

/** {@inheritDoc} */
public function getCachedUids($mailbox, $uidvalid) {
$this->_loadSliceMap($mailbox, $uidvalid);
return array_unique(array_merge(
array_keys($this->_slicemap[$mailbox]['s']),
(isset($this->_update[$mailbox]) ? $this->_update[$mailbox]['add'] : [])
));
public function getCachedUids($mailbox, $uidvalid): array {
// Delete cached data of mailbox if uidvalid has changed
if ($this->uidvalid !== null && $uidvalid !== null && $this->uidvalid !== (int)$uidvalid) {
$this->_deleteMailbox($mailbox);
}

// Refresh cached uids lazily
if ($this->cachedUids === null) {
$mailboxEntity = $this->mailboxMapper->find($this->account, $mailbox);
$this->cachedUids = $this->messageMapper->findAllUids($mailboxEntity);
}

if ($this->uidvalid === null && $uidvalid !== null) {
$this->uidvalid = (int)$uidvalid;
}

return array_merge([], $this->cachedUids);
}

/**
Expand Down Expand Up @@ -235,12 +240,15 @@ public function setMetaData($mailbox, $data) {
$this->_toUpdate($mailbox, 'slicemap', true);
}

/**
* {@inheritDoc}
*
* @return void
*/
public function deleteMsgs($mailbox, $uids) {
public function deleteMsgs($mailbox, $uids): void {
// Delete uids from the db cache
$mailboxEntity = $this->mailboxMapper->find($this->account, $mailbox);
$this->messageMapper->deleteByUid($mailboxEntity, ...$uids);
if ($this->cachedUids !== null) {
$this->cachedUids = array_diff($this->cachedUids, $uids);
}

// Delete uids from the memory cache
$this->_loadSliceMap($mailbox);

$slicemap = &$this->_slicemap[$mailbox];
Expand Down Expand Up @@ -298,6 +306,8 @@ public function deleteMailbox($mailbox) {
public function clear($lifetime) {
$this->_cache->clear();
$this->_data = $this->_loaded = $this->_slicemap = $this->_update = [];
$this->cachedUids = null;
$this->uidvalid = null;
}

/**
Expand Down Expand Up @@ -339,6 +349,9 @@ protected function _deleteMailbox($mbox): void {
$this->_slicemap[$mbox],
$this->_update[$mbox]
);

$this->cachedUids = null;
$this->uidvalid = null;
}

/**
Expand Down
32 changes: 32 additions & 0 deletions lib/Cache/CacheFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Mail\Cache;

use OCA\Mail\Account;
use OCA\Mail\Db\MailboxMapper;
use OCA\Mail\Db\MessageMapper;
use OCP\ICache;

class CacheFactory {
public function __construct(
private MailboxMapper $mailboxMapper,
private MessageMapper $messageMapper,
) {
}

public function newCache(Account $account, ICache $cache): Cache {
return new Cache(
$this->messageMapper,
$this->mailboxMapper,
$account,
$cache,
);
}
}
18 changes: 13 additions & 5 deletions lib/IMAP/IMAPClientFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
use Horde_Imap_Client_Password_Xoauth2;
use Horde_Imap_Client_Socket;
use OCA\Mail\Account;
use OCA\Mail\Cache\Cache;
use OCA\Mail\Cache\CacheFactory;
use OCA\Mail\Events\BeforeImapClientCreated;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\EventDispatcher\IEventDispatcher;
Expand All @@ -40,17 +40,20 @@ class IMAPClientFactory {
private $eventDispatcher;

private ITimeFactory $timeFactory;
private CacheFactory $hordeCacheFactory;

public function __construct(ICrypto $crypto,
IConfig $config,
ICacheFactory $cacheFactory,
IEventDispatcher $eventDispatcher,
ITimeFactory $timeFactory) {
ITimeFactory $timeFactory,
CacheFactory $hordeCacheFactory) {
$this->crypto = $crypto;
$this->config = $config;
$this->cacheFactory = $cacheFactory;
$this->eventDispatcher = $eventDispatcher;
$this->timeFactory = $timeFactory;
$this->hordeCacheFactory = $hordeCacheFactory;
}

/**
Expand Down Expand Up @@ -113,10 +116,15 @@ public function getClient(Account $account, bool $useCache = true): Horde_Imap_C
);
if ($useCache && $this->cacheFactory->isAvailable()) {
$params['cache'] = [
'backend' => new Cache([
'cacheob' => $this->cacheFactory->createDistributed(md5((string)$account->getId())),
])];
'backend' => $this->hordeCacheFactory->newCache(
$account,
$this->cacheFactory->createDistributed(md5((string)$account->getId())),
),
];
} else {
// WARNING: This is very dangerous! We **will** miss changes when using QRESYNC without
// actually persisting changes to the cache. Especially vanished messages will
// be missed.
/**
* If we don't use a cache we use a null cache to trick Horde into
* using QRESYNC/CONDSTORE if they are available
Expand Down
50 changes: 50 additions & 0 deletions tests/Integration/Framework/Caching.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Mail\Tests\Integration\Framework;

use OC\Memcache\Factory;
use OCA\Mail\Cache\CacheFactory;
use OCA\Mail\IMAP\IMAPClientFactory;
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IConfig;
use OCP\Profiler\IProfiler;
use OCP\Security\ICrypto;
use OCP\Server;
use Psr\Log\LoggerInterface;

class Caching {
/**
* Force usage of a real cache as configured in system config. The original ICacheFactory
* service closure is hard-coded to always return an instance of ArrayCache when the global
* PHPUNIT_RUN is defined.
*/
public static function getImapClientFactoryAndConfiguredCacheFactory(?ICrypto $crypto = null): array {
$config = Server::get(IConfig::class);
$cacheFactory = new Factory(
'mail-integration-tests',
Server::get(LoggerInterface::class),
Server::get(IProfiler::class),
$config->getSystemValue('memcache.local', null),
$config->getSystemValue('memcache.distributed', null),
$config->getSystemValue('memcache.locking', null),
$config->getSystemValueString('redis_log_file')
);
$imapClient = new IMAPClientFactory(
$crypto ?? Server::get(ICrypto::class),
$config,
$cacheFactory,
Server::get(IEventDispatcher::class),
Server::get(ITimeFactory::class),
Server::get(CacheFactory::class),
);
return [$imapClient, $cacheFactory];
}
}
25 changes: 25 additions & 0 deletions tests/Integration/Framework/ImapTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,31 @@ public function deleteMessage($mailbox, $id, ?MailAccount $account = null) {
}
}

/**
* Delete a message without informing Horde or the db cache. This simulates another client
* deleting a message on IMAP.
*
* @param int[] $uids
*/
public function deleteMessagesExternally(string $mailbox, array $uids): void {
$client = new Horde_Imap_Client_Socket([
'username' => '[email protected]',
'password' => 'mypassword',
'hostspec' => '127.0.0.1',
'port' => 993,
'secure' => 'ssl',
]);
$ids = new Horde_Imap_Client_Ids($uids);
try {
$client->expunge($mailbox, [
'ids' => $ids,
'delete' => true,
]);
} finally {
$client->logout();
}
}

/**
* @param Horde_Imap_Client_Socket $client
* @param string $mailbox
Expand Down
Loading

0 comments on commit 44c5315

Please sign in to comment.