From 152a906c1349849af633a6b542f7f011703ad086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Laurens=20St=C3=B6tzel?= Date: Sun, 5 Nov 2023 22:33:20 +0100 Subject: [PATCH] fix: Don't process subclass indexes with SingleCollection inheritance Fixes https://github.com/doctrine/mongodb-odm/issues/2561 --- .../MongoDB/Mapping/ClassMetadataFactory.php | 6 +- lib/Doctrine/ODM/MongoDB/SchemaManager.php | 94 +++++++++++-------- .../MongoDB/Tests/Functional/IndexesTest.php | 89 ++++++++++++++++++ .../ODM/MongoDB/Tests/SchemaManagerTest.php | 51 +++++++++- tests/Documents/CmsProduct.php | 8 ++ tests/Documents/Project.php | 1 + tests/Documents/SubProject.php | 8 ++ 7 files changed, 217 insertions(+), 40 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php index d5f33c01ea..c61fe2a31a 100644 --- a/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php +++ b/lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php @@ -139,7 +139,11 @@ protected function doLoadMetadata($class, $parent, $rootEntityFound, array $nonS $class->setIdGeneratorType($parent->generatorType); $this->addInheritedFields($class, $parent); $this->addInheritedRelations($class, $parent); - $this->addInheritedIndexes($class, $parent); + if ($parent->isMappedSuperclass) { + // only inherit indexes if parent won't apply them itself + $this->addInheritedIndexes($class, $parent); + } + $this->setInheritedShardKey($class, $parent); $class->setIdentifier($parent->identifier); $class->setVersioned($parent->isVersioned); diff --git a/lib/Doctrine/ODM/MongoDB/SchemaManager.php b/lib/Doctrine/ODM/MongoDB/SchemaManager.php index 138f29bb59..7b5332ddd5 100644 --- a/lib/Doctrine/ODM/MongoDB/SchemaManager.php +++ b/lib/Doctrine/ODM/MongoDB/SchemaManager.php @@ -92,6 +92,12 @@ public function updateIndexes(?int $maxTimeMs = null, ?WriteConcern $writeConcer continue; } + if ($class->isInheritanceTypeSingleCollection() && count($class->parentClasses) > 0) { + // Skip document nodes that use the same collection as one of their parents. + // Indexes will be added by the parent document. + continue; + } + $this->updateDocumentIndexes($class->name, $maxTimeMs, $writeConcern); } } @@ -172,56 +178,68 @@ private function doGetDocumentIndexes(string $documentName, array &$visited): ar return []; } - $visited[$documentName] = true; - - $class = $this->dm->getClassMetadata($documentName); - $indexes = $this->prepareIndexes($class); - $embeddedDocumentIndexes = []; + $class = $this->dm->getClassMetadata($documentName); + $processClasses = [$class]; - // Add indexes from embedded & referenced documents - foreach ($class->fieldMappings as $fieldMapping) { - if (isset($fieldMapping['embedded'])) { - if (isset($fieldMapping['targetDocument'])) { - $possibleEmbeds = [$fieldMapping['targetDocument']]; - } elseif (isset($fieldMapping['discriminatorMap'])) { - $possibleEmbeds = array_unique($fieldMapping['discriminatorMap']); - } else { - continue; - } + if ($class->isInheritanceTypeSingleCollection()) { + // process all subclasses as well + foreach ($class->subClasses as $subClassName) { + $processClasses[] = $this->metadataFactory->getMetadataFor($subClassName); + } + } - foreach ($possibleEmbeds as $embed) { - if (isset($embeddedDocumentIndexes[$embed])) { - $embeddedIndexes = $embeddedDocumentIndexes[$embed]; + $indexes = []; + $embeddedDocumentIndexes = []; + foreach ($processClasses as $class) { + $visited[$class->name] = true; + + $indexes = array_merge($indexes, $this->prepareIndexes($class)); + + // Add indexes from embedded & referenced documents + foreach ($class->fieldMappings as $fieldMapping) { + if (isset($fieldMapping['embedded'])) { + if (isset($fieldMapping['targetDocument'])) { + $possibleEmbeds = [$fieldMapping['targetDocument']]; + } elseif (isset($fieldMapping['discriminatorMap'])) { + $possibleEmbeds = array_unique($fieldMapping['discriminatorMap']); } else { - $embeddedIndexes = $this->doGetDocumentIndexes($embed, $visited); - $embeddedDocumentIndexes[$embed] = $embeddedIndexes; + continue; } - foreach ($embeddedIndexes as $embeddedIndex) { - foreach ($embeddedIndex['keys'] as $key => $value) { - $embeddedIndex['keys'][$fieldMapping['name'] . '.' . $key] = $value; - unset($embeddedIndex['keys'][$key]); + foreach ($possibleEmbeds as $embed) { + if (isset($embeddedDocumentIndexes[$embed])) { + $embeddedIndexes = $embeddedDocumentIndexes[$embed]; + } else { + $embeddedIndexes = $this->doGetDocumentIndexes($embed, $visited); + $embeddedDocumentIndexes[$embed] = $embeddedIndexes; } - if (isset($embeddedIndex['options']['name'])) { - $embeddedIndex['options']['name'] = sprintf('%s_%s', $fieldMapping['name'], $embeddedIndex['options']['name']); - } + foreach ($embeddedIndexes as $embeddedIndex) { + foreach ($embeddedIndex['keys'] as $key => $value) { + $embeddedIndex['keys'][$fieldMapping['name'] . '.' . $key] = $value; + unset($embeddedIndex['keys'][$key]); + } - $indexes[] = $embeddedIndex; + if (isset($embeddedIndex['options']['name'])) { + $embeddedIndex['options']['name'] = sprintf('%s_%s', $fieldMapping['name'], $embeddedIndex['options']['name']); + } + + $indexes[] = $embeddedIndex; + } } - } - } elseif (isset($fieldMapping['reference']) && isset($fieldMapping['targetDocument'])) { - foreach ($indexes as $idx => $index) { - $newKeys = []; - foreach ($index['keys'] as $key => $v) { - if ($key === $fieldMapping['name']) { - $key = ClassMetadata::getReferenceFieldName($fieldMapping['storeAs'], $key); + } elseif (isset($fieldMapping['reference']) && isset($fieldMapping['targetDocument'])) { + foreach ($indexes as $idx => $index) { + $newKeys = []; + foreach ($index['keys'] as $key => $v) { + if ($key === $fieldMapping['name']) { + $key = ClassMetadata::getReferenceFieldName($fieldMapping['storeAs'], $key); + } + + $newKeys[$key] = $v; } - $newKeys[$key] = $v; + $indexes[$idx]['keys'] = $newKeys; } - - $indexes[$idx]['keys'] = $newKeys; } } } diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/IndexesTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/IndexesTest.php index 4dd7567023..cc76f0cd86 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/IndexesTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/IndexesTest.php @@ -9,6 +9,8 @@ use Doctrine\ODM\MongoDB\Tests\BaseTestCase; use MongoDB\Driver\Exception\BulkWriteException; +use function key; + class IndexesTest extends BaseTestCase { /** @param class-string $class */ @@ -239,6 +241,30 @@ public function testPartialIndexCreation(): void self::assertSame(['counter' => ['$gt' => 5]], $indexes[0]['options']['partialFilterExpression']); self::assertTrue($indexes[0]['options']['unique']); } + + public function testSubclassDoesNotInheritParentIndexes(): void + { + $baseIndexes = $this->dm->getSchemaManager()->getDocumentIndexes(QueryableBaseDocument::class); + $extendedIndexes = $this->dm->getSchemaManager()->getDocumentIndexes(ExtendedSubDocument::class); + + self::assertCount(1, $baseIndexes); + self::assertSame('foo', key($baseIndexes[0]['keys'])); + + self::assertCount(1, $extendedIndexes); + self::assertSame('bar', key($extendedIndexes[0]['keys'])); + } + + public function testSubclassInheritsSuperclassIndexes(): void + { + $baseIndexes = $this->dm->getSchemaManager()->getDocumentIndexes(NonQueryableBaseDocument::class); + $extendedIndexes = $this->dm->getSchemaManager()->getDocumentIndexes(ExtendedInheritedDocument::class); + + self::assertCount(0, $baseIndexes); + + self::assertCount(2, $extendedIndexes); + self::assertSame('bar', key($extendedIndexes[0]['keys'])); + self::assertSame('foo', key($extendedIndexes[1]['keys'])); + } } /** @ODM\Document */ @@ -657,3 +683,66 @@ class DocumentWithIndexInDiscriminatedEmbeds */ public $embedded; } + +/** @ODM\Document */ +class QueryableBaseDocument +{ + /** + * @ODM\Id + * + * @var string|null + */ + public $id; + + /** + * @ODM\Field(type="string") + * @ODM\Index + * + * @var string|null + */ + public $foo; +} + +/** @ODM\Document */ +class ExtendedSubDocument extends QueryableBaseDocument +{ + /** + * @ODM\Field(type="string") + * @ODM\Index + * + * @var string|null + */ + public $bar; +} + + +/** @ODM\MappedSuperclass */ +class NonQueryableBaseDocument +{ + /** + * @ODM\Id + * + * @var string|null + */ + public $id; + + /** + * @ODM\Field(type="string") + * @ODM\Index + * + * @var string|null + */ + public $foo; +} + +/** @ODM\Document */ +class ExtendedInheritedDocument extends NonQueryableBaseDocument +{ + /** + * @ODM\Field(type="string") + * @ODM\Index + * + * @var string|null + */ + public $bar; +} diff --git a/tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php b/tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php index e16ebb6f58..38cc96c859 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php @@ -16,6 +16,7 @@ use Documents\CmsProduct; use Documents\Comment; use Documents\File; +use Documents\Project; use Documents\SchemaValidated; use Documents\Sharded\ShardedOne; use Documents\Sharded\ShardedOneWithDifferentKey; @@ -41,6 +42,7 @@ use function array_map; use function assert; use function in_array; +use function key; use function MongoDB\BSON\fromJSON; use function MongoDB\BSON\toPHP; @@ -60,6 +62,7 @@ class SchemaManagerTest extends BaseTestCase SimpleReferenceUser::class, ShardedOne::class, ShardedOneWithDifferentKey::class, + Project::class, ]; /** @psalm-var list */ @@ -285,7 +288,7 @@ public function testEnsureDocumentIndexesWithTwoLevelInheritance(array $expected $collectionName = $this->dm->getClassMetadata(CmsProduct::class)->getCollection(); $collection = $this->documentCollections[$collectionName]; $collection - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('createIndex') ->with($this->anything(), $this->writeOptions($expectedWriteOptions)); @@ -317,6 +320,52 @@ public function testUpdateDocumentIndexesShouldCreateMappedIndexes(array $expect $this->schemaManager->updateDocumentIndexes(CmsArticle::class, $maxTimeMs, $writeConcern); } + /** @psalm-param IndexOptions $expectedWriteOptions */ + public function testUpdateDocumentIndexesShouldCreateIndexesFromMappedSuperclass(): void + { + $collectionName = $this->dm->getClassMetadata(CmsProduct::class)->getCollection(); + $collection = $this->documentCollections[$collectionName]; + $collection + ->expects($this->once()) + ->method('listIndexes') + ->willReturn(new IndexInfoIteratorIterator(new ArrayIterator([]))); + $collection + ->expects($this->exactly(2)) + ->method('createIndex') + ->with($this->anything(), $this->anything()); + $collection + ->expects($this->never()) + ->method('dropIndex') + ->with($this->anything()); + + $this->schemaManager->updateDocumentIndexes(CmsProduct::class); + } + + /** @psalm-param IndexOptions $expectedWriteOptions */ + public function testUpdateDocumentIndexesShouldCreateIndexFromSubclasses(): void + { + $collectionName = $this->dm->getClassMetadata(Project::class)->getCollection(); + $collection = $this->documentCollections[$collectionName]; + $collection + ->expects($this->once()) + ->method('listIndexes') + ->willReturn(new IndexInfoIteratorIterator(new ArrayIterator([]))); + + $matcher = $this->exactly(2); + $collection + ->expects($matcher) + ->method('createIndex') + ->willReturnCallback(static function ($key, $value) use ($matcher) { + self::assertSame(['name', 'externalId'][$matcher->numberOfInvocations() - 1], key($key)); + }); + $collection + ->expects($this->never()) + ->method('dropIndex') + ->with($this->anything()); + + $this->schemaManager->updateDocumentIndexes(Project::class); + } + /** * @psalm-param IndexOptions $expectedWriteOptions * diff --git a/tests/Documents/CmsProduct.php b/tests/Documents/CmsProduct.php index 034f2f1a49..4aacf49a36 100644 --- a/tests/Documents/CmsProduct.php +++ b/tests/Documents/CmsProduct.php @@ -15,4 +15,12 @@ class CmsProduct extends CmsContent * @var string|null */ public $name; + + /** + * @ODM\Field(type="string") + * @ODM\Index + * + * @var string|null + */ + public $productId; } diff --git a/tests/Documents/Project.php b/tests/Documents/Project.php index e2976333bc..64c26b9adf 100644 --- a/tests/Documents/Project.php +++ b/tests/Documents/Project.php @@ -25,6 +25,7 @@ class Project /** * @ODM\Field(type="string") + * @ODM\UniqueIndex * * @var string|null */ diff --git a/tests/Documents/SubProject.php b/tests/Documents/SubProject.php index 50dc32dcba..026ec80169 100644 --- a/tests/Documents/SubProject.php +++ b/tests/Documents/SubProject.php @@ -10,6 +10,14 @@ /** @ODM\Document */ class SubProject extends Project { + /** + * @ODM\Field(type="string") + * @ODM\Index + * + * @var string|null + */ + private $externalId; + /** * @ODM\EmbedMany(targetDocument=Issue::class) *