perf(messages): cut redundant work when opening a message

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;
 	}
 
 	/**

lib/Integration/KItinerary/ItineraryExtractor.php

@@ -45,18 +45,34 @@ private function findAvailableAdapter(): ?Adapter {
 		return null;
 	}
 
-	public function extract(string $content): Itinerary {
+	/**
+	 * Look the adapter up once and remember the result
+	 */
+	private function getAdapter(): ?Adapter {
 		if ($this->adapter === null) {
 			$this->adapter = $this->findAvailableAdapter() ?? false;
 		}
-		if ($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 {
+		$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,

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<IMAPAttachment> $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,

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();

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() {

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('<p>sanitized</p>');
+
+		$message = new IMAPMessage(
+			1234,
+			'<memo-test@example.com>',
+			[],
+			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',
+			'<p>html body</p>',
+			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('<p>sanitized</p>', $message->getHtmlBody(123));
+		$this->assertSame('<p>sanitized</p>', $message->getHtmlBody(123));
+		$this->assertSame('<p>sanitized</p>', $message->getHtmlBody(456));
+	}
+
 	public function testSerialize() {
 		$data = new Horde_Imap_Client_Data_Fetch();
 		$data->setUid(1234);

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 = '<html><body>hello</body></html>';
+		$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);