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

Correct DQL INSTANCE OF to filter all possible child classes #6392

Merged
merged 29 commits into from
Aug 18, 2017

Conversation

Jean85
Copy link
Contributor

@Jean85 Jean85 commented Apr 12, 2017

This is the rebased version of #1441, which fixes #4646.
Here is the original text:

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.

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.
Copy link
Contributor

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Status: Needs work.

{
parent::setUp();

$this->_schemaTool->createSchema(array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use short array syntax instead.

$this->assertCount(1, $result);

foreach ($result as $r) {
$this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Person', $r);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use ::class constant instead of hardcoding the QCN here.

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());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use assertSame() instead.

@Jean85
Copy link
Contributor Author

Jean85 commented Apr 18, 2017

All done, thanks for the review @phansys

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Can you please extract some methods from SqlWalker#walkInstanceOfExpression() it was already messy and now it's a bit harder to understand things properly.

Also could you try to relate the tests, and entities that each test use, to the ticket (as we have on Functional\Ticket classes) just to keep the "standard" we're having...

{
parent::setUp();

$this->_schemaTool->createSchema(array(
Copy link
Member

Choose a reason for hiding this comment

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

Please use short array syntax

parent::setUp();

$this->_schemaTool->createSchema(array(
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfAbstractTest\Person'),
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax

$this->assertCount(1, $result);

foreach ($result as $r) {
$this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Person', $r);
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax


foreach ($result as $r) {
$this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Person', $r);
$this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Employee', $r);
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax


$this->_schemaTool->createSchema(array(
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfAbstractTest\Person'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfAbstractTest\Employee'),
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax

* @InheritanceType(value="JOINED")
* @DiscriminatorColumn(name="kind", type="string")
* @DiscriminatorMap(value={
* "employee": "Doctrine\Tests\ORM\Functional\InstanceOfAbstractTest\Employee"
Copy link
Member

Choose a reason for hiding this comment

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

You can use "employee": Employee::class

parent::setUp();

$this->_schemaTool->createSchema(array(
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Person'),
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax


$this->_schemaTool->createSchema(array(
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Person'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Employee'),
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax

$this->_schemaTool->createSchema(array(
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Person'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Employee'),
$this->_em->getClassMetadata(__NAMESPACE__ . '\InstanceOfMultiLevelTest\Engineer'),
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax

$this->assertCount(3, $result);

foreach ($result as $r) {
$this->assertInstanceOf('Doctrine\Tests\ORM\Functional\InstanceOfMultiLevelTest\Person', $r);
Copy link
Member

Choose a reason for hiding this comment

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

Please use ::class syntax

@Jean85
Copy link
Contributor Author

Jean85 commented May 3, 2017

Thanks for the review @lcobucci!
I hope that I addressed al your concerns with my latest commits.

@Jean85
Copy link
Contributor Author

Jean85 commented May 3, 2017

A lot of tests are failing, probably for an unrelated issue... should I rebase? Or merge master?

[EDIT] Scrap that, the build is green 👍

$currentMetadata = $this->em->getClassMetadata($class);
$currentDiscriminator = $currentMetadata->discriminatorValue;

if (is_string($currentDiscriminator) && ! array_key_exists($currentDiscriminator, $discriminators)) {
Copy link
Member

Choose a reason for hiding this comment

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

A discriminator may be also an integer or similar - why do you want this to only work with string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know! I'll do a !== null then.

}

// Trim first backslash
$parameter = ltrim($parameter, '\\');
Copy link
Member

Choose a reason for hiding this comment

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

You should do $this->em->getClassMetadata($parameter); here - this already provides the ltrim() logic for you (internally)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! I was also already doing the getClassMetadata a few lines after, I'll just move that up!

// Trim first backslash
$parameter = ltrim($parameter, '\\');

if ($parameter !== $discrClass->name && ! array_key_exists($parameter, $knownSubclasses)) {
Copy link
Member

Choose a reason for hiding this comment

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

$metadata->getClass()->isSubclassOf(...) maybe?

Copy link
Member

Choose a reason for hiding this comment

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

This also has the effect of getting rid of INSTANCE OF checks where INSTANCE OF is pointing at the topmost class (which is most likely a bug in userland code)

$currentDiscriminator = $currentMetadata->discriminatorValue;

if (is_string($currentDiscriminator) && ! array_key_exists($currentDiscriminator, $discriminators)) {
$discriminators[$currentDiscriminator] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Consider using null if that true has no meaning

$sqlParameterList[] = $this->conn->quote($dis);
}

return '(' . implode(', ', $sqlParameterList) . ')';
Copy link
Member

Choose a reason for hiding this comment

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

According to this logic, the list may be empty, leading to a crash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How that could happen? Since I check if the class is unrelated at line 2297, at least one discriminator should pop up, or an exception be thrown...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but that flow makes it hard to follow: ideally we should array_map() over $instanceOfExpr->value, but let's see where the code goes after the additional tests provided by @taueres

}

foreach (array_keys($discriminators) as $dis) {
$sqlParameterList[] = $this->conn->quote($dis);
Copy link
Member

Choose a reason for hiding this comment

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

array_map([$this->conn, 'quote'], array_keys($discriminators))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may already have values inside $sqlParameterList, see line 2289, so I think it's better to do it like that.

Copy link
Member

Choose a reason for hiding this comment

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

Then just array_merge() with it

@@ -0,0 +1,114 @@
<?php

namespace Doctrine\Tests\ORM\Functional\Ticket {
Copy link
Member

Choose a reason for hiding this comment

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

Please put the entire file under a single namespace

*/
private $specialization;

public function getSpecialization()
Copy link
Member

Choose a reason for hiding this comment

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

Remove all accessors that aren't strictly needed: prefer public properties if they are needed (although note that these tests don't really need the properties to work, so you should drop everything but the identifier)

@@ -502,7 +502,7 @@ public function testSupportsInstanceOfExpressionsInWherePart()
{
$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')"
Copy link
Member

Choose a reason for hiding this comment

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

Ouch, this is a bad bug. Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All credits to @taueres !

@Ocramius Ocramius added the Bug label Jun 21, 2017
@Jean85
Copy link
Contributor Author

Jean85 commented Jun 21, 2017

Ok seems that I've addressed all your concerns, @Ocramius @lcobucci
Build is green with low deps, other failures seems unrelated, maybe they come from master?

@taueres
Copy link
Contributor

taueres commented Jun 21, 2017

Hi! Thank you very much for resuming this PR, I really appreciate that!
I checked this fix once more but unfortunately the issue still occurs when using INSTANCE OF with parameters.

Here is an example:

$dql = 'SELECT p FROM Person p WHERE p INSTANCE OF :parameter';
$query = $em->createQuery($dql);
$query->setParameter('parameter', $em->getClassMetadata(Person::class));

// $result contains only direct instances of `Person`, no subclasses
$result = $query->getResult();

I guess we need to change something about parameter resolution. I'll definitely have a look in the following days.

@taueres
Copy link
Contributor

taueres commented Jun 21, 2017

I wrote a failing test case for the issue I mentioned before: https://gist.github.com/taueres/05ff035bb2f72cddd7711b7f93cd9d6a

@Jean85
Copy link
Contributor Author

Jean85 commented Jun 22, 2017

Thanks @taueres!
I've found that your test case fails also when doing this:

$dql = 'SELECT p FROM Doctrine\Tests\ORM\Functional\Ticket\PersonTicket4646Parametric p
        WHERE p INSTANCE OF :parameter';
$query = $this->_em->createQuery($dql);
$query->setParameter(
    'parameter',
    PersonTicket4646Parametric::class
);

Do we want to check for this additional uses, @Ocramius ?

@Ocramius
Copy link
Member

$query->setParameter(
    'parameter',
    PersonTicket4646Parametric::class
);

This currently requires binding a ClassMetadata instance:

$query->setParameter(
    'parameter',
    $em->getClassMetadata(PersonTicket4646Parametric::class)
);

We can fix it, yes

@Jean85
Copy link
Contributor Author

Jean85 commented Jun 22, 2017

Ok I've added the failing test case. I'll try to work on that soon; @taueres if you have any tips, I'm all ears! 😄

@Ocramius Ocramius changed the title [REBASED][QUERY] "INSTANCE OF" now behaves correctly with subclasses Correct DQL INSTANCE OF to filter all possible child classes Jun 24, 2017
taueres and others added 4 commits June 24, 2017 10:58
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.
@Ocramius Ocramius self-assigned this Jun 26, 2017
@taueres
Copy link
Contributor

taueres commented Jul 7, 2017

To solve the duplication issue we could create something like:

class HierarchyDiscriminatorResolver
{
    public static function resolveDiscriminatorsForClass(ClassMetadata $rootClassMetadata, EntityManagerInterface $entityManager)
    {
        /* Duplicate code goes here */
        /* return list of discriminator */
    }
}

In the proposed solution, the EntityManager dependency is needed in order to retrieve ClassMetadata for subClasses.

I would not put such logic into ClassMetadata because atm ClassMetadata is not coupled to EntityManager.

@Jean85
Copy link
Contributor Author

Jean85 commented Jul 7, 2017

This seems a feasible solution for me... @Ocramius ? What do you think?

@Ocramius
Copy link
Member

@Jean85 as long as the class is marked as @internal, sure 👍

Totally missed these comments before btw.

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 18, 2017

Don't worry @Ocramius, I was on vacation anyway 😄
Deduplication of code done, build is green, this is ready for a final review!

* @package Doctrine\ORM\Utility
* @internal This class exists only to avoid code duplication, do not reuse it externally
*/
class HierarchyDiscriminatorResolver
Copy link
Member

Choose a reason for hiding this comment

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

This class needs to be tested in isolation

Copy link
Member

Choose a reason for hiding this comment

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

final

use Doctrine\ORM\EntityManagerInterface;

/**
* Class HierarchyDiscriminatorResolver
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line


/**
* Class HierarchyDiscriminatorResolver
* @package Doctrine\ORM\Utility
Copy link
Member

Choose a reason for hiding this comment

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

Remove this line

*/
class HierarchyDiscriminatorResolver
{
public static function resolveDiscriminatorsForClass(
Copy link
Member

Choose a reason for hiding this comment

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

Add a private constructor

Copy link
Member

Choose a reason for hiding this comment

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

An explanation of the method is needed

public static function resolveDiscriminatorsForClass(
ClassMetadata $rootClassMetadata,
EntityManagerInterface $entityManager
) {
Copy link
Member

Choose a reason for hiding this comment

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

Return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot that the target branch is 7+! Good!

$hierarchyClasses[] = $rootClassMetadata->name;

$discriminators = [];
foreach ($hierarchyClasses as $class) {
Copy link
Member

Choose a reason for hiding this comment

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

Empty line before logical blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should enforce this in the Lint stage of the CI!

Copy link
Member

@lcobucci lcobucci Aug 18, 2017

Choose a reason for hiding this comment

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

We're going to 😉

@taueres
Copy link
Contributor

taueres commented Aug 18, 2017

According to the DQL spec. Users can provide multiple classes for an INSTANCE OF statement.

InstanceOfExpression     ::= IdentificationVariable ["NOT"] "INSTANCE" ["OF"]
    (InstanceOfParameter | "(" InstanceOfParameter {"," InstanceOfParameter}* ")")

Do we have some tests covering this specific use case?

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Some nitpicking 😄

* @return string The list in parentheses of valid child discriminators from the given class
* @throws QueryException
*/
private function getChildDiscriminatorsFromClassMetadata(ClassMetadataInfo $rootClass, AST\InstanceOfExpression $instanceOfExpr)
Copy link
Member

Choose a reason for hiding this comment

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

Please add the return type declaration


class Ticket4646InstanceOfAbstractTest extends OrmFunctionalTestCase
{
protected function setUp()
Copy link
Member

Choose a reason for hiding this comment

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

: void

]);
}

public function testInstanceOf()
Copy link
Member

Choose a reason for hiding this comment

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

: void

$query = $this->_em->createQuery($dql);
$result = $query->getResult();

$this->assertCount(1, $result);
Copy link
Member

Choose a reason for hiding this comment

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

Please use self::assert* for new tests


class Ticket4646InstanceOfMultiLevelTest extends OrmFunctionalTestCase
{
protected function setUp()
Copy link
Member

Choose a reason for hiding this comment

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

: void

]);
}

public function testInstanceOf()
Copy link
Member

Choose a reason for hiding this comment

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

: void

*/
private $id;

public function getId()
Copy link
Member

Choose a reason for hiding this comment

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

: ?int

*/
private $id;

public function getId()
Copy link
Member

Choose a reason for hiding this comment

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

: ?int

$query = $this->_em->createQuery($dql);
$result = $query->getResult();

$this->assertCount(2, $result);
Copy link
Member

Choose a reason for hiding this comment

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

Here too

*/
private $id;

public function getId()
Copy link
Member

Choose a reason for hiding this comment

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

: ?int

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 18, 2017

@lcobucci @Ocramius done as per review!

@Jean85
Copy link
Contributor Author

Jean85 commented Aug 18, 2017

I've also added a new functional test to cover the case of multiple parameters passed to the INSTANCEOF, as suggested by @taueres

[EDIT] Sorry, made a mistake, fixing the build now

@Ocramius Ocramius dismissed lcobucci’s stale review August 18, 2017 19:33

Review comments handled

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🚢

@Ocramius Ocramius merged commit 1a0bb82 into doctrine:master Aug 18, 2017
@Ocramius
Copy link
Member

Thanks @Jean85 @taueres and @phansys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants