Skip to content

Fix: Double metadata while migrating #787

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Meldiron wants to merge 8 commits into from
+176 −10
Closed

Fix: Double metadata while migrating #787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f91272
Fix double metdata when migrating
Meldiron Jan 15, 2026
7ece8a8
AI review fixes
Meldiron Jan 15, 2026
3923367
Fix bug
Meldiron Jan 15, 2026
64c760c
Add looseness check
Meldiron Jan 15, 2026
72598ce
Revert "Fix bug"
Meldiron Jan 15, 2026
af14b29
Update Database.php
Meldiron Jan 15, 2026
b085f70
fix leftover
Meldiron Jan 15, 2026
64bd5f3
Loosen assertions
Meldiron Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 18 additions & 4 deletions src/Database/Database.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2047,7 +2047,11 @@ public function createAttribute(string $collection, string $id, string $type, in
}
}

$collection->setAttribute('attributes', $attribute, Document::SET_TYPE_APPEND);
// Append attribute, removing any duplicates
$metadataAttributes = $collection->getAttribute('attributes', []);
$metadataAttributes = \array_filter($metadataAttributes, fn ($attribute) => $attribute['$id'] !== $id);
$metadataAttributes[] = $attribute;
$collection->setAttribute('attributes', \array_values($metadataAttributes));

$this->updateMetadata(
collection: $collection,
Expand Down Expand Up @@ -2169,10 +2173,17 @@ public function createAttributes(string $collection, array $attributes): bool
}
}

// Append attribute, removing any duplicates
$newAttributeIds = \array_map(fn ($attr) => $attr['$id'], $attributeDocuments);
$metadataAttributes = $collection->getAttribute('attributes', []);
$metadataAttributes = \array_filter($metadataAttributes, fn ($attr) => !\in_array($attr['$id'], $newAttributeIds));

foreach ($attributeDocuments as $attributeDocument) {
$collection->setAttribute('attributes', $attributeDocument, Document::SET_TYPE_APPEND);
$metadataAttributes[] = $attributeDocument;
}

$collection->setAttribute('attributes', \array_values($metadataAttributes));

