From 5196d437912505eebd86c180010b7178008164ce Mon Sep 17 00:00:00 2001 From: Olivier Date: Fri, 7 Aug 2026 15:36:26 +0200 Subject: [PATCH 1/2] fix(ContentManager): look up existing row before insertOrUpdate insertOrUpdate() can only fall back to update() correctly when the entity's id is already known ahead of time. submitContent() always built a fresh QueueContentItem (no id), so re-submitting content that was already indexed (unique constraint on app_id/provider_id/item_id) threw InvalidArgumentException: Entity which should be updated has no id, and the update silently never happened (caught and logged). Look up the existing row by its unique key first and reuse its id when present, calling insert()/update() explicitly instead of relying on insertOrUpdate()'s exception-driven fallback. Fixes #261 Signed-off-by: Olivier --- lib/Db/QueueContentItemMapper.php | 30 ++++++++++++++++++++++++++ lib/Public/ContentManager.php | 35 +++++++++++++++++++++---------- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/lib/Db/QueueContentItemMapper.php b/lib/Db/QueueContentItemMapper.php index d07ab5a9..885a354b 100644 --- a/lib/Db/QueueContentItemMapper.php +++ b/lib/Db/QueueContentItemMapper.php @@ -10,6 +10,8 @@ namespace OCA\ContextChat\Db; use OCA\ContextChat\Service\ProviderConfigService; +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\QBMapper; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -123,6 +125,34 @@ public function countLocked() : array { return $stats; } + /** + * Finds the existing queue item for a given (app_id, provider_id, item_id) + * triple, matching the table's unique index. Used to look up the real id + * of an already-indexed item before updating it, since insertOrUpdate() + * can only fall back to update() correctly when the entity's id is known + * ahead of time. + * + * @throws Exception + */ + public function findByUniqueKey(string $appId, string $providerId, string $itemId): ?QueueContentItem { + $qb = $this->db->getQueryBuilder(); + $qb->select(QueueContentItem::$columns) + ->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))); + + try { + return $this->findEntity($qb); + } catch (DoesNotExistException) { + return null; + } catch (MultipleObjectsReturnedException) { + // Should not happen given the unique index, but fall back to + // "not found" rather than crashing the caller. + return null; + } + } + /** * @throws Exception */ diff --git a/lib/Public/ContentManager.php b/lib/Public/ContentManager.php index e97c653d..562ff564 100644 --- a/lib/Public/ContentManager.php +++ b/lib/Public/ContentManager.php @@ -126,18 +126,31 @@ public function submitContent(string $appId, array $items): void { continue; } - $dbItem = new QueueContentItem(); - $dbItem->setItemId($item->itemId); - $dbItem->setAppId($appId); - $dbItem->setProviderId($item->providerId); - $dbItem->setTitle($item->title); - $dbItem->setContent($item->content); - $dbItem->setDocumentType($item->documentType); - $dbItem->setLastModified($item->lastModified); - $dbItem->setUsers(implode(',', $item->users)); - try { - $this->mapper->insertOrUpdate($dbItem); + // Re-use the existing row (and its id) when this item was + // already indexed before, instead of always constructing a + // fresh entity. insertOrUpdate() only knows how to fall back + // to update() when the entity's id is already known, which a + // freshly-built entity never has, so it would otherwise fail + // with "Entity which should be updated has no id" on every + // re-submission of already-indexed content. + $dbItem = $this->mapper->findByUniqueKey($appId, $item->providerId, $item->itemId) + ?? new QueueContentItem(); + + $dbItem->setItemId($item->itemId); + $dbItem->setAppId($appId); + $dbItem->setProviderId($item->providerId); + $dbItem->setTitle($item->title); + $dbItem->setContent($item->content); + $dbItem->setDocumentType($item->documentType); + $dbItem->setLastModified($item->lastModified); + $dbItem->setUsers(implode(',', $item->users)); + + if ($dbItem->getId() === null) { + $this->mapper->insert($dbItem); + } else { + $this->mapper->update($dbItem); + } } catch (Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); } From 27a789ca63da135e536a3f49af00832496bb7752 Mon Sep 17 00:00:00 2001 From: kyteinsky Date: Fri, 28 Aug 2026 06:03:13 +0530 Subject: [PATCH 2/2] fix: address review comments Signed-off-by: kyteinsky Assisted-by: Github Copilot:claude-opus-5 --- lib/Db/QueueContentItemMapper.php | 31 ++++++------ lib/Public/ContentManager.php | 43 ++++++++-------- tests/integration/ContentManagerTest.php | 62 ++++++++++++++++++++++-- 3 files changed, 93 insertions(+), 43 deletions(-) diff --git a/lib/Db/QueueContentItemMapper.php b/lib/Db/QueueContentItemMapper.php index 885a354b..2cc79cfc 100644 --- a/lib/Db/QueueContentItemMapper.php +++ b/lib/Db/QueueContentItemMapper.php @@ -10,8 +10,6 @@ namespace OCA\ContextChat\Db; use OCA\ContextChat\Service\ProviderConfigService; -use OCP\AppFramework\Db\DoesNotExistException; -use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\QBMapper; use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; @@ -126,31 +124,30 @@ public function countLocked() : array { } /** - * Finds the existing queue item for a given (app_id, provider_id, item_id) - * triple, matching the table's unique index. Used to look up the real id - * of an already-indexed item before updating it, since insertOrUpdate() - * can only fall back to update() correctly when the entity's id is known - * ahead of time. + * 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 findByUniqueKey(string $appId, string $providerId, string $itemId): ?QueueContentItem { + public function findIdByUniqueKey(string $appId, string $providerId, string $itemId): ?int { $qb = $this->db->getQueryBuilder(); - $qb->select(QueueContentItem::$columns) + $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))); + ->andWhere($qb->expr()->eq('item_id', $qb->createNamedParameter($itemId))) + ->setMaxResults(1); - try { - return $this->findEntity($qb); - } catch (DoesNotExistException) { - return null; - } catch (MultipleObjectsReturnedException) { - // Should not happen given the unique index, but fall back to - // "not found" rather than crashing the caller. + $result = $qb->executeQuery(); + $id = $result->fetchOne(); + $result->closeCursor(); + + if ($id === false) { return null; } + return (int)$id; } /** diff --git a/lib/Public/ContentManager.php b/lib/Public/ContentManager.php index 562ff564..263705ff 100644 --- a/lib/Public/ContentManager.php +++ b/lib/Public/ContentManager.php @@ -126,33 +126,34 @@ public function submitContent(string $appId, array $items): void { continue; } - try { - // Re-use the existing row (and its id) when this item was - // already indexed before, instead of always constructing a - // fresh entity. insertOrUpdate() only knows how to fall back - // to update() when the entity's id is already known, which a - // freshly-built entity never has, so it would otherwise fail - // with "Entity which should be updated has no id" on every - // re-submission of already-indexed content. - $dbItem = $this->mapper->findByUniqueKey($appId, $item->providerId, $item->itemId) - ?? new QueueContentItem(); - - $dbItem->setItemId($item->itemId); - $dbItem->setAppId($appId); - $dbItem->setProviderId($item->providerId); - $dbItem->setTitle($item->title); - $dbItem->setContent($item->content); - $dbItem->setDocumentType($item->documentType); - $dbItem->setLastModified($item->lastModified); - $dbItem->setUsers(implode(',', $item->users)); + $dbItem = new QueueContentItem(); + $dbItem->setItemId($item->itemId); + $dbItem->setAppId($appId); + $dbItem->setProviderId($item->providerId); + $dbItem->setTitle($item->title); + $dbItem->setContent($item->content); + $dbItem->setDocumentType($item->documentType); + $dbItem->setLastModified($item->lastModified); + $dbItem->setUsers(implode(',', $item->users)); - if ($dbItem->getId() === null) { + try { + // 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); } }