Skip to content

Commit d674b5b

Browse files
kyteinskykyteinsky
committed
fix: address review comments
Signed-off-by: kyteinsky <kyteinsky@gmail.com>
1 parent 5196d43 commit d674b5b

3 files changed

Lines changed: 93 additions & 43 deletions

File tree

‎lib/Db/QueueContentItemMapper.php‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@
1010
namespace OCA\ContextChat\Db;
1111

1212
use OCA\ContextChat\Service\ProviderConfigService;
13-
use OCP\AppFramework\Db\DoesNotExistException;
14-
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
1513
use OCP\AppFramework\Db\QBMapper;
1614
use OCP\DB\Exception;
1715
use OCP\DB\QueryBuilder\IQueryBuilder;
@@ -126,31 +124,30 @@ public function countLocked() : array {
126124
}
127125

128126
/**
129-
* Finds the existing queue item for a given (app_id, provider_id, item_id)
130-
* triple, matching the table's unique index. Used to look up the real id
131-
* of an already-indexed item before updating it, since insertOrUpdate()
132-
* can only fall back to update() correctly when the entity's id is known
133-
* ahead of time.
127+
* Finds the id of the queue item for a given (app_id, provider_id, item_id)
128+
* triple, matching the table's unique index. Used to look up the id of an
129+
* already-queued item before updating it, since update() needs the id and
130+
* every other column is overwritten anyway.
134131
*
135132
* @throws Exception
136133
*/
137-
public function findByUniqueKey(string $appId, string $providerId, string $itemId): ?QueueContentItem {
134+
public function findIdByUniqueKey(string $appId, string $providerId, string $itemId): ?int {
138135
$qb = $this->db->getQueryBuilder();
139-
$qb->select(QueueContentItem::$columns)
136+
$qb->select('id')
140137
->from($this->getTableName())
141138
->where($qb->expr()->eq('app_id', $qb->createNamedParameter($appId)))
142139
->andWhere($qb->expr()->eq('provider_id', $qb->createNamedParameter($providerId)))
143-
->andWhere($qb->expr()->eq('item_id', $qb->createNamedParameter($itemId)));
140+
->andWhere($qb->expr()->eq('item_id', $qb->createNamedParameter($itemId)))
141+
->setMaxResults(1);
144142

145-
try {
146-
return $this->findEntity($qb);
147-
} catch (DoesNotExistException) {
148-
return null;
149-
} catch (MultipleObjectsReturnedException) {
150-
// Should not happen given the unique index, but fall back to
151-
// "not found" rather than crashing the caller.
143+
$result = $qb->executeQuery();
144+
$id = $result->fetchOne();
145+
$result->closeCursor();
146+
147+
if ($id === false) {
152148
return null;
153149
}
150+
return (int)$id;
154151
}
155152

156153
/**

‎lib/Public/ContentManager.php‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -126,33 +126,34 @@ public function submitContent(string $appId, array $items): void {
126126
continue;
127127
}
128128

129-
try {
130-
// Re-use the existing row (and its id) when this item was
131-
// already indexed before, instead of always constructing a
132-
// fresh entity. insertOrUpdate() only knows how to fall back
133-
// to update() when the entity's id is already known, which a
134-
// freshly-built entity never has, so it would otherwise fail
135-
// with "Entity which should be updated has no id" on every
136-
// re-submission of already-indexed content.
137-
$dbItem = $this->mapper->findByUniqueKey($appId, $item->providerId, $item->itemId)
138-
?? new QueueContentItem();
139-
140-
$dbItem->setItemId($item->itemId);
141-
$dbItem->setAppId($appId);
142-
$dbItem->setProviderId($item->providerId);
143-
$dbItem->setTitle($item->title);
144-
$dbItem->setContent($item->content);
145-
$dbItem->setDocumentType($item->documentType);
146-
$dbItem->setLastModified($item->lastModified);
147-
$dbItem->setUsers(implode(',', $item->users));
129+
$dbItem = new QueueContentItem();
130+
$dbItem->setItemId($item->itemId);
131+
$dbItem->setAppId($appId);
132+
$dbItem->setProviderId($item->providerId);
133+
$dbItem->setTitle($item->title);
134+
$dbItem->setContent($item->content);
135+
$dbItem->setDocumentType($item->documentType);
136+
$dbItem->setLastModified($item->lastModified);
137+
$dbItem->setUsers(implode(',', $item->users));
148138

149-
if ($dbItem->getId() === null) {
139+
try {
140+
// insertOrUpdate() can only fall back to update() when the entity's
141+
// id is already known, which a freshly built entity never has, so it
142+
// fails with "Entity which should be updated has no id" whenever the
143+
// unique index on (app_id, provider_id, item_id) is hit. Look up the
144+
// id of the already-queued row first and update that row instead.
145+
$id = $this->mapper->findIdByUniqueKey($appId, $item->providerId, $item->itemId);
146+
if ($id === null) {
150147
$this->mapper->insert($dbItem);
151148
} else {
149+
$dbItem->setId($id);
152150
$this->mapper->update($dbItem);
153151
}
154152
} catch (Exception $e) {
155-
$this->logger->error($e->getMessage(), ['exception' => $e]);
153+
$this->logger->error(
154+
"Error adding content item id {$item->itemId} from app {$appId}: {$e->getMessage()}",
155+
['exception' => $e],
156+
);
156157
}
157158
}
158159
}

‎tests/integration/ContentManagerTest.php‎

Lines changed: 57 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
use DateTime;
1313
use OCA\ContextChat\AppInfo\Application;
1414
use OCA\ContextChat\BackgroundJobs\InitialContentImportJob;
15-
use OCA\ContextChat\BackgroundJobs\SubmitContentJob;
15+
use OCA\ContextChat\Db\QueueContentItem;
1616
use OCA\ContextChat\Db\QueueContentItemMapper;
1717
use OCA\ContextChat\Event\ContentProviderRegisterEvent;
1818
use OCA\ContextChat\Logger;
@@ -21,6 +21,7 @@
2121
use OCA\ContextChat\Public\IContentProvider;
2222
use OCA\ContextChat\Service\ActionScheduler;
2323
use OCA\ContextChat\Service\ProviderConfigService;
24+
use OCP\AppFramework\Services\IAppConfig;
2425
use OCP\BackgroundJob\IJobList;
2526
use OCP\EventDispatcher\IEventDispatcher;
2627
use OCP\IServerContainer;
@@ -31,6 +32,8 @@
3132
use Test\TestCase;
3233

3334
class ContentManagerTest extends TestCase {
35+
/** @var MockObject | IAppConfig */
36+
private IAppConfig $appConfig;
3437
/** @var MockObject | QueueContentItemMapper */
3538
private QueueContentItemMapper $mapper;
3639
/** @var MockObject | ProviderConfigService */
@@ -52,6 +55,7 @@ public function setUp(): void {
5255
$this->jobList = Server::get(IJobList::class);
5356
$this->logger = Server::get(LoggerInterface::class);
5457

58+
$this->appConfig = $this->createMock(IAppConfig::class);
5559
$this->mapper = $this->createMock(QueueContentItemMapper::class);
5660
$this->providerConfig = $this->createMock(ProviderConfigService::class);
5761
$this->actionService = $this->createMock(ActionScheduler::class);
@@ -92,6 +96,7 @@ public function setUp(): void {
9296

9397
$this->contentManager = new ContentManager(
9498
$this->jobList,
99+
$this->appConfig,
95100
$this->providerConfig,
96101
$this->mapper,
97102
$this->actionService,
@@ -167,17 +172,64 @@ public function testSubmitContent(): void {
167172
),
168173
];
169174

175+
$this->mapper
176+
->expects($this->once())
177+
->method('findIdByUniqueKey')
178+
->with($appId, 'provider-id', 'item-id')
179+
->willReturn(null);
180+
170181
$this->mapper
171182
->expects($this->once())
172183
->method('insert');
173184

174-
$this->jobList->remove(SubmitContentJob::class, null);
175-
$this->assertFalse($this->jobList->has(SubmitContentJob::class, null));
185+
$this->mapper
186+
->expects($this->never())
187+
->method('update');
176188

177189
$this->contentManager->submitContent($appId, $items);
190+
}
191+
192+
public function testSubmitContentUpdatesAlreadyQueuedItem(): void {
193+
$appId = 'test';
194+
$items = [
195+
new ContentItem(
196+
'item-id',
197+
'provider-id',
198+
'new title',
199+
'new content',
200+
'email-file',
201+
new DateTime(),
202+
['user1', 'user2'],
203+
),
204+
];
178205

179-
$this->assertTrue($this->jobList->has(SubmitContentJob::class, null));
180-
$this->jobList->remove(SubmitContentJob::class, null);
206+
$this->mapper
207+
->expects($this->once())
208+
->method('findIdByUniqueKey')
209+
->with($appId, 'provider-id', 'item-id')
210+
->willReturn(42);
211+
212+
$this->mapper
213+
->expects($this->never())
214+
->method('insert');
215+
216+
$this->mapper
217+
->expects($this->once())
218+
->method('update')
219+
->with($this->callback(function (QueueContentItem $dbItem) use ($appId) {
220+
// update() needs the id of the already queued row, otherwise
221+
// QBMapper throws "Entity which should be updated has no id"
222+
$this->assertSame(42, $dbItem->getId());
223+
$this->assertSame($appId, $dbItem->getAppId());
224+
$this->assertSame('provider-id', $dbItem->getProviderId());
225+
$this->assertSame('item-id', $dbItem->getItemId());
226+
$this->assertSame('new title', $dbItem->getTitle());
227+
$this->assertSame('new content', $dbItem->getContent());
228+
$this->assertSame('user1,user2', $dbItem->getUsers());
229+
return true;
230+
}));
231+
232+
$this->contentManager->submitContent($appId, $items);
181233
}
182234
}
183235

0 commit comments

Comments
 (0)