Skip to content

Fix #1021 order by hidden eager loading #1022

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.

Sign up for GitHub

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

Merged
soyuka merged 3 commits into from
Apr 1, 2017
+165 −3
Merged

Fix #1021 order by hidden eager loading #1022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecb9d24
Fix #1021 order by hidden eager loading
soyuka Mar 30, 2017
53de77b
fix appveyor
Mar 30, 2017
7a59035
Fix #1023 allow filter eager loading on composite identifiers
Mar 30, 2017
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
25 changes: 22 additions & 3 deletions src/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,31 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
}

$joinParts = $queryBuilder->getDQLPart('join');
$originAlias = 'o';

if (!$joinParts || !isset($joinParts['o'])) {
if (!$joinParts || !isset($joinParts[$originAlias])) {
return;
}

$queryBuilderClone = clone $queryBuilder;
$queryBuilderClone->resetDQLPart('where');
$queryBuilderClone->andWhere($queryBuilderClone->expr()->in('o', $this->getQueryBuilderWithNewAliases($queryBuilder, $queryNameGenerator)->getDQL()));

$classMetadata = $queryBuilder->getEntityManager()->getClassMetadata($resourceClass);

if (!$classMetadata->isIdentifierComposite) {
$replacementAlias = $queryNameGenerator->generateJoinAlias($originAlias);
$in = $this->getQueryBuilderWithNewAliases($queryBuilder, $queryNameGenerator, $originAlias, $replacementAlias);
$in->select($replacementAlias);
$queryBuilderClone->andWhere($queryBuilderClone->expr()->in($originAlias, $in->getDQL()));
} else {
// Because Doctrine doesn't support WHERE ( foo, bar ) IN () (https://github.com/doctrine/doctrine2/issues/5238), we are building as many subqueries as they are identifiers
foreach ($classMetadata->identifier as $identifier) {
$replacementAlias = $queryNameGenerator->generateJoinAlias($originAlias);
$in = $this->getQueryBuilderWithNewAliases($queryBuilder, $queryNameGenerator, $originAlias, $replacementAlias);
$in->select("IDENTITY($replacementAlias.$identifier)");
$queryBuilderClone->andWhere($queryBuilderClone->expr()->in("$originAlias.$identifier", $in->getDQL()));
}
}

$queryBuilder->resetDQLPart('where');
$queryBuilder->add('where', $queryBuilderClone->getDQLPart('where'));
Expand All @@ -72,14 +89,16 @@ public function applyToCollection(QueryBuilder $queryBuilder, QueryNameGenerator
private function getQueryBuilderWithNewAliases(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $originAlias = 'o', string $replacement = 'o_2')
{
$queryBuilderClone = clone $queryBuilder;
$queryBuilderClone->select($replacement);

$joinParts = $queryBuilder->getDQLPart('join');
$wherePart = $queryBuilder->getDQLPart('where');

//reset parts
$queryBuilderClone->resetDQLPart('join');
$queryBuilderClone->resetDQLPart('where');
$queryBuilderClone->resetDQLPart('orderBy');
$queryBuilderClone->resetDQLPart('groupBy');
$queryBuilderClone->resetDQLPart('having');

//Change from alias
$from = $queryBuilderClone->getDQLPart('from')[0];
Expand Down
143 changes: 143 additions & 0 deletions tests/Bridge/Doctrine/Orm/Extension/FilterEagerLoadingExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGeneratorInterface;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Resource\ResourceMetadata;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar;
use ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\Foo;
use Doctrine\ORM\EntityManager;
use Doctrine\ORM\Mapping\ClassMetadataInfo;
use Doctrine\ORM\Query\Expr;
use Doctrine\ORM\QueryBuilder;

Expand Down Expand Up @@ -112,6 +114,7 @@ public function testApplyCollection()

$em = $this->prophesize(EntityManager::class);
$em->getExpressionBuilder()->shouldBeCalled()->willReturn(new Expr());
$em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));

$qb = new QueryBuilder($em->reveal());

Expand All @@ -123,10 +126,150 @@ public function testApplyCollection()

$queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
$queryNameGenerator->generateJoinAlias('colors')->shouldBeCalled()->willReturn('colors_2');
$queryNameGenerator->generateJoinAlias('o')->shouldBeCalled()->willReturn('o_2');

$filterEagerLoadingExtension = new FilterEagerLoadingExtension($resourceMetadataFactoryProphecy->reveal(), true);
$filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), DummyCar::class, 'get');

