From a9b72dc6443e96d66da14ee8c3e62c317ef4b70c Mon Sep 17 00:00:00 2001 From: Alex DiMarco Date: Sat, 25 Jul 2026 16:09:01 -0400 Subject: [PATCH 1/2] perf(messages): cut redundant work when opening a message Three targeted reductions of per-click cost, based on profiling the message-open path: - IMAPMessage::getHtmlBody now memoizes the sanitized HTML. getBody called it twice per request (cache write + full message), running the expensive HTMLPurifier pass twice on the same input - ItineraryService::extract checks adapter availability BEFORE opening an IMAP connection and downloading the message body plus all raw attachments. Without a KItinerary adapter installed (the common case), all of that work fed an extractor that is a no-op; the empty result is now computed upfront and cached like a real one - downloadAttachment responses are browser-cacheable for 24h (private, immutable, matching what core's PreviewController does for file previews): attachment content never changes for a given message and attachment id, yet inline images were re-downloaded on every message open, each one paying a fresh IMAP login Covered by new unit tests: memoization (sanitizer runs once per id), itinerary cache hits and the no-adapter skip performing zero IMAP work, and the Cache-Control headers on both attachment branches. Assisted-by: Claude:claude-fable-5 Signed-off-by: Alex DiMarco --- lib/Controller/MessagesController.php | 21 +++-- .../KItinerary/ItineraryExtractor.php | 11 +++ lib/Model/IMAPMessage.php | 12 ++- lib/Service/ItineraryService.php | 9 +++ .../Controller/MessagesControllerTest.php | 62 +++++++++++++++ tests/Unit/Model/IMAPMessageTest.php | 45 +++++++++++ tests/Unit/Service/ItineraryServiceTest.php | 78 +++++++++++++++++++ 7 files changed, 231 insertions(+), 7 deletions(-) diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php index 022964f703..625af8a950 100755 --- a/lib/Controller/MessagesController.php +++ b/lib/Controller/MessagesController.php @@ -783,19 +783,28 @@ public function downloadAttachment(int $id, // Body party and embedded messages do not have a name $attachmentName = $attachment->getName(); if ($attachmentName === null) { - return new AttachmentDownloadResponse( + $response = new AttachmentDownloadResponse( $attachment->getContent(), $this->l10n->t('Embedded message %s', [ $attachmentId, ]) . '.eml', $attachment->getType() ); + } else { + $response = new AttachmentDownloadResponse( + $attachment->getContent(), + $attachmentName, + $attachment->getType() + ); } - return new AttachmentDownloadResponse( - $attachment->getContent(), - $attachmentName, - $attachment->getType() - ); + + // The content of an attachment is immutable for a given message and + // attachment id, so let the browser cache it (e.g. inline images are + // requested again on every message open otherwise). 24h matches what + // Nextcloud core uses for file previews (core PreviewController). + $response->cacheFor(24 * 3600, false, true); + + return $response; } /** diff --git a/lib/Integration/KItinerary/ItineraryExtractor.php b/lib/Integration/KItinerary/ItineraryExtractor.php index 90c470e546..cf9fb3b15b 100644 --- a/lib/Integration/KItinerary/ItineraryExtractor.php +++ b/lib/Integration/KItinerary/ItineraryExtractor.php @@ -45,6 +45,17 @@ private function findAvailableAdapter(): ?Adapter { return null; } + /** + * Whether any KItinerary adapter is available on this system, without + * doing any extraction work + */ + public function hasAdapter(): bool { + if ($this->adapter === null) { + $this->adapter = $this->findAvailableAdapter() ?? false; + } + return $this->adapter !== false; + } + public function extract(string $content): Itinerary { if ($this->adapter === null) { $this->adapter = $this->findAvailableAdapter() ?? false; diff --git a/lib/Model/IMAPMessage.php b/lib/Model/IMAPMessage.php index ed9d95a144..685ad36fa4 100644 --- a/lib/Model/IMAPMessage.php +++ b/lib/Model/IMAPMessage.php @@ -47,6 +47,9 @@ class IMAPMessage implements IMessage, JsonSerializable { use ConvertAddresses; + private ?string $sanitizedHtmlBody = null; + private ?int $sanitizedHtmlBodyId = null; + /** * @param list $inlineAttachments */ @@ -302,7 +305,14 @@ public function jsonSerialize() { * @return string */ public function getHtmlBody(int $id): string { - return $this->htmlService->sanitizeHtmlMailBody( + // Sanitizing is expensive and callers request the same body more than + // once per request (cache write + full message), so memoize the result + if ($this->sanitizedHtmlBody !== null && $this->sanitizedHtmlBodyId === $id) { + return $this->sanitizedHtmlBody; + } + + $this->sanitizedHtmlBodyId = $id; + return $this->sanitizedHtmlBody = $this->htmlService->sanitizeHtmlMailBody( $id, $this->htmlMessage, $this->inlineAttachments, diff --git a/lib/Service/ItineraryService.php b/lib/Service/ItineraryService.php index ef285b7f71..9557766422 100644 --- a/lib/Service/ItineraryService.php +++ b/lib/Service/ItineraryService.php @@ -56,6 +56,15 @@ public function extract(Account $account, Mailbox $mailbox, int $id): Itinerary return $cached; } + if (!$this->extractor->hasAdapter()) { + // Without an adapter, extraction is a no-op. Skip the expensive + // IMAP download of the message body and all attachments. + $this->logger->debug('No KItinerary adapter is available, skipping itinerary extraction'); + $itinerary = new Itinerary(); + $this->cache->set($this->buildCacheKey($account, $mailbox, $id), json_encode($itinerary), self::CACHE_TTL); + return $itinerary; + } + $client = $this->clientFactory->getClient($account); try { $itinerary = new Itinerary(); diff --git a/tests/Unit/Controller/MessagesControllerTest.php b/tests/Unit/Controller/MessagesControllerTest.php index 245d2253ca..ea56d42f1d 100644 --- a/tests/Unit/Controller/MessagesControllerTest.php +++ b/tests/Unit/Controller/MessagesControllerTest.php @@ -340,12 +340,74 @@ public function testDownloadAttachment() { ->will($this->returnValue($type)); $expected = new AttachmentDownloadResponse($contents, $name, $type); + $expected->cacheFor(24 * 3600, false, true); $response = $this->controller->downloadAttachment( $id, $attachmentId ); $this->assertEquals($expected, $response); + // IOPS guard: the response must be browser-cacheable so repeated + // message opens do not trigger new requests (and IMAP logins) + $this->assertSame( + 'private, max-age=86400, immutable', + $response->getHeaders()['Cache-Control'] ?? null + ); + } + + public function testDownloadEmbeddedMessageAttachmentIsCacheable() { + $accountId = 17; + $mailboxId = 987; + $id = 123; + $uid = 321; + $attachmentId = '2'; + + // Embedded messages and body parts have no name + $contents = 'embedded rfc822 bytes'; + $type = 'message/rfc822'; + $message = new \OCA\Mail\Db\Message(); + $message->setMailboxId($mailboxId); + $message->setUid($uid); + $mailbox = new \OCA\Mail\Db\Mailbox(); + $mailbox->setName('INBOX'); + $mailbox->setAccountId($accountId); + $this->mailManager->expects($this->once()) + ->method('getMessage') + ->with($this->userId, $id) + ->willReturn($message); + $this->mailManager->expects($this->once()) + ->method('getMailbox') + ->with($this->userId, $mailboxId) + ->willReturn($mailbox); + $this->mailManager->expects($this->once()) + ->method('getMailAttachment') + ->with($this->account, $mailbox, $message, $attachmentId) + ->will($this->returnValue($this->attachment)); + $this->accountService->expects($this->once()) + ->method('find') + ->with($this->equalTo($this->userId), $this->equalTo($accountId)) + ->will($this->returnValue($this->account)); + $this->attachment->expects($this->once()) + ->method('getContent') + ->will($this->returnValue($contents)); + $this->attachment->expects($this->any()) + ->method('getName') + ->will($this->returnValue(null)); + $this->attachment->expects($this->once()) + ->method('getType') + ->will($this->returnValue($type)); + + $response = $this->controller->downloadAttachment( + $id, + $attachmentId + ); + + // The null-name (.eml) branch must be cacheable too + $this->assertSame( + 'private, max-age=86400, immutable', + $response->getHeaders()['Cache-Control'] ?? null + ); + $this->assertStringContainsString('.eml', $response->getHeaders()['Content-Disposition'] ?? ''); } public function testSaveSingleAttachment() { diff --git a/tests/Unit/Model/IMAPMessageTest.php b/tests/Unit/Model/IMAPMessageTest.php index 388f1fd933..4b432048af 100644 --- a/tests/Unit/Model/IMAPMessageTest.php +++ b/tests/Unit/Model/IMAPMessageTest.php @@ -91,6 +91,51 @@ public function testIconvHtmlMessage() { $this->assertEquals($plainTextBody, $actualPlainTextBody); } + public function testGetHtmlBodyIsMemoized(): void { + // Two calls with the same id must sanitize only once; a different id + // must recompute because attachment URLs embed the message id + $this->htmlService->expects($this->exactly(2)) + ->method('sanitizeHtmlMailBody') + ->willReturn('

sanitized

'); + + $message = new IMAPMessage( + 1234, + '', + [], + AddressList::parse('from@mail.com'), + AddressList::parse('to@mail.com'), + AddressList::parse('cc@mail.com'), + AddressList::parse('bcc@mail.com'), + AddressList::parse('reply-to@mail.com'), + 'subject', + 'plain body', + '

html body

', + true, + [], + [], + false, + [], + new Horde_Imap_Client_DateTime('2016-01-01 00:00:00'), + '', + 'disposition', + false, + [], + null, + false, + '', + '', + false, + false, + false, + $this->htmlService, + false, + ); + + $this->assertSame('

sanitized

', $message->getHtmlBody(123)); + $this->assertSame('

sanitized

', $message->getHtmlBody(123)); + $this->assertSame('

sanitized

', $message->getHtmlBody(456)); + } + public function testSerialize() { $data = new Horde_Imap_Client_Data_Fetch(); $data->setUid(1234); diff --git a/tests/Unit/Service/ItineraryServiceTest.php b/tests/Unit/Service/ItineraryServiceTest.php index f6d8e5ccb0..5269ed9a25 100644 --- a/tests/Unit/Service/ItineraryServiceTest.php +++ b/tests/Unit/Service/ItineraryServiceTest.php @@ -70,6 +70,9 @@ public function testExtractNoBodyNoAttachments() { $mailbox = new Mailbox(); $mailbox->setName('INBOX'); + $this->itineraryExtractor->expects($this->once()) + ->method('hasAdapter') + ->willReturn(true); $this->itineraryExtractor->expects($this->once()) ->method('extract') ->willReturn(new Itinerary()); @@ -79,6 +82,34 @@ public function testExtractNoBodyNoAttachments() { $this->assertEquals([], $itinerary->jsonSerialize()); } + public function testExtractWithoutAdapterSkipsImapFetch(): void { + $mailAccount = new MailAccount(); + $mailAccount->setId(100); + $mailAccount->setUserId('1'); + $account = new Account($mailAccount); + + $mailbox = new Mailbox(); + $mailbox->setName('INBOX'); + + $this->itineraryExtractor->expects($this->once()) + ->method('hasAdapter') + ->willReturn(false); + $this->itineraryExtractor->expects($this->never()) + ->method('extract'); + $this->imapClientFactory->expects($this->never()) + ->method('getClient'); + $this->messageMapper->expects($this->never()) + ->method('getHtmlBody'); + $this->messageMapper->expects($this->never()) + ->method('getRawAttachments'); + + $itinerary = $this->service->extract($account, $mailbox, 13); + + $this->assertEquals([], $itinerary->jsonSerialize()); + // The empty result is cached so subsequent opens skip the probe too + $this->assertNotNull($this->service->getCached($account, $mailbox, 13)); + } + public function testExtractFromBody() { $mailAccount = new MailAccount(); $mailAccount->setId(100); @@ -98,6 +129,9 @@ public function testExtractFromBody() { ->method('getHtmlBody') ->with($client, 'INBOX', 13) ->willReturn($body); + $this->itineraryExtractor->expects($this->once()) + ->method('hasAdapter') + ->willReturn(true); $this->itineraryExtractor->expects($this->exactly(2)) ->method('extract') ->withConsecutive([$body], ['["datafrombody"]']) @@ -127,6 +161,9 @@ public function testExtractFromAttachments() { ->method('getRawAttachments') ->with($client, 'INBOX', 13) ->willReturn([$pdf]); + $this->itineraryExtractor->expects($this->once()) + ->method('hasAdapter') + ->willReturn(true); $this->itineraryExtractor->expects($this->exactly(2)) ->method('extract') ->withConsecutive([$pdf], ['["datafrompdf"]']) @@ -137,6 +174,47 @@ public function testExtractFromAttachments() { $this->assertEquals(['datafrompdf'], $itinerary->jsonSerialize()); } + public function testExtractSecondCallIsServedFromCacheWithoutImapIo(): void { + // IOPS guard: reopening a message must not touch IMAP (or even probe + // the adapter) again — the first extraction's result has to be served + // from the cache. The once()/exactly() expectations fail this test if + // the second extract() call performs any backend work. + $mailAccount = new MailAccount(); + $mailAccount->setId(100); + $mailAccount->setUserId('1'); + $account = new Account($mailAccount); + + $mailbox = new Mailbox(); + $mailbox->setName('INBOX'); + + $client = $this->createStub(Horde_Imap_Client_Socket::class); + $this->imapClientFactory->expects($this->once()) + ->method('getClient') + ->with($account) + ->willReturn($client); + $body = 'hello'; + $this->messageMapper->expects($this->once()) + ->method('getHtmlBody') + ->with($client, 'INBOX', 13) + ->willReturn($body); + $this->messageMapper->expects($this->once()) + ->method('getRawAttachments') + ->with($client, 'INBOX', 13) + ->willReturn([]); + $this->itineraryExtractor->expects($this->once()) + ->method('hasAdapter') + ->willReturn(true); + $this->itineraryExtractor->expects($this->exactly(2)) + ->method('extract') + ->willReturn(new Itinerary(['datafrombody'])); + + $first = $this->service->extract($account, $mailbox, 13); + $second = $this->service->extract($account, $mailbox, 13); + + $this->assertEquals($first->jsonSerialize(), $second->jsonSerialize()); + $this->assertEquals(['datafrombody'], $second->jsonSerialize()); + } + public function testGetCachedMiss(): void { $mailAccount = new MailAccount(); $mailAccount->setId(100); From 79ea8af085ce7e95d6b7599e5a41688ee5788173 Mon Sep 17 00:00:00 2001 From: Alex DiMarco Date: Sun, 26 Jul 2026 21:45:52 -0400 Subject: [PATCH 2/2] fix(itinerary): resolve the KItinerary adapter through a single helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hasAdapter() duplicated the memoization from extract(), so the $this->findAvailableAdapter() ?? false assignment appeared twice while tests/psalm-baseline.xml only carries one entry for it — static analysis failed on the second occurrence. Look the adapter up in one private helper that hands out a nullable Adapter. Behaviour is unchanged, the false sentinel no longer leaks into the public methods and the psalm baseline stays accurate. Assisted-by: Claude:claude-opus-5 Signed-off-by: Alex DiMarco --- .../KItinerary/ItineraryExtractor.php | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/lib/Integration/KItinerary/ItineraryExtractor.php b/lib/Integration/KItinerary/ItineraryExtractor.php index cf9fb3b15b..25bd7c90ef 100644 --- a/lib/Integration/KItinerary/ItineraryExtractor.php +++ b/lib/Integration/KItinerary/ItineraryExtractor.php @@ -46,28 +46,33 @@ private function findAvailableAdapter(): ?Adapter { } /** - * Whether any KItinerary adapter is available on this system, without - * doing any extraction work + * Look the adapter up once and remember the result */ - public function hasAdapter(): bool { + private function getAdapter(): ?Adapter { if ($this->adapter === null) { $this->adapter = $this->findAvailableAdapter() ?? false; } - return $this->adapter !== false; + return $this->adapter === false ? null : $this->adapter; + } + + /** + * Whether any KItinerary adapter is available on this system, without + * doing any extraction work + */ + public function hasAdapter(): bool { + return $this->getAdapter() !== null; } public function extract(string $content): Itinerary { - if ($this->adapter === null) { - $this->adapter = $this->findAvailableAdapter() ?? false; - } - if ($this->adapter === false) { + $adapter = $this->getAdapter(); + if ($adapter === null) { $this->logger->info('KItinerary binary adapter is not available, can\'t extract information'); return new Itinerary(); } try { - return (new Extractor($this->adapter))->extractFromString($content); + return (new Extractor($adapter))->extractFromString($content); } catch (KItineraryRuntimeException $e) { $this->logger->error('Could not extract itinerary function from KItinerary integration: ' . $e->getMessage(), [ 'exception' => $e,