Skip to content

fix(contextchat): use a consistent item id for indexed messages #13443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
kesselb wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(contextchat): use a consistent item id for indexed messages #13443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/BackgroundJob/ContextChat/SubmitContentJob.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'] ?? '',
Expand Down
46 changes: 40 additions & 6 deletions lib/ContextChat/ContextChatProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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);
Comment on lines +119 to +123

@kyteinsky kyteinsky Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getItemUrl is called for every item in the sources list when the output is rendered, true
but without this change, it was still not looked up until the link was clicked by the user. Here, every link would be checked even when not clicked.

not sure what's the difference between uid and id but the with id approach, the message was only checked when the user clicked on it and the webpage from 'mail.page.thread' opened.

I'm not familiar with the mail's code so sorry if the point does not make much sense.

@kesselb kesselb Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, that's a valid concern. The previous version was a cheap call.

id = oc_mail_messages.id
uid = identifier on the imap server, unique per mailbox, also stored as oc_mail_messages.uid

Better would be the id, but some of the events (e.g. MessageDeletedEvent) does not carry it.

But that might change very soon, and hence I will move this PR back to draft for now and continue once the JMAP changes are in. At best we have id everywhere, otherwise I will add a new entrypoint that accepts mailboxId/uid doing the lookup internally.

} catch (\Throwable) {
// Context chat fails the whole answer if this throws
Comment on lines +124 to +125

@kyteinsky kyteinsky Aug 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for pointing this out, fixed in nextcloud/context_chat#266

$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],
);
}

/**
Expand All @@ -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";
}

}
8 changes: 7 additions & 1 deletion tests/Unit/BackgroundJob/ContextChat/SubmitContentJobTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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]);
Expand Down Expand Up @@ -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);
Expand Down
74 changes: 66 additions & 8 deletions tests/Unit/ContextChat/ContextChatProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,6 +44,9 @@ class ContextChatProviderTest extends TestCase {
/** @var MessageMapper|MockObject */
private $messageMapper;

/** @var MailboxMapper|MockObject */
private $mailboxMapper;

/** @var IURLGenerator|MockObject */
private $urlGenerator;

Expand All @@ -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);
Expand All @@ -78,6 +84,7 @@ protected function setUp(): void {
$this->accountService,
$this->mailManager,
$this->messageMapper,
$this->mailboxMapper,
$this->urlGenerator,
$this->userManager,
$this->contentManager,
Expand All @@ -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 = [];
Expand Down Expand Up @@ -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());
}
Expand All @@ -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'));
}
}
Loading