From 55882ca7a646d4c1c908a9b7bc3fd7b8345f658a Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 27 May 2017 15:56:57 +0200 Subject: [PATCH 1/5] #6464 add test --- .../ORM/Functional/Ticket/GH6464Test.php | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php new file mode 100644 index 000000000..a69166d5d --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php @@ -0,0 +1,89 @@ +_schemaTool->createSchema( + [ + $this->_em->getClassMetadata(GH6464Post::class), + $this->_em->getClassMetadata(GH6464User::class), + $this->_em->getClassMetadata(GH6464Author::class), + ] + ); + } + + /** + * Verifies that SqlWalker generates valid SQL for an INNER JOIN to CTI table + * + * SqlWalker needs to generate nested INNER JOIN statements, otherwise there would be INNER JOIN + * statements without an ON clause, which are valid on e.g. MySQL but rejected by PostgreSQL. + */ + public function testIssue() + { + + $qb = $this->_em->createQueryBuilder(); + + $query = $qb + ->select('p', 'a') + ->from(GH6464Post::class, 'p') + ->innerJoin(GH6464Author::class, 'a', 'WITH', 'p.authorId = a.id') + ->getQuery(); + + $this->assertNotRegExp( + '/INNER JOIN \w+ \w+ INNER JOIN/', + $query->getSQL(), + 'As of GH-6464, every INNER JOIN should have an ON clause, which is missing here' + ); + + // Query shouldn't yield a result, yet it shouldn't crash (anymore) + $this->assertEquals([], $query->getResult()); + } +} + +/** @Entity */ +class GH6464Post +{ + /** @Id @Column(type="integer") @GeneratedValue */ + public $id; + + /** @Column(type="integer") */ + public $authorId; + + /** @Column(length=100) */ + public $title; + + /** @Column(type="text") */ + public $text; +} + +/** + * @Entity + * @InheritanceType("JOINED") + * @DiscriminatorColumn(name="discr", type="string") + * @DiscriminatorMap({"author" = "GH6464Author"}) + */ +abstract class GH6464User +{ + /** @Id @Column(type="integer") @GeneratedValue */ + public $id; +} + +/** @Entity */ +class GH6464Author extends GH6464User +{ + /** @Column(length=50) */ + public $displayName; +} From 99fdbf550df0e070eb63df834676b9e12fedeae7 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 27 May 2017 19:03:16 +0200 Subject: [PATCH 2/5] generate nested join sql for CTIs, closes #6464 --- lib/Doctrine/ORM/Query/SqlWalker.php | 13 +++++++++---- .../Tests/ORM/Query/SelectSqlGenerationTest.php | 6 +++--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 98f58ad8c..0fc3090f8 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -872,10 +872,11 @@ class SqlWalker implements TreeWalker * Walks down a RangeVariableDeclaration AST node, thereby generating the appropriate SQL. * * @param AST\RangeVariableDeclaration $rangeVariableDeclaration + * @param bool $buildNestedJoins * * @return string */ - public function walkRangeVariableDeclaration($rangeVariableDeclaration) + public function walkRangeVariableDeclaration($rangeVariableDeclaration, $buildNestedJoins = false) { $class = $this->em->getClassMetadata($rangeVariableDeclaration->abstractSchemaName); $dqlAlias = $rangeVariableDeclaration->aliasIdentificationVariable; @@ -891,7 +892,11 @@ class SqlWalker implements TreeWalker ); if ($class->isInheritanceTypeJoined()) { - $sql .= $this->_generateClassTableInheritanceJoins($class, $dqlAlias); + if ($buildNestedJoins) { + $sql = '(' . $sql . $this->_generateClassTableInheritanceJoins($class, $dqlAlias) . ')'; + } else { + $sql .= $this->_generateClassTableInheritanceJoins($class, $dqlAlias); + } } return $sql; @@ -1137,11 +1142,11 @@ class SqlWalker implements TreeWalker $conditions[] = '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')'; } - $condExprConjunction = ($class->isInheritanceTypeJoined() && $joinType != AST\Join::JOIN_TYPE_LEFT && $joinType != AST\Join::JOIN_TYPE_LEFTOUTER) + $condExprConjunction = ($class->isInheritanceTypeJoined() && $joinType != AST\Join::JOIN_TYPE_LEFT && $joinType != AST\Join::JOIN_TYPE_LEFTOUTER && empty($conditions)) ? ' AND ' : ' ON '; - $sql .= $this->walkRangeVariableDeclaration($joinDeclaration); + $sql .= $this->walkRangeVariableDeclaration($joinDeclaration, !empty($conditions)); // Apply remaining inheritance restrictions $discrSql = $this->_generateDiscriminatorColumnConditionSQL([$dqlAlias]); diff --git a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php index c10afbb89..9e04c3183 100644 --- a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php +++ b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php @@ -198,12 +198,12 @@ class SelectSqlGenerationTest extends OrmTestCase { $this->assertSqlGeneration( 'SELECT e FROM Doctrine\Tests\Models\Company\CompanyEmployee e JOIN Doctrine\Tests\Models\Company\CompanyManager m WITH e.id = m.id', - 'SELECT c0_.id AS id_0, c0_.name AS name_1, c1_.salary AS salary_2, c1_.department AS department_3, c1_.startDate AS startDate_4, c0_.discr AS discr_5 FROM company_employees c1_ INNER JOIN company_persons c0_ ON c1_.id = c0_.id INNER JOIN company_managers c2_ INNER JOIN company_employees c4_ ON c2_.id = c4_.id INNER JOIN company_persons c3_ ON c2_.id = c3_.id AND (c0_.id = c3_.id)' + 'SELECT c0_.id AS id_0, c0_.name AS name_1, c1_.salary AS salary_2, c1_.department AS department_3, c1_.startDate AS startDate_4, c0_.discr AS discr_5 FROM company_employees c1_ INNER JOIN company_persons c0_ ON c1_.id = c0_.id INNER JOIN (company_managers c2_ INNER JOIN company_employees c4_ ON c2_.id = c4_.id INNER JOIN company_persons c3_ ON c2_.id = c3_.id) ON (c0_.id = c3_.id)' ); $this->assertSqlGeneration( 'SELECT e FROM Doctrine\Tests\Models\Company\CompanyEmployee e LEFT JOIN Doctrine\Tests\Models\Company\CompanyManager m WITH e.id = m.id', - 'SELECT c0_.id AS id_0, c0_.name AS name_1, c1_.salary AS salary_2, c1_.department AS department_3, c1_.startDate AS startDate_4, c0_.discr AS discr_5 FROM company_employees c1_ INNER JOIN company_persons c0_ ON c1_.id = c0_.id LEFT JOIN company_managers c2_ INNER JOIN company_employees c4_ ON c2_.id = c4_.id INNER JOIN company_persons c3_ ON c2_.id = c3_.id ON (c0_.id = c3_.id)' + 'SELECT c0_.id AS id_0, c0_.name AS name_1, c1_.salary AS salary_2, c1_.department AS department_3, c1_.startDate AS startDate_4, c0_.discr AS discr_5 FROM company_employees c1_ INNER JOIN company_persons c0_ ON c1_.id = c0_.id LEFT JOIN (company_managers c2_ INNER JOIN company_employees c4_ ON c2_.id = c4_.id INNER JOIN company_persons c3_ ON c2_.id = c3_.id) ON (c0_.id = c3_.id)' ); } @@ -2211,7 +2211,7 @@ class SelectSqlGenerationTest extends OrmTestCase // the where clause when not joining onto that table $this->assertSqlGeneration( 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyContract c LEFT JOIN Doctrine\Tests\Models\Company\CompanyEmployee e WITH e.id = c.salesPerson WHERE c.completed = true', - "SELECT c0_.id AS id_0, c0_.completed AS completed_1, c0_.fixPrice AS fixPrice_2, c0_.hoursWorked AS hoursWorked_3, c0_.pricePerHour AS pricePerHour_4, c0_.maxPrice AS maxPrice_5, c0_.discr AS discr_6 FROM company_contracts c0_ LEFT JOIN company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id ON (c2_.id = c0_.salesPerson_id) WHERE (c0_.completed = 1) AND c0_.discr IN ('fix', 'flexible', 'flexultra')" + "SELECT c0_.id AS id_0, c0_.completed AS completed_1, c0_.fixPrice AS fixPrice_2, c0_.hoursWorked AS hoursWorked_3, c0_.pricePerHour AS pricePerHour_4, c0_.maxPrice AS maxPrice_5, c0_.discr AS discr_6 FROM company_contracts c0_ LEFT JOIN (company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id) ON (c2_.id = c0_.salesPerson_id) WHERE (c0_.completed = 1) AND c0_.discr IN ('fix', 'flexible', 'flexultra')" ); } From 025ed1147bc981ae24a93c351408bc9aa71c4698 Mon Sep 17 00:00:00 2001 From: Stefan Siegl Date: Sat, 29 Jul 2017 12:41:57 +0200 Subject: [PATCH 3/5] #6464 code review updates --- lib/Doctrine/ORM/Query/SqlWalker.php | 23 +++++++++++++++---- .../ORM/Functional/Ticket/GH6464Test.php | 13 +---------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 0fc3090f8..713f5534e 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -872,11 +872,23 @@ class SqlWalker implements TreeWalker * Walks down a RangeVariableDeclaration AST node, thereby generating the appropriate SQL. * * @param AST\RangeVariableDeclaration $rangeVariableDeclaration + * + * @return string + */ + public function walkRangeVariableDeclaration($rangeVariableDeclaration) + { + return $this->generateRangeVariableDeclarationSQL($rangeVariableDeclaration, false); + } + + /** + * Generate appropriate SQL for RangeVariableDeclaration AST node + * + * @param AST\RangeVariableDeclaration $rangeVariableDeclaration * @param bool $buildNestedJoins * * @return string */ - public function walkRangeVariableDeclaration($rangeVariableDeclaration, $buildNestedJoins = false) + private function generateRangeVariableDeclarationSQL($rangeVariableDeclaration, $buildNestedJoins) { $class = $this->em->getClassMetadata($rangeVariableDeclaration->abstractSchemaName); $dqlAlias = $rangeVariableDeclaration->aliasIdentificationVariable; @@ -1132,7 +1144,7 @@ class SqlWalker implements TreeWalker : ' INNER JOIN '; switch (true) { - case ($joinDeclaration instanceof \Doctrine\ORM\Query\AST\RangeVariableDeclaration): + case ($joinDeclaration instanceof AST\RangeVariableDeclaration): $class = $this->em->getClassMetadata($joinDeclaration->abstractSchemaName); $dqlAlias = $joinDeclaration->aliasIdentificationVariable; $tableAlias = $this->getSQLTableAlias($class->table['name'], $dqlAlias); @@ -1142,11 +1154,12 @@ class SqlWalker implements TreeWalker $conditions[] = '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')'; } - $condExprConjunction = ($class->isInheritanceTypeJoined() && $joinType != AST\Join::JOIN_TYPE_LEFT && $joinType != AST\Join::JOIN_TYPE_LEFTOUTER && empty($conditions)) + $isUnconditionalJoin = empty($conditions); + $condExprConjunction = ($class->isInheritanceTypeJoined() && $joinType != AST\Join::JOIN_TYPE_LEFT && $joinType != AST\Join::JOIN_TYPE_LEFTOUTER && $isUnconditionalJoin) ? ' AND ' : ' ON '; - $sql .= $this->walkRangeVariableDeclaration($joinDeclaration, !empty($conditions)); + $sql .= $this->generateRangeVariableDeclarationSQL($joinDeclaration, !$isUnconditionalJoin); // Apply remaining inheritance restrictions $discrSql = $this->_generateDiscriminatorColumnConditionSQL([$dqlAlias]); @@ -1168,7 +1181,7 @@ class SqlWalker implements TreeWalker break; - case ($joinDeclaration instanceof \Doctrine\ORM\Query\AST\JoinAssociationDeclaration): + case ($joinDeclaration instanceof AST\JoinAssociationDeclaration): $sql .= $this->walkJoinAssociationDeclaration($joinDeclaration, $joinType, $join->conditionalExpression); break; } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php index a69166d5d..75762e7d7 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php @@ -33,10 +33,7 @@ class GH6464Test extends OrmFunctionalTestCase */ public function testIssue() { - - $qb = $this->_em->createQueryBuilder(); - - $query = $qb + $query = $this->_em->createQueryBuilder() ->select('p', 'a') ->from(GH6464Post::class, 'p') ->innerJoin(GH6464Author::class, 'a', 'WITH', 'p.authorId = a.id') @@ -61,12 +58,6 @@ class GH6464Post /** @Column(type="integer") */ public $authorId; - - /** @Column(length=100) */ - public $title; - - /** @Column(type="text") */ - public $text; } /** @@ -84,6 +75,4 @@ abstract class GH6464User /** @Entity */ class GH6464Author extends GH6464User { - /** @Column(length=50) */ - public $displayName; } From 91a50916121beff07e413d4905cf5f63dec7dac6 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Fri, 11 Aug 2017 21:39:13 +0200 Subject: [PATCH 4/5] #6464 #6475 cleaning up test - removed invalid fetch join, CS --- .../Tests/ORM/Functional/Ticket/GH6464Test.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php index 75762e7d7..adc6f7b16 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php @@ -16,13 +16,11 @@ class GH6464Test extends OrmFunctionalTestCase { parent::setUp(); - $this->_schemaTool->createSchema( - [ - $this->_em->getClassMetadata(GH6464Post::class), - $this->_em->getClassMetadata(GH6464User::class), - $this->_em->getClassMetadata(GH6464Author::class), - ] - ); + $this->_schemaTool->createSchema([ + $this->_em->getClassMetadata(GH6464Post::class), + $this->_em->getClassMetadata(GH6464User::class), + $this->_em->getClassMetadata(GH6464Author::class), + ]); } /** @@ -34,7 +32,7 @@ class GH6464Test extends OrmFunctionalTestCase public function testIssue() { $query = $this->_em->createQueryBuilder() - ->select('p', 'a') + ->select('p') ->from(GH6464Post::class, 'p') ->innerJoin(GH6464Author::class, 'a', 'WITH', 'p.authorId = a.id') ->getQuery(); From 9ad91ddc1c57542833532a89bcc22faacb4dcd32 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Fri, 11 Aug 2017 21:45:23 +0200 Subject: [PATCH 5/5] #6464 #6475 using PHP 7.1 `void` and `string` return hints where available --- lib/Doctrine/ORM/Query/SqlWalker.php | 2 +- tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 713f5534e..6e2e41ddc 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -888,7 +888,7 @@ class SqlWalker implements TreeWalker * * @return string */ - private function generateRangeVariableDeclarationSQL($rangeVariableDeclaration, $buildNestedJoins) + private function generateRangeVariableDeclarationSQL($rangeVariableDeclaration, bool $buildNestedJoins) : string { $class = $this->em->getClassMetadata($rangeVariableDeclaration->abstractSchemaName); $dqlAlias = $rangeVariableDeclaration->aliasIdentificationVariable; diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php index adc6f7b16..1d75cdf3d 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php @@ -12,7 +12,7 @@ class GH6464Test extends OrmFunctionalTestCase /** * {@inheritDoc} */ - protected function setUp() + protected function setUp() : void { parent::setUp(); @@ -29,7 +29,7 @@ class GH6464Test extends OrmFunctionalTestCase * SqlWalker needs to generate nested INNER JOIN statements, otherwise there would be INNER JOIN * statements without an ON clause, which are valid on e.g. MySQL but rejected by PostgreSQL. */ - public function testIssue() + public function testIssue() : void { $query = $this->_em->createQueryBuilder() ->select('p')