From 2f898fcb47e1f777c0772c6f2712b9d94a7ebb06 Mon Sep 17 00:00:00 2001 From: Karol Lamparski Date: Tue, 10 Dec 2019 11:18:03 +0100 Subject: [PATCH] Fix logic of protection against adding same join multiple times Before that commit `QueryBuilderHelper::addJoinOnce` did not allow to add multiple joins for same association even if new join should have different type than already existing. That means that extensions and filters may affects each other depending on the apply order. Moreover this commit removes forcing left join if in query already exists at least one left join before. That behaviour also could affect filters depending of apply order. --- .../Doctrine/Orm/Util/QueryBuilderHelper.php | 9 +-- src/Bridge/Doctrine/Orm/Util/QueryChecker.php | 17 ----- .../Orm/Util/QueryBuilderHelperTest.php | 71 ++++++++++++++++++- 3 files changed, 75 insertions(+), 22 deletions(-) diff --git a/src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php b/src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php index 55b2eb1b16f..7579414325e 100644 --- a/src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php +++ b/src/Bridge/Doctrine/Orm/Util/QueryBuilderHelper.php @@ -33,7 +33,8 @@ private function __construct() */ public static function addJoinOnce(QueryBuilder $queryBuilder, QueryNameGeneratorInterface $queryNameGenerator, string $alias, string $association, string $joinType = null, string $conditionType = null, string $condition = null, string $originAlias = null, string $newAlias = null): string { - $join = self::getExistingJoin($queryBuilder, $alias, $association, $originAlias); + $joinType = $joinType ?? Join::INNER_JOIN; + $join = self::getExistingJoin($queryBuilder, $alias, $association, $originAlias, $joinType); if (null !== $join) { return $join->getAlias(); @@ -42,7 +43,7 @@ public static function addJoinOnce(QueryBuilder $queryBuilder, QueryNameGenerato $associationAlias = $newAlias ?? $queryNameGenerator->generateJoinAlias($association); $query = "$alias.$association"; - if (Join::LEFT_JOIN === $joinType || QueryChecker::hasLeftJoin($queryBuilder)) { + if (Join::LEFT_JOIN === $joinType) { $queryBuilder->leftJoin($query, $associationAlias, $conditionType, $condition); } else { $queryBuilder->innerJoin($query, $associationAlias, $conditionType, $condition); @@ -160,7 +161,7 @@ public static function traverseJoins(string $alias, QueryBuilder $queryBuilder, /** * Gets the existing join from QueryBuilder DQL parts. */ - private static function getExistingJoin(QueryBuilder $queryBuilder, string $alias, string $association, string $originAlias = null): ?Join + private static function getExistingJoin(QueryBuilder $queryBuilder, string $alias, string $association, ?string $originAlias, string $joinType): ?Join { $parts = $queryBuilder->getDQLPart('join'); $rootAlias = $originAlias ?? $queryBuilder->getRootAliases()[0]; @@ -171,7 +172,7 @@ private static function getExistingJoin(QueryBuilder $queryBuilder, string $alia foreach ($parts[$rootAlias] as $join) { /** @var Join $join */ - if (sprintf('%s.%s', $alias, $association) === $join->getJoin()) { + if (sprintf('%s.%s', $alias, $association) === $join->getJoin() && $join->getJoinType() === $joinType) { return $join; } } diff --git a/src/Bridge/Doctrine/Orm/Util/QueryChecker.php b/src/Bridge/Doctrine/Orm/Util/QueryChecker.php index ff1ed13c86b..7820f68d76d 100644 --- a/src/Bridge/Doctrine/Orm/Util/QueryChecker.php +++ b/src/Bridge/Doctrine/Orm/Util/QueryChecker.php @@ -15,7 +15,6 @@ use Doctrine\Common\Persistence\ManagerRegistry; use Doctrine\ORM\Mapping\ClassMetadata; -use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; /** @@ -162,22 +161,6 @@ public static function hasOrderByOnToManyJoin(QueryBuilder $queryBuilder, Manage return self::hasOrderByOnFetchJoinedToManyAssociation($queryBuilder, $managerRegistry); } - /** - * Determines whether the QueryBuilder already has a left join. - */ - public static function hasLeftJoin(QueryBuilder $queryBuilder): bool - { - foreach ($queryBuilder->getDQLPart('join') as $joins) { - foreach ($joins as $join) { - if (Join::LEFT_JOIN === $join->getJoinType()) { - return true; - } - } - } - - return false; - } - /** * Determines whether the QueryBuilder has a joined to-many association. */ diff --git a/tests/Bridge/Doctrine/Orm/Util/QueryBuilderHelperTest.php b/tests/Bridge/Doctrine/Orm/Util/QueryBuilderHelperTest.php index a921350f0cf..1014520e85c 100644 --- a/tests/Bridge/Doctrine/Orm/Util/QueryBuilderHelperTest.php +++ b/tests/Bridge/Doctrine/Orm/Util/QueryBuilderHelperTest.php @@ -20,6 +20,7 @@ use Doctrine\Common\Persistence\ManagerRegistry; use Doctrine\ORM\EntityManagerInterface; use Doctrine\ORM\Mapping\ClassMetadata; +use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; use PHPUnit\Framework\TestCase; use Prophecy\Argument; @@ -55,8 +56,49 @@ public function testAddJoinOnce(?string $originAliasForJoinOnce, string $expecte } /** - * @dataProvider provideAddJoinOnce + * @dataProvider provideAddJoinOnceWithMixedJoinTypes */ + public function testAddJoinOnceWithMixedJoinTypes( + string $originAliasForJoinOnce, + string $newAlias, + string $joinType, + string $expectedAlias, + string $expectedJoinType + ): void { + $queryBuilder = new QueryBuilder($this->prophesize(EntityManagerInterface::class)->reveal()); + $queryBuilder->from('foo', 'f'); + $queryBuilder->from('foo', 'f2'); + $queryBuilder->join('f.bar', 'b'); + $queryBuilder->join('f2.bar', 'b2'); + $queryBuilder->leftJoin('f2.bar', 'bl2'); + + $queryNameGenerator = $this->prophesize(QueryNameGeneratorInterface::class); + + $alias = QueryBuilderHelper::addJoinOnce( + $queryBuilder, + $queryNameGenerator->reveal(), + $originAliasForJoinOnce, + 'bar', + $joinType, + null, + null, + $originAliasForJoinOnce, + $newAlias + ); + + /** @var Join $join */ + foreach ($queryBuilder->getDQLPart('join')[$originAliasForJoinOnce] as $join) { + if ($join->getAlias() === $alias) { + $addedJoinType = $join->getJoinType(); + } + } + + $this->assertEqualsCanonicalizing( + [$expectedAlias, $expectedJoinType], + [$alias, $addedJoinType ?? null] + ); + } + public function testAddJoinOnceWithSpecifiedNewAlias() { $queryBuilder = new QueryBuilder($this->prophesize(EntityManagerInterface::class)->reveal()); @@ -140,4 +182,31 @@ public function provideAddJoinOnce(): array ], ]; } + + public function provideAddJoinOnceWithMixedJoinTypes(): array + { + return [ + 'Adding new join for already joined association but with different type' => [ + 'f', + 'bl', + Join::LEFT_JOIN, + 'bl', + Join::LEFT_JOIN, + ], + 'Adding already existing join with type left' => [ + 'f2', + 'bl8', + Join::LEFT_JOIN, + 'bl2', + Join::LEFT_JOIN, + ], + 'Adding already existing join with type inner' => [ + 'f2', + 'b8', + Join::INNER_JOIN, + 'b2', + Join::INNER_JOIN, + ], + ]; + } }