From e798bfe34a4ec2b685567717f18b747508a47b5c Mon Sep 17 00:00:00 2001 From: Sergio Santoro Date: Mon, 29 Jun 2015 22:05:26 +0200 Subject: [PATCH] [QUERY] "INSTANCE OF" now behaves correctly with subclasses There was a bug in the "INSTANCE OF" operator as described in https://groups.google.com/forum/#!topic/doctrine-user/B8raq8CNMgg "INSTANCE OF" was not taking into account subclasses. It was merely translating the class to its discriminator. This is not correct since the class can have subtypes and those are, indeed, still instance of the superclass. Also, classes may not have a discriminator (e.g. abstract classes). This commit also provides useful tests to avoid regression. --- lib/Doctrine/ORM/Query/SqlWalker.php | 38 +++-- .../ORM/Functional/InstanceOfAbstractTest.php | 113 +++++++++++++ .../Functional/InstanceOfMultiLevelTest.php | 153 ++++++++++++++++++ .../Tests/ORM/Functional/InstanceOfTest.php | 119 ++++++++++++++ .../ORM/Query/SelectSqlGenerationTest.php | 6 +- 5 files changed, 413 insertions(+), 16 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/InstanceOfAbstractTest.php create mode 100644 tests/Doctrine/Tests/ORM/Functional/InstanceOfMultiLevelTest.php create mode 100644 tests/Doctrine/Tests/ORM/Functional/InstanceOfTest.php diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 57287c7cb..62350c389 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -2038,8 +2038,10 @@ class SqlWalker implements TreeWalker $sql .= $class->discriminatorColumn['name'] . ($instanceOfExpr->not ? ' NOT IN ' : ' IN '); - $sqlParameterList = []; + $knownSubclasses = array_flip($discrClass->subClasses); + $sqlParameterList = []; + $discriminators = []; foreach ($instanceOfExpr->value as $parameter) { if ($parameter instanceof AST\InputParameter) { $this->rsm->addMetadataParameterMapping($parameter->name, 'discriminatorValue'); @@ -2049,21 +2051,31 @@ class SqlWalker implements TreeWalker continue; } - // Get name from ClassMetadata to resolve aliases. - $entityClassName = $this->em->getClassMetadata($parameter)->name; - $discriminatorValue = $class->discriminatorValue; + // Trim first backslash + $parameter = ltrim($parameter, '\\'); - if ($entityClassName !== $class->name) { - $discrMap = array_flip($class->discriminatorMap); - - if ( ! isset($discrMap[$entityClassName])) { - throw QueryException::instanceOfUnrelatedClass($entityClassName, $class->rootEntityName); - } - - $discriminatorValue = $discrMap[$entityClassName]; + // Check parameter is really in the hierarchy + if ($parameter !== $discrClass->name && ! array_key_exists($parameter, $knownSubclasses)) { + throw QueryException::instanceOfUnrelatedClass($parameter, $discrClass->name); } - $sqlParameterList[] = $this->conn->quote($discriminatorValue); + // Include discriminators for parameter class and its subclass + $metadata = $this->em->getClassMetadata($parameter); + $hierarchyClasses = $metadata->subClasses; + $hierarchyClasses[] = $metadata->name; + + foreach ($hierarchyClasses as $class) { + $currentMetadata = $this->em->getClassMetadata($class); + $currentDiscriminator = $currentMetadata->discriminatorValue; + + if (is_string($currentDiscriminator) && ! array_key_exists($currentDiscriminator, $discriminators)) { + $discriminators[$currentDiscriminator] = true; + } + } + } + + foreach (array_keys($discriminators) as $dis) { + $sqlParameterList[] = $this->conn->quote($dis); } $sql .= '(' . implode(', ', $sqlParameterList) . ')'; diff --git a/tests/Doctrine/Tests/ORM/Functional/InstanceOfAbstractTest.php b/tests/Doctrine/Tests/ORM/Functional/InstanceOfAbstractTest.php new file mode 100644 index 000000000..a84643f84 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/InstanceOfAbstractTest.php @@ -0,0 +1,113 @@ +_schemaTool->createSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfAbstractTest\Person'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfAbstractTest\Employee'), + )); + } + + public function testInstanceOf() + { + $this->loadData(); + + $dql = 'SELECT p FROM Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Person p + WHERE p INSTANCE OF Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Person'; + $query = $this->_em->createQuery($dql); + $result = $query->getResult(); + + $this->assertCount(1, $result); + + foreach ($result as $r) { + $this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Person', $r); + $this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Employee', $r); + $this->assertEquals('bar', $r->getName()); + } + } + + private function loadData() + { + $employee = new InstanceOfAbstractTest\Employee(); + $employee->setName('bar'); + $employee->setDepartement('qux'); + + $this->_em->persist($employee); + + $this->_em->flush($employee); + } + } +} + +namespace Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest { + + /** + * @Entity() + * @Table(name="instance_of_abstract_test_person") + * @InheritanceType(value="JOINED") + * @DiscriminatorColumn(name="kind", type="string") + * @DiscriminatorMap(value={ + * "employee": "Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Employee" + * }) + */ + abstract class Person + { + /** + * @Id() + * @GeneratedValue() + * @Column(type="integer") + */ + private $id; + + /** + * @Column(type="string") + */ + private $name; + + public function getId() + { + return $this->id; + } + + public function getName() + { + return $this->name; + } + + public function setName($name) + { + $this->name = $name; + } + } + + /** + * @Entity() + * @Table(name="instance_of_abstract_test_employee") + */ + class Employee extends Person + { + /** + * @Column(type="string") + */ + private $departement; + + public function getDepartement() + { + return $this->departement; + } + + public function setDepartement($departement) + { + $this->departement = $departement; + } + } + +} diff --git a/tests/Doctrine/Tests/ORM/Functional/InstanceOfMultiLevelTest.php b/tests/Doctrine/Tests/ORM/Functional/InstanceOfMultiLevelTest.php new file mode 100644 index 000000000..c4b5bbcd2 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/InstanceOfMultiLevelTest.php @@ -0,0 +1,153 @@ +_schemaTool->createSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Person'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Employee'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Engineer'), + )); + } + + public function testInstanceOf() + { + $this->loadData(); + + $dql = 'SELECT p FROM Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Person p + WHERE p INSTANCE OF Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Person'; + $query = $this->_em->createQuery($dql); + $result = $query->getResult(); + + $this->assertCount(3, $result); + + foreach ($result as $r) { + $this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Person', $r); + if ($r instanceof InstanceOfMultiLevelTest\Engineer) { + $this->assertEquals('foobar', $r->getName()); + $this->assertEquals('doctrine', $r->getSpecialization()); + } elseif ($r instanceof InstanceOfMultiLevelTest\Employee) { + $this->assertEquals('bar', $r->getName()); + $this->assertEquals('qux', $r->getDepartement()); + } else { + $this->assertEquals('foo', $r->getName()); + } + } + } + + private function loadData() + { + $person = new InstanceOfMultiLevelTest\Person(); + $person->setName('foo'); + + $employee = new InstanceOfMultiLevelTest\Employee(); + $employee->setName('bar'); + $employee->setDepartement('qux'); + + $engineer = new InstanceOfMultiLevelTest\Engineer(); + $engineer->setName('foobar'); + $engineer->setDepartement('dep'); + $engineer->setSpecialization('doctrine'); + + $this->_em->persist($person); + $this->_em->persist($employee); + $this->_em->persist($engineer); + + $this->_em->flush(array($person, $employee, $engineer)); + } + } +} + +namespace Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest { + /** + * @Entity() + * @Table(name="instance_of_multi_level_test_person") + * @InheritanceType(value="JOINED") + * @DiscriminatorColumn(name="kind", type="string") + * @DiscriminatorMap(value={ + * "person": "Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Person", + * "employee": "Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Employee", + * "engineer": "Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Engineer", + * }) + */ + class Person + { + /** + * @Id() + * @GeneratedValue() + * @Column(type="integer") + */ + private $id; + + /** + * @Column(type="string") + */ + private $name; + + public function getId() + { + return $this->id; + } + + public function getName() + { + return $this->name; + } + + public function setName($name) + { + $this->name = $name; + } + } + + /** + * @Entity() + * @Table(name="instance_of_multi_level_employee") + */ + class Employee extends Person + { + /** + * @Column(type="string") + */ + private $departement; + + public function getDepartement() + { + return $this->departement; + } + + public function setDepartement($departement) + { + $this->departement = $departement; + } + } + + /** + * @Entity() + * @Table(name="instance_of_multi_level_engineer") + */ + class Engineer extends Employee + { + /** + * @Column(type="string") + */ + private $specialization; + + public function getSpecialization() + { + return $this->specialization; + } + + public function setSpecialization($specialization) + { + $this->specialization = $specialization; + } + } +} diff --git a/tests/Doctrine/Tests/ORM/Functional/InstanceOfTest.php b/tests/Doctrine/Tests/ORM/Functional/InstanceOfTest.php new file mode 100644 index 000000000..7f593c3f4 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/InstanceOfTest.php @@ -0,0 +1,119 @@ +_schemaTool->createSchema(array( + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfTest\Person'), + $this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfTest\Employee'), + )); + } + + public function testInstanceOf() + { + $this->loadData(); + + $dql = 'SELECT p FROM Doctrine\Tests\ORM\Functional\InstanceOfTest\Person p + WHERE p INSTANCE OF Doctrine\Tests\ORM\Functional\InstanceOfTest\Person'; + $query = $this->_em->createQuery($dql); + $result = $query->getResult(); + + $this->assertCount(2, $result); + + foreach ($result as $r) { + $this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfTest\Person', $r); + if ($r instanceof InstanceOfTest\Employee) { + $this->assertEquals('bar', $r->getName()); + } else { + $this->assertEquals('foo', $r->getName()); + } + } + } + + private function loadData() + { + $person = new InstanceOfTest\Person(); + $person->setName('foo'); + + $employee = new InstanceOfTest\Employee(); + $employee->setName('bar'); + $employee->setDepartement('qux'); + + $this->_em->persist($person); + $this->_em->persist($employee); + + $this->_em->flush(array($person, $employee)); + } + } +} + +namespace Doctrine\Tests\ORM\Functional\InstanceOfTest { + /** + * @Entity() + * @Table(name="instance_of_test_person") + * @InheritanceType(value="JOINED") + * @DiscriminatorColumn(name="kind", type="string") + * @DiscriminatorMap(value={ + * "person": "Doctrine\Tests\ORM\Functional\InstanceOfTest\Person", + * "employee": "Doctrine\Tests\ORM\Functional\InstanceOfTest\Employee" + * }) + */ + class Person + { + /** + * @Id() + * @GeneratedValue() + * @Column(type="integer") + */ + private $id; + + /** + * @Column(type="string") + */ + private $name; + + public function getId() + { + return $this->id; + } + + public function getName() + { + return $this->name; + } + + public function setName($name) + { + $this->name = $name; + } + } + + /** + * @Entity() + * @Table(name="instance_of_test_employee") + */ + class Employee extends Person + { + /** + * @Column(type="string") + */ + private $departement; + + public function getDepartement() + { + return $this->departement; + } + + public function setDepartement($departement) + { + $this->departement = $departement; + } + } +} diff --git a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php index b6bdce323..f9fb0b820 100644 --- a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php +++ b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php @@ -502,7 +502,7 @@ class SelectSqlGenerationTest extends OrmTestCase { $this->assertSqlGeneration( "SELECT u FROM Doctrine\Tests\Models\Company\CompanyPerson u WHERE u INSTANCE OF Doctrine\Tests\Models\Company\CompanyEmployee", - "SELECT c0_.id AS id_0, c0_.name AS name_1, c0_.discr AS discr_2 FROM company_persons c0_ WHERE c0_.discr IN ('employee')" + "SELECT c0_.id AS id_0, c0_.name AS name_1, c0_.discr AS discr_2 FROM company_persons c0_ WHERE c0_.discr IN ('manager', 'employee')" ); } @@ -511,7 +511,7 @@ class SelectSqlGenerationTest extends OrmTestCase // This also uses FQCNs starting with or without a backslash in the INSTANCE OF parameter $this->assertSqlGeneration( "SELECT u FROM Doctrine\Tests\Models\Company\CompanyPerson u WHERE u INSTANCE OF (Doctrine\Tests\Models\Company\CompanyEmployee, \Doctrine\Tests\Models\Company\CompanyManager)", - "SELECT c0_.id AS id_0, c0_.name AS name_1, c0_.discr AS discr_2 FROM company_persons c0_ WHERE c0_.discr IN ('employee', 'manager')" + "SELECT c0_.id AS id_0, c0_.name AS name_1, c0_.discr AS discr_2 FROM company_persons c0_ WHERE c0_.discr IN ('manager', 'employee')" ); } @@ -522,7 +522,7 @@ class SelectSqlGenerationTest extends OrmTestCase { $this->assertSqlGeneration( "SELECT u FROM Doctrine\Tests\Models\Company\CompanyPerson u WHERE u INSTANCE OF \Doctrine\Tests\Models\Company\CompanyEmployee", - "SELECT c0_.id AS id_0, c0_.name AS name_1, c0_.discr AS discr_2 FROM company_persons c0_ WHERE c0_.discr IN ('employee')" + "SELECT c0_.id AS id_0, c0_.name AS name_1, c0_.discr AS discr_2 FROM company_persons c0_ WHERE c0_.discr IN ('manager', 'employee')" ); }