diff --git a/src/Database/Database.php b/src/Database/Database.php index 74e75d883..6d4b3c86d 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -806,29 +806,47 @@ public function skipRelationships(callable $callback): mixed * * @param Document $collection * @param array $documents + * @param array $selections Select queries from the caller, preserved so the refetch honors the original projection * @return array + * @throws DatabaseException */ - protected function refetchDocuments(Document $collection, array $documents): array + protected function refetchDocuments(Document $collection, array $documents, array $selections = []): array { if (empty($documents)) { return $documents; } - $docIds = array_map(fn ($doc) => $doc->getId(), $documents); - - // Fetch fresh copies with computed operator values - $refetched = $this->getAuthorization()->skip(fn () => $this->silent( - fn () => $this->find($collection->getId(), [Query::equal('$id', $docIds)]) - )); + $sequences = array_map(function ($doc) { + $sequence = $doc->getSequence(); + if ($sequence === null) { + throw new DatabaseException('Cannot refetch document without a $sequence: ' . $doc->getId()); + } + return $sequence; + }, $documents); + // Fetch fresh copies with computed operator values, preserving the caller's projection. + // Chunk by maxQueryValues (the batch can be up to INSERT_BATCH_SIZE) and bound each find() + // to the chunk size, otherwise find()'s default limit would silently drop rows past it. $refetchedMap = []; - foreach ($refetched as $doc) { - $refetchedMap[$doc->getId()] = $doc; + foreach (\array_chunk($sequences, \max(1, $this->maxQueryValues)) as $chunk) { + $refetched = $this->getAuthorization()->skip(fn () => $this->silent( + fn () => $this->find( + $collection->getId(), + array_merge([ + Query::equal('$sequence', $chunk), + Query::limit(\count($chunk)), + ], $selections) + ) + )); + + foreach ($refetched as $doc) { + $refetchedMap[$doc->getSequence()] = $doc; + } } $result = []; - foreach ($documents as $doc) { - $result[] = $refetchedMap[$doc->getId()] ?? $doc; + foreach ($documents as $index => $doc) { + $result[$index] = $refetchedMap[$sequences[$index]] ?? $doc; } return $result; @@ -6376,20 +6394,6 @@ public function updateDocument(string $collection, string $id, Document $documen $this->purgeCachedDocument($collection->getId(), $document->getId()); } - // If operators were used, refetch document to get computed values - $hasOperators = false; - foreach ($document->getArrayCopy() as $value) { - if (Operator::isOperator($value)) { - $hasOperators = true; - break; - } - } - - if ($hasOperators) { - $refetched = $this->refetchDocuments($collection, [$document]); - $document = $refetched[0]; - } - return $document; }); @@ -6400,6 +6404,20 @@ public function updateDocument(string $collection, string $id, Document $documen // Purge again after commit so readers cannot re-cache the pre-commit version $this->purgeCachedDocumentInternal($collection->getId(), $id); + // If operators were used, refetch (outside the transaction) to get committed computed values + $hasOperators = false; + foreach ($document->getArrayCopy() as $value) { + if (Operator::isOperator($value)) { + $hasOperators = true; + break; + } + } + + if ($hasOperators) { + $refetched = $this->refetchDocuments($collection, [$document]); + $document = $refetched[0]; + } + if (!$this->inBatchRelationshipPopulation && $this->resolveRelationships) { $documents = $this->silent(fn () => $this->populateDocumentsRelationships([$document], $collection, $this->relationshipFetchDepth)); $document = $documents[0]; @@ -6623,14 +6641,19 @@ public function updateDocuments( } if ($hasOperators) { - $batch = $this->refetchDocuments($collection, $batch); + $batch = $this->refetchDocuments($collection, $batch, $grouped['selections']); } foreach ($batch as $index => $doc) { $doc = $this->adapter->castingAfter($collection, $doc); $doc->removeAttribute('$skipPermissionsUpdate'); $this->purgeCachedDocument($collection->getId(), $doc->getId()); - $doc = $this->decode($collection, $doc); + // The operator refetch goes through find(), which already returns fully decoded + // documents. Decoding again would double-apply the decode filters (and, because + // this call passes no selections, re-materialize non-selected attributes). + if (!$hasOperators) { + $doc = $this->decode($collection, $doc); + } try { $onNext && $onNext($doc, $old[$index]); } catch (Throwable $th) { diff --git a/tests/e2e/Adapter/Scopes/OperatorTests.php b/tests/e2e/Adapter/Scopes/OperatorTests.php index 89c64bee5..77fb5b162 100644 --- a/tests/e2e/Adapter/Scopes/OperatorTests.php +++ b/tests/e2e/Adapter/Scopes/OperatorTests.php @@ -431,6 +431,125 @@ public function testUpdateDocumentsOperatorsWithQueries(): void $database->deleteCollection($collectionId); } + public function testUpdateDocumentsOperatorsWithSelect(): void + { + /** @var Database $database */ + $database = static::getDatabase(); + + if (!$database->getAdapter()->getSupportForOperators()) { + $this->expectNotToPerformAssertions(); + return; + } + + $collectionId = 'test_operators_with_select'; + $database->createCollection($collectionId); + + $database->createAttribute($collectionId, 'category', Database::VAR_STRING, 50, true); + $database->createAttribute($collectionId, 'count', Database::VAR_INTEGER, 0, false, 0); + $database->createAttribute($collectionId, 'score', Database::VAR_FLOAT, 0, false, 0.0); + + for ($i = 1; $i <= 3; $i++) { + $database->createDocument($collectionId, new Document([ + '$id' => "select_doc_{$i}", + '$permissions' => [Permission::read(Role::any()), Permission::update(Role::any())], + 'category' => 'A', + 'count' => $i * 10, + 'score' => $i * 1.5, + ])); + } + + // Update with an operator while selecting only a subset of attributes. + // The operator path refetches to compute values; it must still honor the caller's select. + $updated = []; + $count = $database->updateDocuments( + $collectionId, + new Document([ + 'count' => Operator::increment(100), + ]), + [Query::select(['count']), Query::orderAsc('$id')], + onNext: function (Document $doc) use (&$updated) { + $updated[] = $doc; + } + ); + + $this->assertEquals(3, $count); + $this->assertCount(3, $updated); + + // A plain find with the same select defines the expected projection. + $found = $database->find($collectionId, [Query::select(['count']), Query::orderAsc('$id')]); + + foreach ($updated as $index => $doc) { + // Computed operator value is present and correct. + $this->assertEquals(($index + 1) * 10 + 100, $doc->getAttribute('count')); + + // The operator path must return the same projection as a normal select find, + // i.e. it must not leak the non-selected attributes (regression check). + $this->assertEquals( + \array_keys($found[$index]->getArrayCopy()), + \array_keys($doc->getArrayCopy()) + ); + } + + $database->deleteCollection($collectionId); + } + + public function testUpdateDocumentsOperatorsBatchLargerThanDefaultLimit(): void + { + /** @var Database $database */ + $database = static::getDatabase(); + + if (!$database->getAdapter()->getSupportForOperators()) { + $this->expectNotToPerformAssertions(); + return; + } + + $collectionId = 'test_operators_large_batch'; + $database->createCollection($collectionId); + + $database->createAttribute($collectionId, 'count', Database::VAR_INTEGER, 0, false, 0); + + // More documents than find()'s default limit (25) so the refetch must page/limit correctly. + $total = 60; + for ($i = 1; $i <= $total; $i++) { + $database->createDocument($collectionId, new Document([ + '$id' => "batch_doc_{$i}", + '$permissions' => [Permission::read(Role::any()), Permission::update(Role::any())], + 'count' => $i, + ])); + } + + // Update every document with an operator. The refetch must return computed values for the + // whole batch, not just the first 25 rows (which would otherwise fall back to stale values). + $updated = []; + $count = $database->updateDocuments( + $collectionId, + new Document([ + 'count' => Operator::increment(1000), + ]), + [], + batchSize: $total, + onNext: function (Document $doc) use (&$updated) { + $updated[$doc->getId()] = $doc; + } + ); + + $this->assertEquals($total, $count); + $this->assertCount($total, $updated); + + // Every document must reflect the computed operator value. + for ($i = 1; $i <= $total; $i++) { + $doc = $updated["batch_doc_{$i}"] ?? null; + $this->assertNotNull($doc, "Missing callback document batch_doc_{$i}"); + $this->assertEquals($i + 1000, $doc->getAttribute('count'), "Stale value for batch_doc_{$i}"); + + // And it must be persisted, not just returned. + $fresh = $database->getDocument($collectionId, "batch_doc_{$i}"); + $this->assertEquals($i + 1000, $fresh->getAttribute('count')); + } + + $database->deleteCollection($collectionId); + } + public function testOperatorErrorHandling(): void { /** @var Database $database */