$this->assertEquals('SELECT o FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o LEFT JOIN o.colors colors WHERE o IN(SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2 LEFT JOIN o_2.colors colors_2 WHERE o_2.colors = :foo)', $qb->getDQL());
}

/**
* https://github.com/api-platform/core/issues/1021.
*/
public function testHiddenOrderBy()
{
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$resourceMetadataFactoryProphecy->create(DummyCar::class)->willReturn(new ResourceMetadata(DummyCar::class));

$em = $this->prophesize(EntityManager::class);
$em->getExpressionBuilder()->shouldBeCalled()->willReturn(new Expr());
$em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));

$qb = new QueryBuilder($em->reveal());

$qb->select('o', 'CASE WHEN o.dateCreated IS NULL THEN 0 ELSE 1 END AS HIDDEN _o_dateCreated_null_rank')
->from(DummyCar::class, 'o')
->leftJoin('o.colors', 'colors')
->where('o.colors = :foo')
->orderBy('_o_dateCreated_null_rank DESC')
->setParameter('foo', 1);

$queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
$queryNameGenerator->generateJoinAlias('colors')->shouldBeCalled()->willReturn('colors_2');
$queryNameGenerator->generateJoinAlias('o')->shouldBeCalled()->willReturn('o_2');
$filterEagerLoadingExtension = new FilterEagerLoadingExtension($resourceMetadataFactoryProphecy->reveal(), true);
$filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), DummyCar::class, 'get');

$expected = <<<SQL
SELECT o, CASE WHEN o.dateCreated IS NULL THEN 0 ELSE 1 END AS HIDDEN _o_dateCreated_null_rank
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o
LEFT JOIN o.colors colors
WHERE o IN(
SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2
LEFT JOIN o_2.colors colors_2
WHERE o_2.colors = :foo
) ORDER BY _o_dateCreated_null_rank DESC ASC
SQL;

$this->assertEquals($this->toDQLString($expected), $qb->getDQL());
}

public function testGroupBy()
{
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$resourceMetadataFactoryProphecy->create(DummyCar::class)->willReturn(new ResourceMetadata(DummyCar::class));

$em = $this->prophesize(EntityManager::class);
$em->getExpressionBuilder()->shouldBeCalled()->willReturn(new Expr());
$em->getClassMetadata(DummyCar::class)->shouldBeCalled()->willReturn(new ClassMetadataInfo(DummyCar::class));

$qb = new QueryBuilder($em->reveal());

$qb->select('o', 'count(o.id) as counter')
->from(DummyCar::class, 'o')
->leftJoin('o.colors', 'colors')
->where('o.colors = :foo')
->orderBy('o.colors')
->groupBy('o.colors')
->having('counter > 3')
->setParameter('foo', 1);

$queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
$queryNameGenerator->generateJoinAlias('colors')->shouldBeCalled()->willReturn('colors_2');
$queryNameGenerator->generateJoinAlias('o')->shouldBeCalled()->willReturn('o_2');
$filterEagerLoadingExtension = new FilterEagerLoadingExtension($resourceMetadataFactoryProphecy->reveal(), true);
$filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), DummyCar::class, 'get');

$expected = <<<SQL
SELECT o, count(o.id) as counter
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o
LEFT JOIN o.colors colors WHERE o
IN(
SELECT o_2 FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\DummyCar o_2
LEFT JOIN o_2.colors colors_2
WHERE o_2.colors = :foo
)
GROUP BY o.colors HAVING counter > 3
ORDER BY o.colors ASC
SQL;

$this->assertEquals($this->toDQLString($expected), $qb->getDQL());
}

