diff --git a/lib/Db/QueueContentItemMapper.php b/lib/Db/QueueContentItemMapper.php index d07ab5a9..2cc79cfc 100644 --- a/lib/Db/QueueContentItemMapper.php +++ b/lib/Db/QueueContentItemMapper.php @@ -123,6 +123,33 @@ public function countLocked() : array { return $stats; } + /** + * Finds the id of the queue item for a given (app_id, provider_id, item_id) + * triple, matching the table's unique index. Used to look up the id of an + * already-queued item before updating it, since update() needs the id and + * every other column is overwritten anyway. + * + * @throws Exception + */ + public function findIdByUniqueKey(string $appId, string $providerId, string $itemId): ?int { + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from($this->getTableName()) + ->where($qb->expr()->eq('app_id', $qb->createNamedParameter($appId))) + ->andWhere($qb->expr()->eq('provider_id', $qb->createNamedParameter($providerId))) + ->andWhere($qb->expr()->eq('item_id', $qb->createNamedParameter($itemId))) + ->setMaxResults(1); + + $result = $qb->executeQuery(); + $id = $result->fetchOne(); + $result->closeCursor(); + + if ($id === false) { + return null; + } + return (int)$id; + } + /** * @throws Exception */ diff --git a/lib/Public/ContentManager.php b/lib/Public/ContentManager.php index e97c653d..263705ff 100644 --- a/lib/Public/ContentManager.php +++ b/lib/Public/ContentManager.php @@ -137,9 +137,23 @@ public function submitContent(string $appId, array $items): void { $dbItem->setUsers(implode(',', $item->users)); try { - $this->mapper->insertOrUpdate($dbItem); + // insertOrUpdate() can only fall back to update() when the entity's + // id is already known, which a freshly built entity never has, so it + // fails with "Entity which should be updated has no id" whenever the + // unique index on (app_id, provider_id, item_id) is hit. Look up the + // id of the already-queued row first and update that row instead. + $id = $this->mapper->findIdByUniqueKey($appId, $item->providerId, $item->itemId); + if ($id === null) { + $this->mapper->insert($dbItem); + } else { + $dbItem->setId($id); + $this->mapper->update($dbItem); + } } catch (Exception $e) { - $this->logger->error($e->getMessage(), ['exception' => $e]); + $this->logger->error( + "Error adding content item id {$item->itemId} from app {$appId}: {$e->getMessage()}", + ['exception' => $e], + ); } } } diff --git a/tests/integration/ContentManagerTest.php b/tests/integration/ContentManagerTest.php index f7b3d3ff..2dac29c8 100644 --- a/tests/integration/ContentManagerTest.php +++ b/tests/integration/ContentManagerTest.php @@ -12,7 +12,7 @@ use DateTime; use OCA\ContextChat\AppInfo\Application; use OCA\ContextChat\BackgroundJobs\InitialContentImportJob; -use OCA\ContextChat\BackgroundJobs\SubmitContentJob; +use OCA\ContextChat\Db\QueueContentItem; use OCA\ContextChat\Db\QueueContentItemMapper; use OCA\ContextChat\Event\ContentProviderRegisterEvent; use OCA\ContextChat\Logger; @@ -21,6 +21,7 @@ use OCA\ContextChat\Public\IContentProvider; use OCA\ContextChat\Service\ActionScheduler; use OCA\ContextChat\Service\ProviderConfigService; +use OCP\AppFramework\Services\IAppConfig; use OCP\BackgroundJob\IJobList; use OCP\EventDispatcher\IEventDispatcher; use OCP\IServerContainer; @@ -31,6 +32,8 @@ use Test\TestCase; class ContentManagerTest extends TestCase { + /** @var MockObject | IAppConfig */ + private IAppConfig $appConfig; /** @var MockObject | QueueContentItemMapper */ private QueueContentItemMapper $mapper; /** @var MockObject | ProviderConfigService */ @@ -52,6 +55,7 @@ public function setUp(): void { $this->jobList = Server::get(IJobList::class); $this->logger = Server::get(LoggerInterface::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->mapper = $this->createMock(QueueContentItemMapper::class); $this->providerConfig = $this->createMock(ProviderConfigService::class); $this->actionService = $this->createMock(ActionScheduler::class); @@ -92,6 +96,7 @@ public function setUp(): void { $this->contentManager = new ContentManager( $this->jobList, + $this->appConfig, $this->providerConfig, $this->mapper, $this->actionService, @@ -167,17 +172,64 @@ public function testSubmitContent(): void { ), ]; + $this->mapper + ->expects($this->once()) + ->method('findIdByUniqueKey') + ->with($appId, 'provider-id', 'item-id') + ->willReturn(null); + $this->mapper ->expects($this->once()) ->method('insert'); - $this->jobList->remove(SubmitContentJob::class, null); - $this->assertFalse($this->jobList->has(SubmitContentJob::class, null)); + $this->mapper + ->expects($this->never()) + ->method('update'); $this->contentManager->submitContent($appId, $items); + } + + public function testSubmitContentUpdatesAlreadyQueuedItem(): void { + $appId = 'test'; + $items = [ + new ContentItem( + 'item-id', + 'provider-id', + 'new title', + 'new content', + 'email-file', + new DateTime(), + ['user1', 'user2'], + ), + ]; - $this->assertTrue($this->jobList->has(SubmitContentJob::class, null)); - $this->jobList->remove(SubmitContentJob::class, null); + $this->mapper + ->expects($this->once()) + ->method('findIdByUniqueKey') + ->with($appId, 'provider-id', 'item-id') + ->willReturn(42); + + $this->mapper + ->expects($this->never()) + ->method('insert'); + + $this->mapper + ->expects($this->once()) + ->method('update') + ->with($this->callback(function (QueueContentItem $dbItem) use ($appId) { + // update() needs the id of the already queued row, otherwise + // QBMapper throws "Entity which should be updated has no id" + $this->assertSame(42, $dbItem->getId()); + $this->assertSame($appId, $dbItem->getAppId()); + $this->assertSame('provider-id', $dbItem->getProviderId()); + $this->assertSame('item-id', $dbItem->getItemId()); + $this->assertSame('new title', $dbItem->getTitle()); + $this->assertSame('new content', $dbItem->getContent()); + $this->assertSame('user1,user2', $dbItem->getUsers()); + return true; + })); + + $this->contentManager->submitContent($appId, $items); } }