Skip to content

Commit

Permalink
Merge pull request #2025 from alcaeus/fix-wrong-discriminator-inherit…
Browse files Browse the repository at this point in the history
…ance

Fix wrong usage of discriminator map in complex document inheritance chains
  • Loading branch information
malarzm authored May 3, 2019
2 parents 51093a4 + e7d240c commit 89c4da8
Show file tree
Hide file tree
Showing 5 changed files with 208 additions and 18 deletions.
5 changes: 4 additions & 1 deletion UPGRADE-2.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use ODM 2.0 directly.
* `doctrine/mongodb` is no longer used by ODM. If you've been relying on its
functionality, please update accordingly. Most utility classes from
`doctrine/mongodb` have been merged into their ODM counterparts. Classes
`doctrine/mongodb` have been merged into their ODM counterparts. Classes
handling connections to MongoDB servers are being replaced by the MongoDB
library (`mongodb/mongodb`).
* The constructor signature of `Doctrine\ODM\MongoDB\DocumentManager` as well as
Expand Down Expand Up @@ -261,6 +261,9 @@ ocramius. If you are checking for proxies, the following changed:
cursor.
* The `eagerCursor` helper in `Doctrine\ODM\MongoDB\Query\Builder` and its logic
have been removed entirely without replacement.
* Querying for a mapped superclass in a complex inheritance chain will now only
return children of that specific class instead of all classes in the
inheritance tree.

## Schema manager

Expand Down
4 changes: 4 additions & 0 deletions lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php
Original file line number Diff line number Diff line change
Expand Up @@ -784,6 +784,10 @@ public function setDiscriminatorMap(array $map) : void
throw MappingException::discriminatorNotAllowedForGridFS($this->name);
}

$this->subClasses = [];
$this->discriminatorMap = [];
$this->discriminatorValue = null;

