From 0578bd7cf72bc6159f365120dae14985a78ea418 Mon Sep 17 00:00:00 2001 From: fogelito Date: Mon, 7 Sep 2026 11:21:17 +0300 Subject: [PATCH 1/4] Run --- src/Database/Database.php | 101 +++++++++++++++++++- tests/e2e/Adapter/Scopes/AttributeTests.php | 74 ++++++++++++++ 2 files changed, 170 insertions(+), 5 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index 144428665..b9962a616 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2363,7 +2363,8 @@ public function createAttribute(string $collection, string $id, string $type, in collection: $collection, rollbackOperation: fn () => $this->cleanupAttribute($collection->getId(), $id), shouldRollback: $created, - operationDescription: "attribute creation '{$id}'" + operationDescription: "attribute creation '{$id}'", + apply: fn (Document $metadata) => $this->appendMetadataAttribute($metadata, $attribute) ); $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId())); @@ -2569,7 +2570,12 @@ public function createAttributes(string $collection, array $attributes): bool rollbackOperation: fn () => $this->cleanupAttributes($collection->getId(), $attributeDocuments), shouldRollback: $created, operationDescription: 'attributes creation', - rollbackReturnsErrors: true + rollbackReturnsErrors: true, + apply: function (Document $metadata) use ($attributeDocuments) { + foreach ($attributeDocuments as $attributeDocument) { + $this->appendMetadataAttribute($metadata, $attributeDocument); + } + } ); $this->withRetries(fn () => $this->purgeCachedCollection($collection->getId())); @@ -4793,7 +4799,8 @@ public function createIndex(string $collection, string $id, string $type, array collection: $collection, rollbackOperation: fn () => $this->cleanupIndex($collection->getId(), $id), shouldRollback: $created, - operationDescription: "index creation '{$id}'" + operationDescription: "index creation '{$id}'", + apply: fn (Document $metadata) => $this->appendMetadataIndex($metadata, $index) ); $this->trigger(self::EVENT_INDEX_CREATE, $index); @@ -10922,6 +10929,7 @@ private function cleanupIndex( * @param string $operationDescription Description of the operation for error messages * @param bool $rollbackReturnsErrors Whether rollback operation returns error array (true) or throws (false) * @param bool $silentRollback Whether rollback errors should be silently caught (true) or thrown (false) + * @param callable(Document): void|null $apply Applies the change to a fresh, locked copy of the metadata row * @return void * @throws DatabaseException If metadata persistence fails after all retries */ @@ -10931,12 +10939,15 @@ private function updateMetadata( bool $shouldRollback, string $operationDescription = 'operation', bool $rollbackReturnsErrors = false, - bool $silentRollback = false + bool $silentRollback = false, + ?callable $apply = null ): void { try { if ($collection->getId() !== self::METADATA) { $this->withRetries( - fn () => $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)) + fn () => $apply === null + ? $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)) + : $this->rebaseMetadata($collection->getId(), $apply) ); } } catch (\Throwable $e) { @@ -10978,6 +10989,86 @@ private function updateMetadata( } } + /** + * Apply a change to a collection's metadata row inside the transaction that + * locks it, rather than writing back a copy read earlier. + * + * The caller's copy is read before the schema change and can come from + * cache, so writing it whole drops any entry another writer added in + * between — leaving a column whose metadata never mentions it. Re-reading + * under `FOR UPDATE` here serializes the change against every other writer + * of the row, in this process or another, with no distributed lock. It + * cannot wrap the schema change too: MySQL implicitly commits at DDL, which + * would release the lock mid-call. + * + * $apply must be idempotent — withTransaction() and withRetries() can both + * run it again on a fresh copy. + * + * @param callable(Document): void $apply + * @throws DatabaseException + */ + private function rebaseMetadata(string $collectionId, callable $apply): void + { + $this->withTransaction(function () use ($collectionId, $apply) { + $collection = $this->authorization->skip(fn () => $this->silent( + fn () => $this->getDocument(self::METADATA, $collectionId, forUpdate: true) + )); + + if ($collection->isEmpty()) { + throw new NotFoundException('Collection not found'); + } + + $apply($collection); + + $this->silent(fn () => $this->updateDocument(self::METADATA, $collectionId, $collection)); + }); + } + + /** + * Append an attribute to a collection's metadata list, leaving the list + * alone when it already holds the key (keys are case-insensitive). + * + * Converging rather than throwing keeps createAttribute()'s existing policy + * for a key another writer got to first — it already suppresses the + * adapter's DuplicateException for a column that exists in the schema only + * — and keeps the entry describing that column instead of replacing it with + * this caller's spec. + */ + private function appendMetadataAttribute(Document $collection, Document $attribute): void + { + $key = \strtolower($attribute->getAttribute('key', $attribute->getId())); + + /** @var array $attributes */ + $attributes = $collection->getAttribute('attributes', []); + + foreach ($attributes as $existing) { + if (\strtolower($existing->getAttribute('key', $existing->getId())) === $key) { + return; + } + } + + $collection->setAttribute('attributes', $attribute, Document::SET_TYPE_APPEND); + } + + /** + * Append an index to a collection's metadata list, leaving the list alone + * when it already holds the ID (IDs are case-insensitive). Converges for + * the same reason as appendMetadataAttribute(). + */ + private function appendMetadataIndex(Document $collection, Document $index): void + { + /** @var array $indexes */ + $indexes = $collection->getAttribute('indexes', []); + + foreach ($indexes as $existing) { + if (\strtolower($existing->getId()) === \strtolower($index->getId())) { + return; + } + } + + $collection->setAttribute('indexes', $index, Document::SET_TYPE_APPEND); + } + /** * Rollback metadata state by removing specified attributes from collection * diff --git a/tests/e2e/Adapter/Scopes/AttributeTests.php b/tests/e2e/Adapter/Scopes/AttributeTests.php index 8f68ea824..144dca8a2 100644 --- a/tests/e2e/Adapter/Scopes/AttributeTests.php +++ b/tests/e2e/Adapter/Scopes/AttributeTests.php @@ -4,6 +4,8 @@ use Exception; use Throwable; +use Utopia\Cache\Adapter\None as NoneCache; +use Utopia\Cache\Cache; use Utopia\Database\Database; use Utopia\Database\DateTime; use Utopia\Database\Document; @@ -31,6 +33,19 @@ private function createRandomString(int $length = 10): string return \substr(\bin2hex(\random_bytes(\max(1, \intval(($length + 1) / 2)))), 0, $length); } + /** + * Attribute keys as the collection's metadata row lists them. + * + * @return array + */ + private function metadataAttributeKeys(Database $database, string $collection): array + { + return \array_map( + fn (Document $attribute) => $attribute->getAttribute('key', $attribute->getId()), + $database->getCollection($collection)->getAttribute('attributes', []) + ); + } + /** * Using phpunit dataProviders to check that all these combinations of types/defaults throw exceptions * https://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers @@ -2608,4 +2623,63 @@ public function testStringTypeAttributes(): void $updatedDoc = $database->getDocument('stringTypes', 'doc1'); $this->assertEquals('Updated varchar value', $updatedDoc->getAttribute('varchar_field')); } + + /** + * A peer process adds an attribute while this process still holds the + * collection as it was before that write — the copy it would otherwise + * write back whole. The peer's attribute has to survive in the metadata + * list, not only as a column in the physical schema. + */ + public function testCreateAttributeConcurrentlyKeepsPeerAttribute(): void + { + /** @var Database $database */ + $database = $this->getDatabase(); + + $collection = 'concurrentAttribute'; + + // A peer process: same database, its own cache, so its writes do not + // purge the copy this process is about to read. + $peer = (new Database($database->getAdapter(), new Cache(new NoneCache()))) + ->setAuthorization(self::$authorization); + + $database->createCollection($collection, [ + new Document([ + '$id' => ID::custom('first'), + 'type' => Database::VAR_STRING, + 'size' => 32, + 'required' => false, + ]), + ], permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + + // Read it once, so this process's copy predates the peer's write. + $before = $this->metadataAttributeKeys($database, $collection); + $this->assertContains('first', $before); + + $this->assertTrue($peer->createAttribute($collection, 'peer', Database::VAR_STRING, 32, false)); + $this->assertTrue($database->createAttribute($collection, 'mine', Database::VAR_STRING, 32, false)); + + $this->assertEqualsCanonicalizing( + [...$before, 'peer', 'mine'], + $this->metadataAttributeKeys($peer, $collection), + 'Peer attribute is missing from the collection metadata' + ); + + // The list has to match the schema, so a write naming every attribute + // must keep every value: an attribute absent from metadata is dropped. + $document = $database->createDocument($collection, new Document([ + '$id' => ID::custom('row'), + '$permissions' => [Permission::read(Role::any())], + 'first' => 'a', + 'peer' => 'b', + 'mine' => 'c', + ])); + + $this->assertSame('b', $document->getAttribute('peer')); + $this->assertSame('c', $document->getAttribute('mine')); + + $this->assertTrue($database->deleteCollection($collection)); + } } From ffe8270cd2df1731293e8e9280ba1475e46ca972 Mon Sep 17 00:00:00 2001 From: fogelito Date: Mon, 7 Sep 2026 11:33:18 +0300 Subject: [PATCH 2/4] Race test --- tests/unit/CreateAttributeRaceTest.php | 176 +++++++++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 tests/unit/CreateAttributeRaceTest.php diff --git a/tests/unit/CreateAttributeRaceTest.php b/tests/unit/CreateAttributeRaceTest.php new file mode 100644 index 000000000..a5df2763a --- /dev/null +++ b/tests/unit/CreateAttributeRaceTest.php @@ -0,0 +1,176 @@ +adapter = new DatabaseMemory(); + $this->database = new Database($this->adapter, new Cache(new CacheMemory())); + $this->database + ->setDatabase('utopiaTests') + ->setNamespace('attribute_race_' . uniqid()); + $this->database->getAuthorization()->addRole(Role::any()->toString()); + $this->database->create(); + + // Same database, its own cache: its writes do not purge the copy the + // first process is about to read. + $this->peer = (new Database($this->adapter, new Cache(new NoneCache()))) + ->setAuthorization($this->database->getAuthorization()); + } + + public function testCreateAttributeKeepsPeerAttribute(): void + { + $collection = $this->seed('concurrentAttribute'); + + $before = $this->attributeKeys($this->database, $collection); + $this->assertContains('first', $before); + + $this->assertTrue($this->peer->createAttribute($collection, 'peer', Database::VAR_STRING, 32, false)); + $this->assertTrue($this->database->createAttribute($collection, 'mine', Database::VAR_STRING, 32, false)); + + $this->assertEqualsCanonicalizing( + [...$before, 'peer', 'mine'], + $this->attributeKeys($this->peer, $collection), + 'Peer attribute is missing from the collection metadata' + ); + + // The list has to match the schema: an attribute the metadata does not + // mention is dropped from a write that names it. + $document = $this->database->createDocument($collection, new Document([ + '$id' => ID::custom('row'), + '$permissions' => [Permission::read(Role::any())], + 'first' => 'a', + 'peer' => 'b', + 'mine' => 'c', + ])); + + $this->assertSame('b', $document->getAttribute('peer')); + $this->assertSame('c', $document->getAttribute('mine')); + } + + public function testCreateAttributesKeepsPeerAttribute(): void + { + $collection = $this->seed('concurrentAttributes'); + + $before = $this->attributeKeys($this->database, $collection); + + $this->assertTrue($this->peer->createAttribute($collection, 'peer', Database::VAR_STRING, 32, false)); + + $this->assertTrue($this->database->createAttributes($collection, [ + ['$id' => ID::custom('batchA'), 'type' => Database::VAR_STRING, 'size' => 32, 'required' => false], + ['$id' => ID::custom('batchB'), 'type' => Database::VAR_STRING, 'size' => 32, 'required' => false], + ])); + + $this->assertEqualsCanonicalizing( + [...$before, 'peer', 'batchA', 'batchB'], + $this->attributeKeys($this->peer, $collection), + 'Peer attribute is missing from the collection metadata' + ); + } + + public function testCreateIndexKeepsPeerIndex(): void + { + $collection = $this->seed('concurrentIndex', ['first', 'second']); + + $before = \array_map( + fn (Document $index) => $index->getId(), + $this->database->getCollection($collection)->getAttribute('indexes', []) + ); + + $this->assertTrue($this->peer->createIndex($collection, 'peerIdx', Database::INDEX_KEY, ['first'])); + $this->assertTrue($this->database->createIndex($collection, 'mineIdx', Database::INDEX_KEY, ['second'])); + + $ids = \array_map( + fn (Document $index) => $index->getId(), + $this->peer->getCollection($collection)->getAttribute('indexes', []) + ); + + $this->assertEqualsCanonicalizing( + [...$before, 'peerIdx', 'mineIdx'], + $ids, + 'Peer index is missing from the collection metadata' + ); + } + + /** + * Both processes create the same key. The append converges on the entry + * describing the column that exists, rather than listing the key twice or + * replacing the winner's spec with the loser's. + */ + public function testCreateAttributeConvergesOnAKeyThePeerAlreadyAdded(): void + { + $collection = $this->seed('convergingAttribute'); + + $this->database->getCollection($collection); + + $this->assertTrue($this->peer->createAttribute($collection, 'shared', Database::VAR_STRING, 32, false)); + $this->assertTrue($this->database->createAttribute($collection, 'shared', Database::VAR_STRING, 64, false)); + + $attributes = \array_values(\array_filter( + $this->peer->getCollection($collection)->getAttribute('attributes', []), + fn (Document $attribute) => $attribute->getAttribute('key', $attribute->getId()) === 'shared' + )); + + $this->assertCount(1, $attributes, 'Attribute key was listed twice in the collection metadata'); + $this->assertSame(32, $attributes[0]->getAttribute('size'), 'Metadata describes a column width the schema does not have'); + } + + /** + * @param array $attributes + */ + private function seed(string $collection, array $attributes = ['first']): string + { + $this->database->createCollection($collection, \array_map(fn (string $key) => new Document([ + '$id' => ID::custom($key), + 'type' => Database::VAR_STRING, + 'size' => 32, + 'required' => false, + ]), $attributes), permissions: [ + Permission::read(Role::any()), + Permission::create(Role::any()), + ]); + + return $collection; + } + + /** + * @return array + */ + private function attributeKeys(Database $database, string $collection): array + { + return \array_map( + fn (Document $attribute) => $attribute->getAttribute('key', $attribute->getId()), + $database->getCollection($collection)->getAttribute('attributes', []) + ); + } +} From 5752b233bf6890660b0dded76cf276a5b2cbbb34 Mon Sep 17 00:00:00 2001 From: fogelito Date: Mon, 7 Sep 2026 11:42:59 +0300 Subject: [PATCH 3/4] Remove the SET_TYPE_APPEND --- src/Database/Database.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Database/Database.php b/src/Database/Database.php index b9962a616..29019c145 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -2357,8 +2357,6 @@ public function createAttribute(string $collection, string $id, string $type, in } } - $collection->setAttribute('attributes', $attribute, Document::SET_TYPE_APPEND); - $this->updateMetadata( collection: $collection, rollbackOperation: fn () => $this->cleanupAttribute($collection->getId(), $id), @@ -2561,10 +2559,6 @@ public function createAttributes(string $collection, array $attributes): bool } } - foreach ($attributeDocuments as $attributeDocument) { - $collection->setAttribute('attributes', $attributeDocument, Document::SET_TYPE_APPEND); - } - $this->updateMetadata( collection: $collection, rollbackOperation: fn () => $this->cleanupAttributes($collection->getId(), $attributeDocuments), @@ -4793,8 +4787,6 @@ public function createIndex(string $collection, string $id, string $type, array } } - $collection->setAttribute('indexes', $index, Document::SET_TYPE_APPEND); - $this->updateMetadata( collection: $collection, rollbackOperation: fn () => $this->cleanupIndex($collection->getId(), $id), From ecaa3ae3cbd1cb6429017e2701913844b3b37b18 Mon Sep 17 00:00:00 2001 From: fogelito Date: Mon, 7 Sep 2026 11:50:11 +0300 Subject: [PATCH 4/4] comment --- src/Database/Database.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Database/Database.php b/src/Database/Database.php index 29019c145..45fbb8b57 100644 --- a/src/Database/Database.php +++ b/src/Database/Database.php @@ -10993,6 +10993,13 @@ private function updateMetadata( * cannot wrap the schema change too: MySQL implicitly commits at DDL, which * would release the lock mid-call. * + * On an adapter reporting no update-lock support (Redis, Memory, Mongo) the + * read emits no lock and the transaction gives no cross-process isolation, + * so this narrows the window to the read-write pair rather than closing it. + * That is still strictly better than writing back a copy taken before the + * schema change: the read cannot be served from cache. Serializing those + * adapters would need an advisory lock, which this class does not own. + * * $apply must be idempotent — withTransaction() and withRetries() can both * run it again on a fresh copy. *