Skip to content

Commit e85729d

Browse files
premtsd-codepremtsd-code
committed
Fix tenant-per-document key collisions and add ignore tests
- createDocuments ignore pre-fetch: group by tenant with composite key (tenant:id) to avoid cross-tenant false positives - upsertDocumentsWithIncrease batch-fetch: use composite key (tenant:id) so documents from different tenants with the same ID don't overwrite each other in the lookup map - Add tests for createDocuments with ignore=true: duplicates are silently skipped, originals unchanged, new docs inserted
1 parent 59aab83 commit e85729d

2 files changed

Lines changed: 172 additions & 19 deletions

File tree

‎src/Database/Database.php‎

Lines changed: 54 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -5658,22 +5658,46 @@ public function createDocuments(
56585658
// When ignore mode is on and relationships are being resolved,
56595659
// pre-fetch existing document IDs so we skip relationship writes for duplicates
56605660
$preExistingIds = [];
5661+
$tenantPerDocument = $this->adapter->getSharedTables() && $this->adapter->getTenantPerDocument();
56615662
if ($ignore && $this->resolveRelationships) {
5662-
$inputIds = \array_values(\array_unique(\array_filter(
5663-
\array_map(fn (Document $doc) => $doc->getId(), $documents)
5664-
)));
5665-
5666-
foreach (\array_chunk($inputIds, $this->maxQueryValues) as $idChunk) {
5667-
$existing = $this->authorization->skip(fn () => $this->silent(fn () => $this->find(
5668-
$collection->getId(),
5669-
[
5670-
Query::equal('$id', $idChunk),
5671-
Query::select(['$id']),
5672-
Query::limit(\count($idChunk)),
5673-
]
5663+
if ($tenantPerDocument) {
5664+
$idsByTenant = [];
5665+
foreach ($documents as $doc) {
5666+
$idsByTenant[$doc->getTenant()][] = $doc->getId();
5667+
}
5668+
foreach ($idsByTenant as $tenant => $tenantIds) {
5669+
$tenantIds = \array_values(\array_unique($tenantIds));
5670+
foreach (\array_chunk($tenantIds, $this->maxQueryValues) as $idChunk) {
5671+
$existing = $this->authorization->skip(fn () => $this->withTenant($tenant, fn () => $this->silent(fn () => $this->find(
5672+
$collection->getId(),
5673+
[
5674+
Query::equal('$id', $idChunk),
5675+
Query::select(['$id']),
5676+
Query::limit(\count($idChunk)),
5677+
]
5678+
))));
5679+
foreach ($existing as $doc) {
5680+
$preExistingIds[$tenant . ':' . $doc->getId()] = true;
5681+
}
5682+
}
5683+
}
5684+
} else {
5685+
$inputIds = \array_values(\array_unique(\array_filter(
5686+
\array_map(fn (Document $doc) => $doc->getId(), $documents)
56745687
)));
5675-
foreach ($existing as $doc) {
5676-
$preExistingIds[$doc->getId()] = true;
5688+
5689+
foreach (\array_chunk($inputIds, $this->maxQueryValues) as $idChunk) {
5690+
$existing = $this->authorization->skip(fn () => $this->silent(fn () => $this->find(
5691+
$collection->getId(),
5692+
[
5693+
Query::equal('$id', $idChunk),
5694+
Query::select(['$id']),
5695+
Query::limit(\count($idChunk)),
5696+
]
5697+
)));
5698+
foreach ($existing as $doc) {
5699+
$preExistingIds[$doc->getId()] = true;
5700+
}
56775701
}
56785702
}
56795703
}
@@ -5717,8 +5741,14 @@ public function createDocuments(
57175741
}
57185742
}
57195743

5720-
if ($this->resolveRelationships && !isset($preExistingIds[$document->getId()])) {
5721-
$document = $this->silent(fn () => $this->createDocumentRelationships($collection, $document));
5744+
if ($this->resolveRelationships) {
5745+
$preExistKey = $tenantPerDocument
5746+
? $document->getTenant() . ':' . $document->getId()
5747+
: $document->getId();
5748+
5749+
if (!isset($preExistingIds[$preExistKey])) {
5750+
$document = $this->silent(fn () => $this->createDocumentRelationships($collection, $document));
5751+
}
57225752
}
57235753

57245754
$document = $this->adapter->castingBefore($collection, $document);
@@ -7145,12 +7175,14 @@ public function upsertDocumentsWithIncrease(
71457175
// Batch-fetch existing documents in one query instead of N individual getDocument() calls
71467176
$ids = \array_filter(\array_map(fn ($doc) => $doc->getId(), $documents));
71477177
$existingDocs = [];
7178+
$upsertTenantPerDocument = $this->getSharedTables() && $this->getTenantPerDocument();
71487179

71497180
if (!empty($ids)) {
71507181
$uniqueIds = \array_values(\array_unique($ids));
71517182

7152-
if ($this->getSharedTables() && $this->getTenantPerDocument()) {
7183+
if ($upsertTenantPerDocument) {
71537184
// Group IDs by tenant and fetch each group separately
7185+
// Use composite key tenant:id to avoid cross-tenant collisions
71547186
$idsByTenant = [];
71557187
foreach ($documents as $doc) {
71567188
$tenant = $doc->getTenant();
@@ -7164,7 +7196,7 @@ public function upsertDocumentsWithIncrease(
71647196
[Query::equal('$id', $idChunk), Query::limit(\count($idChunk))],
71657197
))));
71667198
foreach ($fetched as $doc) {
7167-
$existingDocs[$doc->getId()] = $doc;
7199+
$existingDocs[$tenant . ':' . $doc->getId()] = $doc;
71687200
}
71697201
}
71707202
}
@@ -7182,7 +7214,10 @@ public function upsertDocumentsWithIncrease(
71827214
}
71837215

71847216
foreach ($documents as $key => $document) {
7185-
$old = $existingDocs[$document->getId()] ?? new Document();
7217+
$lookupKey = $upsertTenantPerDocument
7218+
? $document->getTenant() . ':' . $document->getId()
7219+
: $document->getId();
7220+
$old = $existingDocs[$lookupKey] ?? new Document();
71867221

71877222
// Extract operators early to avoid comparison issues
71887223
$documentArray = $document->getArrayCopy();

‎tests/e2e/Adapter/Scopes/DocumentTests.php‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7722,4 +7722,122 @@ public function testRegexInjection(): void
77227722
// }
77237723
// $database->deleteCollection($collectionName);
77247724
// }
7725+
7726+
public function testCreateDocumentsIgnoreDuplicates(): void
7727+
{
7728+
/** @var Database $database */
7729+
$database = $this->getDatabase();
7730+
7731+
$database->createCollection(__FUNCTION__);
7732+
$database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true);
7733+
7734+
// Insert initial documents
7735+
$database->createDocuments(__FUNCTION__, [
7736+
new Document([
7737+
'$id' => 'doc1',
7738+
'name' => 'Original A',
7739+
'$permissions' => [
7740+
Permission::read(Role::any()),
7741+
Permission::create(Role::any()),
7742+
],
7743+
]),
7744+
new Document([
7745+
'$id' => 'doc2',
7746+
'name' => 'Original B',
7747+
'$permissions' => [
7748+
Permission::read(Role::any()),
7749+
Permission::create(Role::any()),
7750+
],
7751+
]),
7752+
]);
7753+
7754+
// Without ignore, duplicates should throw
7755+
try {
7756+
$database->createDocuments(__FUNCTION__, [
7757+
new Document([
7758+
'$id' => 'doc1',
7759+
'name' => 'Duplicate A',
7760+
'$permissions' => [
7761+
Permission::read(Role::any()),
7762+
Permission::create(Role::any()),
7763+
],
7764+
]),
7765+
]);
7766+
$this->fail('Expected DuplicateException');
7767+
} catch (DuplicateException $e) {
7768+
$this->assertNotEmpty($e->getMessage());
7769+
}
7770+
7771+
// With ignore, duplicates should be silently skipped
7772+
$count = $database->createDocuments(__FUNCTION__, [
7773+
new Document([
7774+
'$id' => 'doc1',
7775+
'name' => 'Duplicate A',
7776+
'$permissions' => [
7777+
Permission::read(Role::any()),
7778+
Permission::create(Role::any()),
7779+
],
7780+
]),
7781+
new Document([
7782+
'$id' => 'doc3',
7783+
'name' => 'New C',
7784+
'$permissions' => [
7785+
Permission::read(Role::any()),
7786+
Permission::create(Role::any()),
7787+
],
7788+
]),
7789+
], ignore: true);
7790+
7791+
// doc3 should exist, doc1 should retain original value
7792+
$doc1 = $database->getDocument(__FUNCTION__, 'doc1');
7793+
$this->assertSame('Original A', $doc1->getAttribute('name'));
7794+
7795+
$doc3 = $database->getDocument(__FUNCTION__, 'doc3');
7796+
$this->assertSame('New C', $doc3->getAttribute('name'));
7797+
7798+
// Total should be 3 (doc1, doc2, doc3)
7799+
$all = $database->find(__FUNCTION__);
7800+
$this->assertCount(3, $all);
7801+
}
7802+
7803+
public function testCreateDocumentsIgnoreAllDuplicates(): void
7804+
{
7805+
/** @var Database $database */
7806+
$database = $this->getDatabase();
7807+
7808+
$database->createCollection(__FUNCTION__);
7809+
$database->createAttribute(__FUNCTION__, 'name', Database::VAR_STRING, 128, true);
7810+
7811+
// Insert initial document
7812+
$database->createDocuments(__FUNCTION__, [
7813+
new Document([
7814+
'$id' => 'existing',
7815+
'name' => 'Original',
7816+
'$permissions' => [
7817+
Permission::read(Role::any()),
7818+
Permission::create(Role::any()),
7819+
],
7820+
]),
7821+
]);
7822+
7823+
// With ignore, inserting only duplicates should succeed with no new rows
7824+
$count = $database->createDocuments(__FUNCTION__, [
7825+
new Document([
7826+
'$id' => 'existing',
7827+
'name' => 'Duplicate',
7828+
'$permissions' => [
7829+
Permission::read(Role::any()),
7830+
Permission::create(Role::any()),
7831+
],
7832+
]),
7833+
], ignore: true);
7834+
7835+
// Original document should be unchanged
7836+
$doc = $database->getDocument(__FUNCTION__, 'existing');
7837+
$this->assertSame('Original', $doc->getAttribute('name'));
7838+
7839+
// Still only 1 document
7840+
$all = $database->find(__FUNCTION__);
7841+
$this->assertCount(1, $all);
7842+
}
77257843
}

0 commit comments

Comments
 (0)