Merge pull request #262 from oboeglen/fix/insertOrUpdate-missing-id

fix(ContentManager): look up existing row before insertOrUpdate

Author: kyteinsky

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

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

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