$this->updateMetadata(
collection: $collection,
rollbackOperation: fn () => $this->cleanupAttributes($collection->getId(), $attributeDocuments),
Expand Down Expand Up @@ -2235,9 +2246,12 @@ private function validateAttribute(
// Attribute IDs are case-insensitive
$attributes = $collection->getAttribute('attributes', []);

// Loosen verification during migration
$isLoose = $this->adapter->getSharedTables() && $this->isMigrating();

/** @var array<Document> $attributes */
foreach ($attributes as $attribute) {
if (\strtolower($attribute->getId()) === \strtolower($id)) {
if (!$isLoose && \strtolower($attribute->getId()) === \strtolower($id)) {
throw new DuplicateException('Attribute already exists in metadata');
}
}
Expand All @@ -2246,7 +2260,7 @@ private function validateAttribute(
$schema = $this->getSchemaAttributes($collection->getId());
foreach ($schema as $attribute) {
$newId = $this->adapter->filter($attribute->getId());
if (\strtolower($newId) === \strtolower($id)) {
if (!$isLoose && \strtolower($newId) === \strtolower($id)) {
throw new DuplicateException('Attribute already exists in schema');
}
}
Expand Down
152 changes: 152 additions & 0 deletions tests/e2e/Adapter/Scopes/AttributeTests.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2195,4 +2195,156 @@ public function testCreateAttributesDelete(): void
$this->assertCount(1, $attrs);
$this->assertEquals('b', $attrs[0]['$id']);
}

public function testCreateAttributeWhileMigrating(): void
{
/** @var Database $database */
$database = $this->getDatabase();

// Skip test if adapter doesn't support shared tables
if (!$database->getAdapter()->getSharedTables()) {
$this->expectNotToPerformAssertions();
return;
}

// Prepare collection
$database->createCollection('migration_test');
$database->setMigrating(true);

// First creation, as usual
$this->assertTrue($database->createAttribute('migration_test', 'statusColumn', Database::VAR_STRING, 128, true));

$collection = $database->getCollection('migration_test');
$attributes = $collection->getAttribute('attributes');
$this->assertCount(1, $attributes);
$this->assertSame('statusColumn', $attributes[0]['$id']);

// Second creation, no exceptions, no duplicates
$result = $database->createAttribute('migration_test', 'statusColumn', Database::VAR_STRING, 128, true);
$this->assertTrue($result);

$collection = $database->getCollection('migration_test');
$attributes = $collection->getAttribute('attributes');
$this->assertCount(1, $attributes);
$this->assertSame('statusColumn', $attributes[0]['$id']);

// Third creation, same as second, once more, just in case
$result = $database->createAttribute('migration_test', 'statusColumn', Database::VAR_STRING, 128, true);
$this->assertTrue($result);

$collection = $database->getCollection('migration_test');
$attributes = $collection->getAttribute('attributes');
$this->assertCount(1, $attributes);
$this->assertSame('statusColumn', $attributes[0]['$id']);

// Cleanup
$database->setMigrating(false);
$database->deleteCollection('migration_test');
}

public function testCreateAttributesWhileMigrating(): void
{
/** @var Database $database */
$database = $this->getDatabase();

// Skip test if adapter doesn't support shared tables
if (!$database->getAdapter()->getSharedTables()) {
$this->expectNotToPerformAssertions();
return;
}

// Skip test if adapter doesn't support batch create attributes
if (!$database->getAdapter()->getSupportForBatchCreateAttributes()) {
$this->expectNotToPerformAssertions();
return;
}

// Prepare collection
$database->createCollection('migration_test_batch');
$database->setMigrating(true);

// First creation, as usual
$attributes = [
[
'$id' => 'statusColumn',
'type' => Database::VAR_STRING,
'size' => 128,
'required' => true
],
[
'$id' => 'count',
'type' => Database::VAR_INTEGER,
'size' => 0,
'required' => false
],
];

$result = $database->createAttributes('migration_test_batch', $attributes);
$this->assertTrue($result);

$collection = $database->getCollection('migration_test_batch');
$attrs = $collection->getAttribute('attributes');
$this->assertCount(2, $attrs);
$this->assertSame('statusColumn', $attrs[0]['$id']);
$this->assertSame('count', $attrs[1]['$id']);

// Second creation, no exceptions, no duplicates
$result = $database->createAttributes('migration_test_batch', $attributes);
$this->assertTrue($result);

$collection = $database->getCollection('migration_test_batch');
$attrs = $collection->getAttribute('attributes');
$this->assertCount(2, $attrs);
$this->assertSame('statusColumn', $attrs[0]['$id']);
$this->assertSame('count', $attrs[1]['$id']);

// Third creation, same as second, once more, just in case
$result = $database->createAttributes('migration_test_batch', $attributes);
$this->assertTrue($result);

$collection = $database->getCollection('migration_test_batch');
$attrs = $collection->getAttribute('attributes');
$this->assertCount(2, $attrs);
$this->assertSame('statusColumn', $attrs[0]['$id']);
$this->assertSame('count', $attrs[1]['$id']);

// Test partial overlap - create one new attribute and one existing
$mixedAttributes = [
[
'$id' => 'statusColumn', // existing
'type' => Database::VAR_STRING,
'size' => 128,
'required' => true
],
[
'$id' => 'active', // new
'type' => Database::VAR_BOOLEAN,
'size' => 0,
'required' => false
],
];

$result = $database->createAttributes('migration_test_batch', $mixedAttributes);
$this->assertTrue($result);

// Ensure all attributes exist
$collection = $database->getCollection('migration_test_batch');
$attrs = $collection->getAttribute('attributes');
$this->assertCount(3, $attrs);

// TODO: Eventuelly rewrite to assertSame for each item; currently their order differs between providers
$attributes = [
$attrs[0]['$id'],
$attrs[1]['$id'],
$attrs[2]['$id']
];

$this->assertContains('statusColumn', $attributes);
$this->assertContains('count', $attributes);
$this->assertContains('active', $attributes);

// Cleanup
$database->setMigrating(false);
$database->deleteCollection('migration_test_batch');
}
}