diff --git a/CHANGELOG.md b/CHANGELOG.md index 6820d0021..76f7a6e33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,46 @@ # Changelog +## 8.1.4 + +8.1.4 fixes `deleteDocuments()` deleting documents that stopped matching its queries after they were found. It needs +no code change; a custom adapter can opt in to the fix, see +[Upgrading from 8.1.3 to 8.1.4](UPGRADE.md#upgrading-from-813-to-814). + +### Added + +- `Adapter::deleteDocumentsMatching(Document $collection, array $sequences, array $permissionIds, array $queries): int` + deletes the rows of `$sequences` that match every filter query in `$queries`, as `find()` is handed them, removes + the permission rows of `$permissionIds` but those of a row kept because it no longer matches, and returns the + number of rows deleted. It calls `deleteDocuments()` with the queries readable through the new protected + `Adapter::deletionFilters()`, so an override of `deleteDocuments()` still runs, on a `Pool` too. Memory, SQLite, + MariaDB, MySQL, PostgreSQL, MongoDB and Redis delete only the rows that match them; `Pool` and `ReadWritePool` hand + them to the pooled adapter. A custom adapter that ignores them deletes every row of `$sequences`, as before. + +### Fixed + +- `deleteDocuments()` deletes only the documents that still match its filters when each batch is deleted. It found a + batch and then deleted it by internal id alone, so a document another write changed in between, so that it no + longer matched (such as a presence refreshed before an expired-presence cleanup deleted it), was deleted anyway, + cascaded to its related documents, lost its permission rows, and was counted and handed to `onNext`. Each batch + with filters on anything but `$sequence` now reads its documents again, with the same queries, in the transaction + that deletes them, and deletes them with `Adapter::deleteDocumentsMatching()`. A document that no longer matches is + kept with its related documents and permission rows, and is neither counted, handed to `onNext` nor in + `Event::DocumentsDelete`'s count. A limit counts the documents found, kept ones included, so a kept document is not + replaced by a later one. When a document changes between that read and the delete, the batch is rolled back and + tried again, and fails with `Exception\Contention` once the transaction's retries are spent; the batches before it + stay deleted and `Event::DocumentsDelete` reports them. In a transaction begun with `startTransaction()` the retry + runs in a savepoint, which on MySQL and MariaDB reads the transaction's snapshot again and fails the same way; a + MongoDB replica set has no savepoints, so the batch is not retried and its partial delete stays in the transaction. + Either way the caller rolls the whole transaction back and runs it again. Without transactions, as on a standalone + MongoDB server, nothing is rolled back: the documents still stored are kept and left out of the count, `onNext`, + the cache purges and the event, the remaining batches are deleted, a document another deleter removed meanwhile is + counted by this call, and a cascade that already ran for a kept document stays, so cascades are best-effort there. + The adapter's count is a hint: only a document of the batch still stored after the delete counts as changed, so a + custom adapter that reports a wrong count does not fail the delete. A delete without filters, or with filters only + on `$sequence`, runs as before. +- A bulk delete on SQLite, MariaDB, MySQL and PostgreSQL binds the floats of its filters in fixed-point notation, as + `find()` binds them, so it matches the rows the find matched. + ## 8.1.3 8.1.3 adds a hook that rewrites the queries of a read. It changes nothing for a database without one; see diff --git a/UPGRADE.md b/UPGRADE.md index fe5d03261..79ce574f2 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,3 +1,25 @@ +# Upgrading from 8.1.3 to 8.1.4 + +8.1.4 is a patch release and needs no code change. `Database::deleteDocuments()` keeps its signature and now deletes +only the documents that still match its filters when they are deleted; see [CHANGELOG.md](CHANGELOG.md#814). + +`Adapter::deleteDocuments()` keeps its signature. A batch with filters reaches the adapter through the new +`Adapter::deleteDocumentsMatching()`, which calls `deleteDocuments()` with the filters readable through +`$this->deletionFilters()`, so an override of `deleteDocuments()` in a subclass of a bundled adapter still runs and, +when it calls its parent, deletes only the rows that match. A custom adapter that extends `Adapter` directly deletes +every row it is handed, as in 8.1.3; to keep a document another write changed after it was found, delete only the +rows that match every query of `deletionFilters()` (filter queries, as `find()` is handed them), keep the permission +rows of a row kept, and return the number of rows deleted. + +The count `deleteDocuments()` returns is a hint: a lower count than the rows handed makes `Database::deleteDocuments()` +check which documents are still stored, and a higher one is ignored, so a custom adapter that counts differently still +works. + +`deleteDocuments()` can now throw `Exception\Contention` when documents keep changing between the check and the delete +of a batch. Inside a transaction begun with `startTransaction()`, roll it back and run it again: on MySQL and MariaDB +the retry in the savepoint rereads the same snapshot, and a MongoDB replica set has no savepoint, so the partial +delete of the batch stays in the transaction until it is rolled back. + # Upgrading from 8.1.2 to 8.1.3 8.1.3 is a patch release and needs no code change. It adds `Hook\Queries`, which rewrites the queries of a read diff --git a/src/Database/Adapter.php b/src/Database/Adapter.php index 769bba3ea..612d4507f 100644 --- a/src/Database/Adapter.php +++ b/src/Database/Adapter.php @@ -42,6 +42,9 @@ abstract class Adapter /** @var Value|null */ private ?Value $ignoringDuplicates = null; + /** @var Value>|null */ + private ?Value $deletionFilters = null; + protected bool $tenantPerDocument = false; protected int $inTransaction = 0; @@ -773,9 +776,57 @@ abstract public function deleteDocument(Document $collection, string $id): bool; /** * @param array $sequences * @param array $permissionIds + * @return int The number of document rows deleted. Database::deleteDocuments() takes it as a hint: a count below + * the sequences given makes it check which of the documents are still stored, and a higher one is + * ignored. */ abstract public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int; + /** + * Delete the rows of the sequences given that match every filter query, and the permission rows of the + * permission ids given but those of a row kept because it no longer matches. Database::deleteDocuments() calls + * it for a batch it deletes with filters, so that a document another write changed after it was found is kept. + * + * This implementation calls deleteDocuments() with the queries as its deletionFilters(), so that an override of + * deleteDocuments() still runs. The bundled adapters delete only the rows that match them; an adapter that + * ignores them deletes every row of the sequences given, as before. An adapter that extends Adapter directly + * honours them by reading deletionFilters() in deleteDocuments(), or by overriding this method. + * + * @param array $sequences + * @param array $permissionIds + * @param array $queries Filter queries as find() is handed them, which every deleted row must match + * @return int The number of rows deleted + */ + public function deleteDocumentsMatching(Document $collection, array $sequences, array $permissionIds, array $queries): int + { + return $this->deletionFiltersValue()->with( + $queries, + fn (): int => $this->deleteDocuments($collection, $sequences, $permissionIds), + ); + } + + /** + * The filter queries every row deleteDocuments() deletes must match: those deleteDocumentsMatching() was handed + * while it runs, else none. + * + * @return array + */ + protected function deletionFilters(): array + { + return $this->deletionFiltersValue()->get(); + } + + /** + * @return Value> + */ + private function deletionFiltersValue(): Value + { + /** @var array $none */ + $none = []; + + return $this->deletionFilters ??= new Value($none); + } + /** * Find Documents * diff --git a/src/Database/Adapter/Memory.php b/src/Database/Adapter/Memory.php index a348f8974..792cfe8bb 100644 --- a/src/Database/Adapter/Memory.php +++ b/src/Database/Adapter/Memory.php @@ -1726,6 +1726,12 @@ public function deleteDocument(Document $collection, string $id): bool return true; } + /** + * Deletes only the rows that match every one of the deletionFilters(). + * + * @param array $sequences + * @param array $permissionIds + */ #[\Override] public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int { @@ -1734,14 +1740,16 @@ public function deleteDocuments(Document $collection, array $sequences, array $p throw new NotFoundException('Collection not found'); } - $seqSet = []; - foreach ($sequences as $seq) { - $seqSet[(string) $seq] = true; + $sequenceSet = []; + foreach ($sequences as $sequence) { + $sequenceSet[(string) $sequence] = true; } + $filters = Query::groupByType($this->deletionFilters())->filters; $count = 0; $deletedIds = []; - foreach ($this->data[$key]['documents'] as $docKey => $row) { + $keptIds = []; + foreach ($this->data[$key]['documents'] as $storageKey => $row) { // With sharedTables the row map is keyed by "tenant|uid" so sequence // collisions across tenants are possible. Skip rows that don't belong // to the current tenant so we never delete another tenant's data. @@ -1749,19 +1757,24 @@ public function deleteDocuments(Document $collection, array $sequences, array $p continue; } $rowId = $row[Storage::SEQUENCE] ?? ''; - $rowUid = $row[Storage::UID] ?? $docKey; - if (isset($seqSet[\is_scalar($rowId) ? (string) $rowId : ''])) { - $deletedIds[\is_scalar($rowUid) ? (string) $rowUid : $docKey] = true; + $rowUid = $row[Storage::UID] ?? $storageKey; + if (isset($sequenceSet[\is_scalar($rowId) ? (string) $rowId : ''])) { + if (! $this->matchesAll($row, $filters)) { + $keptIds[\is_scalar($rowUid) ? (string) $rowUid : $storageKey] = true; + + continue; + } + $deletedIds[\is_scalar($rowUid) ? (string) $rowUid : $storageKey] = true; $oldSignatures = $this->rowUniqueSignatures($key, $row); - unset($this->data[$key]['documents'][$docKey]); - $this->journal(function () use ($key, $docKey, $row): void { - $this->data[$key]['documents'][$docKey] = $row; + unset($this->data[$key]['documents'][$storageKey]); + $this->journal(function () use ($key, $storageKey, $row): void { + $this->data[$key]['documents'][$storageKey] = $row; }); foreach ($oldSignatures as $indexId => $hash) { - if (($this->uniqueIndexHashes[$key][$indexId][$hash] ?? null) === $docKey) { + if (($this->uniqueIndexHashes[$key][$indexId][$hash] ?? null) === $storageKey) { unset($this->uniqueIndexHashes[$key][$indexId][$hash]); - $this->journal(function () use ($key, $indexId, $hash, $docKey): void { - $this->uniqueIndexHashes[$key][$indexId][$hash] = $docKey; + $this->journal(function () use ($key, $indexId, $hash, $storageKey): void { + $this->uniqueIndexHashes[$key][$indexId][$hash] = $storageKey; }); } } @@ -1769,17 +1782,17 @@ public function deleteDocuments(Document $collection, array $sequences, array $p } } - $permSet = ! empty($permissionIds) + $permissionSet = ! empty($permissionIds) ? \array_flip(\array_map('strval', $permissionIds)) : []; - if (! empty($deletedIds) || ! empty($permSet)) { + if (! empty($deletedIds) || ! empty($permissionSet)) { $tenant = $this->getTenant(); foreach (\array_keys($deletedIds) as $documentId) { $this->removePermissionsForDocument($key, (string) $documentId, $tenant, $this->sharedTables); } - foreach (\array_keys($permSet) as $documentId) { - if (isset($deletedIds[$documentId])) { + foreach (\array_keys($permissionSet) as $documentId) { + if (isset($deletedIds[$documentId]) || isset($keptIds[$documentId])) { continue; } $this->removePermissionsForDocument($key, (string) $documentId, $tenant, $this->sharedTables); @@ -2432,6 +2445,21 @@ protected function fusedFilter(string $key, string $collectionId, array $queries return $output; } + /** + * @param array $row + * @param array $queries + */ + private function matchesAll(array $row, array $queries): bool + { + foreach ($queries as $query) { + if (! $this->matches($row, $query)) { + return false; + } + } + + return true; + } + /** * @param array $row */ diff --git a/src/Database/Adapter/Mongo.php b/src/Database/Adapter/Mongo.php index 2a407912c..687b9dd10 100644 --- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ -2290,6 +2290,8 @@ public function deleteDocument(Document $collection, string $id): bool } /** + * Deletes only the rows that match every one of the deletionFilters(). + * * @param array $sequences * @param array $permissionIds * @@ -2301,14 +2303,17 @@ public function deleteDocuments(Document $collection, array $sequences, array $p $collectionId = $collection->getId(); $name = $this->getNamespace().'_'.$this->filter($collectionId); - foreach ($sequences as $index => $sequence) { - $sequences[$index] = $sequence; - } - /** @var array $filters */ $filters = $this->buildFilters([new Query(Method::Equal, Storage::SEQUENCE, $sequences)]); $filters = $this->applyTenantFilter($filters, $collectionId); + $queries = \array_map(static fn (Query $query): Query => clone $query, $this->deletionFilters()); + $this->escapeQueryAttributes($collection, $queries); + $matching = $this->buildFilters($queries); + if ($matching !== []) { + $filters = ['$and' => [$filters, $matching]]; + } + $filters = $this->replaceInternalIdsKeys($filters, '$', '_', $this->operators); $options = $this->getTransactionOptions(); diff --git a/src/Database/Adapter/Pool.php b/src/Database/Adapter/Pool.php index a05f4efcc..950ff23e2 100644 --- a/src/Database/Adapter/Pool.php +++ b/src/Database/Adapter/Pool.php @@ -864,11 +864,19 @@ public function deleteDocument(Document $collection, string $id): bool return $result; } + /** + * Hands the pooled adapter the deletionFilters() through its deleteDocumentsMatching(), as they are scoped to + * this pool, so that an override of this method still runs for a filtered batch. + */ #[\Override] public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int { + $filters = $this->deletionFilters(); + /** @var int $result */ - $result = $this->delegate(__FUNCTION__, \func_get_args()); + $result = $filters === [] + ? $this->delegate(__FUNCTION__, [$collection, $sequences, $permissionIds]) + : $this->delegate('deleteDocumentsMatching', [$collection, $sequences, $permissionIds, $filters]); return $result; } diff --git a/src/Database/Adapter/Redis.php b/src/Database/Adapter/Redis.php index 457cc7ade..f3d87e607 100644 --- a/src/Database/Adapter/Redis.php +++ b/src/Database/Adapter/Redis.php @@ -1392,6 +1392,12 @@ public function deleteDocument(Document $collection, string $id): bool }); } + /** + * Deletes only the rows that match every one of the deletionFilters(). + * + * @param array $sequences + * @param array $permissionIds + */ #[\Override] public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int { @@ -1400,25 +1406,26 @@ public function deleteDocuments(Document $collection, array $sequences, array $p } $collectionId = $this->filter($collection->getId()); - $idxKey = $this->idxKey($collectionId); + $indexKey = $this->idxKey($collectionId); + $queries = $this->deletionFilters(); - return $this->transaction(function (RedisClient $redis) use ($collectionId, $sequences, $permissionIds, $idxKey): int { + return $this->transaction(function (RedisClient $redis) use ($collectionId, $sequences, $permissionIds, $queries, $indexKey): int { $sequenceSet = []; foreach ($sequences as $sequence) { $sequenceSet[(string) $sequence] = true; } - $allIds = $redis->sMembers($idxKey); + $allIds = $redis->sMembers($indexKey); if (! \is_array($allIds)) { $allIds = []; } - $docKeys = []; + $documentKeys = []; $redis->multi(\Redis::PIPELINE); foreach ($allIds as $id) { - $docKey = $this->docKey($collectionId, (string) $id); - $docKeys[(string) $id] = $docKey; - $redis->get($docKey); + $documentKey = $this->docKey($collectionId, (string) $id); + $documentKeys[(string) $id] = $documentKey; + $redis->get($documentKey); } $payloads = $redis->exec(); if (! \is_array($payloads)) { @@ -1426,6 +1433,7 @@ public function deleteDocuments(Document $collection, array $sequences, array $p } $deleted = []; + $kept = []; foreach ($allIds as $position => $id) { $payload = $payloads[$position] ?? false; if (! \is_string($payload) || $payload === '') { @@ -1433,28 +1441,34 @@ public function deleteDocuments(Document $collection, array $sequences, array $p } $document = $this->decode($payload); $matchesSequence = isset($sequenceSet[(string) $document->getSequence()]); - if ($matchesSequence) { - $deleted[$document->getId()] = ['payload' => $payload, 'docKey' => $docKeys[(string) $id]]; + if (! $matchesSequence) { + continue; } + if ($this->filterDocumentsByQueries($collectionId, [$document], $queries) === []) { + $kept[$document->getId()] = true; + + continue; + } + $deleted[$document->getId()] = ['payload' => $payload, 'documentKey' => $documentKeys[(string) $id]]; } foreach ($deleted as $documentId => $deleteEntry) { - $deletedDocKey = $deleteEntry['docKey']; + $deletedKey = $deleteEntry['documentKey']; $this->journal('deleteDoc', [ 'collection' => $collectionId, 'id' => (string) $documentId, 'payload' => $deleteEntry['payload'], - 'docKey' => $deletedDocKey, - 'idxKey' => $idxKey, + 'docKey' => $deletedKey, + 'idxKey' => $indexKey, ]); $this->clearPermissions($collectionId, (string) $documentId); - $redis->del($deletedDocKey); - $redis->sRem($idxKey, \strtolower((string) $documentId)); + $redis->del($deletedKey); + $redis->sRem($indexKey, \strtolower((string) $documentId)); } foreach ($permissionIds as $permissionId) { $documentId = (string) $permissionId; - if (isset($deleted[$documentId])) { + if (isset($deleted[$documentId]) || isset($kept[$documentId])) { continue; } $this->clearPermissions($collectionId, $documentId); diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index 4bb7215ec..6780fe816 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -1257,6 +1257,8 @@ public function upsertDocuments(Document $collection, array $changes, ?string $i } /** + * Deletes only the rows that match every one of the deletionFilters(). + * * @param array $sequences * @param array $permissionIds * @@ -1273,9 +1275,16 @@ public function deleteDocuments(Document $collection, array $sequences, array $p try { $name = $this->filter($collection->getId()); + $filters = \array_values(\array_filter( + $this->mainRowQueries($this->deletionFilters(), $collection), + static fn (Query $query): bool => ! $query->getMethod()->isVector(), + )); $builder = $this->newBuilder($name); $builder->filter([BaseQuery::equal(Storage::SEQUENCE, \array_values($sequences))]); + if ($filters !== []) { + $this->applyFilters($builder, $filters, $name, $this->unqualifiedTable($name)); + } $result = $builder->delete(); $statement = $this->executeResult($result, Event::DocumentsDelete); @@ -1283,13 +1292,58 @@ public function deleteDocuments(Document $collection, array $sequences, array $p throw new DatabaseException('Failed to delete documents'); } + $deleted = $statement->rowCount(); + if ($filters !== [] && $deleted < \count($sequences)) { + $permissionIds = $this->withoutStoredDocuments($name, $permissionIds); + } + $context = $this->writeContext(); $this->runWriteHooks(fn ($hook) => $hook->afterDocumentDelete($name, \array_values($permissionIds), $context)); } catch (Throwable $e) { throw new DatabaseException($e->getMessage(), $e->getCode(), $e); } - return $statement->rowCount(); + return $deleted; + } + + /** + * The name a statement on the collection's table refers to its columns by when it gives the table no alias. + * + * @throws DatabaseException + */ + private function unqualifiedTable(string $name): string + { + $table = $this->getTableRaw($name); + $dot = \strrpos($table, '.'); + + return $dot === false ? $table : \substr($table, $dot + 1); + } + + /** + * @param array $documentIds + * @return array + * + * @throws DatabaseException + */ + private function withoutStoredDocuments(string $name, array $documentIds): array + { + if ($documentIds === []) { + return []; + } + + $builder = $this->newBuilder($name); + $builder->select([Storage::UID]); + $builder->filter([BaseQuery::equal(Storage::UID, \array_values($documentIds))]); + + $stored = []; + foreach ($this->executeSelect($builder, Event::DocumentFind, $name) as $row) { + $id = $row[Storage::UID] ?? null; + if (\is_string($id)) { + $stored[$id] = true; + } + } + + return \array_values(\array_filter($documentIds, static fn (string $id): bool => ! isset($stored[$id]))); } /** @@ -3500,7 +3554,8 @@ protected function writeContext(array $skipPermissions = []): WriteContext * Does NOT call execute() - the caller is responsible for that. * * Floats are bound as 7.x bound them: a find() binds them in fixed-point notation, every other statement as - * PHP writes them, so a write keeps a magnitude fixed point would round to zero. + * PHP writes them, so a write keeps a magnitude fixed point would round to zero. A bulk delete binds them as a + * find() does, so its filters match the rows the find() of the documents it deletes matched. * * @param string $collection The collection the statement reads or writes, for the profiler * @return PDOStatement|DatabasePDOStatement|PDOStatementProxy @@ -3513,7 +3568,7 @@ protected function executeResult(Statement $result, ?Event $event = null, string if (\is_bool($value) && $this->supports(Capability::IntegerBooleans)) { $value = (int) $value; } - if (\is_float($value) && $event === Event::DocumentFind) { + if (\is_float($value) && ($event === Event::DocumentFind || $event === Event::DocumentsDelete)) { $prepared->bindValue($i + 1, $this->getFloatPrecision($value), PDO::PARAM_STR); } elseif (\is_float($value)) { $prepared->bindValue($i + 1, $value, PDO::PARAM_STR); diff --git a/src/Database/Exception/Contention.php b/src/Database/Exception/Contention.php index 5e0dd347b..8897dd66e 100644 --- a/src/Database/Exception/Contention.php +++ b/src/Database/Exception/Contention.php @@ -6,6 +6,10 @@ * Thrown when the engine aborts a statement over a lock conflict with a concurrent transaction: a deadlock, a lock * wait timeout, a serialization failure, a lock that is not available or a busy database. The engine may have rolled * the whole transaction back with it, so nothing the transaction wrote is stored and running it again can succeed. + * + * The library throws it too when a concurrent write changes what a transaction read before the transaction writes + * it, as when deleteDocuments() deletes fewer documents of a batch than still matched its filters; the transaction + * is rolled back, and running it again can succeed. */ class Contention extends Transaction { diff --git a/src/Database/Trait/Documents.php b/src/Database/Trait/Documents.php index 93da4a57e..88d4eb32d 100644 --- a/src/Database/Trait/Documents.php +++ b/src/Database/Trait/Documents.php @@ -26,6 +26,7 @@ use Utopia\Database\Exception as DatabaseException; use Utopia\Database\Exception\Authorization as AuthorizationException; use Utopia\Database\Exception\Conflict as ConflictException; +use Utopia\Database\Exception\Contention as ContentionException; use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Exception\Limit as LimitException; use Utopia\Database\Exception\NotFound as NotFoundException; @@ -43,6 +44,7 @@ use Utopia\Database\Id; use Utopia\Database\Operator; use Utopia\Database\OperatorType; +use Utopia\Database\ParsedQuery; use Utopia\Database\PermissionType; use Utopia\Database\Query; use Utopia\Database\RelationshipSide; @@ -3056,6 +3058,19 @@ private function dispatchDeleted(string $collection, Document $document, array $ /** * Deletes all documents which match the given queries, respecting relationship onDelete options. * + * Each batch deletes only the documents that still match the filters when it is deleted: a document another write + * changed after it was found, so that it no longer matches, is kept, and is neither counted, handed to $onNext nor + * cascaded. A limit counts the documents found, kept ones included. Without a transaction, as on a standalone + * MongoDB server, a document changed between the check and the delete is kept the same way, but its cascade may + * already have run, and a document another deleter removed meanwhile is counted as deleted by this call. + * + * Each batch commits on its own, so a batch that fails leaves the batches before it deleted; they were handed to + * $onNext and Event::DocumentsDelete reports them when the failure is a contention. In a transaction begun with + * startTransaction(), a batch that fails with a contention is retried in its savepoint, which on MySQL and MariaDB + * reads the transaction's snapshot again and fails the same way; a MongoDB replica set has no savepoints, so the + * batch is not retried and its partial delete stays in the transaction. Either way, roll the whole transaction + * back and run it again. + * * @param string $collection The collection identifier * @param array $queries Queries to filter documents for deletion * @param int $batchSize Number of documents per batch deletion, at most BATCH_SIZE @@ -3070,6 +3085,8 @@ private function dispatchDeleted(string $collection, Document $document, array $ * @throws LimitException When $batchSize is above BATCH_SIZE * @throws QueryException * @throws RestrictedException + * @throws ContentionException When documents keep changing between the check and the delete of a batch, in a + * transaction * @throws Throwable */ public function deleteDocuments( @@ -3115,6 +3132,7 @@ public function deleteDocuments( $originalLimit = $limit; $last = $cursor; + $found = 0; $modified = 0; while (true) { @@ -3146,78 +3164,251 @@ public function deleteDocuments( break; } - $sequences = []; - $permissionIds = []; - $cacheTarget = $collection->getId() === self::METADATA ? $batch : $collection->getId(); - $this->withMutation(Event::DocumentsDelete, $cacheTarget, function () use ($collection, $sequences, $permissionIds, $batch) { - foreach ($batch as $document) { - $seq = $document->getSequence(); - if ($seq !== null) { - $sequences[] = $seq; - } - if (! empty($document->getPermissions())) { - $permissionIds[] = $document->getId(); - } - - if ($this->relationshipHook?->isEnabled()) { - $this->silent(fn () => $this->relationshipHook->beforeDocumentDelete( - $collection, - $document - )); - } - - // Check if document was updated after the request timestamp - try { - $oldUpdatedAt = new PhpDateTime($document->getUpdatedAt() ?? 'now'); - } catch (Exception $e) { - throw new DatabaseException($e->getMessage(), $e->getCode(), $e); - } - - $requestTimestamp = $this->requestTimestamp()->get(); - if ($requestTimestamp !== null && $oldUpdatedAt > $requestTimestamp) { - throw new ConflictException('Document was updated after the request timestamp'); - } - } - - $this->adapter->deleteDocuments( - $collection, - $sequences, - $permissionIds - ); - - foreach ($batch as $document) { - $this->withDocumentTenant( - $document, - fn () => $this->advanceCollectionCacheEpoch($collection->getId(), $document->getId()) - ); + try { + $deleted = $this->withMutation(Event::DocumentsDelete, $cacheTarget, fn (): array => $this->deleteBatch($collection, $batch, $parsed)); + } catch (ContentionException $error) { + if ($modified > 0 && ! $this->adapter->inTransaction()) { + $this->announceBatchDeleted($collection, $modified); } - $this->queueDocumentPurges($collection->getId(), $batch); - }); + throw $error; + } - foreach ($batch as $document) { + foreach ($deleted as $document) { if ($onNext !== null) { $onNext($document, clone $document); } $modified++; } + $found += \count($batch); + if (count($batch) < $batchSize) { break; - } elseif ($originalLimit && $modified >= $originalLimit) { + } elseif ($originalLimit && $found >= $originalLimit) { break; } $last = \end($batch); } + $this->announceBatchDeleted($collection, $modified); + + return $modified; + } + + private function announceBatchDeleted(Collection $collection, int $modified): void + { $listeners = $this->listens(Event::DocumentsDelete); if ($listeners !== []) { $this->dispatch(new Event\Document\BatchDeleted($collection->getId(), $modified), $listeners); } + } - return $modified; + /** + * Delete the documents of a batch deleteDocuments() found that still match its filters, in the transaction the + * caller runs it in, and return them. + * + * A document whose filters are all on its immutable sequence still matches, so such a batch is not read again. + * The adapter's count is only a hint: when it is short, the batch's documents still stored are read, and one that + * is still stored changed between the read and the delete. In a transaction the batch then fails with a + * contention the transaction retries; without one, as on a standalone MongoDB server, the documents still stored + * are left out, a document another deleter removed meanwhile is reported as deleted by this call, and a cascade + * that already ran for a kept document stays, so cascades are best-effort there. + * + * @param array $batch + * @return array + * + * @throws ConflictException + * @throws ContentionException + * @throws DatabaseException + * @throws QueryException + * @throws TimeoutException + * @throws Exception + */ + private function deleteBatch(Collection $collection, array $batch, ParsedQuery $parsed): array + { + $filters = []; + $documents = self::onSequenceOnly($parsed->filters) ? $batch : $this->stillMatching($collection, $batch, $parsed, $filters); + + $sequences = []; + $permissionIds = []; + + foreach ($documents as $document) { + $sequence = $document->getSequence(); + if ($sequence !== null) { + $sequences[] = $sequence; + } + if (! empty($document->getPermissions())) { + $permissionIds[] = $document->getId(); + } + + if ($this->relationshipHook?->isEnabled()) { + $this->silent(fn () => $this->relationshipHook->beforeDocumentDelete( + $collection, + $document + )); + } + + try { + $oldUpdatedAt = new PhpDateTime($document->getUpdatedAt() ?? 'now'); + } catch (Exception $error) { + throw new DatabaseException($error->getMessage(), $error->getCode(), $error); + } + + $requestTimestamp = $this->requestTimestamp()->get(); + if ($requestTimestamp !== null && $oldUpdatedAt > $requestTimestamp) { + throw new ConflictException('Document was updated after the request timestamp'); + } + } + + if ($documents === []) { + return []; + } + + if ($filters === []) { + $this->adapter->deleteDocuments($collection, $sequences, $permissionIds); + } elseif ($this->adapter->deleteDocumentsMatching($collection, $sequences, $permissionIds, $filters) < \count($sequences)) { + $removed = $this->withoutStored($collection, $documents); + if (\count($removed) < \count($documents)) { + if ($this->adapter->inTransaction()) { + throw new ContentionException('Documents were changed while they were being deleted'); + } + + $documents = $removed; + } + } + + foreach ($documents as $document) { + $this->withDocumentTenant( + $document, + fn () => $this->advanceCollectionCacheEpoch($collection->getId(), $document->getId()) + ); + } + + $this->queueDocumentPurges($collection->getId(), $documents); + + return $documents; + } + + /** + * Whether every filter, and every filter nested in one, is on the sequence, which no write changes. + * + * @param array $filters + */ + private static function onSequenceOnly(array $filters): bool + { + foreach ($filters as $filter) { + if ($filter->isNested()) { + /** @var array $nested */ + $nested = $filter->getValues(); + if (! self::onSequenceOnly($nested)) { + return false; + } + } elseif ($filter->getAttribute() !== Document::SEQUENCE) { + return false; + } + } + + return true; + } + + /** + * @param array $documents + * @return list + * + * @throws DatabaseException + * @throws QueryException + * @throws TimeoutException + * @throws Exception + */ + private function withoutStored(Collection $collection, array $documents): array + { + $sequences = []; + foreach ($documents as $document) { + $sequences[] = (string) $document->getSequence(); + } + + $queries = [Query::equal(Document::SEQUENCE, $sequences), Query::limit(\count($sequences))]; + + /** @var array $stored */ + $stored = $this->silent(fn () => $this->skipQueryHooks(fn () => $this->skipValidation(fn () => $this->skipRelationships( + fn () => $this->authorization->skip(fn () => $this->fetch($collection, $queries, PermissionType::Delete, false)) + )))); + + $storedSequences = []; + foreach ($stored as $document) { + $storedSequences[(string) $document->getSequence()] = true; + } + + return \array_values(\array_filter( + $documents, + static fn (Document $document): bool => ! isset($storedSequences[(string) $document->getSequence()]), + )); + } + + /** + * The documents of a batch deleteDocuments() found that still match its queries, read again in the transaction + * that deletes them, in the order they were found, with the filters as the adapter applies them. + * + * @param array $batch + * @param array $filters + * @param-out array $filters + * @return list + * + * @throws DatabaseException + * @throws QueryException + * @throws TimeoutException + * @throws Exception + */ + private function stillMatching(Collection $collection, array $batch, ParsedQuery $parsed, array &$filters): array + { + $filters = []; + $sequences = []; + foreach ($batch as $document) { + $sequence = $document->getSequence(); + if ($sequence !== null) { + $sequences[] = $sequence; + } + } + + if ($sequences === []) { + return []; + } + + $queries = [ + Query::equal(Document::SEQUENCE, $sequences), + Query::limit(\count($sequences)), + ...$parsed->selections, + ...$parsed->filters, + ]; + + /** @var array{array, array|null} $read */ + $read = $this->silent(fn () => $this->skipQueryHooks(fn () => $this->skipValidation(function () use ($collection, $queries): array { + $documents = $this->fetch($collection, $queries, PermissionType::Delete, false, $adapterQueries); + + return [$documents, $adapterQueries]; + }))); + [$current, $adapterQueries] = $read; + + if ($adapterQueries !== null) { + $filters = Query::groupByType($adapterQueries)->filters; + } + + $matching = []; + foreach ($current as $document) { + $matching[(string) $document->getSequence()] = $document; + } + + $documents = []; + foreach ($sequences as $sequence) { + if (isset($matching[$sequence])) { + $documents[] = $matching[$sequence]; + } + } + + return $documents; } /** @@ -4001,6 +4192,9 @@ public function find(string $collection, array $queries = [], PermissionType $fo /** * @param array $queries * @param bool $aggregate Whether the queries must aggregate (aggregate()) or must not (find()) + * @param array|null $adapterQueries Receives the queries as the adapter's find() was handed them, or + * null when they can match no document and the adapter is not read + * @param-out array|null $adapterQueries * @return array * * @throws DatabaseException @@ -4008,8 +4202,10 @@ public function find(string $collection, array $queries = [], PermissionType $fo * @throws TimeoutException * @throws Exception */ - private function fetch(Collection $collection, array $queries, PermissionType $forPermission, bool $aggregate): array + private function fetch(Collection $collection, array $queries, PermissionType $forPermission, bool $aggregate, ?array &$adapterQueries = null): array { + $adapterQueries = null; + $queryCacheQueries = $queries; $this->checkQueryTypes($queries); @@ -4174,6 +4370,7 @@ private function fetch(Collection $collection, array $queries, PermissionType $f $results = []; } else { $queries = $convertedQueries; + $adapterQueries = $queries; $cacheEntry = null; $cacheGeneration = ''; diff --git a/tests/e2e/Adapter/Scopes/DocumentTests.php b/tests/e2e/Adapter/Scopes/DocumentTests.php index 6335b61bb..82d4ff312 100644 --- a/tests/e2e/Adapter/Scopes/DocumentTests.php +++ b/tests/e2e/Adapter/Scopes/DocumentTests.php @@ -8276,6 +8276,97 @@ public function testDeleteBulkDocumentsQueries(): void $database->deleteCollection('bulk_delete_queries'); } + public function testDeleteBulkDocumentsKeepsDocumentsThatStoppedMatching(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + $collectionId = 'bulk_delete_stopped_matching'; + $expired = '2000-01-01T00:00:00.000+00:00'; + $refreshed = '2100-01-01T00:00:00.000+00:00'; + $filter = Query::lessThan('expiresAt', '2050-01-01T00:00:00.000+00:00'); + + $database->createCollection(Collection::create(id: $collectionId, attributes: [ + Attribute::datetime(key: 'expiresAt', required: true), + ], permissions: [ + Permission::create(Role::any()), + Permission::read(Role::any()), + Permission::update(Role::any()), + Permission::delete(Role::any()), + ], documentSecurity: false)); + + foreach (['first' => $expired, 'second' => $expired, 'third' => $expired, 'fourth' => $refreshed] as $id => $expiresAt) { + $database->createDocument($collectionId, new Document([ + '$id' => $id, + '$permissions' => [Permission::read(Role::any())], + 'expiresAt' => $expiresAt, + ])); + } + + $found = $database->find($collectionId, [$filter]); + $this->assertCount(3, $found); + $database->updateDocument($collectionId, 'second', new Document(['expiresAt' => $refreshed])); + + $collection = $database->getCollection($collectionId); + $sequences = []; + $ids = []; + foreach ($found as $document) { + $sequence = $document->getSequence(); + $this->assertNotNull($sequence); + $sequences[] = $sequence; + $ids[] = $document->getId(); + } + + $deleted = $database->getAdapter()->deleteDocumentsMatching($collection, $sequences, $ids, $database->convertQueries($collection, [$filter])); + + $this->assertSame(2, $deleted); + $remaining = \array_map(static fn (Document $document): string => $document->getId(), $database->find($collectionId, [Query::orderAsc('$id')])); + $this->assertSame(['fourth', 'second'], $remaining); + $this->assertSame($refreshed, $database->getDocument($collectionId, 'second')->getAttribute('expiresAt')); + + $database->deleteCollection($collectionId); + } + + public function testDeleteBulkDocumentsWithAFloatFilterDeletesWhatTheFindMatched(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + $collectionId = 'bulk_delete_float_filter'; + $filter = Query::greaterThan('score', 1.0E-20); + + $database->createCollection(Collection::create(id: $collectionId, attributes: [ + Attribute::float(key: 'score', required: true), + ], permissions: [ + Permission::create(Role::any()), + Permission::read(Role::any()), + Permission::delete(Role::any()), + ], documentSecurity: false)); + + foreach (['tiny' => 1.0E-21, 'large' => 1.5, 'negative' => -1.0] as $id => $score) { + $database->createDocument($collectionId, new Document([ + '$id' => $id, + '$permissions' => [Permission::read(Role::any())], + 'score' => $score, + ])); + } + + $found = \array_map(static fn (Document $document): string => $document->getId(), $database->find($collectionId, [$filter])); + $deleted = []; + $count = $database->deleteDocuments($collectionId, [$filter], onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + \sort($found); + \sort($deleted); + $this->assertContains('large', $found); + $this->assertSame(\count($found), $count); + $this->assertSame($found, $deleted); + $remaining = \array_map(static fn (Document $document): string => $document->getId(), $database->find($collectionId)); + $this->assertSame([], \array_values(\array_intersect($found, $remaining))); + $this->assertContains('negative', $remaining); + + $database->deleteCollection($collectionId); + } + public function testDeleteBulkDocumentsWithCallbackSupport(): void { /** @var Database $database */ diff --git a/tests/unit/Documents/DeleteDocumentsRaceTest.php b/tests/unit/Documents/DeleteDocumentsRaceTest.php new file mode 100644 index 000000000..cdb79be50 --- /dev/null +++ b/tests/unit/Documents/DeleteDocumentsRaceTest.php @@ -0,0 +1,528 @@ + + */ + public static function adapters(): iterable + { + yield 'memory' => [static fn (): Memory => new Memory()]; + yield 'sqlite' => [static fn (): SQLite => SQLite::inMemory()]; + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testADocumentChangedAfterTheFindIsKept(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::REFRESHED]); + $engine->afterFind = $this->refreshOnce($database, 'b'); + + $deleted = []; + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()], onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + $this->assertSame(2, $count, 'The refreshed document is not counted'); + $this->assertSame(['a', 'c'], $deleted, 'The refreshed document is not handed to $onNext'); + $this->assertSame(['b', 'd'], $this->stored($database)); + $this->assertSame(self::REFRESHED, $this->expiry($database, 'b')); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testADocumentChangedAfterALaterBatchIsFoundIsKept(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::EXPIRED, 'e' => self::EXPIRED, 'f' => self::REFRESHED]); + $engine->afterFind = $this->refreshOnce($database, 'd'); + + $deleted = []; + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()], batchSize: 2, onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + $this->assertSame(4, $count); + $this->assertSame(['a', 'b', 'c', 'e'], $deleted); + $this->assertSame(['d', 'f'], $this->stored($database)); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testTheLimitCountsTheDocumentsFoundSoAKeptDocumentIsNotReplaced(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::EXPIRED, 'e' => self::EXPIRED]); + $engine->afterFind = $this->refreshOnce($database, 'b'); + + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired(), Query::limit(4)], batchSize: 2); + + $this->assertSame(3, $count); + $this->assertSame(['b', 'e'], $this->stored($database), 'Only the first four documents found are deleted, less the one refreshed'); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testACascadeSkipsTheChildrenOfADocumentChangedAfterTheFind(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $database->createCollection(Collection::create(self::CHILDREN, permissions: $this->permissions(), documentSecurity: false)); + $database->createRelationship(self::COLLECTION, Relationship::oneToMany(self::CHILDREN, key: 'sessions', twoWay: true, twoWayKey: 'presence', onDelete: RelationshipDeleteAction::Cascade)); + $database->createDocument(self::CHILDREN, new Document([Document::ID => 'a-session'])); + $database->createDocument(self::CHILDREN, new Document([Document::ID => 'b-session'])); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED], ['a' => ['a-session'], 'b' => ['b-session']]); + $engine->afterFind = $this->refreshOnce($database, 'b'); + + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()]); + + $this->assertSame(1, $count); + $this->assertSame(['b'], $this->stored($database)); + $this->assertSame(['b-session'], $this->stored($database, self::CHILDREN), 'The refreshed document keeps its children'); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testADocumentChangedRightBeforeTheDeleteRollsTheBatchBack(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED]); + $engine->beforeDelete = function (Document $collection) use ($database): void { + if ($collection->getId() === self::COLLECTION) { + $database->updateDocument(self::COLLECTION, 'b', new Document(['expiresAt' => self::REFRESHED])); + } + }; + + try { + $database->deleteDocuments(self::COLLECTION, [$this->expired()]); + $this->fail('A document that stops matching between the check and the delete must fail the batch'); + } catch (ContentionException) { + } + + $this->assertSame(3, $engine->deletes, 'The batch is tried again before it fails'); + $this->assertSame(['a', 'b', 'c'], $this->stored($database), 'Nothing of the failed batch is stored'); + } + + public function testADocumentChangedRightBeforeTheDeleteIsKeptWhenTheRetryReadsItAgain(): void + { + $engine = new Memory(); + [$database] = $this->database($engine); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED]); + $engine->beforeDelete = function (Document $collection) use ($database, $engine): void { + if ($collection->getId() === self::COLLECTION && $engine->deletes === 1) { + $engine->detached(fn (): Document => $database->updateDocument(self::COLLECTION, 'b', new Document(['expiresAt' => self::REFRESHED]))); + } + }; + + $deleted = []; + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()], onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + $this->assertSame(2, $engine->deletes, 'The batch is rolled back once and deleted on the retry'); + $this->assertSame(2, $count, 'Only the retry that committed is counted'); + $this->assertSame(['a', 'c'], $deleted, 'Each deleted document is handed to $onNext once'); + $this->assertSame(['b'], $this->stored($database)); + $this->assertSame(self::REFRESHED, $this->expiry($database, 'b')); + } + + public function testAContentionInALaterBatchReportsTheBatchesAlreadyDeleted(): void + { + [$database, $engine] = $this->database(new Memory()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::EXPIRED]); + $engine->beforeDelete = function (Document $collection) use ($database, $engine): void { + if ($collection->getId() === self::COLLECTION && $engine->deletes > 1) { + $database->updateDocument(self::COLLECTION, 'd', new Document(['expiresAt' => self::REFRESHED])); + } + }; + $events = $this->batchEvents($database); + + $deleted = []; + try { + $database->deleteDocuments(self::COLLECTION, [$this->expired()], batchSize: 2, onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + $this->fail('The second batch must fail with a contention'); + } catch (ContentionException) { + } + + $this->assertSame(['a', 'b'], $deleted); + $this->assertSame(['c', 'd'], $this->stored($database), 'The first batch stays deleted'); + $this->assertSame([2], $events->counts, 'The deleted batch is reported'); + } + + public function testWithoutATransactionADocumentChangedRightBeforeTheDeleteIsKeptAndTheRestReported(): void + { + [$database, $engine] = $this->database(new Standalone()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::EXPIRED, 'e' => self::REFRESHED]); + $engine->beforeDelete = function (Document $collection) use ($database, $engine): void { + if ($collection->getId() === self::COLLECTION && $engine->deletes === 1) { + $database->updateDocument(self::COLLECTION, 'b', new Document(['expiresAt' => self::REFRESHED])); + } + }; + $events = $this->batchEvents($database); + + $deleted = []; + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()], batchSize: 2, onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + $this->assertSame(3, $count, 'The document kept is not counted, and the next batch is still deleted'); + $this->assertSame(['a', 'c', 'd'], $deleted, 'Every deleted document is handed to $onNext'); + $this->assertSame([3], $events->counts); + $this->assertSame(['b', 'e'], $this->stored($database)); + $this->assertSame(['b', 'e'], \array_map(static fn (Document $document): string => $document->getId(), $database->find(self::COLLECTION)), 'The kept document keeps its permission rows'); + } + + /** + * @return iterable}> + */ + public static function floatFinds(): iterable + { + yield 'memory compares exactly' => [static fn (): Memory => new Memory(), ['large']]; + yield 'sqlite binds fixed point' => [static fn (): SQLite => SQLite::inMemory(), ['large', 'tiny']]; + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + * @param list $expected + */ + #[DataProvider('floatFinds')] + public function testAFloatFilterDeletesTheDocumentsTheFindMatched(Closure $adapter, array $expected): void + { + [$database] = $this->database($adapter()); + $database->createCollection(Collection::create( + id: 'scores', + attributes: [Attribute::float(key: 'score')], + permissions: $this->permissions(), + documentSecurity: false, + )); + foreach (['tiny' => 1.0E-21, 'large' => 1.5, 'negative' => -1.0] as $id => $score) { + $database->createDocument('scores', new Document([Document::ID => $id, 'score' => $score])); + } + $filter = Query::greaterThan('score', 1.0E-20); + + $found = \array_map(static fn (Document $document): string => $document->getId(), $database->find('scores', [$filter])); + \sort($found); + $this->assertSame($expected, $found); + + $deleted = []; + $count = $database->deleteDocuments('scores', [$filter], onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + \sort($deleted); + + $this->assertSame(\count($found), $count, 'The delete binds a float as the find does'); + $this->assertSame($found, $deleted); + $remaining = \array_map(static fn (Document $document): string => $document->getId(), $database->find('scores')); + $this->assertSame([], \array_values(\array_intersect($found, $remaining))); + $this->assertContains('negative', $remaining); + } + + public function testAPoolSubclassThatWrapsDeleteDocumentsRunsForAFilteredBatch(): void + { + $engine = new Memory(); + $pool = new AuditingPool(new UtopiaPool(new Stack(), 'race', 2, static fn (): Memory => $engine, timeout: 0.0)); + [$database] = $this->database($pool); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::REFRESHED]); + $engine->afterFind = $this->refreshOnce($database, 'b'); + + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()]); + + $this->assertSame(1, $pool->deletes, 'The pool\'s override of deleteDocuments() runs'); + $this->assertSame(2, $count); + $this->assertSame(['b', 'd'], $this->stored($database), 'The pooled adapter is handed the filters'); + } + + /** + * @return iterable): int}> + */ + public static function miscounts(): iterable + { + yield 'none' => [static fn (int $deleted, array $permissionIds): int => 0]; + yield 'permission rows too' => [static fn (int $deleted, array $permissionIds): int => $deleted + \count($permissionIds)]; + } + + /** + * @param Closure(int, array): int $report + */ + #[DataProvider('miscounts')] + public function testACountTheAdapterGetsWrongDoesNotFailADeleteWithoutARace(Closure $report): void + { + [$database] = $this->database(new Miscounting($report)); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::REFRESHED]); + + $deleted = []; + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()], batchSize: 2, onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + $this->assertSame(3, $count); + $this->assertSame(['a', 'b', 'c'], $deleted); + $this->assertSame(['d'], $this->stored($database)); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testABatchFilteredOnlyOnTheSequenceIsNotReadAgain(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::REFRESHED, 'c' => self::EXPIRED]); + $sequences = $database->getAuthorization()->skip(fn (): array => \array_map( + static fn (Document $document): string => (string) $document->getSequence(), + $database->find(self::COLLECTION, [Query::equal(Document::ID, ['a', 'c'])]), + )); + $finds = $engine->finds; + + $this->assertSame(2, $database->deleteDocuments(self::COLLECTION, [Query::equal(Document::SEQUENCE, $sequences)], batchSize: 2)); + $this->assertSame(2, $engine->finds - $finds, 'Each batch is found once and not read again'); + $this->assertSame(['b'], $this->stored($database)); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testTheAdapterDeletesOnlyTheRowsThatStillMatchAndKeepsTheirPermissions(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::REFRESHED]); + $found = $database->find(self::COLLECTION, [$this->expired()]); + $database->updateDocument(self::COLLECTION, 'b', new Document(['expiresAt' => self::REFRESHED])); + + $collection = $database->getCollection(self::COLLECTION); + $sequences = \array_values(\array_filter(\array_map(static fn (Document $document): ?string => $document->getSequence(), $found))); + $permissionIds = \array_map(static fn (Document $document): string => $document->getId(), $found); + + $count = $engine->deleteDocumentsMatching($collection, $sequences, $permissionIds, $database->convertQueries($collection, [$this->expired()])); + + $this->assertSame(2, $count); + $this->assertSame(['b', 'd'], $this->stored($database)); + $this->assertSame(['b', 'd'], \array_map(static fn (Document $document): string => $document->getId(), $database->find(self::COLLECTION)), 'The kept row keeps its permission rows'); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testADeleteWithNoRaceDeletesEveryMatchingDocumentAndItsPermissions(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::EXPIRED, 'c' => self::EXPIRED, 'd' => self::REFRESHED]); + + $deleted = []; + $count = $database->deleteDocuments(self::COLLECTION, [$this->expired()], batchSize: 2, onNext: function (Document $document) use (&$deleted): void { + $deleted[] = $document->getId(); + }); + + $this->assertSame(3, $count); + $this->assertSame(['a', 'b', 'c'], $deleted); + $this->assertSame(2, $engine->deletes); + $this->assertSame(['d'], $this->stored($database)); + + $database->getAuthorization()->skip(fn (): Document => $database->createDocument(self::COLLECTION, new Document([ + Document::ID => 'a', + Document::PERMISSIONS => [], + 'expiresAt' => self::EXPIRED, + ]))); + $this->assertSame(['d'], \array_map(static fn (Document $document): string => $document->getId(), $database->find(self::COLLECTION)), 'A deleted document leaves no permission rows behind'); + } + + /** + * @param Closure(): (Memory|SQLite) $adapter + */ + #[DataProvider('adapters')] + public function testADeleteWithoutFiltersDeletesEveryDocument(Closure $adapter): void + { + [$database, $engine] = $this->database($adapter()); + $this->seed($database, ['a' => self::EXPIRED, 'b' => self::REFRESHED, 'c' => self::EXPIRED]); + $finds = $engine->finds; + + $this->assertSame(3, $database->deleteDocuments(self::COLLECTION, batchSize: 2)); + $this->assertSame(2, $engine->finds - $finds, 'Each batch is found once and not read again'); + $this->assertSame([], $this->stored($database)); + $this->assertSame(2, $engine->deletes); + } + + /** + * @template T of Adapter + * + * @param T $adapter + * @return array{Database, T} + */ + private function database(Adapter $adapter): array + { + $authorization = new Authorization(); + $authorization->addRole(self::OWNER); + + $database = new Database($adapter, new Cache(new None())); + $database + ->setAuthorization($authorization) + ->setDatabase('race') + ->setNamespace('race_'.\uniqid()); + $database->create(); + $database->addHook(new Relationships()); + $database->addHook(new Permissions()); + + $database->createCollection(Collection::create( + id: self::COLLECTION, + attributes: [Attribute::datetime(key: 'expiresAt')], + permissions: [Permission::create(Role::any())], + documentSecurity: true, + )); + + return [$database, $adapter]; + } + + /** + * @return object{counts: list} + */ + private function batchEvents(Database $database): object + { + $events = new class () implements Lifecycle { + /** @var list */ + public array $counts = []; + + #[\Override] + public function handle(Domain $event): void + { + if ($event instanceof BatchDeleted) { + $this->counts[] = $event->count; + } + } + }; + $database->addHook($events); + + return $events; + } + + /** + * @return list + */ + private function permissions(): array + { + return [ + Permission::create(Role::any()), + Permission::read(Role::any()), + Permission::update(Role::any()), + Permission::delete(Role::any()), + ]; + } + + /** + * @param array $expiries + * @param array> $sessions + */ + private function seed(Database $database, array $expiries, array $sessions = []): void + { + foreach ($expiries as $id => $expiresAt) { + $attributes = [ + Document::ID => $id, + Document::PERMISSIONS => [ + Permission::read(Role::user('owner')), + Permission::update(Role::user('owner')), + Permission::delete(Role::user('owner')), + ], + 'expiresAt' => $expiresAt, + ]; + if (isset($sessions[$id])) { + $attributes['sessions'] = $sessions[$id]; + } + $database->createDocument(self::COLLECTION, new Document($attributes)); + } + } + + /** + * @return Closure(Document, array): void + */ + private function refreshOnce(Database $database, string $id): Closure + { + return (new Refresh($database, self::COLLECTION, $id, ['expiresAt' => self::REFRESHED]))(...); + } + + private function expired(): Query + { + return Query::lessThan('expiresAt', self::NOW); + } + + /** + * @return list + */ + private function stored(Database $database, string $collection = self::COLLECTION): array + { + $ids = $database->getAuthorization()->skip(fn (): array => $database->skipRelationships(fn (): array => \array_map( + static fn (Document $document): string => $document->getId(), + $database->find($collection), + ))); + \sort($ids); + + return $ids; + } + + private function expiry(Database $database, string $id): mixed + { + return $database->getAuthorization()->skip(fn (): Document => $database->getDocument(self::COLLECTION, $id))->getAttribute('expiresAt'); + } +} diff --git a/tests/unit/Documents/Interleaving/AuditingPool.php b/tests/unit/Documents/Interleaving/AuditingPool.php new file mode 100644 index 000000000..648078426 --- /dev/null +++ b/tests/unit/Documents/Interleaving/AuditingPool.php @@ -0,0 +1,22 @@ +deletes++; + + return parent::deleteDocuments($collection, $sequences, $permissionIds); + } +} diff --git a/tests/unit/Documents/Interleaving/Interleaves.php b/tests/unit/Documents/Interleaving/Interleaves.php new file mode 100644 index 000000000..c564b20a4 --- /dev/null +++ b/tests/unit/Documents/Interleaving/Interleaves.php @@ -0,0 +1,55 @@ +): void)|null + */ + public ?Closure $afterFind = null; + + /** + * @var (Closure(Document): void)|null + */ + public ?Closure $beforeDelete = null; + + public int $finds = 0; + + public int $deletes = 0; + + #[\Override] + public function find(Document $collection, array $queries = [], ?int $limit = 25, ?int $offset = null, array $orderAttributes = [], array $orderTypes = [], array $cursor = [], CursorDirection $cursorDirection = CursorDirection::After, PermissionType $forPermission = PermissionType::Read): array + { + $this->finds++; + + $documents = parent::find($collection, $queries, $limit, $offset, $orderAttributes, $orderTypes, $cursor, $cursorDirection, $forPermission); + + if ($this->afterFind !== null) { + ($this->afterFind)($collection, $documents); + } + + return $documents; + } + + #[\Override] + public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int + { + $this->deletes++; + + if ($this->beforeDelete !== null) { + ($this->beforeDelete)($collection); + } + + return parent::deleteDocuments($collection, $sequences, $permissionIds); + } +} diff --git a/tests/unit/Documents/Interleaving/Memory.php b/tests/unit/Documents/Interleaving/Memory.php new file mode 100644 index 000000000..b9cfb1115 --- /dev/null +++ b/tests/unit/Documents/Interleaving/Memory.php @@ -0,0 +1,32 @@ +inTransaction; + $journals = $this->journals; + $this->inTransaction = 0; + $this->journals = []; + + try { + $write(); + } finally { + $this->inTransaction = $depth; + $this->journals = $journals; + } + } +} diff --git a/tests/unit/Documents/Interleaving/Miscounting.php b/tests/unit/Documents/Interleaving/Miscounting.php new file mode 100644 index 000000000..66e5a85c4 --- /dev/null +++ b/tests/unit/Documents/Interleaving/Miscounting.php @@ -0,0 +1,27 @@ + $permissionIds): int $report + */ + public function __construct(private readonly Closure $report) + { + parent::__construct(); + } + + #[\Override] + public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int + { + return ($this->report)(parent::deleteDocuments($collection, $sequences, $permissionIds), $permissionIds); + } +} diff --git a/tests/unit/Documents/Interleaving/Refresh.php b/tests/unit/Documents/Interleaving/Refresh.php new file mode 100644 index 000000000..27833d8e5 --- /dev/null +++ b/tests/unit/Documents/Interleaving/Refresh.php @@ -0,0 +1,44 @@ + $changes + */ + public function __construct( + private readonly Database $database, + private readonly string $collection, + private readonly string $id, + private readonly array $changes, + ) { + } + + /** + * @param array $documents + */ + public function __invoke(Document $collection, array $documents): void + { + if ($this->done || $collection->getId() !== $this->collection) { + return; + } + + foreach ($documents as $document) { + if ($document->getId() === $this->id) { + $this->done = true; + $this->database->updateDocument($this->collection, $this->id, new Document($this->changes)); + + return; + } + } + } +} diff --git a/tests/unit/Documents/Interleaving/SQLite.php b/tests/unit/Documents/Interleaving/SQLite.php new file mode 100644 index 000000000..267d15de6 --- /dev/null +++ b/tests/unit/Documents/Interleaving/SQLite.php @@ -0,0 +1,16 @@ +find([$filter]); + $this->createAdapter()->deleteDocumentsMatching(new Document(['$id' => self::COLLECTION]), ['1'], [], [$filter]); + + $found = $this->calls['find'][0]['$and'] ?? null; + + $deleted = $this->calls['delete'][0]['$and'] ?? null; + + $this->assertNotNull($found); + $this->assertIsArray($deleted); + $this->assertSame(self::values(['$and' => $found]), self::values($deleted[1] ?? null), 'The delete must match the rows the find matched'); + } + public function testCountRethrowsDriverErrors(): void { $this->aggregateError = new MongoException('invalid pipeline', 2); @@ -311,6 +326,27 @@ private function find(array $queries): array return $this->createAdapter()->find(new Document(['$id' => self::COLLECTION]), $queries); } + /** + * The filter with every object, such as a MongoDB\BSON\UTCDateTime, replaced by its class and value, so that two + * filters built separately compare by value. + */ + private static function values(mixed $filter): mixed + { + if (\is_array($filter)) { + return \array_map(self::values(...), $filter); + } + + if ($filter instanceof \Stringable) { + return [$filter::class => (string) $filter]; + } + + if (\is_object($filter)) { + return [$filter::class => self::values(\get_object_vars($filter))]; + } + + return $filter; + } + /** * @param array $filter * @return list @@ -432,6 +468,19 @@ public function update(string $collection, array $where = [], array $updates = [ return 0; } + /** + * @param array $filters + * @param array $deleteOptions + * @param array $options + */ + #[\Override] + public function delete(string $collection, array $filters = [], int $limit = 1, array $deleteOptions = [], array $options = []): int + { + ($this->record)('delete', $filters); + + return 0; + } + /** * @param array $indexes * @param array $options