From e662482225e3bcaa5f517f28b07bf138250a67c1 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:43:51 +1300 Subject: [PATCH 01/11] fix(adapter): keep storage columns out of document writes that skip validation filter() keeps underscores, so with validation skipped a document key such as `_uid` or `_u!id` reached the column the storage keeps for the document's own id and rewrote it on create or update, `_id` chose the sequence, and a bulk update with `_permissions` replaced the document's permissions. An upsert also let `_ten!ant` move a new row to another tenant, since the filtered key overwrote the tenant the write hooks set. Every SQL and Memory write now takes the document's attributes through writableAttributes(), which drops a key whose filtered name is a reserved storage column before operators are extracted, matching how Mongo drops storage keys rather than refusing the write. An upsert splits each change's writable attributes into operators and updates once and hands the result to executeUpsertBatch, rather than extracting them up to three times per change. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter.php | 22 ++--- src/Database/Adapter/MariaDB.php | 24 ++--- src/Database/Adapter/Memory.php | 52 +++++------ src/Database/Adapter/Postgres.php | 4 +- src/Database/Adapter/SQL.php | 61 ++++++------- src/Database/Adapter/SQLite.php | 26 +++--- tests/e2e/Adapter/Scopes/DocumentTests.php | 100 +++++++++++++++++++++ tests/unit/WritableAttributesTest.php | 41 +++++++++ 8 files changed, 229 insertions(+), 101 deletions(-) create mode 100644 tests/unit/WritableAttributesTest.php diff --git a/src/Database/Adapter.php b/src/Database/Adapter.php index ab968f0f0..9f6291daf 100644 --- a/src/Database/Adapter.php +++ b/src/Database/Adapter.php @@ -958,12 +958,6 @@ protected function getInternalKeyForAttribute(string $attribute): string } /** - * Process-lifetime cache for {@see self::filter()}. Keys are referentially - * stable across the request lifetime and frequently re-queried per-row, so - * caching the regex result amortizes the preg_replace cost across all - * decode/encode/build passes. Bounded to avoid unbounded growth from - * unusual input. - * * @var array */ private static array $filteredKeyCache = []; @@ -995,10 +989,6 @@ public function filter(string $value): string } /** - * The column an attribute a caller names is written to. filter() keeps underscores, so a name such as `_tenant` - * or `_ten.ant` would reach a column the storage keeps for the document itself and move the document to another - * tenant, or rewrite its id or timestamps. - * * @throws NotFoundException When the attribute names a reserved column */ protected function writableColumn(string $attribute): string @@ -1012,6 +1002,18 @@ protected function writableColumn(string $attribute): string return $column; } + /** + * @return array + */ + protected function writableAttributes(Document $document): array + { + return \array_filter( + $document->getAttributes(), + fn (int|string $key): bool => ! Storage::isReserved($this->filter((string) $key)), + \ARRAY_FILTER_USE_KEY + ); + } + /** * The row as every write hook decorates a row written for the document. * diff --git a/src/Database/Adapter/MariaDB.php b/src/Database/Adapter/MariaDB.php index 7b86d6d00..cc1a825b4 100644 --- a/src/Database/Adapter/MariaDB.php +++ b/src/Database/Adapter/MariaDB.php @@ -623,7 +623,7 @@ public function createDocument(Document $collection, Document $document): Docume $spatialAttributes = $this->getSpatialAttributes($collection); $collection = $collection->getId(); - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $attributes[Storage::CREATED_AT] = $document->getCreatedAt(); $attributes[Storage::UPDATED_AT] = $document->getUpdatedAt(); $attributes[Storage::PERMISSIONS] = \json_encode($document->getPermissions()); @@ -671,27 +671,27 @@ public function createDocument(Document $collection, Document $document): Docume $context = $this->writeContext(); try { $this->runWriteHooks(fn ($hook) => $hook->afterDocumentCreate($name, [$document], $context)); - } catch (PDOException $e) { - $isOrphanedPermission = $e->getCode() === '23000' - && isset($e->errorInfo[1]) - && $e->errorInfo[1] === 1062 - && \str_contains($e->getMessage(), Storage::INDEX_1); + } catch (PDOException $exception) { + $isOrphanedPermission = $exception->getCode() === '23000' + && isset($exception->errorInfo[1]) + && $exception->errorInfo[1] === 1062 + && \str_contains($exception->getMessage(), Storage::INDEX_1); if (! $isOrphanedPermission) { - throw $e; + throw $exception; } // Clean up orphaned permissions from a previous failed delete, then retry $cleanupBuilder = $this->newBuilder(Storage::permissionsTable($name)); $cleanupBuilder->filter([BaseQuery::equal(Storage::PERMISSIONS_DOCUMENT, [$document->getId()])]); $cleanupResult = $cleanupBuilder->delete(); - $cleanupStmt = $this->executeResult($cleanupResult, Event::PermissionsDelete); - $this->execute($cleanupStmt); + $cleanupStatement = $this->executeResult($cleanupResult, Event::PermissionsDelete); + $this->execute($cleanupStatement); $this->runWriteHooks(fn ($hook) => $hook->afterDocumentCreate($name, [$document], $context)); } - } catch (PDOException $e) { - throw $this->processException($e); + } catch (PDOException $exception) { + throw $this->processException($exception); } return $document; @@ -711,7 +711,7 @@ public function updateDocument(Document $collection, string $id, Document $docum $spatialAttributes = $this->getSpatialAttributes($collection); $collection = $collection->getId(); - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $attributes[Storage::CREATED_AT] = $document->getCreatedAt(); $attributes[Storage::UPDATED_AT] = $document->getUpdatedAt(); $attributes[Storage::PERMISSIONS] = json_encode($document->getPermissions()); diff --git a/src/Database/Adapter/Memory.php b/src/Database/Adapter/Memory.php index 9ead7a952..af8760bbd 100644 --- a/src/Database/Adapter/Memory.php +++ b/src/Database/Adapter/Memory.php @@ -1391,10 +1391,7 @@ public function updateDocument(Document $collection, string $id, Document $docum } [$oldKey, $existing] = $located; - // Resolve any Operator-typed attributes against the existing row before - // computing the new payload so unique-index checks see the post-update - // values, matching MariaDB's atomic UPDATE semantics. - $resolved = $this->applyOperators($document->getAttributes(), $existing); + $resolved = $this->applyOperators($this->writableAttributes($document), $existing); foreach ($resolved as $attribute => $value) { $document->setAttribute($attribute, $value); } @@ -1535,7 +1532,7 @@ public function updateDocuments(Document $collection, Document $updates, array $ throw new NotFoundException('Collection not found'); } - $attributes = $updates->getAttributes(); + $attributes = $this->writableAttributes($updates); $hasCreatedAt = ! empty($updates->getCreatedAt()); $hasUpdatedAt = ! empty($updates->getUpdatedAt()); $hasPermissions = $updates->offsetExists(Document::PERMISSIONS); @@ -1547,14 +1544,14 @@ public function updateDocuments(Document $collection, Document $updates, array $ // before any write lands, so a uniqueness violation on document N // does not leave documents 0..N-1 partially committed. $prepared = []; - foreach ($documents as $doc) { - $uid = $doc->getId(); - $docKey = $this->documentKey($uid); - if (! isset($this->data[$key]['documents'][$docKey])) { + foreach ($documents as $document) { + $uid = $document->getId(); + $documentKey = $this->documentKey($uid); + if (! isset($this->data[$key]['documents'][$documentKey])) { continue; } - $existingRow = $this->data[$key]['documents'][$docKey]; + $existingRow = $this->data[$key]['documents'][$documentKey]; // Resolve operators per-row — each document's existing values feed // back into operator evaluation, so $attributes cannot be evaluated @@ -1574,8 +1571,8 @@ public function updateDocuments(Document $collection, Document $updates, array $ $prepared[] = [ 'uid' => $uid, - 'docKey' => $docKey, - 'attrs' => $resolved, + 'documentKey' => $documentKey, + 'attributes' => $resolved, 'newSignatures' => $newSignatures, 'oldSignatures' => $oldSignatures, ]; @@ -1583,20 +1580,20 @@ public function updateDocuments(Document $collection, Document $updates, array $ // Phase-1: hashed unique-index validation. For each pending row, probe // the hash table — but recognise that any hashed entry currently - // pointing at the row's own docKey is about to be replaced by the new + // pointing at the row's own document key is about to be replaced by the new // signature, so it is not a conflict. We also build a per-batch // pending map so two siblings collapsing to the same value collide. $pendingByIndex = []; foreach ($prepared as $entry) { - $docKey = $entry['docKey']; + $documentKey = $entry['documentKey']; foreach ($entry['newSignatures'] as $indexId => $hash) { $existing = $this->uniqueIndexHashes[$key][$indexId][$hash] ?? null; - if ($existing !== null && $existing !== $docKey) { + if ($existing !== null && $existing !== $documentKey) { // The entry's own pre-update hash binding does not count // as a conflict — it'll be removed when its row is rewritten. $existingIsSelf = false; foreach ($prepared as $other) { - if ($other['docKey'] === $existing && ($other['oldSignatures'][$indexId] ?? null) === $hash) { + if ($other['documentKey'] === $existing && ($other['oldSignatures'][$indexId] ?? null) === $hash) { $existingIsSelf = true; break; } @@ -1605,22 +1602,22 @@ public function updateDocuments(Document $collection, Document $updates, array $ throw new UniqueException(UniqueException::MESSAGE); } } - if (isset($pendingByIndex[$indexId][$hash]) && $pendingByIndex[$indexId][$hash] !== $docKey) { + if (isset($pendingByIndex[$indexId][$hash]) && $pendingByIndex[$indexId][$hash] !== $documentKey) { throw new UniqueException(UniqueException::MESSAGE); } - $pendingByIndex[$indexId][$hash] = $docKey; + $pendingByIndex[$indexId][$hash] = $documentKey; } } $tenant = $this->getTenant(); foreach ($prepared as $entry) { $uid = $entry['uid']; - $docKey = $entry['docKey']; - $resolved = $entry['attrs']; + $documentKey = $entry['documentKey']; + $resolved = $entry['attributes']; - $previousRow = $this->data[$key]['documents'][$docKey]; + $previousRow = $this->data[$key]['documents'][$documentKey]; - $row = &$this->data[$key]['documents'][$docKey]; + $row = &$this->data[$key]['documents'][$documentKey]; foreach ($resolved as $attribute => $value) { $row[$this->filter($attribute)] = $value; } @@ -1636,8 +1633,8 @@ public function updateDocuments(Document $collection, Document $updates, array $ } unset($row); - $this->journal(function () use ($key, $docKey, $previousRow): void { - $this->data[$key]['documents'][$docKey] = $previousRow; + $this->journal(function () use ($key, $documentKey, $previousRow): void { + $this->data[$key]['documents'][$documentKey] = $previousRow; }); if ($hasPermissions) { @@ -1656,7 +1653,7 @@ public function updateDocuments(Document $collection, Document $updates, array $ $indexId, $entry['newSignatures'][$indexId] ?? null, $entry['oldSignatures'][$indexId] ?? null, - $docKey, + $documentKey, ); } } @@ -2026,10 +2023,7 @@ public function getAttributeWidth(Document $collection): int protected function documentToRow(Document $document): array { $row = []; - foreach ($document->getAttributes() as $attribute => $value) { - // Store native PHP values directly — no JSON encoding for arrays. - // The Database casting layer accepts already-decoded arrays - // (decodeArrayValue / decodeObjectValue both pass through arrays). + foreach ($this->writableAttributes($document) as $attribute => $value) { $row[$this->filter($attribute)] = $value; } diff --git a/src/Database/Adapter/Postgres.php b/src/Database/Adapter/Postgres.php index 162cfdb1f..7c7b62842 100644 --- a/src/Database/Adapter/Postgres.php +++ b/src/Database/Adapter/Postgres.php @@ -1106,7 +1106,7 @@ public function createDocument(Document $collection, Document $document): Docume $spatialAttributes = $this->getSpatialAttributes($collection); $collection = $collection->getId(); - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $attributes[Storage::CREATED_AT] = $document->getCreatedAt(); $attributes[Storage::UPDATED_AT] = $document->getUpdatedAt(); $attributes[Storage::PERMISSIONS] = \json_encode($document->getPermissions()); @@ -1171,7 +1171,7 @@ public function updateDocument(Document $collection, string $id, Document $docum $spatialAttributes = $this->getSpatialAttributes($collection); $collection = $collection->getId(); - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $attributes[Storage::CREATED_AT] = $document->getCreatedAt(); $attributes[Storage::UPDATED_AT] = $document->getUpdatedAt(); $attributes[Storage::PERMISSIONS] = \json_encode($document->getPermissions()); diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index a80614295..994b688f6 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -908,7 +908,7 @@ private function buildDocumentsInsert(string $name, array $documents, array $spa } foreach ($documents as $document) { - foreach ($document->getAttributes() as $key => $value) { + foreach ($this->writableAttributes($document) as $key => $value) { $attributeKeySet[$key] = true; } } @@ -1134,7 +1134,7 @@ public function updateDocuments(Document $collection, Document $updates, array $ $spatialAttributes = $this->getSpatialAttributes($collection); $collection = $collection->getId(); - $attributes = $updates->getAttributes(); + $attributes = $this->writableAttributes($updates); if (! empty($updates->getUpdatedAt())) { $attributes[Storage::UPDATED_AT] = $updates->getUpdatedAt(); @@ -1156,9 +1156,6 @@ public function updateDocuments(Document $collection, Document $updates, array $ $builder = $this->newBuilder($name); - // Single pass over update attributes, bucketing into regular / spatial / - // operator and applying JSON / boolean conversions inline. Hoisted - // guards keep the hot path branch-light. $spatialMap = \array_fill_keys($spatialAttributes, true); $intBools = $this->supports(Capability::IntegerBooleans); @@ -1262,33 +1259,25 @@ public function upsertDocuments(Document $collection, array $changes, ?string $i $collection = $collection->getId(); $name = $this->filter($collection); + $extractions = \array_map( + fn (Change $change): array => Operator::extractOperators($this->writableAttributes($change->new)), + $changes + ); $hasOperators = false; - $firstChange = $changes[0]; - $firstDoc = $firstChange->new; - $firstExtracted = Operator::extractOperators($firstDoc->getAttributes()); - - if (! empty($firstExtracted['operators'])) { - $hasOperators = true; - } else { - foreach ($changes as $change) { - $doc = $change->new; - $extracted = Operator::extractOperators($doc->getAttributes()); - if (! empty($extracted['operators'])) { - $hasOperators = true; - break; - } + foreach ($extractions as $extracted) { + if (! empty($extracted['operators'])) { + $hasOperators = true; + break; } } if (! $hasOperators) { - $this->executeUpsertBatch($name, $changes, $spatialAttributes, $increase ?? '', [], $attributeDefaults, false); + $this->executeUpsertBatch($name, $changes, $extractions, $spatialAttributes, $increase ?? '', [], $attributeDefaults, false); } else { $groups = []; - foreach ($changes as $change) { - $document = $change->new; - $extracted = Operator::extractOperators($document->getAttributes()); - $operators = $extracted['operators']; + foreach ($changes as $index => $change) { + $operators = $extractions[$index]['operators']; if (empty($operators)) { $signature = 'no_ops'; @@ -1303,23 +1292,25 @@ public function upsertDocuments(Document $collection, array $changes, ?string $i if (! isset($groups[$signature])) { $groups[$signature] = [ - 'documents' => [], + 'changes' => [], + 'extractions' => [], 'operators' => $operators, ]; } - $groups[$signature]['documents'][] = $change; + $groups[$signature]['changes'][$index] = $change; + $groups[$signature]['extractions'][$index] = $extractions[$index]; } foreach ($groups as $group) { - $this->executeUpsertBatch($name, $group['documents'], $spatialAttributes, '', $group['operators'], $attributeDefaults, true); + $this->executeUpsertBatch($name, $group['changes'], $group['extractions'], $spatialAttributes, '', $group['operators'], $attributeDefaults, true); } } $context = $this->writeContext(); $this->runWriteHooks(fn ($hook) => $hook->afterDocumentUpsert($name, $changes, $context)); - } catch (PDOException $e) { - throw $this->processException($e); + } catch (PDOException $exception) { + throw $this->processException($exception); } return \array_map(static fn (Change $change): Document => $change->new, $changes); @@ -3900,6 +3891,7 @@ private function transformQuery(Event $event, string $sql): string * * @param string $name The filtered collection name * @param array $changes The changes to upsert + * @param array, updates: array}> $extractions Each change's writable attributes split into operators and updates, keyed as $changes * @param list $spatialAttributes Spatial column names * @param string $attribute Increment attribute name (empty if none) * @param array $operators Operator map keyed by attribute name @@ -3910,6 +3902,7 @@ private function transformQuery(Event $event, string $sql): string protected function executeUpsertBatch( string $name, array $changes, + array $extractions, array $spatialAttributes, string $attribute, array $operators, @@ -3932,13 +3925,12 @@ protected function executeUpsertBatch( $allColumnNames = []; $documentsData = []; - foreach ($changes as $change) { + foreach ($changes as $index => $change) { $document = $change->new; + $currentRegularAttributes = $extractions[$index]['updates']; if ($hasOperators) { - $extracted = Operator::extractOperators($document->getAttributes()); - $currentRegularAttributes = $extracted['updates']; - $extractedOperators = $extracted['operators']; + $extractedOperators = $extractions[$index]['operators']; if ($change->old->isEmpty() && ! empty($extractedOperators)) { foreach ($extractedOperators as $operatorKey => $operator) { @@ -3951,7 +3943,6 @@ protected function executeUpsertBatch( $currentRegularAttributes[Storage::CREATED_AT] = $document->getCreatedAt() ? $document->getCreatedAt() : null; $currentRegularAttributes[Storage::UPDATED_AT] = $document->getUpdatedAt() ? $document->getUpdatedAt() : null; } else { - $currentRegularAttributes = $document->getAttributes(); $currentRegularAttributes[Storage::UID] = $document->getId(); $currentRegularAttributes[Storage::CREATED_AT] = $document->getCreatedAt() ? DateTime::setTimezone($document->getCreatedAt()) : null; $currentRegularAttributes[Storage::UPDATED_AT] = $document->getUpdatedAt() ? DateTime::setTimezone($document->getUpdatedAt()) : null; @@ -6535,7 +6526,7 @@ private function remapRow(array &$row): void */ protected function buildDocumentRow(Document $document, array $attributeKeys, array $spatialMap = [], ?bool $intBools = null): array { - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $row = [ Storage::UID => $document->getId(), Storage::CREATED_AT => $document->getCreatedAt(), diff --git a/src/Database/Adapter/SQLite.php b/src/Database/Adapter/SQLite.php index 13c453b1f..8a9b88a2d 100644 --- a/src/Database/Adapter/SQLite.php +++ b/src/Database/Adapter/SQLite.php @@ -1213,7 +1213,7 @@ public function createDocument(Document $collection, Document $document): Docume $this->syncWriteHooks(); $collection = $collection->getId(); - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $attributes[Storage::CREATED_AT] = $document->getCreatedAt(); $attributes[Storage::UPDATED_AT] = $document->getUpdatedAt(); $attributes[Storage::PERMISSIONS] = json_encode($document->getPermissions()); @@ -1272,7 +1272,7 @@ public function updateDocument(Document $collection, string $id, Document $docum $spatialAttributes = $this->getSpatialAttributes($collection); $collection = $collection->getId(); - $attributes = $document->getAttributes(); + $attributes = $this->writableAttributes($document); $attributes[Storage::CREATED_AT] = $document->getCreatedAt(); $attributes[Storage::UPDATED_AT] = $document->getUpdatedAt(); $attributes[Storage::PERMISSIONS] = json_encode($document->getPermissions()); @@ -2249,6 +2249,7 @@ protected function getConflictTenantIncrementExpression(string $column): string * * @param string $name The filtered collection name * @param array $changes The changes to upsert + * @param array, updates: array}> $extractions Each change's writable attributes split into operators and updates, keyed as $changes * @param array $spatialAttributes Spatial column names * @param string $attribute Increment attribute name (empty if none) * @param array $operators Operator map keyed by attribute name @@ -2260,6 +2261,7 @@ protected function getConflictTenantIncrementExpression(string $column): string protected function executeUpsertBatch( string $name, array $changes, + array $extractions, array $spatialAttributes, string $attribute, array $operators, @@ -2272,13 +2274,12 @@ protected function executeUpsertBatch( $allColumnNames = []; $documentsData = []; - foreach ($changes as $change) { + foreach ($changes as $index => $change) { $document = $change->new; + $currentRegularAttributes = $extractions[$index]['updates']; if ($hasOperators) { - $extracted = Operator::extractOperators($document->getAttributes()); - $currentRegularAttributes = $extracted['updates']; - $extractedOperators = $extracted['operators']; + $extractedOperators = $extractions[$index]['operators']; if ($change->old->isEmpty() && ! empty($extractedOperators)) { foreach ($extractedOperators as $operatorKey => $operator) { @@ -2295,7 +2296,6 @@ protected function executeUpsertBatch( $currentRegularAttributes[Storage::CREATED_AT] = $document->getCreatedAt() ? $document->getCreatedAt() : null; $currentRegularAttributes[Storage::UPDATED_AT] = $document->getUpdatedAt() ? $document->getUpdatedAt() : null; } else { - $currentRegularAttributes = $document->getAttributes(); $currentRegularAttributes[Storage::UID] = $document->getId(); $currentRegularAttributes[Storage::CREATED_AT] = $document->getCreatedAt() ? DatabaseDateTime::setTimezone($document->getCreatedAt()) : null; $currentRegularAttributes[Storage::UPDATED_AT] = $document->getUpdatedAt() ? DatabaseDateTime::setTimezone($document->getUpdatedAt()) : null; @@ -2309,15 +2309,15 @@ protected function executeUpsertBatch( $currentRegularAttributes = $this->decorateRow($currentRegularAttributes, $document); - foreach (\array_keys($currentRegularAttributes) as $colName) { - $allColumnNames[$colName] = true; + foreach (\array_keys($currentRegularAttributes) as $column) { + $allColumnNames[$column] = true; } $documentsData[] = ['regularAttributes' => $currentRegularAttributes]; } - foreach (\array_keys($operators) as $colName) { - $allColumnNames[$colName] = true; + foreach (\array_keys($operators) as $column) { + $allColumnNames[$column] = true; } $allColumnNames = \array_keys($allColumnNames); @@ -2358,8 +2358,8 @@ protected function executeUpsertBatch( } $regularAttributes = []; - foreach ($allColumnNames as $colName) { - $regularAttributes[$colName] = null; + foreach ($allColumnNames as $column) { + $regularAttributes[$column] = null; } foreach ($documentsData[0]['regularAttributes'] as $key => $value) { $regularAttributes[$key] = $value; diff --git a/tests/e2e/Adapter/Scopes/DocumentTests.php b/tests/e2e/Adapter/Scopes/DocumentTests.php index 6f619144b..c081da149 100644 --- a/tests/e2e/Adapter/Scopes/DocumentTests.php +++ b/tests/e2e/Adapter/Scopes/DocumentTests.php @@ -6462,6 +6462,106 @@ public function testIncreaseOfAStorageColumnIsRefused(): void $database->deleteCollection($collection); } + private function createStorageColumnKeyCollection(Database $database, string $collection): void + { + $database->createCollection(Collection::create( + id: $collection, + attributes: $database->getAdapter()->supports(Capability::DefinedAttributes) ? [Attribute::string(key: 'name', size: 64, required: false)] : [], + permissions: [Permission::create(Role::any()), Permission::read(Role::any()), Permission::update(Role::any())], + documentSecurity: true, + )); + } + + public function testStorageColumnKeyIsNotWrittenOnCreate(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + $collection = 'storageKeyCreate'; + $this->createStorageColumnKeyCollection($database, $collection); + + $database->skipValidation(function () use ($database, $collection): void { + $database->createDocument($collection, new Document(['$id' => 'plain', Storage::UID => 'hijacked', 'name' => 'plain'])); + $database->createDocument($collection, new Document(['$id' => 'mangled', '_u!id' => 'hijacked', 'name' => 'mangled'])); + $database->createDocument($collection, new Document(['$id' => 'sequenced', Storage::SEQUENCE => 777777, 'name' => 'sequenced'])); + $database->createDocuments($collection, [ + new Document(['$id' => 'batched', '_u!id' => 'hijacked', 'name' => 'batched']), + new Document(['$id' => 'batchedSequence', Storage::SEQUENCE => 888888, 'name' => 'batchedSequence']), + ]); + }); + + foreach (['plain', 'mangled', 'sequenced', 'batched', 'batchedSequence'] as $id) { + $document = $database->getDocument($collection, $id); + $this->assertSame($id, $document->getId(), "A storage-column key must not rewrite the id of {$id}"); + $this->assertSame($id, $document->getAttribute('name')); + } + $this->assertTrue($database->getDocument($collection, 'hijacked')->isEmpty()); + $this->assertNotSame('777777', (string) $database->getDocument($collection, 'sequenced')->getSequence()); + $this->assertNotSame('888888', (string) $database->getDocument($collection, 'batchedSequence')->getSequence()); + + $database->deleteCollection($collection); + } + + public function testStorageColumnKeyIsNotWrittenOnUpdate(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + $collection = 'storageKeyUpdate'; + $this->createStorageColumnKeyCollection($database, $collection); + $permissions = [Permission::read(Role::any()), Permission::update(Role::any())]; + $database->createDocument($collection, new Document(['$id' => 'target', '$permissions' => $permissions, 'name' => 'before'])); + + $database->skipValidation(fn (): Document => $database->updateDocument( + $collection, + 'target', + new Document(['$id' => 'target', '_u!id' => 'hijacked', 'name' => 'after']), + )); + + $this->assertTrue($database->getDocument($collection, 'hijacked')->isEmpty()); + $document = $database->getDocument($collection, 'target'); + $this->assertSame('target', $document->getId()); + $this->assertSame('after', $document->getAttribute('name')); + + $database->skipValidation(fn (): int => $database->updateDocuments( + $collection, + new Document([Storage::PERMISSIONS => \json_encode([Permission::read(Role::user('hijacker'))]), 'name' => 'bulk']), + )); + + $document = $database->getDocument($collection, 'target'); + $this->assertSame('bulk', $document->getAttribute('name')); + $this->assertSame($permissions, $document->getPermissions()); + + $database->deleteCollection($collection); + } + + public function testStorageColumnKeyIsNotWrittenOnUpsert(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + + if (! $database->getAdapter()->hasFeature(Feature\Upserts::class)) { + $this->expectNotToPerformAssertions(); + + return; + } + + $collection = 'storageKeyUpsert'; + $this->createStorageColumnKeyCollection($database, $collection); + + $upserted = ['$id' => 'upserted', '$permissions' => [Permission::read(Role::any())], Storage::SEQUENCE => 999999, 'name' => 'upserted']; + if ($database->hasSharedTables()) { + $upserted['_ten!ant'] = (int) $database->getTenant() + 1; + } + + $database->skipValidation(fn (): int => $database->upsertDocuments($collection, [new Document($upserted)])); + + $document = $database->getDocument($collection, 'upserted'); + $this->assertSame('upserted', $document->getId(), 'A storage-column key must not move the document to another tenant'); + $this->assertSame('upserted', $document->getAttribute('name')); + $this->assertNotSame('999999', (string) $document->getSequence()); + + $database->deleteCollection($collection); + } + public function testIncreaseDecreasePreserveDates(): void { $document = $this->initIncreaseDecreaseFixture(); diff --git a/tests/unit/WritableAttributesTest.php b/tests/unit/WritableAttributesTest.php new file mode 100644 index 000000000..9ab4937e7 --- /dev/null +++ b/tests/unit/WritableAttributesTest.php @@ -0,0 +1,41 @@ + + */ + public function writable(Document $document): array + { + return $this->writableAttributes($document); + } + }; + + $document = new Document([ + '$id' => 'kept', + '$permissions' => [], + 'name' => 'kept', + '_private' => 'kept', + Storage::UID => 'dropped', + '_u!id' => 'dropped', + Storage::SEQUENCE => 1, + Storage::TENANT => 2, + '_ten.ant' => 2, + Storage::PERMISSIONS => '[]', + Storage::CREATED_AT => 'dropped', + Storage::UPDATED_AT => 'dropped', + ]); + + $this->assertSame(['name' => 'kept', '_private' => 'kept'], $adapter->writable($document)); + } +} From 60146515d08ef6a09b58c572daefe45402d4bf19 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:44:08 +1300 Subject: [PATCH 02/11] fix(sql): run ddl one statement per prepare Collection, relationship and analyze DDL joined several statements with ';' into one prepare, which only ran on MySQL and MariaDB because multi-statement execution was left enabled on every connection, and on PostgreSQL because emulated prepares send a simple query. Keeping multi-statement execution on widens what an injected fragment can do. executeStatements() now runs each statement through its own prepare, so transforms, events and the profiler still see every statement, and a later statement's error fails the call without MariaDB having to walk its rowsets. No transaction is opened, since createCollection may run inside a caller's transaction. The MySQL-family e2e connections turn multi-statement execution off so any regression fails there. PostgreSQL ran a multi-statement simple query as one implicit transaction, so DDL loses that atomicity here. createCollection marks a collection created once its table exists and discards both tables on any failure in its internal phase (permissions table, internal indexes); only a duplicate declared index keeps them. deleteCollection converges on retry: when the main table is already gone it reports NotFound after still dropping the permissions table. Relationship create and delete converge through the executor added in a following commit. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/MariaDB.php | 19 +-- src/Database/Adapter/Postgres.php | 19 ++- src/Database/Adapter/SQL.php | 87 +++++----- src/Database/Adapter/SQLite.php | 39 +---- tests/e2e/Adapter/Base.php | 8 + tests/e2e/Adapter/MariaDBTest.php | 4 +- tests/e2e/Adapter/MirrorTest.php | 4 +- tests/e2e/Adapter/MySQLTest.php | 2 +- tests/e2e/Adapter/PoolTest.php | 2 +- .../e2e/Adapter/SharedTables/MariaDBTest.php | 2 +- tests/e2e/Adapter/SharedTables/MySQLTest.php | 2 +- .../MariaDBCreateCollectionCleanupTest.php | 8 +- .../Adapter/MariaDBDeleteCollectionTest.php | 8 +- .../PostgresCreateCollectionCleanupTest.php | 60 +++++-- .../SQLChildSideRelationshipRenameTest.php | 4 +- tests/unit/Adapter/SingleStatementDDLTest.php | 157 ++++++++++++++++++ .../Collections/AnalyzeCollectionTest.php | 2 +- tests/unit/EngineErrorMappingTest.php | 7 +- tests/unit/PostgresIdentifierTest.php | 8 +- tests/unit/PostgresQueryBehaviorTest.php | 6 +- tests/unit/SQLTimeoutScopeTest.php | 4 +- 21 files changed, 312 insertions(+), 140 deletions(-) create mode 100644 tests/unit/Adapter/SingleStatementDDLTest.php diff --git a/src/Database/Adapter/MariaDB.php b/src/Database/Adapter/MariaDB.php index cc1a825b4..57c56e967 100644 --- a/src/Database/Adapter/MariaDB.php +++ b/src/Database/Adapter/MariaDB.php @@ -315,7 +315,7 @@ public function deleteCollection(string $collection): bool $permissions = $schema->table($this->getTableRaw(Storage::permissionsTable($id)))->dropIfExists(); try { - return $this->executeStatement($main->query.'; '.$permissions->query, Event::CollectionDelete); + return $this->executeStatements([$main->query, $permissions->query], Event::CollectionDelete); } catch (PDOException $e) { $error = $this->processException($e); if ($error instanceof NotFoundException) { @@ -1065,23 +1065,6 @@ protected function decodePolygon(string $wkb): array */ private int $appliedRound = 0; - /** - * MySQL and MariaDB run every statement of a multi-statement string, but report only the first one's error from - * execute(); a later statement's error surfaces when its result is reached. Each result is reached here, so a - * statement that fails after another succeeded fails the call instead of being lost. - */ - #[\Override] - protected function executeStatement(string $sql, Event $event): bool - { - $statement = $this->prepareStatement($sql, $event); - $executed = $this->execute($statement); - - while ($statement->nextRowset()) { - } - - return $executed; - } - /** * @param PDOStatement|DatabasePDOStatement|PDOStatementProxy $statement */ diff --git a/src/Database/Adapter/Postgres.php b/src/Database/Adapter/Postgres.php index 7c7b62842..0d40093d6 100644 --- a/src/Database/Adapter/Postgres.php +++ b/src/Database/Adapter/Postgres.php @@ -270,8 +270,6 @@ public function createCollection(string $collection, array $attributes = [], arr $indexStatements[] = $schema->createIndex($tableRaw, $permissionsIndex, [Storage::PERMISSIONS], method: 'gin')->query; } - $collectionSql = $collectionResult->query.'; '.implode('; ', $indexStatements); - $permissionsTable = $schema->table($permissionsTableRaw); $permissionsTable->id(Storage::SEQUENCE); $permissionsTable->integer(Storage::TENANT)->nullable()->default(null); @@ -294,14 +292,19 @@ public function createCollection(string $collection, array $attributes = [], arr $permissionsIndexStatements[] = $schema->createIndex($permissionsTableRaw, $permissionIndex, [Storage::PERMISSIONS_PERMISSION, Storage::PERMISSIONS_TYPE], method: 'btree')->query; } - $permissionsSql = $permissionsResult->query.'; '.implode('; ', $permissionsIndexStatements); - $created = false; + $declaringIndexes = false; try { - $this->executeStatement($collectionSql, Event::CollectionCreate); + $this->executeStatement($collectionResult->query, Event::CollectionCreate); $created = true; - $this->executeStatement($permissionsSql, Event::CollectionCreate); + $this->executeStatements([ + ...$indexStatements, + $permissionsResult->query, + ...$permissionsIndexStatements, + ], Event::CollectionCreate); + + $declaringIndexes = true; foreach ($indexes as $index) { $indexAttributesWithType = []; @@ -328,7 +331,7 @@ public function createCollection(string $collection, array $attributes = [], arr $error = $this->processException($error); } - if ($created && ! ($error instanceof DuplicateException)) { + if ($created && ! ($declaringIndexes && $error instanceof DuplicateException)) { $this->discardCreatedCollection($id); } @@ -353,7 +356,7 @@ public function analyzeCollection(string $collection): bool $permissions = $schema->analyzeTable($this->getTableRaw(Storage::permissionsTable($name))); try { - return $this->executeStatement($main->query.'; '.$permissions->query, Event::CollectionUpdate); + return $this->executeStatements([$main->query, $permissions->query], Event::CollectionUpdate); } catch (PDOException $e) { throw $this->processException($e); } diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index 994b688f6..06c1a498c 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -3081,7 +3081,7 @@ public function deleteCollection(string $collection): bool $permissions = $schema->table($this->getTableRaw(Storage::permissionsTable($id)))->dropIfExists(); try { - return $this->executeStatement($main->query.'; '.$permissions->query, Event::CollectionDelete); + return $this->executeStatements([$main->query, $permissions->query], Event::CollectionDelete); } catch (PDOException $e) { $error = $this->processException($e); if ($error instanceof NotFoundException && $this->inTransaction === 0) { @@ -3111,7 +3111,7 @@ protected function dropCreatedCollection(string $id): void $main = $schema->table($this->getTableRaw($id))->dropIfExists(); $permissions = $schema->table($this->getTableRaw(Storage::permissionsTable($id)))->dropIfExists(); - $this->executeStatement($main->query.'; '.$permissions->query, Event::CollectionCreate); + $this->executeStatements([$main->query, $permissions->query], Event::CollectionCreate); } /** @@ -3136,18 +3136,16 @@ public function createRelationship(string $collection, Relationship $relationshi return $result->query; }; - $sql = match ($relationship->type) { - RelationshipType::OneToOne => $addColumn($name, $key) . ';' . ($relationship->twoWay ? $addColumn($relatedName, $twoWayKey) . ';' : ''), - RelationshipType::OneToMany => $addColumn($relatedName, $twoWayKey) . ';', - RelationshipType::ManyToOne => $addColumn($name, $key) . ';', - RelationshipType::ManyToMany => null, + $statements = match ($relationship->type) { + RelationshipType::OneToOne => $relationship->twoWay + ? [$addColumn($name, $key), $addColumn($relatedName, $twoWayKey)] + : [$addColumn($name, $key)], + RelationshipType::OneToMany => [$addColumn($relatedName, $twoWayKey)], + RelationshipType::ManyToOne => [$addColumn($name, $key)], + RelationshipType::ManyToMany => [], }; - if ($sql === null) { - return true; - } - - return $this->executeStatement($sql, Event::AttributeCreate); + return $this->executeStatements($statements, Event::AttributeCreate); } /** @@ -3175,52 +3173,48 @@ public function updateRelationship(string $collection, Relationship $relationshi return $result->query; }; - $sql = ''; + $statements = []; switch ($relationship->type) { case RelationshipType::OneToOne: if (($twoWay || $side === RelationshipSide::Parent) && $newKey !== null && $key !== $newKey) { - $sql = $renameColumn($name, $key, $newKey) . ';'; + $statements[] = $renameColumn($name, $key, $newKey); } if (($twoWay || $side === RelationshipSide::Child) && $newTwoWayKey !== null && $twoWayKey !== $newTwoWayKey) { - $sql .= $renameColumn($relatedName, $twoWayKey, $newTwoWayKey) . ';'; + $statements[] = $renameColumn($relatedName, $twoWayKey, $newTwoWayKey); } break; case RelationshipType::OneToMany: if ($side === RelationshipSide::Parent) { if ($newTwoWayKey !== null && $twoWayKey !== $newTwoWayKey) { - $sql = $renameColumn($relatedName, $twoWayKey, $newTwoWayKey) . ';'; + $statements[] = $renameColumn($relatedName, $twoWayKey, $newTwoWayKey); } } elseif ($newKey !== null && $key !== $newKey) { - $sql = $renameColumn($name, $key, $newKey) . ';'; + $statements[] = $renameColumn($name, $key, $newKey); } break; case RelationshipType::ManyToOne: if ($side === RelationshipSide::Child) { if ($newTwoWayKey !== null && $twoWayKey !== $newTwoWayKey) { - $sql = $renameColumn($relatedName, $twoWayKey, $newTwoWayKey) . ';'; + $statements[] = $renameColumn($relatedName, $twoWayKey, $newTwoWayKey); } } elseif ($newKey !== null && $key !== $newKey) { - $sql = $renameColumn($name, $key, $newKey) . ';'; + $statements[] = $renameColumn($name, $key, $newKey); } break; case RelationshipType::ManyToMany: $junctionName = $this->getJunctionName($collection, $relationship->relatedCollection, $side); if ($newKey !== null && $key !== $newKey) { - $sql = $renameColumn($junctionName, $key, $newKey) . ';'; + $statements[] = $renameColumn($junctionName, $key, $newKey); } if ($newTwoWayKey !== null && $twoWayKey !== $newTwoWayKey) { - $sql .= $renameColumn($junctionName, $twoWayKey, $newTwoWayKey) . ';'; + $statements[] = $renameColumn($junctionName, $twoWayKey, $newTwoWayKey); } break; } - if ($sql === '') { - return true; - } - - return $this->executeStatement($sql, Event::AttributeUpdate); + return $this->executeStatements($statements, Event::AttributeUpdate); } /** @@ -3246,31 +3240,31 @@ public function deleteRelationship(string $collection, Relationship $relationshi return $result->query; }; - $sql = ''; + $statements = []; switch ($relationship->type) { case RelationshipType::OneToOne: if ($side === RelationshipSide::Parent) { - $sql = $dropColumn($name, $key) . ';'; + $statements[] = $dropColumn($name, $key); if ($twoWay) { - $sql .= $dropColumn($relatedName, $twoWayKey) . ';'; + $statements[] = $dropColumn($relatedName, $twoWayKey); } } else { - $sql = $dropColumn($relatedName, $twoWayKey) . ';'; + $statements[] = $dropColumn($relatedName, $twoWayKey); if ($twoWay) { - $sql .= $dropColumn($name, $key) . ';'; + $statements[] = $dropColumn($name, $key); } } break; case RelationshipType::OneToMany: - $sql = $side === RelationshipSide::Parent - ? $dropColumn($relatedName, $twoWayKey) . ';' - : $dropColumn($name, $key) . ';'; + $statements[] = $side === RelationshipSide::Parent + ? $dropColumn($relatedName, $twoWayKey) + : $dropColumn($name, $key); break; case RelationshipType::ManyToOne: - $sql = $side === RelationshipSide::Parent - ? $dropColumn($name, $key) . ';' - : $dropColumn($relatedName, $twoWayKey) . ';'; + $statements[] = $side === RelationshipSide::Parent + ? $dropColumn($name, $key) + : $dropColumn($relatedName, $twoWayKey); break; case RelationshipType::ManyToMany: $junctionName = $this->getJunctionName($collection, $relationship->relatedCollection, $side); @@ -3278,11 +3272,12 @@ public function deleteRelationship(string $collection, Relationship $relationshi $junctionResult = $schema->table($this->getTableRaw($junctionName))->drop(); $permissionsResult = $schema->table($this->getTableRaw(Storage::permissionsTable($junctionName)))->drop(); - $sql = $junctionResult->query . '; ' . $permissionsResult->query; + $statements[] = $junctionResult->query; + $statements[] = $permissionsResult->query; break; } - return $this->executeStatement($sql, Event::AttributeDelete); + return $this->executeStatements($statements, Event::AttributeDelete); } /** @@ -3875,6 +3870,20 @@ protected function executeStatement(string $sql, Event $event): bool return $this->execute($this->prepareStatement($sql, $event)); } + /** + * @param list $statements + */ + protected function executeStatements(array $statements, Event $event): bool + { + foreach ($statements as $statement) { + if (! $this->executeStatement($statement, $event)) { + return false; + } + } + + return true; + } + private function transformQuery(Event $event, string $sql): string { foreach ($this->transforms as $transform) { diff --git a/src/Database/Adapter/SQLite.php b/src/Database/Adapter/SQLite.php index 8a9b88a2d..e335c83bd 100644 --- a/src/Database/Adapter/SQLite.php +++ b/src/Database/Adapter/SQLite.php @@ -670,19 +670,17 @@ public function deleteCollection(string $collection): bool { $id = $this->filter($collection); + $statements = []; + // FTS5 shadow tables don't drop with the parent. foreach ($this->findFulltextTables($id) as $ftsTable) { - $sql = 'DROP TABLE IF EXISTS '.$this->quote($ftsTable); - $this->execute($this->prepare($sql, event: Event::CollectionDelete)); + $statements[] = 'DROP TABLE IF EXISTS '.$this->quote($ftsTable); } - $sql = "DROP TABLE IF EXISTS {$this->getTable($id)}"; - - $this->execute($this->prepare($sql, event: Event::CollectionDelete)); + $statements[] = "DROP TABLE IF EXISTS {$this->getTable($id)}"; + $statements[] = "DROP TABLE IF EXISTS {$this->getTable(Storage::permissionsTable($id))}"; - $sql = "DROP TABLE IF EXISTS {$this->getTable(Storage::permissionsTable($id))}"; - - $this->execute($this->prepare($sql, event: Event::CollectionDelete)); + $this->executeStatements($statements, Event::CollectionDelete); unset($this->ftsTableCache[$this->getFulltextTablePrefix($id)]); @@ -2468,13 +2466,6 @@ public function createAttributes(string $collection, array $attributes): bool return true; } - /** - * SQL::createRelationship concatenates multiple ALTER TABLE statements - * with `;` and runs them through a single prepare/execute, which only - * works because MySQL accepts multi-statement queries. SQLite's PDO - * driver runs the first statement and silently drops the rest, so - * re-implement the dispatch with one statement per call. - */ #[\Override] public function createRelationship(string $collection, Relationship $relationship): bool { @@ -2496,11 +2487,7 @@ public function createRelationship(string $collection, Relationship $relationshi RelationshipType::ManyToMany => [], }; - foreach ($statements as $statement) { - $this->execute($this->prepare($statement, event: Event::AttributeCreate)); - } - - return true; + return $this->executeStatements($statements, Event::AttributeCreate); } #[\Override] @@ -2555,11 +2542,7 @@ public function updateRelationship(string $collection, Relationship $relationshi break; } - foreach ($statements as $statement) { - $this->execute($this->prepare($statement, event: Event::AttributeUpdate)); - } - - return true; + return $this->executeStatements($statements, Event::AttributeUpdate); } #[\Override] @@ -2605,11 +2588,7 @@ public function deleteRelationship(string $collection, Relationship $relationshi break; } - foreach ($statements as $statement) { - $this->execute($this->prepare($statement, event: Event::AttributeDelete)); - } - - return true; + return $this->executeStatements($statements, Event::AttributeDelete); } /** diff --git a/tests/e2e/Adapter/Base.php b/tests/e2e/Adapter/Base.php index 7f7a1080f..0cb953b1c 100644 --- a/tests/e2e/Adapter/Base.php +++ b/tests/e2e/Adapter/Base.php @@ -3,6 +3,7 @@ namespace Tests\E2E\Adapter; use PDO; +use Pdo\Mysql as MySQLPDO; use PHPUnit\Framework\TestCase; use Tests\E2E\Adapter\Scopes\AggregationTests; use Tests\E2E\Adapter\Scopes\AttributeTests; @@ -88,6 +89,13 @@ abstract class Base extends TestCase PDO::ATTR_STRINGIFY_FETCHES => true, ]; + /** + * @var array + */ + protected const array MYSQL_PDO_ATTRIBUTES = [ + MySQLPDO::ATTR_MULTI_STATEMENTS => false, + ] + self::PDO_ATTRIBUTES; + protected static string $namespace; protected static ?Authorization $authorization = null; diff --git a/tests/e2e/Adapter/MariaDBTest.php b/tests/e2e/Adapter/MariaDBTest.php index dbea51b47..498e89d46 100644 --- a/tests/e2e/Adapter/MariaDBTest.php +++ b/tests/e2e/Adapter/MariaDBTest.php @@ -44,7 +44,7 @@ public function getDatabase(bool $fresh = false): Database $dbUser = 'root'; $dbPass = 'password'; - $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::PDO_ATTRIBUTES); + $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::MYSQL_PDO_ATTRIBUTES); $redis = new Redis(); $redis->connect('redis', 6379); @@ -157,7 +157,7 @@ private function connect(): Database 'mysql:host=mariadb;port=3306;charset=utf8mb4', 'root', 'password', - [PhpPDO::ATTR_PERSISTENT => false] + self::PDO_ATTRIBUTES, + [PhpPDO::ATTR_PERSISTENT => false] + self::MYSQL_PDO_ATTRIBUTES, ); $main = $this->getDatabase(); diff --git a/tests/e2e/Adapter/MirrorTest.php b/tests/e2e/Adapter/MirrorTest.php index 2d3d0725a..11a7485ea 100644 --- a/tests/e2e/Adapter/MirrorTest.php +++ b/tests/e2e/Adapter/MirrorTest.php @@ -53,7 +53,7 @@ protected function getDatabase(bool $fresh = false): Mirror $dbUser = 'root'; $dbPass = 'password'; - $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::PDO_ATTRIBUTES); + $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::MYSQL_PDO_ATTRIBUTES); $redis = new Redis(); $redis->connect('redis'); @@ -68,7 +68,7 @@ protected function getDatabase(bool $fresh = false): Mirror $mirrorUser = 'root'; $mirrorPass = 'password'; - $mirrorPdo = new PDO("mysql:host={$mirrorHost};port={$mirrorPort};charset=utf8mb4", $mirrorUser, $mirrorPass, self::PDO_ATTRIBUTES); + $mirrorPdo = new PDO("mysql:host={$mirrorHost};port={$mirrorPort};charset=utf8mb4", $mirrorUser, $mirrorPass, self::MYSQL_PDO_ATTRIBUTES); $mirrorRedis = new Redis(); $mirrorRedis->connect('redis-mirror'); diff --git a/tests/e2e/Adapter/MySQLTest.php b/tests/e2e/Adapter/MySQLTest.php index 5eaf3d034..1d6f4bebb 100644 --- a/tests/e2e/Adapter/MySQLTest.php +++ b/tests/e2e/Adapter/MySQLTest.php @@ -41,7 +41,7 @@ public function getDatabase(): Database $dbUser = 'root'; $dbPass = 'password'; - $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::PDO_ATTRIBUTES); + $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::MYSQL_PDO_ATTRIBUTES); $redis = new Redis(); $redis->connect('redis', 6379); diff --git a/tests/e2e/Adapter/PoolTest.php b/tests/e2e/Adapter/PoolTest.php index 99a6de45e..2484e6151 100644 --- a/tests/e2e/Adapter/PoolTest.php +++ b/tests/e2e/Adapter/PoolTest.php @@ -60,7 +60,7 @@ public function getDatabase(): Database dsn: "mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", username: $dbUser, password: $dbPass, - config: self::PDO_ATTRIBUTES, + config: self::MYSQL_PDO_ATTRIBUTES, )); }, timeout: 0.0); diff --git a/tests/e2e/Adapter/SharedTables/MariaDBTest.php b/tests/e2e/Adapter/SharedTables/MariaDBTest.php index 25278ca71..07ec6a47a 100644 --- a/tests/e2e/Adapter/SharedTables/MariaDBTest.php +++ b/tests/e2e/Adapter/SharedTables/MariaDBTest.php @@ -40,7 +40,7 @@ public function getDatabase(bool $fresh = false): Database $dbUser = 'root'; $dbPass = 'password'; - $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::PDO_ATTRIBUTES); + $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::MYSQL_PDO_ATTRIBUTES); $redis = new Redis(); $redis->connect('redis', 6379); $redis->select(7); diff --git a/tests/e2e/Adapter/SharedTables/MySQLTest.php b/tests/e2e/Adapter/SharedTables/MySQLTest.php index d910b5928..616c18524 100644 --- a/tests/e2e/Adapter/SharedTables/MySQLTest.php +++ b/tests/e2e/Adapter/SharedTables/MySQLTest.php @@ -43,7 +43,7 @@ public function getDatabase(): Database $dbUser = 'root'; $dbPass = 'password'; - $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::PDO_ATTRIBUTES); + $pdo = new PDO("mysql:host={$dbHost};port={$dbPort};charset=utf8mb4", $dbUser, $dbPass, self::MYSQL_PDO_ATTRIBUTES); $redis = new Redis(); $redis->connect('redis', 6379); diff --git a/tests/unit/Adapter/MariaDBCreateCollectionCleanupTest.php b/tests/unit/Adapter/MariaDBCreateCollectionCleanupTest.php index 02a80890e..44328df03 100644 --- a/tests/unit/Adapter/MariaDBCreateCollectionCleanupTest.php +++ b/tests/unit/Adapter/MariaDBCreateCollectionCleanupTest.php @@ -48,11 +48,9 @@ public function testAPermissionsTableThatFailsDropsTheCollectionTable(string $cl $this->assertSame('Query timed out', $error->getMessage()); } - $this->assertCount(3, $this->statements); - $this->assertSame( - 'DROP TABLE IF EXISTS `database`.`namespace_books`; DROP TABLE IF EXISTS `database`.`namespace_books_perms`', - $this->statements[2], - ); + $this->assertCount(4, $this->statements); + $this->assertSame('DROP TABLE IF EXISTS `database`.`namespace_books`', $this->statements[2]); + $this->assertSame('DROP TABLE IF EXISTS `database`.`namespace_books_perms`', $this->statements[3]); } /** diff --git a/tests/unit/Adapter/MariaDBDeleteCollectionTest.php b/tests/unit/Adapter/MariaDBDeleteCollectionTest.php index 28e6fc3cb..605077201 100644 --- a/tests/unit/Adapter/MariaDBDeleteCollectionTest.php +++ b/tests/unit/Adapter/MariaDBDeleteCollectionTest.php @@ -35,12 +35,12 @@ public static function adapters(): array * @param class-string $adapterClass */ #[DataProvider('adapters')] - public function testDropsBothTablesInOneStatement(string $adapterClass): void + public function testDropsTheMainTableThenThePermissionsTable(string $adapterClass): void { $adapter = $this->createAdapter($adapterClass, mainTableExists: true); $this->assertTrue($adapter->deleteCollection('places')); - $this->assertSame([self::MAIN.'; '.self::PERMISSIONS], $this->statements); + $this->assertSame([self::MAIN, self::PERMISSIONS], $this->statements); } /** @@ -58,7 +58,7 @@ public function testDropsThePermissionsTableWhenTheMainTableIsGone(string $adapt $this->assertSame('Collection not found', $e->getMessage()); } - $this->assertSame([self::MAIN.'; '.self::PERMISSIONS, self::PERMISSIONS], $this->statements); + $this->assertSame([self::MAIN, self::PERMISSIONS], $this->statements); } /** @@ -71,7 +71,7 @@ private function createAdapter(string $adapterClass, bool $mainTableExists): Mar $this->statements[] = $query; $statement = $this->createStub(PDOStatement::class); - if (! $mainTableExists && \str_starts_with($query, self::MAIN.';')) { + if (! $mainTableExists && $query === self::MAIN) { $statement->method('execute')->willThrowException($this->unknownTable()); } else { $statement->method('execute')->willReturn(true); diff --git a/tests/unit/Adapter/PostgresCreateCollectionCleanupTest.php b/tests/unit/Adapter/PostgresCreateCollectionCleanupTest.php index 8a4aee71b..366d3ee92 100644 --- a/tests/unit/Adapter/PostgresCreateCollectionCleanupTest.php +++ b/tests/unit/Adapter/PostgresCreateCollectionCleanupTest.php @@ -20,6 +20,10 @@ final class PostgresCreateCollectionCleanupTest extends TestCase { private const string DECLARED_INDEX = 'CREATE INDEX "namespace__books_title_index"'; + private const string UID_INDEX = 'CREATE UNIQUE INDEX "namespace_books_uid"'; + + private const string PERMISSIONS_TABLE = 'CREATE TABLE "database"."namespace_books_perms"'; + /** @var list */ private array $statements = []; @@ -39,12 +43,10 @@ public function testADeclaredIndexThatFailsDropsBothTablesAndReachesTheCaller(): $this->assertSame('Attribute not found', $error->getMessage()); } - $this->assertCount(4, $this->statements); - $this->assertStringStartsWith(self::DECLARED_INDEX, $this->statements[2]); - $this->assertSame( - 'DROP TABLE IF EXISTS "database"."namespace_books"; DROP TABLE IF EXISTS "database"."namespace_books_perms"', - $this->statements[3], - ); + $this->assertCount(11, $this->statements); + $this->assertStringStartsWith(self::DECLARED_INDEX, $this->statements[8]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_books"', $this->statements[9]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_books_perms"', $this->statements[10]); } public function testACleanupThatFailsKeepsTheOriginalErrorAndLogsTheCleanupFailure(): void @@ -65,7 +67,7 @@ public function testACleanupThatFailsKeepsTheOriginalErrorAndLogsTheCleanupFailu $this->assertInstanceOf(NotFoundException::class, $error, 'the index failure reaches the caller, not the failed drop'); $this->assertSame('Attribute not found', $error->getMessage()); - $this->assertStringStartsWith('DROP TABLE IF EXISTS', $this->statements[3]); + $this->assertStringStartsWith('DROP TABLE IF EXISTS', $this->statements[9]); $this->assertStringContainsString("Failed to rollback collection 'books': SQLSTATE[25P02]", $log, 'the failed cleanup is logged'); } @@ -82,12 +84,46 @@ public function testADeclaredIndexThatAlreadyExistsKeepsBothTables(): void $this->assertInstanceOf(PDOException::class, $error->getPrevious()); } - $this->assertCount(3, $this->statements); + $this->assertCount(9, $this->statements); foreach ($this->statements as $statement) { $this->assertStringNotContainsString('DROP TABLE', $statement); } } + public function testAnInternalIndexThatAlreadyExistsDropsBothTables(): void + { + $this->failures[self::UID_INDEX] = $this->engineError('42P07', 'relation "namespace_books_uid" already exists'); + + try { + $this->adapter()->createCollection('books', [Attribute::string('title', size: 64)]); + $this->fail('An internal index that already exists must fail the collection'); + } catch (DuplicateException $error) { + $this->assertInstanceOf(PDOException::class, $error->getPrevious()); + } + + $this->assertCount(4, $this->statements); + $this->assertStringStartsWith(self::UID_INDEX, $this->statements[1]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_books"', $this->statements[2]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_books_perms"', $this->statements[3]); + } + + public function testAPermissionsTableThatAlreadyExistsDropsBothTables(): void + { + $this->failures[self::PERMISSIONS_TABLE] = $this->engineError('42P07', 'relation "namespace_books_perms" already exists'); + + try { + $this->adapter()->createCollection('books', [Attribute::string('title', size: 64)]); + $this->fail('A permissions table that already exists must fail the collection'); + } catch (DuplicateException $error) { + $this->assertInstanceOf(PDOException::class, $error->getPrevious()); + } + + $this->assertCount(8, $this->statements); + $this->assertStringStartsWith(self::PERMISSIONS_TABLE, $this->statements[5]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_books"', $this->statements[6]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_books_perms"', $this->statements[7]); + } + public function testASpatialIndexWithOrdersDropsBothTablesAndReachesTheCaller(): void { try { @@ -99,11 +135,9 @@ public function testASpatialIndexWithOrdersDropsBothTablesAndReachesTheCaller(): $this->assertSame('Spatial indexes with explicit orders are not supported. Remove the orders to create this index.', $error->getMessage()); } - $this->assertCount(3, $this->statements); - $this->assertSame( - 'DROP TABLE IF EXISTS "database"."namespace_places"; DROP TABLE IF EXISTS "database"."namespace_places_perms"', - $this->statements[2], - ); + $this->assertCount(10, $this->statements); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_places"', $this->statements[8]); + $this->assertSame('DROP TABLE IF EXISTS "database"."namespace_places_perms"', $this->statements[9]); } public function testATableThatFailsToBeCreatedDropsNothing(): void diff --git a/tests/unit/Adapter/SQLChildSideRelationshipRenameTest.php b/tests/unit/Adapter/SQLChildSideRelationshipRenameTest.php index 6d2fe9f6b..77db44d1b 100644 --- a/tests/unit/Adapter/SQLChildSideRelationshipRenameTest.php +++ b/tests/unit/Adapter/SQLChildSideRelationshipRenameTest.php @@ -25,8 +25,8 @@ final class SQLChildSideRelationshipRenameTest extends TestCase public static function renames(): iterable { $engines = [ - 'MariaDB' => [MariaDB::class, 'ALTER TABLE `database`.`namespace_%s` RENAME COLUMN `%s` TO `%s`;'], - 'Postgres' => [Postgres::class, 'ALTER TABLE "database"."namespace_%s" RENAME COLUMN "%s" TO "%s";'], + 'MariaDB' => [MariaDB::class, 'ALTER TABLE `database`.`namespace_%s` RENAME COLUMN `%s` TO `%s`'], + 'Postgres' => [Postgres::class, 'ALTER TABLE "database"."namespace_%s" RENAME COLUMN "%s" TO "%s"'], ]; foreach ($engines as $engine => [$class, $statement]) { diff --git a/tests/unit/Adapter/SingleStatementDDLTest.php b/tests/unit/Adapter/SingleStatementDDLTest.php new file mode 100644 index 000000000..ca80ab8ed --- /dev/null +++ b/tests/unit/Adapter/SingleStatementDDLTest.php @@ -0,0 +1,157 @@ + + */ + private array $prepared = []; + + private ?string $refused = null; + + /** + * @return iterable + */ + public static function adapters(): iterable + { + yield 'MariaDB' => [static fn (PDO $pdo): SQL => new MariaDB($pdo)]; + yield 'MySQL' => [static fn (PDO $pdo): SQL => new MySQL($pdo)]; + yield 'Postgres' => [static fn (PDO $pdo): SQL => new Postgres($pdo)]; + yield 'SQLite' => [static fn (PDO $pdo): SQL => new SQLite($pdo)]; + } + + /** + * @return iterable + */ + public static function operations(): iterable + { + $operations = [ + 'create collection' => static fn (SQL $adapter): bool => $adapter->createCollection('books', [Attribute::string(key: 'title', size: 64)]), + 'delete collection' => static fn (SQL $adapter): bool => $adapter->deleteCollection('books'), + 'analyze collection' => static fn (SQL $adapter): bool => $adapter->analyzeCollection('books'), + 'create one-to-one relationship' => static fn (SQL $adapter): bool => $adapter->createRelationship('books', self::oneToOne()), + 'update one-to-one relationship' => static fn (SQL $adapter): bool => $adapter->updateRelationship('books', self::oneToOne(), RelationshipSide::Parent, new Update(key: 'writer', twoWayKey: 'novel')), + 'delete one-to-one relationship' => static fn (SQL $adapter): bool => $adapter->deleteRelationship('books', self::oneToOne(), RelationshipSide::Parent), + 'update many-to-many relationship' => static fn (SQL $adapter): bool => $adapter->updateRelationship('books', self::manyToMany(), RelationshipSide::Parent, new Update(key: 'writers', twoWayKey: 'novels')), + 'delete many-to-many relationship' => static fn (SQL $adapter): bool => $adapter->deleteRelationship('books', self::manyToMany(), RelationshipSide::Parent), + ]; + + foreach (self::adapters() as $adapterName => [$factory]) { + foreach ($operations as $operationName => $operation) { + yield "{$adapterName} {$operationName}" => [$factory, $operation]; + } + } + } + + /** + * @param Closure(PDO): SQL $factory + * @param Closure(SQL): bool $operation + */ + #[DataProvider('operations')] + public function testEveryPrepareHoldsOneStatement(Closure $factory, Closure $operation): void + { + $adapter = $this->adapter($factory); + + $this->assertTrue($operation($adapter)); + $this->assertNotEmpty($this->prepared, 'The operation must prepare its DDL'); + + foreach ($this->prepared as $query) { + $this->assertSame(1, self::countStatements($query), "Prepared more than one statement: {$query}"); + } + } + + /** + * @param Closure(PDO): SQL $factory + */ + #[DataProvider('adapters')] + public function testAFailedCreateDropsItsTablesOneStatementAtATime(Closure $factory): void + { + $adapter = $this->adapter($factory); + $this->refused = 'namespace_books_perms'; + + try { + $adapter->createCollection('books'); + $this->fail('A create whose permissions table is refused must fail'); + } catch (Throwable $error) { + $this->assertNotInstanceOf(AssertionFailedError::class, $error); + } + + $drops = \array_values(\array_filter($this->prepared, static fn (string $query): bool => \str_contains($query, 'DROP TABLE'))); + $this->assertCount(2, $drops, 'The rollback must drop the collection and its permissions table'); + + foreach ($this->prepared as $query) { + $this->assertSame(1, self::countStatements($query), "Prepared more than one statement: {$query}"); + } + } + + private static function oneToOne(): Relationship + { + return Relationship::oneToOne(relatedCollection: 'authors', key: 'author', twoWay: true, twoWayKey: 'book'); + } + + private static function manyToMany(): Relationship + { + return Relationship::manyToMany(relatedCollection: 'authors', key: 'authors', twoWay: true, twoWayKey: 'books'); + } + + private static function countStatements(string $query): int + { + $code = \preg_replace(['#/\*.*?\*/#s', "/'(?:[^']|'')*'/", '/"(?:[^"]|"")*"/', '/`[^`]*`/'], '', $query) ?? $query; + $statements = \array_filter(\array_map(\trim(...), \explode(';', $code)), static fn (string $statement): bool => $statement !== ''); + + return \count($statements); + } + + /** + * @param Closure(PDO): SQL $factory + */ + private function adapter(Closure $factory): SQL + { + $adapter = $factory($this->recorder()); + $adapter->setDatabase('library'); + $adapter->setNamespace('namespace'); + + return $adapter; + } + + private function recorder(): PDO + { + $pdo = $this->createStub(PDO::class); + $pdo->method('prepare')->willReturnCallback(function (string $query): PDOStatement { + $this->prepared[] = $query; + + $refused = $this->refused !== null && \str_starts_with(\ltrim($query), 'CREATE TABLE') && \str_contains($query, $this->refused); + + $statement = $this->createStub(PDOStatement::class); + $statement->method('execute')->willReturnCallback(static fn (): bool => $refused ? throw new PDOException('SQLSTATE[HY000]: General error: refused') : true); + $statement->method('fetchAll')->willReturn([]); + $statement->method('fetch')->willReturn(false); + $statement->method('closeCursor')->willReturn(true); + $statement->method('rowCount')->willReturn(0); + + return $statement; + }); + + return $pdo; + } +} diff --git a/tests/unit/Collections/AnalyzeCollectionTest.php b/tests/unit/Collections/AnalyzeCollectionTest.php index a2beb5c50..354f17234 100644 --- a/tests/unit/Collections/AnalyzeCollectionTest.php +++ b/tests/unit/Collections/AnalyzeCollectionTest.php @@ -66,6 +66,6 @@ public function testPostgresAnalyzesTheTableAndItsPermissions(): void $adapter->setNamespace('namespace'); $this->assertTrue($adapter->analyzeCollection('places')); - $this->assertSame(['ANALYZE "database"."namespace_places"; ANALYZE "database"."namespace_places_perms"'], $this->statements); + $this->assertSame(['ANALYZE "database"."namespace_places"', 'ANALYZE "database"."namespace_places_perms"'], $this->statements); } } diff --git a/tests/unit/EngineErrorMappingTest.php b/tests/unit/EngineErrorMappingTest.php index 79f95cdb2..5c20cc1dd 100644 --- a/tests/unit/EngineErrorMappingTest.php +++ b/tests/unit/EngineErrorMappingTest.php @@ -364,7 +364,7 @@ public function testPostgresDeleteCollectionWithoutItsTableIsNotFoundAndStillDro $this->assertInstanceOf(NotFoundException::class, $error); $this->assertSame('Collection not found', $error->getMessage()); $this->assertSame([ - 'DROP TABLE "utopiaTests"."engine_orders"; DROP TABLE IF EXISTS "utopiaTests"."engine_orders_perms"', + 'DROP TABLE "utopiaTests"."engine_orders"', 'DROP TABLE IF EXISTS "utopiaTests"."engine_orders_perms"', ], $statements); } @@ -387,14 +387,15 @@ public function testPostgresUniqueIndexOverDuplicateObjectPathValuesIsUnique(): $this->assertSame($duplicates, $error->getPrevious()); } - public function testPostgresDeleteCollectionDropsBothTablesInOneStatement(): void + public function testPostgresDeleteCollectionDropsTheMainTableThenThePermissionsTable(): void { $statements = []; $adapter = $this->postgresRecording($statements); $this->assertTrue($adapter->deleteCollection('orders')); $this->assertSame([ - 'DROP TABLE "utopiaTests"."engine_orders"; DROP TABLE IF EXISTS "utopiaTests"."engine_orders_perms"', + 'DROP TABLE "utopiaTests"."engine_orders"', + 'DROP TABLE IF EXISTS "utopiaTests"."engine_orders_perms"', ], $statements); } diff --git a/tests/unit/PostgresIdentifierTest.php b/tests/unit/PostgresIdentifierTest.php index 22e298b48..93a0518fd 100644 --- a/tests/unit/PostgresIdentifierTest.php +++ b/tests/unit/PostgresIdentifierTest.php @@ -38,7 +38,7 @@ public function testCreateCollectionSqlUsesHashedTableName(): void $this->assertNotSame(self::NAMESPACE.'_'.self::COLLECTION, $physical); $this->assertStringContainsString('"'.$physical.'"', $queries[0]); $this->assertStringNotContainsString(self::COLLECTION, $queries[0]); - $this->assertStringContainsString('"'.$permissions.'"', $queries[1]); + $this->assertStringContainsString('"'.$permissions.'"', $queries[5]); } public function testExistsBindsHashedTableName(): void @@ -59,7 +59,7 @@ private function createCollectionQueries(string $collection): array $statement = $this->getMockBuilder(PDOStatement::class) ->disableOriginalConstructor() ->getMock(); - $statement->expects($this->exactly(2)) + $statement->expects($this->exactly(8)) ->method('execute') ->willReturn(true); @@ -67,7 +67,7 @@ private function createCollectionQueries(string $collection): array $pdo = $this->getMockBuilder(PDO::class) ->disableOriginalConstructor() ->getMock(); - $pdo->expects($this->exactly(2)) + $pdo->expects($this->exactly(8)) ->method('prepare') ->willReturnCallback(function (string $sql) use (&$queries, $statement): PDOStatement { $queries[] = $sql; @@ -83,7 +83,7 @@ private function createCollectionQueries(string $collection): array $adapter->setAuthorization($authorization); $this->assertTrue($adapter->createCollection($collection)); - $this->assertCount(2, $queries); + $this->assertCount(8, $queries); return $queries; } diff --git a/tests/unit/PostgresQueryBehaviorTest.php b/tests/unit/PostgresQueryBehaviorTest.php index 479f77176..fe2a835d5 100644 --- a/tests/unit/PostgresQueryBehaviorTest.php +++ b/tests/unit/PostgresQueryBehaviorTest.php @@ -40,7 +40,7 @@ public function testCreateCollectionStoresJsonbPermissionsWithGinIndex(): void $statement = $this->getMockBuilder(\PDOStatement::class) ->disableOriginalConstructor() ->getMock(); - $statement->expects($this->exactly(2)) + $statement->expects($this->exactly(8)) ->method('execute') ->willReturn(true); @@ -48,7 +48,7 @@ public function testCreateCollectionStoresJsonbPermissionsWithGinIndex(): void $pdo = $this->getMockBuilder(\PDO::class) ->disableOriginalConstructor() ->getMock(); - $pdo->expects($this->exactly(2)) + $pdo->expects($this->exactly(8)) ->method('prepare') ->willReturnCallback(function (string $sql) use (&$queries, $statement): \PDOStatement { $queries[] = $sql; @@ -65,7 +65,7 @@ public function testCreateCollectionStoresJsonbPermissionsWithGinIndex(): void $this->assertTrue($adapter->createCollection('movies')); $this->assertStringContainsString('"'.Storage::PERMISSIONS.'" JSONB', $queries[0]); - $this->assertStringContainsString('USING GIN ("'.Storage::PERMISSIONS.'")', $queries[0]); + $this->assertStringContainsString('USING GIN ("'.Storage::PERMISSIONS.'")', $queries[4]); } public function testVectorDistanceIsProjectedHydratedAndOrderedBeforeTieBreaker(): void diff --git a/tests/unit/SQLTimeoutScopeTest.php b/tests/unit/SQLTimeoutScopeTest.php index b8d2f5017..2c1fc9a2f 100644 --- a/tests/unit/SQLTimeoutScopeTest.php +++ b/tests/unit/SQLTimeoutScopeTest.php @@ -41,10 +41,10 @@ public function testPublicReadTimeoutDoesNotAffectCollectionDelete(): void $statement = $this->getMockBuilder(\PDOStatement::class) ->disableOriginalConstructor() ->getMock(); - $statement->expects($this->exactly(2))->method('execute')->willReturn(true); + $statement->expects($this->exactly(3))->method('execute')->willReturn(true); $statement->expects($this->once())->method('fetchAll')->willReturn([]); $statement->expects($this->once())->method('closeCursor')->willReturn(true); - $pdo->expects($this->exactly(2))->method('prepare')->willReturn($statement); + $pdo->expects($this->exactly(3))->method('prepare')->willReturn($statement); $adapter = new MySQL($pdo); $adapter->setDatabase('database'); From 129e2ac2663609dd813ae6d4d6b008f8cdf96edb Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 16:05:34 +1300 Subject: [PATCH 03/11] fix(validator): stop allowlist regexes accepting a trailing newline Without PCRE_DOLLAR_ENDONLY, `$` also matches before a final "\n", so a join alias, aggregate alias, joined column, UUIDv7 sequence, SQL hook identifier, collation or integer string carrying one passed its allowlist and reached the query. The D modifier anchors `$` to the end of the subject, as Rounding and ObjectPath already do. BigInt::integralValue and the Postgres hex-WKB check get it too: "5.0\n" is not a whole decimal and "ab\n" is not hex. Parsers whose input cannot carry a trailing newline (collapsed or trimmed whitespace, first error line) or where D would loosen a deny-list are left unchanged. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/Postgres.php | 4 +- .../Adapter/SQL/Hook/Column/AllowNull.php | 2 +- .../Adapter/SQL/Hook/Permission/Filter.php | 6 +-- src/Database/Validator/BigInt.php | 4 +- src/Database/Validator/Query/Aggregate.php | 2 +- src/Database/Validator/Query/Base.php | 2 +- src/Database/Validator/Query/Join.php | 2 +- src/Database/Validator/Sequence.php | 2 +- tests/unit/Hook/ColumnNameTest.php | 21 ++++++++++ tests/unit/Validator/BigIntTest.php | 15 +++++++ tests/unit/Validator/Query/AggregateTest.php | 6 +++ tests/unit/Validator/Query/BaseTest.php | 41 +++++++++++++++++++ tests/unit/Validator/Query/JoinAliasTest.php | 2 + tests/unit/Validator/SequenceTest.php | 22 ++++++++++ 14 files changed, 119 insertions(+), 12 deletions(-) create mode 100644 tests/unit/Validator/Query/BaseTest.php create mode 100644 tests/unit/Validator/SequenceTest.php diff --git a/src/Database/Adapter/Postgres.php b/src/Database/Adapter/Postgres.php index 0d40093d6..9b22d47bc 100644 --- a/src/Database/Adapter/Postgres.php +++ b/src/Database/Adapter/Postgres.php @@ -62,7 +62,7 @@ class Postgres extends SQL implements Feature\Spatial, Feature\Timeouts private const string QUOTED_IDENTIFIER = '/["\x{AB}\x{BB}\x{201C}\x{201D}\x{201E}\x{300C}\x{300D}][\s\x{A0}\x{202F}]*([^"\x{AB}\x{BB}\x{201C}\x{201D}\x{201E}\x{300C}\x{300D}]+?)[\s\x{A0}\x{202F}]*["\x{AB}\x{BB}\x{201C}\x{201D}\x{201E}\x{300C}\x{300D}]/u'; - private const string HASHED_IDENTIFIER = '/^[0-9a-f]{32}(?:_[A-Za-z0-9_-]+)?$/'; + private const string HASHED_IDENTIFIER = '/^[0-9a-f]{32}(?:_[A-Za-z0-9_-]+)?$/D'; /** * The catalog's format_type() spellings mapped onto getSqlType()'s. @@ -1462,7 +1462,7 @@ protected function decodePolygon(string $wkb): array }, $rings); } - if (preg_match('/^[0-9a-fA-F]+$/', $wkb)) { + if (\preg_match('/^[0-9a-fA-F]+$/D', $wkb) === 1) { $wkb = hex2bin($wkb); if ($wkb === false) { throw new DatabaseException('Invalid hex WKB'); diff --git a/src/Database/Adapter/SQL/Hook/Column/AllowNull.php b/src/Database/Adapter/SQL/Hook/Column/AllowNull.php index 8e8339fd0..ce1d14aa0 100644 --- a/src/Database/Adapter/SQL/Hook/Column/AllowNull.php +++ b/src/Database/Adapter/SQL/Hook/Column/AllowNull.php @@ -9,7 +9,7 @@ final readonly class AllowNull implements Filter { - private const string IDENTIFIER_PATTERN = '/^[a-zA-Z0-9_\-][a-zA-Z0-9_.\-]*$/'; + private const string IDENTIFIER_PATTERN = '/^[a-zA-Z0-9_\-][a-zA-Z0-9_.\-]*$/D'; public function __construct( private Filter $filter, diff --git a/src/Database/Adapter/SQL/Hook/Permission/Filter.php b/src/Database/Adapter/SQL/Hook/Permission/Filter.php index 8e8bbaff1..b0e2bcc9d 100644 --- a/src/Database/Adapter/SQL/Hook/Permission/Filter.php +++ b/src/Database/Adapter/SQL/Hook/Permission/Filter.php @@ -21,13 +21,13 @@ */ readonly class Filter implements FilterHook, JoinFilter { - private const string IDENTIFIER_PATTERN = '/^[a-zA-Z_][a-zA-Z0-9_.\-]*$/'; + private const string IDENTIFIER_PATTERN = '/^[a-zA-Z_][a-zA-Z0-9_.\-]*$/D'; - private const string QUOTED_IDENTIFIER_PATTERN = '/^[a-zA-Z0-9_\-][a-zA-Z0-9_.\-]*$/'; + private const string QUOTED_IDENTIFIER_PATTERN = '/^[a-zA-Z0-9_\-][a-zA-Z0-9_.\-]*$/D'; private const string NO_SEMIJOIN = '/*+ NO_SEMIJOIN() */ '; - private const string COLLATION_PATTERN = '/^[a-zA-Z_][a-zA-Z0-9_]*$/'; + private const string COLLATION_PATTERN = '/^[a-zA-Z_][a-zA-Z0-9_]*$/D'; protected string $documentCollation; diff --git a/src/Database/Validator/BigInt.php b/src/Database/Validator/BigInt.php index 3f95dfd36..94928406a 100644 --- a/src/Database/Validator/BigInt.php +++ b/src/Database/Validator/BigInt.php @@ -64,7 +64,7 @@ public function isValid(mixed $value): bool public static function isIntegerString(string $value, bool $signed = true): bool { - return \preg_match($signed ? '/^-?\d+$/' : '/^\d+$/', $value) === 1; + return \preg_match($signed ? '/^-?\d+$/D' : '/^\d+$/D', $value) === 1; } /** @@ -390,7 +390,7 @@ public static function integralValue(int|float|string $value): int|string|null } if (\is_string($value)) { - return \preg_match('/^(-?\d+)\.0+$/', $value, $matches) === 1 ? self::toNative($matches[1]) : null; + return \preg_match('/^(-?\d+)\.0+$/D', $value, $matches) === 1 ? self::toNative($matches[1]) : null; } if (! \is_float($value) || ! \is_finite($value) || \floor($value) !== $value) { diff --git a/src/Database/Validator/Query/Aggregate.php b/src/Database/Validator/Query/Aggregate.php index cf197460c..02ef52f7c 100644 --- a/src/Database/Validator/Query/Aggregate.php +++ b/src/Database/Validator/Query/Aggregate.php @@ -20,7 +20,7 @@ class Aggregate extends Base public const int MAX_ALIAS_LENGTH = 63; - private const string ALIAS_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*$/'; + private const string ALIAS_PATTERN = '/^[A-Za-z_][A-Za-z0-9_]*$/D'; private const array NUMERIC_METHODS = [ Method::Sum, diff --git a/src/Database/Validator/Query/Base.php b/src/Database/Validator/Query/Base.php index ffbe0e665..dfafd8c68 100644 --- a/src/Database/Validator/Query/Base.php +++ b/src/Database/Validator/Query/Base.php @@ -90,7 +90,7 @@ protected function isValidQuery(Query $query): bool */ protected function isAllowedJoinColumn(string $column): bool { - return $column !== '' && \preg_match('/^[A-Za-z_$][A-Za-z0-9_$]*$/', $column) === 1; + return $column !== '' && \preg_match('/^[A-Za-z_$][A-Za-z0-9_$]*$/D', $column) === 1; } abstract public function getMethodType(): string; diff --git a/src/Database/Validator/Query/Join.php b/src/Database/Validator/Query/Join.php index b3b229e92..b8bafccd7 100644 --- a/src/Database/Validator/Query/Join.php +++ b/src/Database/Validator/Query/Join.php @@ -18,7 +18,7 @@ class Join extends Base /** * The characters of an Appwrite ID but `.`, which separates an alias from its column. */ - public const string ALIAS_PATTERN = '/^[A-Za-z0-9_][A-Za-z0-9_-]*$/'; + public const string ALIAS_PATTERN = '/^[A-Za-z0-9_][A-Za-z0-9_-]*$/D'; /** * PostgreSQL folds an identifier past 63 bytes, so two longer aliases could read as one. diff --git a/src/Database/Validator/Sequence.php b/src/Database/Validator/Sequence.php index 59153747a..88ac6f865 100644 --- a/src/Database/Validator/Sequence.php +++ b/src/Database/Validator/Sequence.php @@ -65,7 +65,7 @@ public function isValid(mixed $value): bool $idType = ColumnType::tryFrom($this->idAttributeType); return match ($idType) { - ColumnType::Uuid7 => \is_string($value) && preg_match('/^[a-f0-9]{8}-[a-f0-9]{4}-7[a-f0-9]{3}-[89ab][a-f0-9]{3}-[a-f0-9]{12}$/i', $value) === 1, + ColumnType::Uuid7 => \is_string($value) && \preg_match('/^[a-f0-9]{8}-[a-f0-9]{4}-7[a-f0-9]{3}-[89ab][a-f0-9]{3}-[a-f0-9]{12}$/Di', $value) === 1, ColumnType::Integer => (new Range(1, Database::MAX_BIG_INT, ColumnType::Integer->value))->isValid($value), default => false, }; diff --git a/tests/unit/Hook/ColumnNameTest.php b/tests/unit/Hook/ColumnNameTest.php index 83effb93e..3422bd663 100644 --- a/tests/unit/Hook/ColumnNameTest.php +++ b/tests/unit/Hook/ColumnNameTest.php @@ -39,6 +39,7 @@ public static function invalidColumns(): iterable yield 'a statement separator' => ['x;y']; yield 'a quote character' => ['x`y']; yield 'an empty name' => ['']; + yield 'a trailing newline' => ["_uid\n"]; } #[DataProvider('invalidColumns')] @@ -110,6 +111,26 @@ public function testPermissionFilterRejectsAPermissionsTableOutsideTheIdentifier $filter->filter('posts'); } + public function testPermissionFilterRejectsAPermissionsTableWithATrailingNewline(): void + { + $filter = new Filter(['any'], static fn (string $table): string => $table."_perms\n"); + + $this->expectException(DatabaseException::class); + $this->expectExceptionMessage("Invalid permissions table name: posts_perms\n"); + + $filter->filter('posts'); + } + + public function testPermissionFilterRejectsACollationWithATrailingNewline(): void + { + $filter = new Filter(['any'], static fn (string $table): string => $table.'_perms'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage("Invalid collation name: utf8mb4_bin\n"); + + $filter->collate("utf8mb4_bin\n"); + } + public function testPermissionFilterWithNoColumnsMatchesOnlyCollectionWidePermissions(): void { $filter = new Filter(['any'], static fn (string $table): string => $table.'_perms', columns: []); diff --git a/tests/unit/Validator/BigIntTest.php b/tests/unit/Validator/BigIntTest.php index 31f4ae659..584f4599a 100644 --- a/tests/unit/Validator/BigIntTest.php +++ b/tests/unit/Validator/BigIntTest.php @@ -27,6 +27,7 @@ public static function wholeNumbers(): iterable yield 'negative decimal with a zero fraction' => ['-7.0', '-7']; yield 'decimal with an empty fraction' => ['7.', '7']; yield 'decimal with a fraction' => ['7.0500', null]; + yield 'integer with a trailing newline' => ["7\n", null]; yield 'exponent' => ['1.8446744073709552e19', null]; yield 'infinity' => ['INF', null]; yield 'empty' => ['', null]; @@ -103,6 +104,7 @@ public static function valuesThatAreNotIntegers(): iterable yield 'null' => [null]; yield 'a word' => ['abc']; yield 'a decimal string' => ['1.5']; + yield 'an integer string with a trailing newline' => ["5\n"]; } public function testPowerRefusesANegativeExponent(): void @@ -180,4 +182,17 @@ public function testNativeIntegersNormalizeToTheirDigits(): void $this->assertSame(\PHP_INT_MIN, BigInt::toNative(\PHP_INT_MIN)); $this->assertSame(12, BigInt::toNative(12)); } + + public function testAnIntegerStringWithATrailingNewlineIsNotAnInteger(): void + { + $this->assertFalse(BigInt::isIntegerString("5\n")); + $this->assertFalse(BigInt::isIntegerString("5\n", false)); + $this->assertFalse((new BigInt(true))->isValid("5\n")); + } + + public function testADecimalWithATrailingNewlineHasNoIntegralValue(): void + { + $this->assertSame(5, BigInt::integralValue('5.0')); + $this->assertNull(BigInt::integralValue("5.0\n")); + } } diff --git a/tests/unit/Validator/Query/AggregateTest.php b/tests/unit/Validator/Query/AggregateTest.php index 7c6a05f2a..550da0cd3 100644 --- a/tests/unit/Validator/Query/AggregateTest.php +++ b/tests/unit/Validator/Query/AggregateTest.php @@ -53,6 +53,12 @@ public function testInvalidAliasRejected(): void $this->assertSame('Invalid aggregate alias', $this->validator->getDescription()); } + public function testAliasWithATrailingNewlineRejected(): void + { + $this->assertFalse($this->validator->isValid(Query::sum('price', "total\n"))); + $this->assertSame('Invalid aggregate alias', $this->validator->getDescription()); + } + public function testUnknownAttributeAllowedWithoutAttributeSupport(): void { $validator = new Aggregate(attributes: [], supportForAttributes: false); diff --git a/tests/unit/Validator/Query/BaseTest.php b/tests/unit/Validator/Query/BaseTest.php new file mode 100644 index 000000000..f92508cd1 --- /dev/null +++ b/tests/unit/Validator/Query/BaseTest.php @@ -0,0 +1,41 @@ +isAllowedJoinColumn($column); + } + + #[\Override] + public function getMethodType(): string + { + return self::METHOD_TYPE_FILTER; + } + }; + + $this->assertSame($allowed, $validator->allows($column)); + } + + /** + * @return iterable + */ + public static function joinColumns(): iterable + { + yield 'an identifier' => ['title', true]; + yield 'an internal attribute' => ['$id', true]; + yield 'an empty name' => ['', false]; + yield 'a dotted path' => ['a.b', false]; + yield 'a trailing newline' => ["title\n", false]; + } +} diff --git a/tests/unit/Validator/Query/JoinAliasTest.php b/tests/unit/Validator/Query/JoinAliasTest.php index a34848567..df010d713 100644 --- a/tests/unit/Validator/Query/JoinAliasTest.php +++ b/tests/unit/Validator/Query/JoinAliasTest.php @@ -75,6 +75,7 @@ public static function rejectedJoins(): iterable yield 'past 63 characters' => [Query::join('orders', \str_repeat('a', 64), [Query::on('$id', 'customerId')]), 'Join alias must be at most 63 characters']; yield 'a dot' => [Query::join('orders', 'a.b', [Query::on('$id', 'customerId')]), $invalid]; yield 'a quote' => [Query::crossJoin('orders', 'x`y'), $invalid]; + yield 'a trailing newline' => [Query::join('orders', "ord\n", [Query::on('$id', 'customerId')]), $invalid]; } #[DataProvider('rejectedTableIds')] @@ -99,6 +100,7 @@ public static function rejectedTableIds(): iterable yield 'a dot' => [Query::leftJoin('orders.v2', '', [Query::on('$id', 'customerId')]), $pattern('orders.v2')]; yield 'a leading hyphen' => [Query::rightJoin('-orders', '', [Query::on('$id', 'customerId')]), $pattern('-orders')]; + yield 'a trailing newline' => [Query::crossJoin("orders\n", ''), $pattern("orders\n")]; yield 'a dot on a cross join' => [Query::crossJoin('orders.v2', ''), $pattern('orders.v2')]; yield 'a dot on a join built without an alias' => [new Query(Method::FullOuterJoin, 'orders.v2', [Query::on('$id', 'customerId')]), $pattern('orders.v2')]; yield 'a dot on a parsed join' => [Query::parseQuery(['method' => 'join', 'attribute' => 'orders.v2', 'values' => ['$id', '=', 'customerId']]), $pattern('orders.v2')]; diff --git a/tests/unit/Validator/SequenceTest.php b/tests/unit/Validator/SequenceTest.php new file mode 100644 index 000000000..d70f85cec --- /dev/null +++ b/tests/unit/Validator/SequenceTest.php @@ -0,0 +1,22 @@ +assertTrue((new Sequence(ColumnType::Uuid7->value, true))->isValid(self::UUID7)); + } + + public function testRejectsAUuid7WithATrailingNewline(): void + { + $this->assertFalse((new Sequence(ColumnType::Uuid7->value, true))->isValid(self::UUID7."\n")); + } +} From 7981f21ba41595a93f6207181975d6ce3b4c185c Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 16:05:20 +1300 Subject: [PATCH 04/11] fix(mongo): scope each read's maxTimeMS to its own event setTimeout() and clearTimeout() ignored their event, so a timeout meant for one event (as Pool::syncTimeouts() forwards each one) bounded every find, count and sum, and clearing any event dropped the global timeout too. The adapter now keeps per-event state through the Timeout trait like the SQL adapters and Pool, find/count/sum read DocumentFind/DocumentCount/DocumentSum, and a non-positive timeout clears that event as Pool does. count() also loses option assignments that getTransactionOptions() always overwrote. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/Mongo.php | 36 +++--- tests/unit/MongoTimeoutTest.php | 203 ++++++++++++++++++++++++++++++++ 2 files changed, 220 insertions(+), 19 deletions(-) create mode 100644 tests/unit/MongoTimeoutTest.php diff --git a/src/Database/Adapter/Mongo.php b/src/Database/Adapter/Mongo.php index aad6142d6..4cfecf7e9 100644 --- a/src/Database/Adapter/Mongo.php +++ b/src/Database/Adapter/Mongo.php @@ -209,13 +209,19 @@ public function capabilities(): array #[\Override] public function setTimeout(int $milliseconds, Event $event = Event::All): void { - $this->timeout = $milliseconds; + if ($milliseconds <= 0) { + $this->clearTimeout($event); + + return; + } + + $this->setTimeoutState($milliseconds, $event); } #[\Override] public function clearTimeout(Event $event = Event::All): void { - $this->timeout = 0; + $this->clearTimeoutState($event); } #[\Override] @@ -2414,8 +2420,9 @@ public function find(Document $collection, array $queries = [], ?int $limit = 25 $options['skip'] = $offset; } - if ($this->timeout) { - $options['maxTimeMS'] = $this->timeout; + $timeout = $this->getTimeout(Event::DocumentFind); + if ($timeout > 0) { + $options['maxTimeMS'] = $timeout; } $selections = $this->getAttributeSelections($queries); @@ -2601,17 +2608,6 @@ public function count(Document $collection, array $queries = [], ?int $max = nul $this->escapeQueryAttributes($collection, $queries); - $filters = []; - $options = []; - - if (! \is_null($max) && $max > 0) { - $options['limit'] = $max; - } - - if ($this->timeout) { - $options['maxTimeMS'] = $this->timeout; - } - /** @var array $filters */ $filters = $this->buildFilters($queries); $filters = $this->applyReadFilters($filters, $collection->getId(), PermissionType::Read); @@ -2626,8 +2622,9 @@ public function count(Document $collection, array $queries = [], ?int $max = nul **/ $options = $this->getTransactionOptions(); - if ($this->timeout) { - $options['maxTimeMS'] = $this->timeout; + $timeout = $this->getTimeout(Event::DocumentCount); + if ($timeout > 0) { + $options['maxTimeMS'] = $timeout; } $pipeline = []; @@ -2719,8 +2716,9 @@ public function sum(Document $collection, string $attribute, array $queries = [] $options = $this->getTransactionOptions(); - if ($this->timeout) { - $options['maxTimeMS'] = $this->timeout; + $timeout = $this->getTimeout(Event::DocumentSum); + if ($timeout > 0) { + $options['maxTimeMS'] = $timeout; } try { diff --git a/tests/unit/MongoTimeoutTest.php b/tests/unit/MongoTimeoutTest.php new file mode 100644 index 000000000..71a95d737 --- /dev/null +++ b/tests/unit/MongoTimeoutTest.php @@ -0,0 +1,203 @@ +> + */ + private array $options = []; + + private Mongo $adapter; + + #[\Override] + protected function setUp(): void + { + $this->options = []; + $this->adapter = $this->createAdapter(); + } + + public function testAnEventTimeoutBoundsOnlyThatEvent(): void + { + $this->adapter->setTimeout(500, Event::DocumentCount); + + $this->assertArrayNotHasKey(self::MAX_TIME, $this->findOptions(), 'a count timeout must not bound find()'); + $this->assertSame(500, $this->countOptions()[self::MAX_TIME] ?? null, 'count() must carry its own timeout'); + $this->assertArrayNotHasKey(self::MAX_TIME, $this->sumOptions(), 'a count timeout must not bound sum()'); + } + + public function testEachReadCarriesItsOwnEventTimeout(): void + { + $this->adapter->setTimeout(100, Event::DocumentFind); + $this->adapter->setTimeout(200, Event::DocumentCount); + $this->adapter->setTimeout(300, Event::DocumentSum); + + $this->assertSame(100, $this->findOptions()[self::MAX_TIME] ?? null); + $this->assertSame(200, $this->countOptions()[self::MAX_TIME] ?? null); + $this->assertSame(300, $this->sumOptions()[self::MAX_TIME] ?? null); + } + + public function testAnEventTimeoutOverridesTheGlobalTimeout(): void + { + $this->adapter->setTimeout(1000); + $this->adapter->setTimeout(50, Event::DocumentFind); + + $this->assertSame(50, $this->findOptions()[self::MAX_TIME] ?? null); + $this->assertSame(1000, $this->countOptions()[self::MAX_TIME] ?? null); + } + + public function testClearingOneEventKeepsTheGlobalTimeout(): void + { + $this->adapter->setTimeout(1000); + $this->adapter->setTimeout(50, Event::DocumentFind); + $this->adapter->clearTimeout(Event::DocumentFind); + + $this->assertSame(1000, $this->findOptions()[self::MAX_TIME] ?? null, 'clearing find must fall back to the global timeout'); + $this->assertSame(1000, $this->countOptions()[self::MAX_TIME] ?? null, 'clearing find must leave count bounded'); + } + + public function testClearingAnEventWithoutItsOwnTimeoutKeepsTheGlobalTimeout(): void + { + $this->adapter->setTimeout(1000); + $this->adapter->clearTimeout(Event::DocumentCount); + + $this->assertSame(1000, $this->countOptions()[self::MAX_TIME] ?? null); + $this->assertSame(1000, $this->findOptions()[self::MAX_TIME] ?? null); + } + + public function testAZeroTimeoutClearsThatEvent(): void + { + $this->adapter->setTimeout(1000); + $this->adapter->setTimeout(50, Event::DocumentCount); + $this->adapter->setTimeout(0, Event::DocumentCount); + + $this->assertSame(1000, $this->countOptions()[self::MAX_TIME] ?? null, 'a zero count timeout must fall back to the global timeout'); + $this->assertSame(1000, $this->findOptions()[self::MAX_TIME] ?? null, 'a zero count timeout must not touch find()'); + $this->assertSame(1000, $this->adapter->getTimeout(Event::DocumentCount)); + } + + public function testClearingTheGlobalTimeoutClearsEveryEvent(): void + { + $this->adapter->setTimeout(1000); + $this->adapter->setTimeout(50, Event::DocumentFind); + $this->adapter->clearTimeout(); + + $this->assertArrayNotHasKey(self::MAX_TIME, $this->findOptions()); + $this->assertArrayNotHasKey(self::MAX_TIME, $this->countOptions()); + $this->assertArrayNotHasKey(self::MAX_TIME, $this->sumOptions()); + } + + /** + * @return array + */ + private function findOptions(): array + { + $this->adapter->find(new Document(['$id' => self::COLLECTION])); + + return $this->lastOptions(); + } + + /** + * @return array + */ + private function countOptions(): array + { + $this->adapter->count(new Document(['$id' => self::COLLECTION])); + + return $this->lastOptions(); + } + + /** + * @return array + */ + private function sumOptions(): array + { + $this->adapter->sum(new Document(['$id' => self::COLLECTION]), 'price'); + + return $this->lastOptions(); + } + + /** + * @return array + */ + private function lastOptions(): array + { + $this->assertNotEmpty($this->options, 'the read must reach the client'); + + return $this->options[\array_key_last($this->options)]; + } + + private function createAdapter(): Mongo + { + $record = function (array $options): void { + $this->options[] = $options; + }; + + $client = new class ($record) extends Client { + /** + * @param Closure(array): void $record + */ + public function __construct(private readonly Closure $record) + { + } + + #[\Override] + public function connect(): self + { + return $this; + } + + #[\Override] + public function close(): void + { + } + + /** + * @param array $filters + * @param array $options + */ + #[\Override] + public function find(string $collection, array $filters = [], array $options = []): stdClass + { + ($this->record)($options); + + return (object) ['cursor' => (object) ['firstBatch' => [], 'id' => 0]]; + } + + /** + * @param array $pipeline + * @param array $options + */ + #[\Override] + public function aggregate(string $collection, array $pipeline, array $options = []): stdClass + { + ($this->record)($options); + + return (object) ['cursor' => (object) ['firstBatch' => []]]; + } + }; + + $authorization = new Authorization(); + $authorization->disable(); + + $adapter = new Mongo($client); + $adapter->setAuthorization($authorization); + $adapter->setNamespace('timeouts'); + + return $adapter; + } +} From 8a3dfa6ea87c69c18039504ead9cac5b39ba0925 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:16:06 +1300 Subject: [PATCH 05/11] fix(sql): let relationship ddl converge after a partial failure With DDL run one statement per prepare, PostgreSQL no longer applies a two-way relationship's columns atomically. The database recovers from a partial create by catching DuplicateException and from a partial delete by catching NotFoundException, but the SQL adapters threw the raw PDOException, so neither recovery ever ran, and a retry stopped at the first column a previous attempt had already handled. Relationship create and delete now map engine errors and carry on past a statement the schema already reflects, then report that refusal, so a retry applies the rest and the database writes or removes the metadata. deleteCollection shares the same path. Inside a transaction the first refusal is thrown at once, as PostgreSQL rejects the rest of an aborted transaction. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/SQL.php | 52 ++++-- src/Database/Adapter/SQLite.php | 4 +- tests/unit/Adapter/PostgresPartialDDLTest.php | 172 ++++++++++++++++++ 3 files changed, 214 insertions(+), 14 deletions(-) create mode 100644 tests/unit/Adapter/PostgresPartialDDLTest.php diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index 06c1a498c..02f1d840b 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -3080,16 +3080,7 @@ public function deleteCollection(string $collection): bool $main = $schema->table($this->getTableRaw($id))->drop(); $permissions = $schema->table($this->getTableRaw(Storage::permissionsTable($id)))->dropIfExists(); - try { - return $this->executeStatements([$main->query, $permissions->query], Event::CollectionDelete); - } catch (PDOException $e) { - $error = $this->processException($e); - if ($error instanceof NotFoundException && $this->inTransaction === 0) { - $this->executeStatement($permissions->query, Event::CollectionDelete); - } - - throw $error; - } + return $this->executeConvergingStatements([$main->query, $permissions->query], Event::CollectionDelete, NotFoundException::class); } /** @@ -3145,7 +3136,7 @@ public function createRelationship(string $collection, Relationship $relationshi RelationshipType::ManyToMany => [], }; - return $this->executeStatements($statements, Event::AttributeCreate); + return $this->executeConvergingStatements($statements, Event::AttributeCreate, DuplicateException::class); } /** @@ -3277,7 +3268,7 @@ public function deleteRelationship(string $collection, Relationship $relationshi break; } - return $this->executeStatements($statements, Event::AttributeDelete); + return $this->executeConvergingStatements($statements, Event::AttributeDelete, NotFoundException::class); } /** @@ -3884,6 +3875,43 @@ protected function executeStatements(array $statements, Event $event): bool return true; } + /** + * Run DDL that a failed earlier attempt may have partly applied: a statement refused with $applied is taken as + * already applied and the rest still run, then that refusal is thrown so the caller knows the schema held part + * of it. Inside a transaction the first refusal is thrown at once, since PostgreSQL refuses every later + * statement of an aborted transaction. + * + * @param list $statements + * @param class-string $applied + * + * @throws DatabaseException + */ + protected function executeConvergingStatements(array $statements, Event $event, string $applied): bool + { + $refusal = null; + + foreach ($statements as $statement) { + try { + if (! $this->executeStatement($statement, $event)) { + return false; + } + } catch (PDOException $exception) { + $error = $this->processException($exception); + if (! $error instanceof $applied || $this->inTransaction > 0) { + throw $error; + } + + $refusal ??= $error; + } + } + + if ($refusal !== null) { + throw $refusal; + } + + return true; + } + private function transformQuery(Event $event, string $sql): string { foreach ($this->transforms as $transform) { diff --git a/src/Database/Adapter/SQLite.php b/src/Database/Adapter/SQLite.php index e335c83bd..cb76fd27f 100644 --- a/src/Database/Adapter/SQLite.php +++ b/src/Database/Adapter/SQLite.php @@ -2487,7 +2487,7 @@ public function createRelationship(string $collection, Relationship $relationshi RelationshipType::ManyToMany => [], }; - return $this->executeStatements($statements, Event::AttributeCreate); + return $this->executeConvergingStatements($statements, Event::AttributeCreate, DuplicateException::class); } #[\Override] @@ -2588,7 +2588,7 @@ public function deleteRelationship(string $collection, Relationship $relationshi break; } - return $this->executeStatements($statements, Event::AttributeDelete); + return $this->executeConvergingStatements($statements, Event::AttributeDelete, NotFoundException::class); } /** diff --git a/tests/unit/Adapter/PostgresPartialDDLTest.php b/tests/unit/Adapter/PostgresPartialDDLTest.php new file mode 100644 index 000000000..eb6c9356c --- /dev/null +++ b/tests/unit/Adapter/PostgresPartialDDLTest.php @@ -0,0 +1,172 @@ + */ + private array $statements = []; + + /** @var array */ + private array $failures = []; + + public function testADeleteWhosePermissionsDropFailedDropsThePermissionsTableOnRetry(): void + { + $adapter = $this->adapter(); + $this->failures[self::DROP_BOOKS_PERMISSIONS] = $this->engineError('57014', 'canceling statement due to statement timeout'); + + try { + $adapter->deleteCollection('books'); + $this->fail('A permissions table that fails to drop must fail the delete'); + } catch (TimeoutException $error) { + $this->assertSame('Query timed out', $error->getMessage()); + } + + $this->statements = []; + $this->failures = [self::DROP_BOOKS => $this->engineError('42P01', 'table "namespace_books" does not exist')]; + + try { + $adapter->deleteCollection('books'); + $this->fail('A retry whose collection table is gone must report it as not found'); + } catch (NotFoundException $error) { + $this->assertSame('Collection not found', $error->getMessage()); + } + + $this->assertSame([self::DROP_BOOKS, self::DROP_BOOKS_PERMISSIONS], $this->statements); + } + + public function testACreateRetriedAfterItsFirstColumnWasAddedAddsTheRest(): void + { + $this->failures[self::ADD_AUTHOR] = $this->engineError('42701', 'column "author" of relation "namespace_books" already exists'); + + try { + $this->adapter()->createRelationship('books', self::oneToOne()); + $this->fail('A relationship column that already exists must be reported as a duplicate'); + } catch (DuplicateException $error) { + $this->assertSame('Attribute already exists', $error->getMessage()); + } + + $this->assertCount(2, $this->statements); + $this->assertStringStartsWith(self::ADD_AUTHOR, $this->statements[0]); + $this->assertStringStartsWith(self::ADD_BOOK, $this->statements[1]); + } + + public function testADeleteRetriedAfterItsFirstColumnWasDroppedDropsTheRest(): void + { + $this->failures[self::DROP_AUTHOR] = $this->engineError('42703', 'column "author" of relation "namespace_books" does not exist'); + + try { + $this->adapter()->deleteRelationship('books', self::oneToOne(), RelationshipSide::Parent); + $this->fail('A relationship column that is already gone must be reported as not found'); + } catch (NotFoundException $error) { + $this->assertSame('Attribute not found', $error->getMessage()); + } + + $this->assertCount(2, $this->statements); + $this->assertStringStartsWith(self::DROP_AUTHOR, $this->statements[0]); + $this->assertStringStartsWith(self::DROP_BOOK, $this->statements[1]); + } + + public function testAColumnThatFailsForAnotherReasonStopsTheCreate(): void + { + $this->failures[self::ADD_AUTHOR] = $this->engineError('57014', 'canceling statement due to statement timeout'); + + try { + $this->adapter()->createRelationship('books', self::oneToOne()); + $this->fail('A relationship column that times out must fail the create'); + } catch (TimeoutException $error) { + $this->assertSame('Query timed out', $error->getMessage()); + } + + $this->assertCount(1, $this->statements); + } + + public function testAnAppliedColumnInsideATransactionStopsTheCreate(): void + { + $adapter = $this->adapter(); + $adapter->startTransaction(); + $started = \count($this->statements); + $this->failures[self::ADD_AUTHOR] = $this->engineError('42701', 'column "author" of relation "namespace_books" already exists'); + + try { + $adapter->createRelationship('books', self::oneToOne()); + $this->fail('A relationship column that already exists must be reported as a duplicate'); + } catch (DuplicateException $error) { + $this->assertSame('Attribute already exists', $error->getMessage()); + } + + $this->assertCount(1, \array_slice($this->statements, $started), 'An aborted transaction refuses every later statement'); + } + + private static function oneToOne(): Relationship + { + return Relationship::oneToOne(relatedCollection: 'authors', key: 'author', twoWay: true, twoWayKey: 'book'); + } + + private function engineError(string $state, string $message): PDOException + { + $error = new class ('SQLSTATE['.$state.']: '.$message, $state) extends PDOException { + public function __construct(string $message, string $state) + { + parent::__construct($message); + $this->code = $state; + } + }; + $error->errorInfo = [$state, 7, $message]; + + return $error; + } + + private function adapter(): Postgres + { + $pdo = $this->createStub(PDO::class); + $pdo->method('beginTransaction')->willReturn(true); + $pdo->method('prepare')->willReturnCallback(function (string $query): PDOStatement { + $this->statements[] = $query; + $failure = null; + foreach ($this->failures as $prefix => $error) { + if (\str_starts_with($query, $prefix)) { + $failure = $error; + } + } + + $statement = $this->createStub(PDOStatement::class); + if ($failure === null) { + $statement->method('execute')->willReturn(true); + } else { + $statement->method('execute')->willThrowException($failure); + } + + return $statement; + }); + + $adapter = new Postgres($pdo); + $adapter->setDatabase('database'); + $adapter->setNamespace('namespace'); + + return $adapter; + } +} From d62d1afeff1d63573cfda4ca46e143ee69c54622 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:09:02 +1300 Subject: [PATCH 06/11] fix(pool): drop a borrowed connection's stale per-event timeouts A pooled connection keeps the per-event timeouts its last holder set, and an adapter prefers a per-event entry over the global one. The global-only shortcut set the global timeout without clearing them, so a handle asking for 1000ms everywhere ran finds under another request's 50ms. Each sync now clears every event the pool holds no entry for, which also avoids a full clear and its extra SET round trip on MariaDB when only the global timeout is set. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/Pool.php | 30 ++++++++++++----------------- tests/unit/Adapter/PoolSyncTest.php | 2 +- tests/unit/PoolTimeoutTest.php | 19 ++++++++++++++++++ 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/Database/Adapter/Pool.php b/src/Database/Adapter/Pool.php index 2654813bf..97e110797 100644 --- a/src/Database/Adapter/Pool.php +++ b/src/Database/Adapter/Pool.php @@ -556,34 +556,28 @@ protected function syncTimeouts(Adapter $adapter): void } /** @var Adapter&Feature\Timeouts $adapter */ - if (empty($this->timeouts)) { + $global = $this->timeouts[Event::All->value] ?? null; + if ($global === null) { $adapter->clearTimeout(); - - return; } - if (count($this->timeouts) === 1 && isset($this->timeouts[Event::All->value])) { - $adapter->setTimeout($this->timeouts[Event::All->value]); + foreach (Event::cases() as $event) { + if ($event === Event::All) { + continue; + } - return; - } + $milliseconds = $this->timeouts[$event->value] ?? null; + if ($milliseconds === null) { + $adapter->clearTimeout($event); - // The concrete adapters keep one timeout scalar, which Postgres writes - // into SET statement_timeout and Mongo into maxTimeMS for every - // statement, so the last value applied is the one every statement runs - // under. Apply the per-event entries first and the global one last, or a - // per-event timeout set after the global one bounds everything. - $adapter->clearTimeout(); - foreach ($this->timeouts as $event => $milliseconds) { - if ($event === Event::All->value) { continue; } - $adapter->setTimeout($milliseconds, Event::from($event)); + $adapter->setTimeout($milliseconds, $event); } - if (isset($this->timeouts[Event::All->value])) { - $adapter->setTimeout($this->timeouts[Event::All->value]); + if ($global !== null) { + $adapter->setTimeout($global); } } diff --git a/tests/unit/Adapter/PoolSyncTest.php b/tests/unit/Adapter/PoolSyncTest.php index 5eed527e8..e37c76bda 100644 --- a/tests/unit/Adapter/PoolSyncTest.php +++ b/tests/unit/Adapter/PoolSyncTest.php @@ -225,7 +225,7 @@ public static function changes(): iterable yield 'timeout per event' => [ static fn (Pool $pool) => $pool->setTimeout(self::EVENT_TIMEOUT, Event::DocumentFind), $timeouts, - [Event::DocumentFind->value => self::EVENT_TIMEOUT, Event::All->value => self::TIMEOUT], + [Event::All->value => self::TIMEOUT, Event::DocumentFind->value => self::EVENT_TIMEOUT], ]; yield 'timeout cleared' => [ static fn (Pool $pool) => $pool->clearTimeout(), diff --git a/tests/unit/PoolTimeoutTest.php b/tests/unit/PoolTimeoutTest.php index 4bbaafd2b..ddf3752fd 100644 --- a/tests/unit/PoolTimeoutTest.php +++ b/tests/unit/PoolTimeoutTest.php @@ -135,6 +135,25 @@ public function testHandleWithNoTimeoutResetsTheConnectionItIsGiven(): void $this->assertSame(0, $connection->getTimeout()); } + public function testGlobalOnlyHandleDropsThePerEventTimeoutTheLastHolderLeft(): void + { + $connection = new TimeoutRecordingMemory(); + $pool = new UtopiaPool(new Stack(), 'memory', 1, fn () => $connection, timeout: 0.0); + + $perEvent = new Pool($pool); + $perEvent->setAuthorization(new Authorization()); + $perEvent->setTimeout(50, Event::DocumentFind); + $perEvent->getDriver(); + + $global = new Pool($pool); + $global->setAuthorization(new Authorization()); + $global->setTimeout(1000); + $global->getDriver(); + + $this->assertSame([Event::All->value => 1000], $connection->timeouts, 'A handle with only a global timeout must not inherit the last holder\'s per-event one'); + $this->assertSame(1000, $connection->getTimeout(Event::DocumentFind)); + } + /** * The pool's own map is what a checkout replays, so clearing every timeout * has to empty it. The inherited implementation walks the events it finds From 51be5064606a8ff94ab21977883c67708d0c26ab Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:25:12 +1300 Subject: [PATCH 07/11] fix(validator): refuse keys that reach a storage column once filtered The adapter drops a key whose filtered name is a reserved column, but the validator checked the raw key, so `_u!id` or `_ten.ant` passed validation and, on schemaless adapters, was accepted and then silently dropped on write. Filtering through the same static helper the adapter uses keeps the two checks in lockstep; keys Validator\Key accepts never start with an underscore, so legitimate attributes are unaffected. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter.php | 10 ++++++++-- src/Database/Validator/Structure.php | 3 ++- tests/unit/Validator/StructureTest.php | 26 ++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/src/Database/Adapter.php b/src/Database/Adapter.php index 9f6291daf..f8ea65360 100644 --- a/src/Database/Adapter.php +++ b/src/Database/Adapter.php @@ -965,11 +965,17 @@ protected function getInternalKeyForAttribute(string $attribute): string private const int FILTERED_KEY_CACHE_LIMIT = 4096; /** - * Filter Keys - * * @throws DatabaseException */ public function filter(string $value): string + { + return self::filterKey($value); + } + + /** + * @throws DatabaseException + */ + public static function filterKey(string $value): string { if (isset(self::$filteredKeyCache[$value])) { return self::$filteredKeyCache[$value]; diff --git a/src/Database/Validator/Structure.php b/src/Database/Validator/Structure.php index 261c22f53..224c4b21d 100644 --- a/src/Database/Validator/Structure.php +++ b/src/Database/Validator/Structure.php @@ -4,6 +4,7 @@ use Closure; use Exception; +use Utopia\Database\Adapter; use Utopia\Database\Adapter\Profile; use Utopia\Database\Attribute; use Utopia\Database\Capability; @@ -240,7 +241,7 @@ protected function checkForUnknownAttributes(array $structure, array $definition // A key named like a column the storage keeps for the document itself, even one declared as an attribute, // would be written over the document's own $sequence, $tenant or $permissions. foreach (\array_keys($structure) as $key) { - if (Storage::isReserved((string) $key)) { + if (Storage::isReserved(Adapter::filterKey((string) $key))) { $this->message = 'Reserved attribute: "'.$key.'"'; return false; diff --git a/tests/unit/Validator/StructureTest.php b/tests/unit/Validator/StructureTest.php index cc87963d4..a8fcdef34 100644 --- a/tests/unit/Validator/StructureTest.php +++ b/tests/unit/Validator/StructureTest.php @@ -365,6 +365,32 @@ public function test_reserved_keys_without_defined_attributes(): void } } + public function test_reserved_keys_after_filtering(): void + { + $document = [ + '$collection' => Id::custom('posts'), + 'title' => 'Demo Title', + 'description' => 'Demo description', + 'rating' => 5, + 'price' => 1.99, + 'published' => true, + 'tags' => ['dog', 'cat', 'mouse'], + 'feedback' => 'team@appwrite.io', + '$createdAt' => '2000-04-01T12:00:00.000+00:00', + '$updatedAt' => '2000-04-01T12:00:00.000+00:00', + ]; + + foreach ([[Capability::UnsignedBigInt], [Capability::DefinedAttributes, Capability::UnsignedBigInt]] as $capabilities) { + $validator = new Structure(new Document($this->collection), Profiles::of(capabilities: $capabilities)); + $this->assertTrue($validator->isValid(new Document($document)), $validator->getDescription()); + + foreach (['_u!id', '_ten.ant', '_perm$issions', '_i d'] as $key) { + $this->assertFalse($validator->isValid(new Document([...$document, $key => 'stored over the document\'s own field'])), $key); + $this->assertSame('Invalid document structure: Reserved attribute: "'.$key.'"', $validator->getDescription()); + } + } + } + public function test_reserved_keys_with_defined_attributes(): void { $collection = $this->collection; From 3240196176fec079625c2b4a91f3a3c3de9739e8 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:44:37 +1300 Subject: [PATCH 08/11] fix(memory): refuse increments of storage columns increaseDocumentAttribute filtered the attribute name itself, so an increment of a reserved column such as `_tenant` or `_ten.ant` rewrote the document's own tenant, sequence or timestamps. It now resolves the column through writableColumn() like the SQL and Mongo adapters, so the increment is refused with NotFound before the document is touched. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/Memory.php | 20 +++++++++--------- tests/unit/Adapter/MemoryWritePathsTest.php | 23 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/Database/Adapter/Memory.php b/src/Database/Adapter/Memory.php index af8760bbd..1d51bada9 100644 --- a/src/Database/Adapter/Memory.php +++ b/src/Database/Adapter/Memory.php @@ -1896,15 +1896,15 @@ public function sum(Document $collection, string $attribute, array $queries = [] #[\Override] public function increaseDocumentAttribute(Document $collection, string $id, string $attribute, int|float|string $value, string $updatedAt, int|float|string|null $min = null, int|float|string|null $max = null): bool { + $column = $this->writableColumn($attribute); $key = $this->key($collection->getId()); - $docKey = $this->documentKey($id); - if (! isset($this->data[$key]['documents'][$docKey])) { + $documentKey = $this->documentKey($id); + if (! isset($this->data[$key]['documents'][$documentKey])) { throw new NotFoundException('Document not found'); } - $column = $this->filter($attribute); - $previousValue = $this->data[$key]['documents'][$docKey][$column] ?? null; - $previousUpdatedAt = $this->data[$key]['documents'][$docKey][Storage::UPDATED_AT] ?? null; + $previousValue = $this->data[$key]['documents'][$documentKey][$column] ?? null; + $previousUpdatedAt = $this->data[$key]['documents'][$documentKey][Storage::UPDATED_AT] ?? null; $current = $previousValue ?? 0; $exact = (\is_int($current) || (\is_string($current) && BigInt::isIntegerString($current))) && (\is_int($value) || (\is_string($value) && BigInt::isIntegerString($value))); @@ -1931,14 +1931,14 @@ public function increaseDocumentAttribute(Document $collection, string $id, stri $result = $current + $value; } - $this->data[$key]['documents'][$docKey][$column] = $result; - $this->data[$key]['documents'][$docKey][Storage::UPDATED_AT] = $updatedAt; + $this->data[$key]['documents'][$documentKey][$column] = $result; + $this->data[$key]['documents'][$documentKey][Storage::UPDATED_AT] = $updatedAt; - $this->journal(function () use ($key, $docKey, $column, $previousValue, $previousUpdatedAt): void { - if (! isset($this->data[$key]['documents'][$docKey])) { + $this->journal(function () use ($key, $documentKey, $column, $previousValue, $previousUpdatedAt): void { + if (! isset($this->data[$key]['documents'][$documentKey])) { return; } - $row = &$this->data[$key]['documents'][$docKey]; + $row = &$this->data[$key]['documents'][$documentKey]; if ($previousValue === null) { unset($row[$column]); } else { diff --git a/tests/unit/Adapter/MemoryWritePathsTest.php b/tests/unit/Adapter/MemoryWritePathsTest.php index 2b8908b75..0ddb0ebd4 100644 --- a/tests/unit/Adapter/MemoryWritePathsTest.php +++ b/tests/unit/Adapter/MemoryWritePathsTest.php @@ -12,12 +12,14 @@ use Utopia\Database\Document; use Utopia\Database\Exception as DatabaseException; use Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\NotFound as NotFoundException; use Utopia\Database\Exception\Unique as UniqueException; use Utopia\Database\Index; use Utopia\Database\Operator; use Utopia\Database\Permission; use Utopia\Database\Query; use Utopia\Database\Role; +use Utopia\Database\Storage; use Utopia\Database\Validator\Authorization; use Utopia\Query\CursorDirection; @@ -156,6 +158,27 @@ public function testRollbackOfAnIncrementRemovesTheValueAndTimestampItAdded(): v $this->assertNull($after->getUpdatedAt()); } + public function testIncreaseRefusesAStorageColumn(): void + { + $adapter = $this->sharedAdapter(); + $this->storeAddress($adapter, 'home', 'x', 1); + $before = $adapter->getDocument($this->collection(), 'home'); + + foreach ([Storage::TENANT, '_ten.ant', Storage::SEQUENCE, Storage::UPDATED_AT] as $attribute) { + try { + $adapter->increaseDocumentAttribute($this->collection(), 'home', $attribute, 5, self::UPDATED_AT); + $this->fail('Incrementing '.$attribute.' must be refused'); + } catch (NotFoundException $exception) { + $this->assertSame('Attribute not found', $exception->getMessage()); + } + } + + $after = $adapter->getDocument($this->collection(), 'home'); + $this->assertSame($before->getTenant(), $after->getTenant()); + $this->assertSame($before->getSequence(), $after->getSequence()); + $this->assertSame($before->getUpdatedAt(), $after->getUpdatedAt()); + } + public function testADivisionOrModuloByZeroThatSkippedValidationKeepsTheValue(): void { $adapter = $this->adapter(); From f6e66157e7c1a46143faad556da4fe063ab35ccc Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 17:09:02 +1300 Subject: [PATCH 09/11] docs: recommend disabling mysql multi-statements The adapters now send one statement per call, so a connection with multi-statements enabled (the MySQL default) only widens what an injected semicolon can do. Co-Authored-By: Claude Opus 5.5 --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 1335b1436..5f755e5ad 100644 --- a/README.md +++ b/README.md @@ -147,6 +147,8 @@ What an adapter supports is reported by `$database->getAdapter()->supports(Capab `Utopia\Database\PDO` wraps PHP's PDO: it reconnects when a connection is lost outside a transaction and retries the call. Each SQL adapter also accepts a plain `PDO`. Pass the PDO attributes your application needs; the ones below are what the adapters expect. +The adapters send one statement per call, so disable MySQL multi-statements (on by default) with `\Pdo\Mysql::ATTR_MULTI_STATEMENTS => false` (PHP 8.4+) for MariaDB and MySQL; an injected `;` then cannot run a second statement. + #### MariaDB ```php @@ -167,6 +169,7 @@ $pdo = new PDO("mysql:host={$host};port={$port};charset=utf8mb4", 'root', 'passw \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION, \PDO::ATTR_EMULATE_PREPARES => true, \PDO::ATTR_STRINGIFY_FETCHES => true, + \Pdo\Mysql::ATTR_MULTI_STATEMENTS => false, ]); $cache = new Cache(new Memory()); // or use any cache adapter you wish @@ -194,6 +197,7 @@ $pdo = new PDO("mysql:host={$host};port={$port};charset=utf8mb4", 'root', 'passw \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION, \PDO::ATTR_EMULATE_PREPARES => true, \PDO::ATTR_STRINGIFY_FETCHES => true, + \Pdo\Mysql::ATTR_MULTI_STATEMENTS => false, ]); $cache = new Cache(new Memory()); // or use any cache adapter you wish From c029eb49c95977c039aab71fcb6df03cc7a2da87 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 18:57:12 +1300 Subject: [PATCH 10/11] style: drop narrating comments and abbreviated catch names A converging DDL run takes a refused statement as one an earlier failed attempt already applied, runs the rest, then rethrows so the caller knows the schema held part of it. Co-Authored-By: Claude Opus 5.5 --- src/Database/Adapter/MariaDB.php | 4 ++-- src/Database/Adapter/SQL.php | 6 ++---- src/Database/Validator/Structure.php | 2 -- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/Database/Adapter/MariaDB.php b/src/Database/Adapter/MariaDB.php index 57c56e967..8c8e1802a 100644 --- a/src/Database/Adapter/MariaDB.php +++ b/src/Database/Adapter/MariaDB.php @@ -316,8 +316,8 @@ public function deleteCollection(string $collection): bool try { return $this->executeStatements([$main->query, $permissions->query], Event::CollectionDelete); - } catch (PDOException $e) { - $error = $this->processException($e); + } catch (PDOException $exception) { + $error = $this->processException($exception); if ($error instanceof NotFoundException) { $this->executeStatement($permissions->query, Event::CollectionDelete); } diff --git a/src/Database/Adapter/SQL.php b/src/Database/Adapter/SQL.php index 02f1d840b..df407ea6d 100644 --- a/src/Database/Adapter/SQL.php +++ b/src/Database/Adapter/SQL.php @@ -3876,10 +3876,8 @@ protected function executeStatements(array $statements, Event $event): bool } /** - * Run DDL that a failed earlier attempt may have partly applied: a statement refused with $applied is taken as - * already applied and the rest still run, then that refusal is thrown so the caller knows the schema held part - * of it. Inside a transaction the first refusal is thrown at once, since PostgreSQL refuses every later - * statement of an aborted transaction. + * Inside a transaction the first refusal is thrown at once, since PostgreSQL refuses every later statement of an + * aborted transaction. * * @param list $statements * @param class-string $applied diff --git a/src/Database/Validator/Structure.php b/src/Database/Validator/Structure.php index 224c4b21d..67661ce72 100644 --- a/src/Database/Validator/Structure.php +++ b/src/Database/Validator/Structure.php @@ -238,8 +238,6 @@ private function isStoredAsNull(string $key): bool */ protected function checkForUnknownAttributes(array $structure, array $definitions): bool { - // A key named like a column the storage keeps for the document itself, even one declared as an attribute, - // would be written over the document's own $sequence, $tenant or $permissions. foreach (\array_keys($structure) as $key) { if (Storage::isReserved(Adapter::filterKey((string) $key))) { $this->message = 'Reserved attribute: "'.$key.'"'; From 6bb4549e4c0ead616be6bf0390d11f9990d669d6 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Thu, 5 Nov 2026 19:05:36 +1300 Subject: [PATCH 11/11] test(pool): expect a read adapter to clear every event's stale timeout The pool now clears each per-event timeout a borrowed connection kept, not only the global one, so the read adapter sees one clear per event. Co-Authored-By: Claude Opus 5.5 --- tests/unit/Adapter/ReadWritePoolTest.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/unit/Adapter/ReadWritePoolTest.php b/tests/unit/Adapter/ReadWritePoolTest.php index 34d72ec08..6ef06877e 100644 --- a/tests/unit/Adapter/ReadWritePoolTest.php +++ b/tests/unit/Adapter/ReadWritePoolTest.php @@ -20,6 +20,7 @@ use Utopia\Database\Collection; use Utopia\Database\Database; use Utopia\Database\Document; +use Utopia\Database\Event; use Utopia\Database\Hook\Permissions; use Utopia\Database\Hook\Write; use Utopia\Database\Index; @@ -346,8 +347,12 @@ public function testReadAdapterClearsStaleTimeout(): void $readAdapter->method('hasFeature')->willReturnCallback( static fn (string $feature): bool => $feature === Feature\Timeouts::class, ); - $readAdapter->expects($this->once()) - ->method('clearTimeout'); + $cleared = []; + $readAdapter->method('clearTimeout')->willReturnCallback( + static function (Event $event = Event::All) use (&$cleared): void { + $cleared[] = $event; + }, + ); $readAdapter->expects($this->once()) ->method('ping') ->willReturn(true); @@ -361,6 +366,7 @@ public function testReadAdapterClearsStaleTimeout(): void $pool->setAuthorization(new Authorization()); $this->assertTrue($pool->delegate('ping', [])); + $this->assertSame(Event::cases(), $cleared); } public function testNonReadNonStandardMethodGoesToWritePool(): void