From 7fadd20faff7e07f889e1c152484c4ea39e95c04 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Fri, 7 Aug 2026 18:31:04 +0200 Subject: [PATCH] fix(contextchat): use a consistent item id for indexed messages Content was indexed as "{mailboxId}:{messageId}" but deleted by bare IMAP uid, so deleting a message never removed it from the knowledge base. Key both sides off account, mailbox and uid via a shared helper. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Daniel Kesselberg # Conflicts: # lib/ContextChat/ContextChatProvider.php --- .../ContextChat/SubmitContentJob.php | 2 +- lib/ContextChat/ContextChatProvider.php | 46 ++++++++++-- .../ContextChat/SubmitContentJobTest.php | 8 +- .../ContextChat/ContextChatProviderTest.php | 74 +++++++++++++++++-- 4 files changed, 114 insertions(+), 16 deletions(-) diff --git a/lib/BackgroundJob/ContextChat/SubmitContentJob.php b/lib/BackgroundJob/ContextChat/SubmitContentJob.php index 11619b92fe..e9633b6812 100644 --- a/lib/BackgroundJob/ContextChat/SubmitContentJob.php +++ b/lib/BackgroundJob/ContextChat/SubmitContentJob.php @@ -135,7 +135,7 @@ protected function run($argument): void { $fullMessage = $imapMessage->getFullMessage($imapMessage->getUid(), true); $items[] = new ContentItem( - "{$mailbox->getId()}:{$message->getId()}", + ContextChatProvider::itemId($account->getId(), $mailbox->getId(), $message->getUid()), $this->contextChatProvider->getId(), $imapMessage->getSubject(), $fullMessage['body'] ?? '', diff --git a/lib/ContextChat/ContextChatProvider.php b/lib/ContextChat/ContextChatProvider.php index 80f60e4ae6..4b711f5ea1 100644 --- a/lib/ContextChat/ContextChatProvider.php +++ b/lib/ContextChat/ContextChatProvider.php @@ -10,6 +10,7 @@ namespace OCA\Mail\ContextChat; use OCA\Mail\AppInfo\Application; +use OCA\Mail\Db\MailboxMapper; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; use OCA\Mail\Events\MessageDeletedEvent; @@ -34,12 +35,12 @@ class ContextChatProvider implements IContentProvider, IEventListener { public const CONTEXT_CHAT_MESSAGE_MAX_AGE = 31557600; // 60 * 60 * 24 * 365.25 (1 year) public const CONTEXT_CHAT_IMPORT_MAX_ITEMS = 1000; public const CONTEXT_CHAT_JOB_INTERVAL = 300; // 60 * 5 (5 minutes) - public function __construct( private TaskService $taskService, private AccountService $accountService, private MailManager $mailManager, private MessageMapper $messageMapper, + private MailboxMapper $mailboxMapper, private IURLGenerator $urlGenerator, private IUserManager $userManager, private IContentManager $contentManager, @@ -72,7 +73,12 @@ public function handle(Event $event): void { } if ($event instanceof MessageDeletedEvent) { - $this->contentManager->deleteContent($this->getAppId(), $this->getId(), [strval($event->getUid())]); + $itemId = self::itemId( + $event->getAccount()->getId(), + $event->getMailbox()->getId(), + $event->getUid(), + ); + $this->contentManager->deleteContent($this->getAppId(), $this->getId(), [$itemId]); return; } } @@ -105,11 +111,29 @@ public function getAppId(): string { * @since 5.2.0 */ public function getItemUrl(string $id): string { - [$mailboxId, $messageId] = explode(':', $id); - if (!$mailboxId || !$messageId) { - return $this->urlGenerator->linkToRouteAbsolute('mail.page.thread', [ 'mailboxId' => $mailboxId, 'id' => 'error']); + [, $mailboxId, $uid] = array_pad(explode(':', $id), 3, null); + if (!$mailboxId || !$uid) { + return $this->urlGenerator->linkToRouteAbsolute('mail.page.index', []); + } + + // Context chat calls this when it renders the sources of an answer, so the + // message id is looked up on demand rather than baked into the item id. + try { + $mailbox = $this->mailboxMapper->findById((int)$mailboxId); + $messageId = $this->messageMapper->getIdForUid($mailbox, (int)$uid); + } catch (\Throwable) { + // Context chat fails the whole answer if this throws + $messageId = null; } - return $this->urlGenerator->linkToRouteAbsolute('mail.page.thread', [ 'mailboxId' => $mailboxId, 'id' => $messageId ]); + + if ($messageId === null) { + return $this->urlGenerator->linkToRouteAbsolute('mail.page.index', []); + } + + return $this->urlGenerator->linkToRouteAbsolute( + 'mail.page.thread', + ['mailboxId' => (int)$mailboxId, 'id' => $messageId], + ); } /** @@ -120,4 +144,14 @@ public function getItemUrl(string $id): string { */ public function triggerInitialImport(): void { } + + /** + * Identifier of a message in the context chat knowledge base. + * + * Keyed by uid because that is all MessageDeletedEvent carries. + */ + public static function itemId(int $accountId, int $mailboxId, int $uid): string { + return "$accountId:$mailboxId:$uid"; + } + } diff --git a/tests/Unit/BackgroundJob/ContextChat/SubmitContentJobTest.php b/tests/Unit/BackgroundJob/ContextChat/SubmitContentJobTest.php index 1581c1aeb2..27804f8fa0 100644 --- a/tests/Unit/BackgroundJob/ContextChat/SubmitContentJobTest.php +++ b/tests/Unit/BackgroundJob/ContextChat/SubmitContentJobTest.php @@ -169,6 +169,7 @@ public function testRunWithContextChat(): void { ->with($mailbox, 0, 0, ContextChatProvider::CONTEXT_CHAT_IMPORT_MAX_ITEMS)->willReturn([2]); $account = $this->createMock(Account::class); $account->expects($this->any())->method('getUserId')->willReturn('user123'); + $account->expects($this->any())->method('getId')->willReturn(5); $this->accountService->expects($this->once())->method('findById')->willReturn($account); $message = new Message(); $message->setId(2); @@ -187,7 +188,10 @@ public function testRunWithContextChat(): void { $client->expects($this->once())->method('close'); $this->contextChatProvider->expects($this->once())->method('getAppId')->willReturn('mail'); $this->contextChatProvider->expects($this->once())->method('getId')->willReturn('mail'); - $this->contentManager->expects($this->once())->method('submitContent'); + $this->contentManager->expects($this->once())->method('submitContent') + ->with('mail', $this->callback( + static fn (array $items) => count($items) === 1 && $items[0]->itemId === '5:1:2' + )); $this->taskService->expects($this->once())->method('setLastMessage')->with($task->getMailboxId(), 2); $this->submitContentJob->setLastRun(0); @@ -261,6 +265,7 @@ public function testRunWithContextChatWithTimeout(): void { ->with($mailbox, 0, 0, ContextChatProvider::CONTEXT_CHAT_IMPORT_MAX_ITEMS)->willReturn([1]); $account = $this->createMock(Account::class); $account->expects($this->any())->method('getUserId')->willReturn('user123'); + $account->expects($this->any())->method('getId')->willReturn(5); $this->accountService->expects($this->once())->method('findById')->with()->willReturn($account); $message = new Message(); $this->messageMapper->expects($this->once())->method('findByIds')->willReturn([$message]); @@ -305,6 +310,7 @@ public function testRunWithContextChatWithEncryptedMessage(): void { ->with($mailbox, 0, 0, ContextChatProvider::CONTEXT_CHAT_IMPORT_MAX_ITEMS)->willReturn([1]); $account = $this->createMock(Account::class); $account->expects($this->any())->method('getUserId')->willReturn('user123'); + $account->expects($this->any())->method('getId')->willReturn(5); $this->accountService->expects($this->once())->method('findById')->with()->willReturn($account); $message = new Message(); $message->setId(1); diff --git a/tests/Unit/ContextChat/ContextChatProviderTest.php b/tests/Unit/ContextChat/ContextChatProviderTest.php index e9d9a449f3..b3541604ec 100644 --- a/tests/Unit/ContextChat/ContextChatProviderTest.php +++ b/tests/Unit/ContextChat/ContextChatProviderTest.php @@ -14,10 +14,12 @@ use OCA\Mail\ContextChat\ContextChatProvider; use OCA\Mail\Db\MailAccount; use OCA\Mail\Db\Mailbox; +use OCA\Mail\Db\MailboxMapper; use OCA\Mail\Db\Message; use OCA\Mail\Db\MessageMapper; use OCA\Mail\Events\MessageDeletedEvent; use OCA\Mail\Events\NewMessagesSynchronized; +use OCA\Mail\Exception\ServiceException; use OCA\Mail\Service\AccountService; use OCA\Mail\Service\ContextChat\TaskService; use OCA\Mail\Service\MailManager; @@ -42,6 +44,9 @@ class ContextChatProviderTest extends TestCase { /** @var MessageMapper|MockObject */ private $messageMapper; + /** @var MailboxMapper|MockObject */ + private $mailboxMapper; + /** @var IURLGenerator|MockObject */ private $urlGenerator; @@ -68,6 +73,7 @@ protected function setUp(): void { $this->accountService = $this->createMock(AccountService::class); $this->mailManager = $this->createMock(MailManager::class); $this->messageMapper = $this->createMock(MessageMapper::class); + $this->mailboxMapper = $this->createMock(MailboxMapper::class); $this->urlGenerator = $this->createMock(IURLGenerator::class); $this->userManager = $this->createMock(IUserManager::class); $this->contentManager = $this->createMock(IContentManager::class); @@ -78,6 +84,7 @@ protected function setUp(): void { $this->accountService, $this->mailManager, $this->messageMapper, + $this->mailboxMapper, $this->urlGenerator, $this->userManager, $this->contentManager, @@ -86,7 +93,9 @@ protected function setUp(): void { } public function provideEvents(): array { - $account = new Account(new MailAccount()); + $mailAccount = new MailAccount(); + $mailAccount->setId(3); + $account = new Account($mailAccount); $mailbox = new Mailbox(); $mailbox->setId(1); $messages = []; @@ -148,6 +157,23 @@ public function testHandleWithContextChat($event) { $this->contextChatProvider->handle($event); } + public function testHandleMessageDeletedUsesTheIndexedItemId(): void { + $mailAccount = new MailAccount(); + $mailAccount->setId(3); + $account = new Account($mailAccount); + $mailbox = new Mailbox(); + $mailbox->setId(1); + $event = new MessageDeletedEvent($account, $mailbox, 4711); + $this->contentManager->expects($this->once()) + ->method('isContextChatAvailable') + ->willReturn(true); + $this->contentManager->expects($this->once()) + ->method('deleteContent') + ->with('mail', $this->anything(), ['3:1:4711']); + + $this->contextChatProvider->handle($event); + } + public function testGetId(): void { $this->assertEquals('mail', $this->contextChatProvider->getId()); } @@ -157,13 +183,45 @@ public function testGetAppId(): void { } public function testGetItemUrl(): void { - $this->urlGenerator->expects($this->once())->method('linkToRouteAbsolute')->willReturnCallback(function ($route, $args) { - $this->assertEquals('mail.page.thread', $route); - $this->assertEquals(1, $args['mailboxId']); - $this->assertEquals(2, $args['id']); - return 'http://localhost/apps/mail/box/1/thread/2'; - }); - $itemUrl = $this->contextChatProvider->getItemUrl('1:2'); + $mailbox = new Mailbox(); + $mailbox->setId(1); + $this->mailboxMapper->expects($this->once())->method('findById')->with(1)->willReturn($mailbox); + $this->messageMapper->expects($this->once())->method('getIdForUid')->with($mailbox, 4711)->willReturn(2); + $this->urlGenerator->expects($this->once())->method('linkToRouteAbsolute') + ->with('mail.page.thread', ['mailboxId' => 1, 'id' => 2]) + ->willReturn('http://localhost/apps/mail/box/1/thread/2'); + + $itemUrl = $this->contextChatProvider->getItemUrl(ContextChatProvider::itemId(3, 1, 4711)); + $this->assertEquals('http://localhost/apps/mail/box/1/thread/2', $itemUrl); } + + public function testGetItemUrlNeverThrows(): void { + $this->mailboxMapper->expects($this->once())->method('findById') + ->willThrowException(new ServiceException('database on fire')); + $this->urlGenerator->expects($this->once())->method('linkToRouteAbsolute') + ->with('mail.page.index', []) + ->willReturn('http://localhost/apps/mail/'); + + $this->assertEquals('http://localhost/apps/mail/', $this->contextChatProvider->getItemUrl('3:1:4711')); + } + + public function testGetItemUrlWithDeletedMessage(): void { + $this->mailboxMapper->expects($this->once())->method('findById')->willReturn(new Mailbox()); + $this->messageMapper->expects($this->once())->method('getIdForUid')->willReturn(null); + $this->urlGenerator->expects($this->once())->method('linkToRouteAbsolute') + ->with('mail.page.index', []) + ->willReturn('http://localhost/apps/mail/'); + + $this->assertEquals('http://localhost/apps/mail/', $this->contextChatProvider->getItemUrl('3:1:4711')); + } + + public function testGetItemUrlWithMalformedId(): void { + $this->mailboxMapper->expects($this->never())->method('findById'); + $this->urlGenerator->expects($this->once())->method('linkToRouteAbsolute') + ->with('mail.page.index', []) + ->willReturn('http://localhost/apps/mail/'); + + $this->assertEquals('http://localhost/apps/mail/', $this->contextChatProvider->getItemUrl('nonsense')); + } }