-
Notifications
You must be signed in to change notification settings - Fork 55
Add skipDuplicates() scope guard to createDocuments #852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
60f1ff1
bee42d4
93a9136
2906dda
63d9902
b0a8392
d8f647c
c88c6ac
16849fe
c6d566e
fd37b69
a24358c
3b783af
3a483f2
eb99cf1
89e4cf8
41704eb
e9e5e76
ae929db
431d378
0fd7c33
9baaa04
3c85013
934ec04
fa0e373
52b189b
fbe5117
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -417,6 +417,8 @@ class Database | |
|
|
||
| protected bool $preserveDates = false; | ||
|
|
||
| protected bool $skipDuplicates = false; | ||
|
|
||
| protected bool $preserveSequence = false; | ||
|
|
||
| protected int $maxQueryValues = 5000; | ||
|
|
@@ -842,6 +844,29 @@ public function skipRelationshipsExistCheck(callable $callback): mixed | |
| } | ||
| } | ||
|
|
||
| public function skipDuplicates(callable $callback): mixed | ||
| { | ||
| $previous = $this->skipDuplicates; | ||
| $this->skipDuplicates = true; | ||
|
|
||
| try { | ||
| return $callback(); | ||
| } finally { | ||
| $this->skipDuplicates = $previous; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Build a tenant-aware identity key for a document. | ||
| * Returns "<tenant>:<id>" in tenant-per-document shared-table mode, otherwise just the id. | ||
| */ | ||
| private function tenantKey(Document $document): string | ||
| { | ||
| return ($this->adapter->getSharedTables() && $this->adapter->getTenantPerDocument()) | ||
| ? $document->getTenant() . ':' . $document->getId() | ||
| : $document->getId(); | ||
| } | ||
|
|
||
| /** | ||
| * Trigger callback for events | ||
| * | ||
|
|
@@ -5700,9 +5725,11 @@ public function createDocuments( | |
| } | ||
|
|
||
| foreach (\array_chunk($documents, $batchSize) as $chunk) { | ||
| $batch = $this->withTransaction(function () use ($collection, $chunk) { | ||
| return $this->adapter->createDocuments($collection, $chunk); | ||
| }); | ||
| $insert = fn () => $this->withTransaction(fn () => $this->adapter->createDocuments($collection, $chunk)); | ||
| // Set adapter flag before withTransaction so Mongo can opt out of a real txn. | ||
| $batch = $this->skipDuplicates | ||
| ? $this->adapter->skipDuplicates($insert) | ||
| : $insert(); | ||
|
Comment on lines
+5728
to
+5732
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard the relationship writes, not just the final batch insert.
🤖 Prompt for AI Agents |
||
|
|
||
| $batch = $this->adapter->getSequences($collection->getId(), $batch); | ||
|
|
||
|
|
@@ -7116,18 +7143,57 @@ public function upsertDocumentsWithIncrease( | |
| $created = 0; | ||
| $updated = 0; | ||
| $seenIds = []; | ||
| foreach ($documents as $key => $document) { | ||
| if ($this->getSharedTables() && $this->getTenantPerDocument()) { | ||
| $old = $this->authorization->skip(fn () => $this->withTenant($document->getTenant(), fn () => $this->silent(fn () => $this->getDocument( | ||
| $collection->getId(), | ||
| $document->getId(), | ||
| )))); | ||
| } else { | ||
| $old = $this->authorization->skip(fn () => $this->silent(fn () => $this->getDocument( | ||
| $collection->getId(), | ||
| $document->getId(), | ||
| ))); | ||
|
|
||
| // Batch-fetch existing documents in one query instead of N individual getDocument() calls. | ||
| // tenantPerDocument: group ids by tenant and run one find() per tenant under withTenant, | ||
| // so cross-tenant batches (e.g. StatsUsage worker) don't get silently scoped to the | ||
| // session tenant and miss rows belonging to other tenants. | ||
| $existingDocs = []; | ||
|
|
||
| if ($this->getSharedTables() && $this->getTenantPerDocument()) { | ||
| $idsByTenant = []; | ||
| foreach ($documents as $doc) { | ||
| if ($doc->getId() !== '') { | ||
| $idsByTenant[$doc->getTenant()][] = $doc->getId(); | ||
| } | ||
| } | ||
| foreach ($idsByTenant as $tenant => $tenantIds) { | ||
| $tenantIds = \array_values(\array_unique($tenantIds)); | ||
| foreach (\array_chunk($tenantIds, \max(1, $this->maxQueryValues)) as $chunk) { | ||
| $found = $this->authorization->skip(fn () => $this->withTenant($tenant, fn () => $this->silent( | ||
| fn () => $this->find($collection->getId(), [ | ||
| Query::equal('$id', $chunk), | ||
| Query::limit($this->maxQueryValues), | ||
| ]) | ||
| ))); | ||
| foreach ($found as $doc) { | ||
| $existingDocs[$this->tenantKey($doc)] = $doc; | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| $docIds = \array_values(\array_unique(\array_filter( | ||
| \array_map(fn (Document $doc) => $doc->getId(), $documents), | ||
| fn ($id) => $id !== '' | ||
| ))); | ||
|
|
||
| if (!empty($docIds)) { | ||
| foreach (\array_chunk($docIds, \max(1, $this->maxQueryValues)) as $chunk) { | ||
| $existing = $this->authorization->skip(fn () => $this->silent( | ||
| fn () => $this->find($collection->getId(), [ | ||
| Query::equal('$id', $chunk), | ||
| Query::limit($this->maxQueryValues), | ||
| ]) | ||
| )); | ||
| foreach ($existing as $doc) { | ||
| $existingDocs[$this->tenantKey($doc)] = $doc; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+7153
to
+7193
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Restore the no-auth This unconditional prefetch changes the Based on learnings, Also applies to: 7213-7314 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. skipPreRead is not merged in main branch yet
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you’d like, I can open a follow-up issue to re-check permission semantics once skipPreRead merges (so we ensure “updates preserve existing permissions; creates default to []; ignore incoming $permissions” in the no-auth + increment case). Happy to proceed if you want that tracked. 🧠 Learnings used |
||
|
|
||
| foreach ($documents as $key => $document) { | ||
| $old = $existingDocs[$this->tenantKey($document)] ?? new Document(); | ||
|
|
||
| // Extract operators early to avoid comparison issues | ||
| $documentArray = $document->getArrayCopy(); | ||
|
|
@@ -7294,7 +7360,7 @@ public function upsertDocumentsWithIncrease( | |
| $document = $this->silent(fn () => $this->createDocumentRelationships($collection, $document)); | ||
| } | ||
|
|
||
| $seenIds[] = $document->getId(); | ||
| $seenIds[] = $this->tenantKey($document); | ||
| $old = $this->adapter->castingBefore($collection, $old); | ||
| $document = $this->adapter->castingBefore($collection, $document); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.