foreach ($map as $value => $className) {
$this->discriminatorMap[$value] = $className;
if ($this->name === $className) {
Expand Down
40 changes: 27 additions & 13 deletions lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public function __construct(
$this->class = $class;
$this->cp = $this->uow->getCollectionPersister();

if ($class->isMappedSuperclass || $class->isEmbeddedDocument || $class->isQueryResultDocument) {
if ($class->isEmbeddedDocument || $class->isQueryResultDocument) {
return;
}

Expand Down Expand Up @@ -918,22 +918,31 @@ public function prepareFieldName(string $fieldName) : string
/**
* Adds discriminator criteria to an already-prepared query.
*
* If the class we're querying has a discriminator field set, we add all
* possible discriminator values to the query. The list of possible
* discriminator values is based on the discriminatorValue of the class
* itself as well as those of all its subclasses.
*
* This method should be used once for query criteria and not be used for
* nested expressions. It should be called before
* {@link DocumentPerister::addFilterToPreparedQuery()}.
*/
public function addDiscriminatorToPreparedQuery(array $preparedQuery) : array
{
/* If the class has a discriminator field, which is not already in the
* criteria, inject it now. The field/values need no preparation.
*/
if ($this->class->hasDiscriminator() && ! isset($preparedQuery[$this->class->discriminatorField])) {
$discriminatorValues = $this->getClassDiscriminatorValues($this->class);
if (count($discriminatorValues) === 1) {
$preparedQuery[$this->class->discriminatorField] = $discriminatorValues[0];
} else {
$preparedQuery[$this->class->discriminatorField] = ['$in' => $discriminatorValues];
}
if (isset($preparedQuery[$this->class->discriminatorField]) || $this->class->discriminatorField === null) {
return $preparedQuery;
}

$discriminatorValues = $this->getClassDiscriminatorValues($this->class);

if ($discriminatorValues === []) {
return $preparedQuery;
}

if (count($discriminatorValues) === 1) {
$preparedQuery[$this->class->discriminatorField] = $discriminatorValues[0];
} else {
$preparedQuery[$this->class->discriminatorField] = ['$in' => $discriminatorValues];
}

return $preparedQuery;
Expand Down Expand Up @@ -1281,11 +1290,16 @@ private function hasQueryOperators($value) : bool
}

/**
* Gets the array of discriminator values for the given ClassMetadata
* Returns the list of discriminator values for the given ClassMetadata
*/
private function getClassDiscriminatorValues(ClassMetadata $metadata) : array
{
$discriminatorValues = [$metadata->discriminatorValue];
$discriminatorValues = [];

if ($metadata->discriminatorValue !== null) {
$discriminatorValues[] = $metadata->discriminatorValue;
}

foreach ($metadata->subClasses as $className) {
$key = array_search($className, $metadata->discriminatorMap);
if (! $key) {
Expand Down
141 changes: 141 additions & 0 deletions tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH1962Test.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
<?php

declare(strict_types=1);

namespace Doctrine\ODM\MongoDB\Tests\Functional\Ticket;

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;
use Doctrine\ODM\MongoDB\Tests\BaseTest;

class GH1962Test extends BaseTest
{
public function testDiscriminatorMaps()
{
$metadata = $this->dm->getClassMetadata(GH1962Superclass::class);
self::assertCount(3, $metadata->discriminatorMap);

$metadata = $this->dm->getClassMetadata(GH1962BarSuperclass::class);
self::assertCount(2, $metadata->discriminatorMap);
}

public function testFetchingDiscriminatedDocuments()
{
$foo = new GH1962FooDocument();
$bar = new GH1962BarDocument();
$baz = new GH1962BazDocument();

$this->dm->persist($foo);
$this->dm->persist($bar);
$this->dm->persist($baz);

$this->dm->flush();
$this->dm->clear();

self::assertCount(3, $this->dm->getRepository(GH1962Superclass::class)->findAll());
self::assertCount(2, $this->dm->getRepository(GH1962BarSuperclass::class)->findAll());

self::assertCount(1, $this->dm->getRepository(GH1962FooDocument::class)->findAll());
self::assertCount(1, $this->dm->getRepository(GH1962BarDocument::class)->findAll());
self::assertCount(1, $this->dm->getRepository(GH1962BazDocument::class)->findAll());
}

public function testFetchingDiscriminatedDocumentsWithoutDiscriminatorMap()
{
$foo = new GH1962FooDocumentWithoutDiscriminatorMap();
$bar = new GH1962BarDocumentWithoutDiscriminatorMap();
$baz = new GH1962BazDocumentWithoutDiscriminatorMap();

$this->dm->persist($foo);
$this->dm->persist($bar);
$this->dm->persist($baz);

$this->dm->flush();
$this->dm->clear();

// Since the extending superclass does not know about its child classes,
// it will yield the same results as querying for the parent class
// Without discriminator maps, only leafs in the inheritance tree will
// use a discriminator value in the query
self::assertCount(3, $this->dm->getRepository(GH1962SuperclassWithoutDiscriminatorMap::class)->findAll());
self::assertCount(3, $this->dm->getRepository(GH1962BarSuperclassWithoutDiscriminatorMap::class)->findAll());

self::assertCount(3, $this->dm->getRepository(GH1962FooDocumentWithoutDiscriminatorMap::class)->findAll());
self::assertCount(3, $this->dm->getRepository(GH1962BarDocumentWithoutDiscriminatorMap::class)->findAll());
self::assertCount(1, $this->dm->getRepository(GH1962BazDocumentWithoutDiscriminatorMap::class)->findAll());
}
}

/**
* @ODM\MappedSuperclass()
* @ODM\DiscriminatorField("type")
* @ODM\InheritanceType("SINGLE_COLLECTION")
* @ODM\DiscriminatorMap({
* "foo"=GH1962FooDocument::class,
* "bar"=GH1962BarDocument::class,
* "baz"=GH1962BazDocument::class
* })
*/
class GH1962Superclass
{
/** @ODM\Id */
public $id;
}

/** @ODM\Document */
class GH1962FooDocument extends GH1962Superclass
{
}

/**
* @ODM\MappedSuperclass()
* @ODM\DiscriminatorMap({
* "bar"=GH1962BarDocument::class,
* "baz"=GH1962BazDocument::class
* })
*/
class GH1962BarSuperclass extends GH1962Superclass
{
}

/** @ODM\Document */
class GH1962BarDocument extends GH1962BarSuperclass
{
}

/** @ODM\Document */
class GH1962BazDocument extends GH1962BarSuperclass
{
}
/**
* @ODM\MappedSuperclass()
* @ODM\DiscriminatorField("type")
* @ODM\InheritanceType("SINGLE_COLLECTION")
*/
class GH1962SuperclassWithoutDiscriminatorMap
{
/** @ODM\Id */
public $id;
}

/** @ODM\Document */
class GH1962FooDocumentWithoutDiscriminatorMap extends GH1962SuperclassWithoutDiscriminatorMap
{
}

/** @ODM\MappedSuperclass() */
class GH1962BarSuperclassWithoutDiscriminatorMap extends GH1962SuperclassWithoutDiscriminatorMap
{
}

/** @ODM\Document */
class GH1962BarDocumentWithoutDiscriminatorMap extends GH1962BarSuperclassWithoutDiscriminatorMap
{
}

/**
* @ODM\Document
* @ODM\DiscriminatorValue(GH1962BazDocumentWithoutDiscriminatorMap::class)
*/
class GH1962BazDocumentWithoutDiscriminatorMap extends GH1962BarSuperclassWithoutDiscriminatorMap
{
}
36 changes: 32 additions & 4 deletions tests/Doctrine/ODM/MongoDB/Tests/Query/BuilderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,25 @@ public function testReferencesGoesThroughDiscriminatorMap()
->field('featureFull')->references($f)
->getQuery()->debug();

$this->assertEquals([ 'featureFull.$id' => new ObjectId($f->id) ], $q1['query']);
$this->assertEquals(
[
'featureFull.$id' => new ObjectId($f->id),
'type' => ['$in' => ['ca', 'cb', 'cc']],
],
$q1['query']
);

$q2 = $this->dm->createQueryBuilder(ParentClass::class)
->field('featureSimple')->references($f)
->getQuery()->debug();

$this->assertEquals([ 'featureSimple' => new ObjectId($f->id) ], $q2['query']);
$this->assertEquals(
[
'featureSimple' => new ObjectId($f->id),
'type' => ['$in' => ['ca', 'cb', 'cc']],
],
$q2['query']
);

$q3 = $this->dm->createQueryBuilder(ParentClass::class)
->field('featurePartial')->references($f)
Expand All @@ -55,6 +67,7 @@ public function testReferencesGoesThroughDiscriminatorMap()
[
'featurePartial.$id' => new ObjectId($f->id),
'featurePartial.$ref' => 'Feature',
'type' => ['$in' => ['ca', 'cb', 'cc']],
],
$q3['query']
);
Expand Down Expand Up @@ -97,13 +110,27 @@ public function testIncludesReferenceToGoesThroughDiscriminatorMap()
->field('featureFullMany')->includesReferenceTo($f)
->getQuery()->debug();

$this->assertEquals([ 'featureFullMany' => [ '$elemMatch' => [ '$id' => new ObjectId($f->id) ] ] ], $q1['query']);
$this->assertEquals(
[
'featureFullMany' => [
'$elemMatch' => ['$id' => new ObjectId($f->id)],
],
'type' => ['$in' => ['ca', 'cb', 'cc']],
],
$q1['query']
);

$q2 = $this->dm->createQueryBuilder(ParentClass::class)
->field('featureSimpleMany')->includesReferenceTo($f)
->getQuery()->debug();

$this->assertEquals([ 'featureSimpleMany' => new ObjectId($f->id) ], $q2['query']);
$this->assertEquals(
[
'featureSimpleMany' => new ObjectId($f->id),
'type' => ['$in' => ['ca', 'cb', 'cc']],
],
$q2['query']
);

$q3 = $this->dm->createQueryBuilder(ParentClass::class)
->field('featurePartialMany')->includesReferenceTo($f)
Expand All @@ -117,6 +144,7 @@ public function testIncludesReferenceToGoesThroughDiscriminatorMap()
'$ref' => 'Feature',
],
],
'type' => ['$in' => ['ca', 'cb', 'cc']],
],
$q3['query']
);
Expand Down

0 comments on commit 89c4da8

Please sign in to comment.