Skip to content
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.

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

fix: Don't process subclass indexes with SingleCollection inheritance #2573

Open
wants to merge 2 commits into
base: 2.10.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadataFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do such a thing as it changes produced ClassMetadata too much:, especially for a patch release. I'm not sure yet if the change would be OK in a minor patch anyway.

}

$this->setInheritedShardKey($class, $parent);
$class->setIdentifier($parent->identifier);
$class->setVersioned($parent->isVersioned);
Expand Down
94 changes: 56 additions & 38 deletions lib/Doctrine/ODM/MongoDB/SchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a failsafe throwing an \InvalidArgumentException when a document that has $class->parentClasses is passed here

// 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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken we should be able to work around my first comment here by applying array_unique or calculating some kind of key for an index that's shared between classes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be dirtier than what you're proposing but we're not changing behaviour that was established for years


// 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;
}
}
}
Expand Down
89 changes: 89 additions & 0 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/IndexesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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;
}
52 changes: 51 additions & 1 deletion tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -60,6 +62,7 @@
SimpleReferenceUser::class,
ShardedOne::class,
ShardedOneWithDifferentKey::class,
Project::class,
];

/** @psalm-var list<class-string> */
Expand Down Expand Up @@ -285,7 +288,7 @@
$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));

Expand Down Expand Up @@ -317,6 +320,53 @@
$this->schemaManager->updateDocumentIndexes(CmsArticle::class, $maxTimeMs, $writeConcern);
}

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([])));

Check failure on line 330 in tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (8.2)

InternalClass

tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php:330:26: InternalClass: MongoDB\Model\IndexInfoIteratorIterator is internal to MongoDB but called from Doctrine\ODM\MongoDB\Tests\SchemaManagerTest (see https://psalm.dev/174)

Check failure on line 330 in tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (8.2)

InternalMethod

tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php:330:26: InternalMethod: Constructor MongoDB\Model\IndexInfoIteratorIterator::__construct is internal to MongoDB, MongoDB, and MongoDB but called from Doctrine\ODM\MongoDB\Tests\SchemaManagerTest::testUpdateDocumentIndexesShouldCreateIndexesFromMappedSuperclass (see https://psalm.dev/175)
$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);
}

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([])));

Check failure on line 350 in tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (8.2)

InternalClass

tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php:350:26: InternalClass: MongoDB\Model\IndexInfoIteratorIterator is internal to MongoDB but called from Doctrine\ODM\MongoDB\Tests\SchemaManagerTest (see https://psalm.dev/174)

Check failure on line 350 in tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php

View workflow job for this annotation

GitHub Actions / Static Analysis with Psalm (8.2)

InternalMethod

tests/Doctrine/ODM/MongoDB/Tests/SchemaManagerTest.php:350:26: InternalMethod: Constructor MongoDB\Model\IndexInfoIteratorIterator::__construct is internal to MongoDB, MongoDB, and MongoDB but called from Doctrine\ODM\MongoDB\Tests\SchemaManagerTest::testUpdateDocumentIndexesShouldCreateIndexFromSubclasses (see https://psalm.dev/175)

$matcher = $this->exactly(2);
$collection
->expects($matcher)
->method('createIndex')
->willReturnCallback(static function ($key, $value) {
static $counter = 0;

self::assertSame(['name', 'externalId'][$counter], key($key));
++$counter;
});
$collection
->expects($this->never())
->method('dropIndex')
->with($this->anything());

$this->schemaManager->updateDocumentIndexes(Project::class);
}

/**
* @psalm-param IndexOptions $expectedWriteOptions
*
Expand Down
8 changes: 8 additions & 0 deletions tests/Documents/CmsProduct.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,12 @@ class CmsProduct extends CmsContent
* @var string|null
*/
public $name;

/**
* @ODM\Field(type="string")
* @ODM\Index
*
* @var string|null
*/
public $productId;
}
1 change: 1 addition & 0 deletions tests/Documents/Project.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class Project

/**
* @ODM\Field(type="string")
* @ODM\UniqueIndex
*
* @var string|null
*/
Expand Down
8 changes: 8 additions & 0 deletions tests/Documents/SubProject.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
*
Expand Down
Loading