From 579beb1dc8505d7447efa712fcaa4d81f9152ea4 Mon Sep 17 00:00:00 2001 From: steven-mpawulo Date: Tue, 21 Jul 2026 11:15:01 +0300 Subject: [PATCH 1/2] feat: move imip tracking into a dedicated table Adds mail_messages_imip table and ImipDataMapper, and switches PreviewEnhancer, MessageMapper, and IMipService to read/write iMIP status there instead of imip_message/imip_processed/imip_error columns on mail_messages. Signed-off-by: steven-mpawulo --- lib/Db/ImipData.php | 37 ++++++ lib/Db/ImipDataMapper.php | 107 ++++++++++++++++++ lib/Db/MessageMapper.php | 18 ++- lib/IMAP/PreviewEnhancer.php | 24 +++- .../Version5201Date20260720120000.php | 66 +++++++++++ lib/Service/IMipService.php | 9 +- tests/Unit/IMAP/PreviewEnhancerTest.php | 7 +- tests/Unit/Service/IMipServiceTest.php | 8 +- 8 files changed, 261 insertions(+), 15 deletions(-) create mode 100644 lib/Db/ImipData.php create mode 100644 lib/Db/ImipDataMapper.php create mode 100644 lib/Migration/Version5201Date20260720120000.php diff --git a/lib/Db/ImipData.php b/lib/Db/ImipData.php new file mode 100644 index 0000000000..a295b0eaed --- /dev/null +++ b/lib/Db/ImipData.php @@ -0,0 +1,37 @@ +addType('imipMessageId', 'integer'); + $this->addType('error', 'boolean'); + $this->addType('processedAt', 'integer'); + } +} diff --git a/lib/Db/ImipDataMapper.php b/lib/Db/ImipDataMapper.php new file mode 100644 index 0000000000..ebb2cba0cf --- /dev/null +++ b/lib/Db/ImipDataMapper.php @@ -0,0 +1,107 @@ + + */ +class ImipDataMapper extends QBMapper { + + /** + * @param IDBConnection $db + */ + public function __construct( + IDBConnection $db, + private ITimeFactory $timeFactory, + ) { + parent::__construct($db, 'mail_messages_imip'); + } + + public function findByMessageId(int $messageId): ?ImipData { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->getTableName()) + ->where( + $qb->expr()->eq('imip_message_id', $qb->createNamedParameter($messageId, IQueryBuilder::PARAM_INT)) + ); + + try { + return $this->findEntity($qb); + } catch (DoesNotExistException) { + return null; + } + } + + /** + * @throws Exception + */ + public function markAsImipMessage(int $messageId): void { + if ($this->findByMessageId($messageId) !== null) { + return; + } + + $imipData = new ImipData(); + $imipData->setImipMessageId($messageId); + $imipData->setError(false); + $imipData->setProcessedAt(null); + $this->insert($imipData); + } + + /** + * @throws Exception + */ + public function markProcessed(int $messageId, bool $error): void { + $qb = $this->db->getQueryBuilder(); + $update = $qb->update($this->getTableName()) + ->set('error', $qb->createNamedParameter($error, IQueryBuilder::PARAM_BOOL)) + ->set('processed_at', $qb->createNamedParameter($this->timeFactory->getTime(), IQueryBuilder::PARAM_INT)) + ->where( + $qb->expr()->eq('imip_message_id', $qb->createNamedParameter($messageId, IQueryBuilder::PARAM_INT)) + ); + + $update->executeStatement(); + + } + + /** + * @throws Exception + * @throws Throwable + */ + public function markProcessedBulk(Message ...$messages): array { + $this->db->beginTransaction(); + + try { + foreach ($messages as $message) { + if (empty($message->getUpdatedFields())) { + continue; + } + + $this->markProcessed($message->getId(), $message->isImipError()); + } + + $this->db->commit(); + } catch (Throwable $e) { + $this->db->rollBack(); + + throw $e; + } + + return $messages; + } + +} diff --git a/lib/Db/MessageMapper.php b/lib/Db/MessageMapper.php index 4aece97fd3..e2fc2cb4cb 100644 --- a/lib/Db/MessageMapper.php +++ b/lib/Db/MessageMapper.php @@ -585,7 +585,6 @@ public function updatePreviewDataBulk(Message ...$messages): array { ->set('preview_text', $query->createParameter('preview_text')) ->set('structure_analyzed', $query->createNamedParameter(true, IQueryBuilder::PARAM_BOOL)) ->set('updated_at', $query->createNamedParameter($this->timeFactory->getTime(), IQueryBuilder::PARAM_INT)) - ->set('imip_message', $query->createParameter('imip_message')) ->set('encrypted', $query->createParameter('encrypted')) ->set('mentions_me', $query->createParameter('mentions_me')) ->where($query->expr()->andX( @@ -616,7 +615,6 @@ public function updatePreviewDataBulk(Message ...$messages): array { $previewText, $previewText === null ? IQueryBuilder::PARAM_NULL : IQueryBuilder::PARAM_STR ); - $query->setParameter('imip_message', $message->isImipMessage(), IQueryBuilder::PARAM_BOOL); $query->setParameter('encrypted', $message->isEncrypted(), IQueryBuilder::PARAM_BOOL); $query->setParameter('mentions_me', $message->getMentionsMe(), IQueryBuilder::PARAM_BOOL); @@ -1564,15 +1562,15 @@ public function findIMipMessagesAscending(): array { $time = $this->timeFactory->getTime() - 60 * 60 * 24 * 14; $qb = $this->db->getQueryBuilder(); - $select = $qb->select('*') - ->from($this->getTableName()) + $select = $qb->select('m.*') + ->from($this->getTableName(), 'm') + ->join('m', 'mail_messages_imip', 'i', $qb->expr()->eq('i.imip_message_id', 'm.id')) ->where( - $qb->expr()->eq('imip_message', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), - $qb->expr()->eq('imip_processed', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), - $qb->expr()->eq('imip_error', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), - $qb->expr()->eq('flag_junk', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), - $qb->expr()->gt('sent_at', $qb->createNamedParameter($time, IQueryBuilder::PARAM_INT)), - )->orderBy('sent_at', 'ASC'); // make sure we don't process newer messages first + $qb->expr()->eq('i.error', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), + $qb->expr()->isNull('i.processed_at'), + $qb->expr()->eq('m.flag_junk', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), + $qb->expr()->gt('m.sent_at', $qb->createNamedParameter($time, IQueryBuilder::PARAM_INT)), + )->orderBy('m.sent_at', 'ASC'); // make sure we don't process newer messages first return $this->findEntities($select); } diff --git a/lib/IMAP/PreviewEnhancer.php b/lib/IMAP/PreviewEnhancer.php index 36a1cc2d80..8419b36ab1 100644 --- a/lib/IMAP/PreviewEnhancer.php +++ b/lib/IMAP/PreviewEnhancer.php @@ -11,6 +11,7 @@ use Horde_Imap_Client_Exception; use OCA\Mail\Account; +use OCA\Mail\Db\ImipDataMapper; use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper as DbMapper; @@ -18,6 +19,7 @@ use OCA\Mail\Service\Attachment\AttachmentService; use OCA\Mail\Service\Avatar\Avatar; use OCA\Mail\Service\AvatarService; +use OCP\DB\Exception; use Psr\Log\LoggerInterface; use function array_key_exists; use function array_map; @@ -40,6 +42,9 @@ class PreviewEnhancer { /** @var AvatarService */ private $avatarService; + /** @var ImipDataMapper */ + private $imipDataMapper; + public function __construct( IMAPClientFactory $clientFactory, ImapMapper $imapMapper, @@ -47,12 +52,14 @@ public function __construct( LoggerInterface $logger, AvatarService $avatarService, private AttachmentService $attachmentService, + ImipDataMapper $imipDataMapper, ) { $this->clientFactory = $clientFactory; $this->imapMapper = $imapMapper; $this->mapper = $dbMapper; $this->logger = $logger; $this->avatarService = $avatarService; + $this->imipDataMapper = $imipDataMapper; } /** @@ -116,6 +123,22 @@ public function process(Account $account, Mailbox $mailbox, array $messages, boo $client->logout(); } + foreach ($messages as $message) { + if (!array_key_exists($message->getUid(), $data)) { + continue; + } + + if ($data[$message->getUid()]->isImipMessage()) { + try { + $this->imipDataMapper->markAsImipMessage($message->getId()); + } catch (Exception $e) { + $this->logger->warning('Could not mark message as imip: ' . $e->getMessage(), [ + 'exception' => $e, + ]); + } + } + } + return $this->mapper->updatePreviewDataBulk(...array_map(static function (Message $message) use ($data) { if (!array_key_exists($message->getUid(), $data)) { // Nothing to do @@ -126,7 +149,6 @@ public function process(Account $account, Mailbox $mailbox, array $messages, boo $message->setFlagAttachments($structureData->hasAttachments()); $message->setPreviewText($structureData->getPreviewText()); $message->setStructureAnalyzed(true); - $message->setImipMessage($structureData->isImipMessage()); $message->setEncrypted($structureData->isEncrypted()); $message->setMentionsMe($structureData->getMentionsMe()); diff --git a/lib/Migration/Version5201Date20260720120000.php b/lib/Migration/Version5201Date20260720120000.php new file mode 100644 index 0000000000..dd57a3a702 --- /dev/null +++ b/lib/Migration/Version5201Date20260720120000.php @@ -0,0 +1,66 @@ +hasTable('mail_messages_imip')) { + $table = $schema->createTable('mail_messages_imip'); + $table->addColumn('id', Types::INTEGER, [ + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->addColumn('imip_message_id', Types::BIGINT, [ + 'notnull' => true, + ]); + $table->addColumn('error', Types::BOOLEAN, [ + 'notnull' => true, + 'default' => false, + ]); + $table->addColumn('processed_at', Types::INTEGER, [ + 'notnull' => false, + 'default' => null, + ]); + $table->setPrimaryKey(['id']); + $table->addUniqueIndex(['imip_message_id'], 'mail_msg_imip_msg_uniq'); + $table->addIndex(['error', 'processed_at'], 'mail_msg_imip_unproc_idx'); + + if ($schema->hasTable('mail_messages')) { + $table->addForeignKeyConstraint( + $schema->getTable('mail_messages'), + ['imip_message_id'], + ['id'], + [ + 'onDelete' => 'CASCADE', + ] + ); + } + } + + return $schema; + } +} diff --git a/lib/Service/IMipService.php b/lib/Service/IMipService.php index 028a2b8a54..2c27cf0236 100644 --- a/lib/Service/IMipService.php +++ b/lib/Service/IMipService.php @@ -10,6 +10,7 @@ namespace OCA\Mail\Service; use OCA\Mail\Account; +use OCA\Mail\Db\ImipDataMapper; use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\MailboxMapper; use OCA\Mail\Db\Message; @@ -32,6 +33,7 @@ class IMipService { private MailManager $mailManager; private MessageMapper $messageMapper; private ServerVersion $serverVersion; + private ImipDataMapper $imipDataMapper; public function __construct( AccountService $accountService, @@ -41,6 +43,7 @@ public function __construct( MailManager $mailManager, MessageMapper $messageMapper, ServerVersion $serverVersion, + ImipDataMapper $imipDataMapper, ) { $this->accountService = $accountService; $this->calendarManager = $manager; @@ -49,6 +52,7 @@ public function __construct( $this->mailManager = $mailManager; $this->messageMapper = $messageMapper; $this->serverVersion = $serverVersion; + $this->imipDataMapper = $imipDataMapper; } public function process(): void { @@ -108,7 +112,8 @@ public function process(): void { $message->setImipProcessed(true); return $message; }, $filteredMessages); // Silently drop from passing to DAV and mark as processed, so we won't run into these messages again. - $this->messageMapper->updateImipData(...$processedMessages); + $this->imipDataMapper->markProcessedBulk(...$processedMessages); + continue; } @@ -184,7 +189,7 @@ public function process(): void { $message->setImipError(true); } } - $this->messageMapper->updateImipData(...$filteredMessages); + $this->imipDataMapper->markProcessedBulk(...$filteredMessages); } } } diff --git a/tests/Unit/IMAP/PreviewEnhancerTest.php b/tests/Unit/IMAP/PreviewEnhancerTest.php index 90730058dc..851888753d 100644 --- a/tests/Unit/IMAP/PreviewEnhancerTest.php +++ b/tests/Unit/IMAP/PreviewEnhancerTest.php @@ -13,6 +13,7 @@ use Horde_Imap_Client_Socket; use OCA\Mail\Address; use OCA\Mail\AddressList; +use OCA\Mail\Db\ImipDataMapper; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper as DbMapper; use OCA\Mail\IMAP\IMAPClientFactory; @@ -41,6 +42,8 @@ class PreviewEnhancerTest extends TestCase { private $previewEnhancer; /** @var AttachmentService|MockObject */ private $attachmentService; + /** @var ImipDataMapper */ + private $imipDataMapper; protected function setUp(): void { parent::setUp(); @@ -51,6 +54,7 @@ protected function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->avatarService = $this->createMock(AvatarService::class); $this->attachmentService = $this->createMock(AttachmentService::class); + $this->imipDataMapper = $this->createMock(ImipDataMapper::class); $this->previewEnhancer = new PreviewEnhancer( $this->imapClientFactory, @@ -58,7 +62,8 @@ protected function setUp(): void { $this->dbMapper, $this->logger, $this->avatarService, - $this->attachmentService + $this->attachmentService, + $this->imipDataMapper ); } diff --git a/tests/Unit/Service/IMipServiceTest.php b/tests/Unit/Service/IMipServiceTest.php index 3496749322..5e50d7f7d8 100644 --- a/tests/Unit/Service/IMipServiceTest.php +++ b/tests/Unit/Service/IMipServiceTest.php @@ -13,6 +13,7 @@ use OCA\Mail\Account; use OCA\Mail\Address; use OCA\Mail\AddressList; +use OCA\Mail\Db\ImipDataMapper; use OCA\Mail\Db\MailAccount; use OCA\Mail\Db\Mailbox; use OCA\Mail\Db\MailboxMapper; @@ -53,6 +54,9 @@ class IMipServiceTest extends TestCase { private OCPServerVersion $OCPServerVersion; + /** @var ImipDataMapper|MockObject */ + private ImipDataMapper $imipDataMapper; + protected function setUp(): void { parent::setUp(); @@ -64,6 +68,7 @@ protected function setUp(): void { $this->messageMapper = $this->createMock(MessageMapper::class); $this->serverVersion = $this->createMock(ServerVersion::class); $this->OCPServerVersion = new OCPServerVersion(); + $this->imipDataMapper = $this->createMock(ImipDataMapper::class); $this->service = new IMipService( $this->accountService, @@ -72,7 +77,8 @@ protected function setUp(): void { $this->mailboxMapper, $this->mailManager, $this->messageMapper, - $this->serverVersion + $this->serverVersion, + $this->imipDataMapper ); } From 5764ed604a024f1924f0af69f4d5767087114065 Mon Sep 17 00:00:00 2001 From: steven-mpawulo Date: Thu, 23 Jul 2026 15:10:23 +0300 Subject: [PATCH 2/2] test: adds ImipDataMapper integration tests Signed-off-by: steven-mpawulo --- lib/Db/ImipData.php | 6 +- tests/Integration/Db/ImipDataMapperTest.php | 106 ++++++++++++++++++++ 2 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 tests/Integration/Db/ImipDataMapperTest.php diff --git a/lib/Db/ImipData.php b/lib/Db/ImipData.php index a295b0eaed..63fdcf1a6b 100644 --- a/lib/Db/ImipData.php +++ b/lib/Db/ImipData.php @@ -21,13 +21,13 @@ */ class ImipData extends Entity { /** @var int */ - protected int $imipMessageId; + protected $imipMessageId; /** @var bool */ - protected bool $error; + protected $error; /** @var int|null */ - protected ?int $processedAt = null; + protected $processedAt; public function __construct() { $this->addType('imipMessageId', 'integer'); diff --git a/tests/Integration/Db/ImipDataMapperTest.php b/tests/Integration/Db/ImipDataMapperTest.php new file mode 100644 index 0000000000..776154af33 --- /dev/null +++ b/tests/Integration/Db/ImipDataMapperTest.php @@ -0,0 +1,106 @@ +db = \OCP\Server::get(\OCP\IDBConnection::class); + $this->time = $this->createMock(ITimeFactory::class); + $this->time->method('getTime')->willReturnCallback(fn () => $this->timestamp); + $this->mapper = new ImipDataMapper($this->db, $this->time); + } + + private function insertMessage(int $uid, int $mailbox_id): int { + $qb = $this->db->getQueryBuilder(); + $insert = $qb->insert('mail_messages') + ->values([ + 'uid' => $qb->createNamedParameter($uid, IQueryBuilder::PARAM_INT), + 'message_id' => $qb->createNamedParameter(''), + 'mailbox_id' => $qb->createNamedParameter($mailbox_id, IQueryBuilder::PARAM_INT), + 'subject' => $qb->createNamedParameter('TEST'), + 'sent_at' => $qb->createNamedParameter($this->time->getTime(), IQueryBuilder::PARAM_INT), + 'in_reply_to' => $qb->createNamedParameter('<>') + ]); + $insert->executeStatement(); + + return $qb->getLastInsertId(); + } + + public function testMarkAsImipMessage(): void { + $messageId = $this->insertMessage(100, 1); + + $this->mapper->markAsImipMessage($messageId); + + $row = $this->mapper->findByMessageId($messageId); + $this->assertNotNull($row); + $this->assertSame($messageId, $row->getImipMessageId()); + $this->assertFalse($row->getError()); + $this->assertNull($row->getProcessedAt()); + } + + public function testMarkProcessed(): void { + $messageId = $this->insertMessage(101, 1); + $this->mapper->markAsImipMessage($messageId); + + $this->mapper->markProcessed($messageId, true); + + $row = $this->mapper->findByMessageId($messageId); + $this->assertNotNull($row); + $this->assertTrue($row->getError()); + $this->assertSame($this->timestamp, $row->getProcessedAt()); + } + + public function testMarkProcessedBulk(): void { + $messageId1 = $this->insertMessage(102, 1); + $messageId2 = $this->insertMessage(103, 1); + + $this->mapper->markAsImipMessage($messageId1); + $this->mapper->markAsImipMessage($messageId2); + + $message1 = new Message(); + $message1->setId($messageId1); + $message1->setImipError(true); + + $message2 = new Message(); + $message2->setId($messageId2); + $message2->resetUpdatedFields(); + + $this->mapper->markProcessedBulk($message1, $message2); + + $row1 = $this->mapper->findByMessageId($messageId1); + $this->assertNotNull($row1->getProcessedAt(), 'Message with an updated field should be marked processed'); + + $row2 = $this->mapper->findByMessageId($messageId2); + $this->assertNull($row2->getProcessedAt(), 'Message with no updated fields should not be marked processed'); + } + + +}