From 68776106132418ef249f3f071b5c5f655a2f52b8 Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Sun, 28 Apr 2013 23:15:48 +0100 Subject: [PATCH 1/7] Initial idea for fixing doctrine bug DDC-2235. The issue is when using DQL to perform a left join on an entity using single table inheritance, doctrine tries to insert an `IN()` predicate into the `WHERE` clause for all of the discriminator values. That makes sense and is valid, so it would be wrong to remove that behaviour. However when using a *left* join having an `IN()` in the main where clause makes the `LEFT JOIN` pretty much useless, as it implicitly creates a `WHERE NOT NULL` clause. This commit attempts to fix that by including an `OR IS NULL` in the query if the join is a `LEFT JOIN`. --- lib/Doctrine/ORM/Query/SqlWalker.php | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 77b8ec1d471..7a751709bcf 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -145,6 +145,13 @@ class SqlWalker implements TreeWalker */ private $rootAliases = array(); + /** + * Any DQL alises used for left joins + * + * @var array + */ + private $leftJoinAliases = array(); + /** * Flag that indicates whether to generate SQL table aliases in the SQL. * These should only be generated for SELECT queries, not for UPDATE/DELETE. @@ -436,8 +443,15 @@ private function _generateDiscriminatorColumnConditionSQL(array $dqlAliases) $values[] = $conn->quote($this->em->getClassMetadata($subclassName)->discriminatorValue); } - $sqlParts[] = (($this->useSqlTableAliases) ? $this->getSQLTableAlias($class->getTableName(), $dqlAlias) . '.' : '') - . $class->discriminatorColumn['name'] . ' IN (' . implode(', ', $values) . ')'; + $field = (($this->useSqlTableAliases) ? $this->getSQLTableAlias($class->getTableName(), $dqlAlias) . '.' : '') + . $class->discriminatorColumn['name']; + $sql = $field . ' IN (' . implode(', ', $values) . ')'; + + if (in_array($dqlAlias, $this->leftJoinAliases)) { + $sql = '(' . $sql . ' OR ' . $field . ' IS NULL )'; + } + + $sqlParts[] = $sql; } $sql = implode(' AND ', $sqlParts); @@ -1026,6 +1040,10 @@ public function walkJoin($join) ? ' LEFT JOIN ' : ' INNER JOIN '; + if ($joinType == AST\Join::JOIN_TYPE_LEFT) { + $this->leftJoinAliases[] = $joinDeclaration->aliasIdentificationVariable; + } + switch (true) { case ($joinDeclaration instanceof \Doctrine\ORM\Query\AST\RangeVariableDeclaration): $class = $this->em->getClassMetadata($joinDeclaration->abstractSchemaName); From 615c25a368a544cbdbfaf3947f76da4047a4a66a Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Mon, 29 Apr 2013 00:07:52 +0100 Subject: [PATCH 2/7] Adjusted whitespace and added unit tests --- lib/Doctrine/ORM/Query/SqlWalker.php | 2 +- .../ORM/Query/SelectSqlGenerationTest.php | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 7a751709bcf..b778f6f9e6c 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -448,7 +448,7 @@ private function _generateDiscriminatorColumnConditionSQL(array $dqlAliases) $sql = $field . ' IN (' . implode(', ', $values) . ')'; if (in_array($dqlAlias, $this->leftJoinAliases)) { - $sql = '(' . $sql . ' OR ' . $field . ' IS NULL )'; + $sql = '(' . $sql . ' OR ' . $field . ' IS NULL)'; } $sqlParts[] = $sql; diff --git a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php index 747df24bbbe..bd3309a2f0e 100644 --- a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php +++ b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php @@ -1458,6 +1458,30 @@ public function testInheritanceTypeSingleTableInLeafClassWithEnabledForcePartial ); } + /** + * @group DDC-2235 + */ + public function testInheritanceTypeSingleTableInLeftJoinIncludesOrIsNull() + { + // Regression test for best-case + $this->assertSqlGeneration( + 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyEmployee e LEFT JOIN Doctrine\Tests\Models\Company\CompanyContract c WITH c.salesPerson = e.id', + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id LEFT JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) WHERE (c0_.discr IN ('fix', 'flexible', 'flexultra') OR c0_.discr IS NULL)" + ); + + // Ensure other WHERE predicates are passed through to the main WHERE clause + $this->assertSqlGeneration( + 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyEmployee e LEFT JOIN Doctrine\Tests\Models\Company\CompanyContract c WITH c.salesPerson = e.id WHERE e.salary > 1000', + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id LEFT JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) WHERE (c1_.salary > 1000) AND (c0_.discr IN ('fix', 'flexible', 'flexultra') OR c0_.discr IS NULL)" + ); + + // Inner joins don't require the IS NULL + $this->assertSqlGeneration( + 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyEmployee e INNER JOIN Doctrine\Tests\Models\Company\CompanyContract c WITH c.salesPerson = e.id', + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id INNER JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) WHERE c0_.discr IN ('fix', 'flexible', 'flexultra')" + ); + } + /** * @group DDC-1161 */ From 080c3e3addc9e3b816c590456913c1fc088059df Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Mon, 29 Apr 2013 00:21:27 +0100 Subject: [PATCH 3/7] Spelling --- lib/Doctrine/ORM/Query/SqlWalker.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index b778f6f9e6c..4559bf89e24 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -146,7 +146,7 @@ class SqlWalker implements TreeWalker private $rootAliases = array(); /** - * Any DQL alises used for left joins + * Any DQL aliases used for left joins * * @var array */ From 8ecc571accf05dad5dffc7e1f49fd64e25990186 Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Mon, 29 Apr 2013 10:03:28 +0100 Subject: [PATCH 4/7] Moved the `IN()` predicate into the `ON` clause of the join --- lib/Doctrine/ORM/Query/SqlWalker.php | 37 +++++++------------ .../ORM/Query/SelectSqlGenerationTest.php | 37 ++++++++++++++++--- 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 4559bf89e24..6203a1fa5d8 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -145,13 +145,6 @@ class SqlWalker implements TreeWalker */ private $rootAliases = array(); - /** - * Any DQL aliases used for left joins - * - * @var array - */ - private $leftJoinAliases = array(); - /** * Flag that indicates whether to generate SQL table aliases in the SQL. * These should only be generated for SELECT queries, not for UPDATE/DELETE. @@ -443,15 +436,8 @@ private function _generateDiscriminatorColumnConditionSQL(array $dqlAliases) $values[] = $conn->quote($this->em->getClassMetadata($subclassName)->discriminatorValue); } - $field = (($this->useSqlTableAliases) ? $this->getSQLTableAlias($class->getTableName(), $dqlAlias) . '.' : '') - . $class->discriminatorColumn['name']; - $sql = $field . ' IN (' . implode(', ', $values) . ')'; - - if (in_array($dqlAlias, $this->leftJoinAliases)) { - $sql = '(' . $sql . ' OR ' . $field . ' IS NULL)'; - } - - $sqlParts[] = $sql; + $sqlParts[] = (($this->useSqlTableAliases) ? $this->getSQLTableAlias($class->getTableName(), $dqlAlias) . '.' : '') + . $class->discriminatorColumn['name'] . ' IN (' . implode(', ', $values) . ')'; } $sql = implode(' AND ', $sqlParts); @@ -824,12 +810,14 @@ public function walkFromClause($fromClause) * * @return string */ - public function walkRangeVariableDeclaration($rangeVariableDeclaration) + public function walkRangeVariableDeclaration($rangeVariableDeclaration, $rootAlias = true) { $class = $this->em->getClassMetadata($rangeVariableDeclaration->abstractSchemaName); $dqlAlias = $rangeVariableDeclaration->aliasIdentificationVariable; - $this->rootAliases[] = $dqlAlias; + if ($rootAlias) { + $this->rootAliases[] = $dqlAlias; + } $sql = $class->getQuotedTableName($this->platform) . ' ' . $this->getSQLTableAlias($class->getTableName(), $dqlAlias); @@ -1035,15 +1023,12 @@ public function walkJoin($join) { $joinType = $join->joinType; $joinDeclaration = $join->joinAssociationDeclaration; + $joinAlias = $joinDeclaration->aliasIdentificationVariable; $sql = ($joinType == AST\Join::JOIN_TYPE_LEFT || $joinType == AST\Join::JOIN_TYPE_LEFTOUTER) ? ' LEFT JOIN ' : ' INNER JOIN '; - if ($joinType == AST\Join::JOIN_TYPE_LEFT) { - $this->leftJoinAliases[] = $joinDeclaration->aliasIdentificationVariable; - } - switch (true) { case ($joinDeclaration instanceof \Doctrine\ORM\Query\AST\RangeVariableDeclaration): $class = $this->em->getClassMetadata($joinDeclaration->abstractSchemaName); @@ -1051,8 +1036,14 @@ public function walkJoin($join) ? ' AND ' : ' ON '; - $sql .= $this->walkRangeVariableDeclaration($joinDeclaration) + $sql .= $this->walkRangeVariableDeclaration($joinDeclaration, false) . $condExprConjunction . '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')'; + + $discrSql = $this->_generateDiscriminatorColumnConditionSQL(array($joinAlias)); + if ( ! empty($discrSql)) { + $sql .= ' AND (' . $discrSql . ')'; + } + break; case ($joinDeclaration instanceof \Doctrine\ORM\Query\AST\JoinAssociationDeclaration): diff --git a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php index bd3309a2f0e..f551667ee0e 100644 --- a/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php +++ b/tests/Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php @@ -1461,24 +1461,49 @@ public function testInheritanceTypeSingleTableInLeafClassWithEnabledForcePartial /** * @group DDC-2235 */ - public function testInheritanceTypeSingleTableInLeftJoinIncludesOrIsNull() + public function testInheritanceTypeSingleTableIncludesDiscriminatorInJoinOnClause() { - // Regression test for best-case + // Regression test for the bug $this->assertSqlGeneration( 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyEmployee e LEFT JOIN Doctrine\Tests\Models\Company\CompanyContract c WITH c.salesPerson = e.id', - "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id LEFT JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) WHERE (c0_.discr IN ('fix', 'flexible', 'flexultra') OR c0_.discr IS NULL)" + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id LEFT JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) AND (c0_.discr IN ('fix', 'flexible', 'flexultra'))" ); // Ensure other WHERE predicates are passed through to the main WHERE clause $this->assertSqlGeneration( 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyEmployee e LEFT JOIN Doctrine\Tests\Models\Company\CompanyContract c WITH c.salesPerson = e.id WHERE e.salary > 1000', - "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id LEFT JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) WHERE (c1_.salary > 1000) AND (c0_.discr IN ('fix', 'flexible', 'flexultra') OR c0_.discr IS NULL)" + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id LEFT JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) AND (c0_.discr IN ('fix', 'flexible', 'flexultra')) WHERE c1_.salary > 1000" ); - // Inner joins don't require the IS NULL + // Test inner joins too $this->assertSqlGeneration( 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyEmployee e INNER JOIN Doctrine\Tests\Models\Company\CompanyContract c WITH c.salesPerson = e.id', - "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id INNER JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) WHERE c0_.discr IN ('fix', 'flexible', 'flexultra')" + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c2_ ON c1_.id = c2_.id INNER JOIN company_contracts c0_ ON (c0_.salesPerson_id = c2_.id) AND (c0_.discr IN ('fix', 'flexible', 'flexultra'))" + ); + + // Test that the discriminator IN() predicate is still added into + // 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 id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 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')" + ); + + // Test that the discriminator IN() predicate is still added + // into the where clause when not joining onto a single table inheritance entity + // via a join association + $this->assertSqlGeneration( + 'SELECT c FROM Doctrine\Tests\Models\Company\CompanyContract c JOIN c.salesPerson s WHERE c.completed = true', + "SELECT c0_.id AS id0, c0_.completed AS completed1, c0_.fixPrice AS fixPrice2, c0_.hoursWorked AS hoursWorked3, c0_.pricePerHour AS pricePerHour4, c0_.maxPrice AS maxPrice5, c0_.discr AS discr6 FROM company_contracts c0_ INNER JOIN company_employees c1_ ON c0_.salesPerson_id = c1_.id LEFT JOIN company_persons c2_ ON c1_.id = c2_.id WHERE (c0_.completed = 1) AND c0_.discr IN ('fix', 'flexible', 'flexultra')" + ); + + // Test that when joining onto an entity using single table inheritance via + // a join association that the discriminator IN() predicate is placed + // into the ON clause of the join + $this->assertSqlGeneration( + 'SELECT e, COUNT(c) FROM Doctrine\Tests\Models\Company\CompanyEmployee e JOIN e.contracts c WHERE e.department = :department', + "SELECT c0_.id AS id0, c0_.name AS name1, c1_.salary AS salary2, c1_.department AS department3, c1_.startDate AS startDate4, COUNT(c2_.id) AS sclr5, c0_.discr AS discr6 FROM company_employees c1_ INNER JOIN company_persons c0_ ON c1_.id = c0_.id INNER JOIN company_contract_employees c3_ ON c1_.id = c3_.employee_id INNER JOIN company_contracts c2_ ON c2_.id = c3_.contract_id AND c2_.discr IN ('fix', 'flexible', 'flexultra') WHERE c1_.department = ?", + array(), + array('department' => 'foobar') ); } From 1d869835148c7b1d248f18d6dcd710d1fdce2ea8 Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Mon, 29 Apr 2013 10:24:55 +0100 Subject: [PATCH 5/7] Small code cleanliness bits --- lib/Doctrine/ORM/Query/SqlWalker.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 6203a1fa5d8..6ba02f06d35 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -437,7 +437,7 @@ private function _generateDiscriminatorColumnConditionSQL(array $dqlAliases) } $sqlParts[] = (($this->useSqlTableAliases) ? $this->getSQLTableAlias($class->getTableName(), $dqlAlias) . '.' : '') - . $class->discriminatorColumn['name'] . ' IN (' . implode(', ', $values) . ')'; + . $class->discriminatorColumn['name'] . ' IN (' . implode(', ', $values) . ')'; } $sql = implode(' AND ', $sqlParts); @@ -807,6 +807,7 @@ public function walkFromClause($fromClause) * Walks down a RangeVariableDeclaration AST node, thereby generating the appropriate SQL. * * @param AST\RangeVariableDeclaration $rangeVariableDeclaration + * @param boolean @rootAlias * * @return string */ From 6db2ff00f9420c8577583bd97fdea50392f2e8fc Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Mon, 29 Apr 2013 18:14:17 +0100 Subject: [PATCH 6/7] Typo --- lib/Doctrine/ORM/Query/SqlWalker.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 6ba02f06d35..6bf6fc7478e 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -807,7 +807,7 @@ public function walkFromClause($fromClause) * Walks down a RangeVariableDeclaration AST node, thereby generating the appropriate SQL. * * @param AST\RangeVariableDeclaration $rangeVariableDeclaration - * @param boolean @rootAlias + * @param boolean $rootAlias * * @return string */ From 0d4d08c5d4cdab0fbe7c640ef8457d86073b769f Mon Sep 17 00:00:00 2001 From: Tom Arnfeld Date: Tue, 28 May 2013 18:13:25 +0100 Subject: [PATCH 7/7] Coding standards --- lib/Doctrine/ORM/Query/SqlWalker.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php index 6bf6fc7478e..55c863adccf 100644 --- a/lib/Doctrine/ORM/Query/SqlWalker.php +++ b/lib/Doctrine/ORM/Query/SqlWalker.php @@ -1041,6 +1041,7 @@ public function walkJoin($join) . $condExprConjunction . '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')'; $discrSql = $this->_generateDiscriminatorColumnConditionSQL(array($joinAlias)); + if ( ! empty($discrSql)) { $sql .= ' AND (' . $discrSql . ')'; }