Fix cache lookup for attachment names by kesselb · Pull Request #12527 · nextcloud/mail

kesselb · 2026-03-06T12:24:08Z

Based on / Requires #12520

kesselb · 2026-03-06T15:15:49Z

lib/Service/Attachment/AttachmentService.php

+		$cacheKey = $account->getId() . $mailbox->getId() . $message->getUid();
+		$cached = $cache->get($cacheKey);
+
+		if (is_array($cached)) {


if ($cached) => if (is_array($cached))

Is the main change. No attachments (empty array) is a valid state. if ([]) is false, and we try to fetch the data over and over again.

Copilot

Pull request overview

This PR fixes attachment-name caching behavior in AttachmentService::getAttachmentNames() so cached “no attachments” results are treated as valid cache hits, reducing repeated IMAP fetches, and adds unit tests to cover the updated caching and exception-handling paths.

Changes:

Update attachment-name cache lookup logic to treat cached empty arrays as hits and add an early return when structure analysis already confirmed no attachments.
Switch attachment-name caching to use a distributed cache instance keyed per user.
Add unit tests for cache hit/miss scenarios and for SmimeDecryptException / ServiceException behavior; add a clarifying TODO in IMAPMessage::getAttachments() docblock.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
`lib/Service/Attachment/AttachmentService.php`	Adjusts attachment-name caching logic and URL building; adds early exit for confirmed no-attachment messages.
`tests/Unit/Service/Attachment/AttachmentServiceTest.php`	Adds focused unit tests covering cache hit/miss and exception cases for `getAttachmentNames()`.
`lib/Model/IMAPMessage.php`	Adds a TODO note documenting a return-type mismatch for `getAttachments()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

lib/Service/Attachment/AttachmentService.php

kesselb · 2026-03-06T16:00:05Z

lib/Service/Attachment/AttachmentService.php

+			return [];
+		}
+
+		$cache = $this->cacheFactory->createDistributed('mail_attachment_names');


Is the main change 2 ;)

It was a local cache before, as discussed in #11596.

Redis is usually always faster than IMAP, and therefore I would use a distributed cache here as well. In a setup with multiple application servers using only a local cache leads to unncessary imap requests.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-06T18:54:55Z

lib/Service/Attachment/AttachmentService.php

+		$result = array_values(array_map(function ($attachment) use ($message) {
+			$downloadUrl = $this->urlGenerator->linkToRouteAbsolute('mail.messages.downloadAttachment', [
+				'id' => $message->getId(),
+				'attachmentId' => $attachment['id'],
+			]);
 			$mimeUrl = $this->mimeTypeDetector->mimeTypeIcon($attachment['mime']);
-			return ['id' => $attachment['id'] , 'fileName' => $attachment['fileName'], 'mime' => $attachment['mime'], 'downloadUrl' => $downloadUrl, 'mimeUrl' => $mimeUrl ];
-		}, $attachments);
-		$this->cache->set($uniqueCacheId, $result);
+			return ['id' => $attachment['id'], 'fileName' => $attachment['fileName'], 'mime' => $attachment['mime'], 'downloadUrl' => $downloadUrl, 'mimeUrl' => $mimeUrl];
+		}, $attachments));


The cached payload includes downloadUrl, which is derived from $message->getId(), but the cache key is built from user/account/mailbox IDs and the IMAP UID only. If the DB message id changes (e.g., re-sync/re-import), cache hits can return stale/broken download URLs. Consider caching only attachment metadata (id/fileName/mime) and always generating URLs at runtime, or include the DB message id in the cache key.

Copilot · 2026-03-06T18:54:55Z

lib/Model/IMAPMessage.php

 	 * @return Horde_Mime_Part[]
+	 * @todo Actually returns list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>, violating the IMessage contract.


The docblock still declares @return Horde_Mime_Part[], but the added @todo states the method actually returns a list of associative arrays. This contradictory annotation can mislead IDEs/static analysis; consider updating the return annotation to the actual structure (e.g., with @return list<array{...}> / @psalm-return ...) and tracking the interface mismatch separately.

Suggested change

* @return Horde_Mime_Part[]

* @todo Actually returns list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>, violating the IMessage contract.

* @return list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>

* @psalm-return list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>

* @todo This concrete return type violates the IMessage contract, which expects Horde_Mime_Part[].

Tracked in #12530.

Psalm is okay with the current version. Adding the correct return type leads to a warning about breaking the interface that needs baseline update or suppressing.

Copilot · 2026-03-06T18:54:55Z

lib/Service/Attachment/AttachmentService.php

+			return ['id' => $attachment['id'], 'fileName' => $attachment['fileName'], 'mime' => $attachment['mime'], 'downloadUrl' => $downloadUrl, 'mimeUrl' => $mimeUrl];
+		}, $attachments));
+
+		$cache->set($cacheKey, $result);


$result is cached without a TTL, and it includes empty arrays as well. Since PreviewEnhancer::process() calls getAttachmentNames() for every message in mailbox listings, this can create an unbounded number of distributed-cache entries over time. Consider setting a reasonable TTL (and/or caching only non-empty results) to avoid long-term cache growth and memory pressure.

Suggested change

$cache->set($cacheKey, $result);

if (!empty($result)) {

// Cache non-empty attachment lists for 1 hour to avoid unbounded cache growth.

$cache->set($cacheKey, $result, 3600);

}

kesselb self-assigned this Mar 6, 2026

kesselb added bug 3. to review labels Mar 6, 2026

kesselb changed the base branch from main to bug/handle-decryption-error March 6, 2026 12:25

kesselb force-pushed the bug/fix-cache-for-attachments branch 3 times, most recently from dd69469 to 68be4e1 Compare March 6, 2026 14:49

Base automatically changed from bug/handle-decryption-error to main March 6, 2026 14:59

kesselb force-pushed the bug/fix-cache-for-attachments branch from b4e3752 to 498f4e3 Compare March 6, 2026 15:09

kesselb marked this pull request as ready for review March 6, 2026 15:13

kesselb requested review from ChristophWurst and GretaD as code owners March 6, 2026 15:13

kesselb commented Mar 6, 2026

View reviewed changes

kesselb requested review from DerDreschner, SebastianKrupinski and Copilot March 6, 2026 15:17

Copilot started reviewing on behalf of kesselb March 6, 2026 15:18 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

lib/Service/Attachment/AttachmentService.php Outdated Show resolved Hide resolved

DerDreschner requested changes Mar 6, 2026

View reviewed changes

DerDreschner approved these changes Mar 6, 2026

View reviewed changes

kesselb commented Mar 6, 2026

View reviewed changes

fix: Invalid cache lookup for emails without attachment

b261f25

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

kesselb force-pushed the bug/fix-cache-for-attachments branch from 21fc08e to b261f25 Compare March 6, 2026 17:00

kesselb requested a review from Copilot March 6, 2026 18:49

Copilot started reviewing on behalf of kesselb March 6, 2026 18:50 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

fixup! fix: Invalid cache lookup for emails without attachment

988cdca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cache lookup for attachment names#12527

Fix cache lookup for attachment names#12527
kesselb wants to merge 2 commits intomainfrom
bug/fix-cache-for-attachments

kesselb commented Mar 6, 2026

Uh oh!

kesselb Mar 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

kesselb Mar 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

kesselb Mar 6, 2026

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		* @return Horde_Mime_Part[]
		* @todo Actually returns list<array{id: string, messageId: int, fileName: string\|null, mime: string, size: int, cid: string\|null, disposition: string\|null}>, violating the IMessage contract.

-	 * @return Horde_Mime_Part[]
-	 * @todo Actually returns list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>, violating the IMessage contract.
+	 * @return list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>
+	 * @psalm-return list<array{id: string, messageId: int, fileName: string|null, mime: string, size: int, cid: string|null, disposition: string|null}>
+	 * @todo This concrete return type violates the IMessage contract, which expects Horde_Mime_Part[].

-		$cache->set($cacheKey, $result);
+		if (!empty($result)) {
+			// Cache non-empty attachment lists for 1 hour to avoid unbounded cache growth.
+			$cache->set($cacheKey, $result, 3600);
+		}

Conversation

kesselb commented Mar 6, 2026

Uh oh!

kesselb Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

kesselb Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

kesselb Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants