fix: Don't process subclass indexes with SingleCollection inheritance

buffcode · buffcode · commit 152a906c1349 · 2023-11-05T22:33:20.000+01:00
Fixes doctrine#2561
diff --git 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
@@ -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
@@ -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
@@ -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<class-string> */
@@ -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
@@ -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
@@ -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
@@ -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)
      *

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@ class Project`
`25`	`25`
`26`	`26`	`/**`
`27`	`27`	`* @ODM\Field(type="string")`
	`28`	`+ * @ODM\UniqueIndex`
`28`	`29`	`*`
`29`	`30`	`* @var string\|null`
`30`	`31`	`*/`