From 76e7801d9c9b2e7c8991b1e46489d29af067f4bb Mon Sep 17 00:00:00 2001 From: fogelito Date: Mon, 31 Aug 2026 15:10:42 +0300 Subject: [PATCH] Run --- src/Database/Adapter/SQL.php | 11 ++- src/Database/Database.php | 36 ++++++++- tests/e2e/Adapter/Scopes/DocumentTests.php | 87 ++++++++++++++++++++++ tests/e2e/Adapter/Scopes/GeneralTests.php | 36 ++++++--- 4 files changed, 155 insertions(+), 15 deletions(-) diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index 2d6043980..9ca4c1aee 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -862,6 +862,7 @@ public function getSequences(string $collection, array $documents): array $documentIds = []; $keys = []; $binds = []; + $tenants = []; foreach ($documents as $i => $document) { if (empty($document->getSequence())) { @@ -873,7 +874,13 @@ public function getSequences(string $collection, array $documents): array $keys[] = $key; if ($this->sharedTables) { - $binds[':_tenant_'.$i] = $document->getTenant(); + $tenant = $document->getTenant(); + + // One placeholder per distinct tenant + if (!\in_array($tenant, $tenants, true)) { + $binds[':_tenant_'.\count($tenants)] = $tenant; + $tenants[] = $tenant; + } } } } @@ -888,7 +895,7 @@ public function getSequences(string $collection, array $documents): array SELECT _uid, _id FROM {$this->getSQLTable($collection)} WHERE {$this->quote('_uid')} IN ({$placeholders}) - {$this->getTenantQuery($collection, tenantCount: \count($documentIds))} + {$this->getTenantQuery($collection, tenantCount: \count($tenants))} "; $stmt = $this->getPDO()->prepare($sql); diff --git a/src/Database/Database.php b/src/Database/Database.php index 4769599a9..144428665 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -5866,6 +5866,10 @@ public function createDocuments( $time = DateTime::now(); $modified = 0; + $hasRelationships = !empty(\array_filter( + $collection->getAttribute('attributes', []), + fn ($attribute) => $attribute['type'] === self::VAR_RELATIONSHIP + )); foreach ($documents as $document) { $createdAt = $document->getCreatedAt(); @@ -5922,7 +5926,11 @@ public function createDocuments( ? $this->adapter->skipDuplicates($insert) : $insert(); - $batch = $this->adapter->getSequences($collection->getId(), $batch); + // A SELECT per batch, read only by relationship population and by whatever the + // caller does with the documents $onNext hands it. Skip it when neither applies. + if ($onNext !== null || $hasRelationships) { + $batch = $this->adapter->getSequences($collection->getId(), $batch); + } if (!$this->inBatchRelationshipPopulation && $this->resolveRelationships) { $batch = $this->silent(fn () => $this->populateDocumentsRelationships($batch, $collection, $this->relationshipFetchDepth)); @@ -7366,6 +7374,10 @@ public function upsertDocumentsWithIncrease( $created = 0; $updated = 0; $seenIds = []; + $hasRelationships = !empty(\array_filter( + $collectionAttributes, + fn ($attribute) => $attribute['type'] === self::VAR_RELATIONSHIP + )); // 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, @@ -7611,7 +7623,21 @@ public function upsertDocumentsWithIncrease( $chunk ))); - $batch = $this->adapter->getSequences($collection->getId(), $batch); + // Every row that already existed was read into $existingDocs above, so its + // sequence is already in hand and does not need fetching a second time. + foreach ($batch as $index => $doc) { + if (empty($doc->getSequence()) && !empty($chunk[$index]->getOld()->getSequence())) { + $doc->setAttribute('$sequence', $chunk[$index]->getOld()->getSequence()); + } + } + + // Nothing leaves this method except through $onNext -- the return value is a + // count -- so a caller that passes none never sees these documents, and the work + // that finishes them has no reader. Bulk writers such as the usage/stats workers + // upsert several batches a second and read nothing back. + if ($onNext !== null || $hasRelationships) { + $batch = $this->adapter->getSequences($collection->getId(), $batch); + } foreach ($chunk as $change) { if ($change->getOld()->isEmpty()) { @@ -7635,7 +7661,11 @@ public function upsertDocumentsWithIncrease( } } - if ($hasOperators) { + // Refetching only exists to hand computed operator values back to the caller, and + // $onNext is the only way anything leaves this method -- the return value is a + // count. $hasOperators still has to reflect the batch, because the decode below + // keys off it. + if ($hasOperators && $onNext !== null) { $batch = $this->refetchDocuments($collection, $batch); } diff --git a/tests/e2e/Adapter/Scopes/DocumentTests.php b/tests/e2e/Adapter/Scopes/DocumentTests.php index cbbe36b8a..fd969cb1b 100644 --- a/tests/e2e/Adapter/Scopes/DocumentTests.php +++ b/tests/e2e/Adapter/Scopes/DocumentTests.php @@ -646,6 +646,14 @@ public function testCreateDocuments(): void $this->assertEquals(5, $document->getAttribute('integer')); $this->assertIsInt($document->getAttribute('bigint')); $this->assertEquals(9223372036854775807, $document->getAttribute('bigint')); + + // The insert does not return the sequence for the rows it wrote, so it is looked + // up afterwards. $onNext is the only way these documents reach the caller. + $this->assertNotEmpty($document->getSequence()); + $this->assertEquals( + $database->getDocument($collection, $document->getId())->getSequence(), + $document->getSequence() + ); } $documents = $database->find($collection, [ @@ -1851,6 +1859,85 @@ public function testPreserveSequenceUpsert(): void $database->deleteCollection($collectionName); } + /** + * upsertDocuments() carries the sequence of every row it already read across to the + * written document, so the follow-up getSequences() lookup only covers the rows that + * were genuinely new. That leaves the batch it receives interleaved -- some documents + * carry a sequence, some do not -- and the tenant placeholders it binds must line up + * with the ones its SQL declares regardless of where the gaps fall. + */ + public function testUpsertSequencesOnMixedBatch(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + + if (!$database->getAdapter()->getSupportForUpserts()) { + $this->expectNotToPerformAssertions(); + return; + } + + $collectionName = 'upsert_mixed_sequences'; + + $database->createCollection($collectionName, permissions: [ + Permission::create(Role::any()), + Permission::read(Role::any()), + Permission::update(Role::any()), + ]); + + if ($database->getAdapter()->getSupportForAttributes()) { + $database->createAttribute($collectionName, 'name', Database::VAR_STRING, 128, true); + } + + $permissions = [ + Permission::read(Role::any()), + Permission::update(Role::any()), + ]; + + $existingSequences = []; + foreach (['existing1', 'existing2'] as $id) { + $created = $database->createDocument($collectionName, new Document([ + '$id' => $id, + '$permissions' => $permissions, + 'name' => $id, + ])); + + $this->assertNotEmpty($created->getSequence()); + $existingSequences[$id] = $created->getSequence(); + } + + // Existing and new rows interleaved, so the new ones sit at odd indexes in the batch. + $upserted = []; + $database->upsertDocuments( + $collectionName, + [ + new Document(['$id' => 'existing1', '$permissions' => $permissions, 'name' => 'existing1 updated']), + new Document(['$id' => 'new1', '$permissions' => $permissions, 'name' => 'new1']), + new Document(['$id' => 'existing2', '$permissions' => $permissions, 'name' => 'existing2 updated']), + new Document(['$id' => 'new2', '$permissions' => $permissions, 'name' => 'new2']), + ], + onNext: function (Document $document) use (&$upserted) { + $upserted[$document->getId()] = $document->getSequence(); + } + ); + + $this->assertCount(4, $upserted); + + foreach (['existing1', 'existing2', 'new1', 'new2'] as $id) { + $this->assertNotEmpty($upserted[$id], "No sequence returned for {$id}"); + $this->assertEquals( + $database->getDocument($collectionName, $id)->getSequence(), + $upserted[$id], + "Wrong sequence returned for {$id}" + ); + } + + // An upsert must not move a row that was already there. + $this->assertEquals($existingSequences['existing1'], $upserted['existing1']); + $this->assertEquals($existingSequences['existing2'], $upserted['existing2']); + + $database->deleteCollection($collectionName); + } + public function testRespectNulls(): Document { /** @var Database $database */ diff --git a/tests/e2e/Adapter/Scopes/GeneralTests.php b/tests/e2e/Adapter/Scopes/GeneralTests.php index 2057197fd..2c86ead2e 100644 --- a/tests/e2e/Adapter/Scopes/GeneralTests.php +++ b/tests/e2e/Adapter/Scopes/GeneralTests.php @@ -609,21 +609,35 @@ public function testSharedTablesTenantPerDocument(): void $this->assertEquals(true, $doc->isEmpty()); - // Upsert new documents with different tenants + // Upsert new documents with different tenants. The sequence lookup binds one + // placeholder per distinct tenant, so a cross-tenant batch has to keep each + // tenant's value at the position its placeholder was named for -- collected here + // because $onNext is the only way these documents reach the caller. $doc4Id = ID::unique(); $doc5Id = ID::unique(); + $sequences = []; $database ->setTenant(null) ->setTenantPerDocument(true) - ->upsertDocuments(__FUNCTION__, [new Document([ - '$id' => $doc4Id, - '$tenant' => 4, - 'name' => 'Superman4', - ]), new Document([ - '$id' => $doc5Id, - '$tenant' => 5, - 'name' => 'Superman5', - ])]); + ->upsertDocuments( + __FUNCTION__, + [new Document([ + '$id' => $doc4Id, + '$tenant' => 4, + 'name' => 'Superman4', + ]), new Document([ + '$id' => $doc5Id, + '$tenant' => 5, + 'name' => 'Superman5', + ])], + onNext: function (Document $document) use (&$sequences) { + $sequences[$document->getId()] = $document->getSequence(); + } + ); + + $this->assertCount(2, $sequences); + $this->assertNotEmpty($sequences[$doc4Id]); + $this->assertNotEmpty($sequences[$doc5Id]); // Set to tenant 4 and read $doc = $database @@ -633,6 +647,7 @@ public function testSharedTablesTenantPerDocument(): void $this->assertEquals('Superman4', $doc['name']); $this->assertEquals(4, $doc->getTenant()); + $this->assertEquals($doc->getSequence(), $sequences[$doc4Id]); // Set to tenant 5 and read $doc = $database @@ -642,6 +657,7 @@ public function testSharedTablesTenantPerDocument(): void $this->assertEquals('Superman5', $doc['name']); $this->assertEquals(5, $doc->getTenant()); + $this->assertEquals($doc->getSequence(), $sequences[$doc5Id]); // Update names via upsert $database