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..25bd7c90ef 100644 --- a/lib/Integration/KItinerary/ItineraryExtractor.php +++ b/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, 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);