public function testCompositeIdentifiers()
{
$resourceMetadataFactoryProphecy = $this->prophesize(ResourceMetadataFactoryInterface::class);
$resourceMetadataFactoryProphecy->create(CompositeRelation::class)->willReturn(new ResourceMetadata(CompositeRelation::class));

$classMetadata = new ClassMetadataInfo(CompositeRelation::class);
$classMetadata->isIdentifierComposite = true;
$classMetadata->identifier = ['item', 'label'];

$em = $this->prophesize(EntityManager::class);
$em->getExpressionBuilder()->shouldBeCalled()->willReturn(new Expr());
$em->getClassMetadata(CompositeRelation::class)->shouldBeCalled()->willReturn($classMetadata);

$qb = new QueryBuilder($em->reveal());

$qb->select('o')
->from(CompositeRelation::class, 'o')
->innerJoin('o.compositeItem', 'item')
->innerJoin('o.compositeLabel', 'label')
->where('item.field1 = :foo')
->setParameter('foo', 1);

$queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class);
$queryNameGenerator->generateJoinAlias('item')->shouldBeCalled()->willReturn('item_2');
$queryNameGenerator->generateJoinAlias('label')->shouldBeCalled()->willReturn('label_2');
$queryNameGenerator->generateJoinAlias('o')->shouldBeCalled()->willReturn('o_2');

$filterEagerLoadingExtension = new FilterEagerLoadingExtension($resourceMetadataFactoryProphecy->reveal(), true);
$filterEagerLoadingExtension->applyToCollection($qb, $queryNameGenerator->reveal(), CompositeRelation::class, 'get');

$expected = <<<SQL
SELECT o
FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o
INNER JOIN o.compositeItem item
INNER JOIN o.compositeLabel label
WHERE o.item IN(
SELECT IDENTITY(o_2.item) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2
INNER JOIN o_2.compositeItem item_2
INNER JOIN o_2.compositeLabel label_2
WHERE item_2.field1 = :foo
) AND o.label IN(
SELECT IDENTITY(o_2.label) FROM ApiPlatform\Core\Tests\Fixtures\TestBundle\Entity\CompositeRelation o_2
INNER JOIN o_2.compositeItem item_2
INNER JOIN o_2.compositeLabel label_2
WHERE item_2.field1 = :foo
)
SQL;

@soyuka soyuka Mar 30, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

WDYT @teohhanhui ? A better fix would be to fix doctrine but this works fine as a temporary fix (will take a long time to get that fixed in doctrine :p). Indeed, it should be WHERE (o.item, o.label) IN (subquery).

@dunglas dunglas Apr 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe can you add a comment with this rationale? We'll remove this temporary when (if) the patch will be merged in Doctrine.

@teohhanhui teohhanhui Apr 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't even know there's such a syntax in SQL. Obviously I don't really use composite keys 😛

Anyway, I think this is fine.

@teohhanhui teohhanhui Apr 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh, but I think the tests are confusing, because there seems to be no eager loading going on (how do we tell?). If so, we can skip query rewriting in such cases, right?

@soyuka soyuka Apr 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The fix will get applied only if the conditions are a match (where clause + eager + composite identifier in this case). There will be no rewriting if those conditions don't match.
I force those conditions with the test.

@teohhanhui teohhanhui Apr 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I mean is that even though $forceEager is true, the query itself might not need to be rewritten if there is no eager loading going on, no? (Not specific to this test case.)

@soyuka soyuka Apr 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Eager loading is active when there are joins, this condition checks that. The rewrite is needed only when there is a where clause.


$this->assertEquals($this->toDQLString($expected), $qb->getDQL());
}

private function toDQLString(string $dql): string
{
return preg_replace('/\\r\\n|\\n/', '', str_replace(' ', '', $dql));

@teohhanhui teohhanhui Apr 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we improve the regex then there'd be no need for the str_replace 😄

@soyuka soyuka Apr 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

haha yeah appveyor behavior was driving me crazy, intent was first to use PHP_EOL then I ended up with the regexp. Anyway, no big deal this is just to make tests more readable.

}
}