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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kesselb wants to merge 1 commit into
base: main
Choose a base branch
from
+114 −16
Open

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

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