fix(documents): 8.1.4 delete only documents that still match in deleteDocuments

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

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

src/Database/Adapter.php

@@ -42,6 +42,9 @@ abstract class Adapter
     /** @var Value<bool>|null */
     private ?Value $ignoringDuplicates = null;
 
+    /** @var Value<array<Query>>|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<string>  $sequences
      * @param  array<string>  $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<string>  $sequences
+     * @param  array<string>  $permissionIds
+     * @param  array<Query>  $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<Query>
+     */
+    protected function deletionFilters(): array
+    {
+        return $this->deletionFiltersValue()->get();
+    }
+
+    /**
+     * @return Value<array<Query>>
+     */
+    private function deletionFiltersValue(): Value
+    {
+        /** @var array<Query> $none */
+        $none = [];
+
+        return $this->deletionFilters ??= new Value($none);
+    }
+
     /**
      * Find Documents
      *

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<string>  $sequences
+     * @param  array<string>  $permissionIds
+     */
     #[\Override]
     public function deleteDocuments(Document $collection, array $sequences, array $permissionIds): int
     {
@@ -1734,52 +1740,59 @@ 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.
             if ($this->sharedTables && ($row[Storage::TENANT] ?? null) !== $this->getTenant()) {
                 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;
                         });
                     }
                 }
                 $count++;
             }
         }
 
-        $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<string, mixed>  $row
+     * @param  array<Query>  $queries
+     */
+    private function matchesAll(array $row, array $queries): bool
+    {
+        foreach ($queries as $query) {
+            if (! $this->matches($row, $query)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
     /**
      * @param  array<string, mixed>  $row
      */

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<string>  $sequences
      * @param  array<string>  $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<string, mixed> $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();

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;
     }