Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a `WITH` clause #6475

stesie · 2017-05-27T17:32:10Z

See #6464 for a detailed example of the problem.

For a query like

$qb
            ->select('p', 'a')
            ->from(GH6464Post::class, 'p')
            ->innerJoin(GH6464Author::class, 'a', 'WITH', 'p.authorId = a.id')
            ->getQuery();

... where GH6464Author is a CTI entity, the sql generation currently generates two JOINs (correct) and attaches both join conditions to the latter JOIN. This works with MySQL and SQLite, however fails on PostgreSQL.

This patch

adds a functional test case that actually tries to run such a query
applies a fix to SqlWalker that makes it output a nested JOIN
adapts test cases in SelectSqlGenerationTest to reflect the change (and make those SQL statements valid -- on PostgreSQL)

stesie · 2017-05-27T17:53:33Z

Test fails seem to be related to changes to symfony/yaml and unrelated to this change (as tests are green on PHP 7.0 and on 7.1 with low dependencies)

Majkl578 · 2017-07-15T22:23:12Z

Can you please rebase this PR onto current master? It should fix build errors on Travis. 👍

stesie · 2017-07-16T15:12:25Z

@Majkl578 rebased, tests are green now

... but scrutinizer fails now. Yet the only issue now caused by my additions (https://scrutinizer-ci.com/g/doctrine/doctrine2/inspections/de206627-1966-4ae5-9710-e883754a9c58/issues/files/tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php) is a false positive as getSql returns a string (as opposed to reported array) ...

Ocramius · 2017-07-22T06:59:42Z

lib/Doctrine/ORM/Query/SqlWalker.php

     *
     * @return string
     */
-    public function walkRangeVariableDeclaration($rangeVariableDeclaration)
+    public function walkRangeVariableDeclaration($rangeVariableDeclaration, $buildNestedJoins = false)


The additional parameter is problematic. The decision is one in the SqlWalker anyway, so the correct solution (IMO) would be to move the method body to a private method, then keep the public API as-is and delegate the call on the public API to the private API, where the new parameter is defined.

Ocramius · 2017-07-22T07:00:10Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php

+    /** @Column(type="integer") */
+    public $authorId;
+
+    /** @Column(length=100) */


Drop any details that aren't strictly needed. If columns aren't affected, they shouldn't be in here

Ocramius · 2017-07-22T07:00:54Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php

+    public function testIssue()
+    {
+
+        $qb = $this->_em->createQueryBuilder();


Please either inline with the code below or use a DQL string instead of the builder

Ocramius · 2017-07-22T07:01:26Z

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -891,7 +892,11 @@ public function walkRangeVariableDeclaration($rangeVariableDeclaration)
        );

        if ($class->isInheritanceTypeJoined()) {


Can this block be moved to a private method too?

I doubt that aids readability since the decision is made by the caller (actually even that one's caller) and not the extracted function itself ... and it just modifies local state (i.e. $sql).

Ocramius · 2017-07-22T07:01:57Z

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -1137,11 +1142,11 @@ public function walkJoin($join)
                    $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))


The empty($conditions) is used twice - can you give it a name and assign it to a meaningful variable?

Ocramius · 2017-08-11T19:36:10Z

tests/Doctrine/Tests/ORM/Functional/Ticket/GH6464Test.php

+    public function testIssue()
+    {
+        $query = $this->_em->createQueryBuilder()
+            ->select('p', 'a')


This sort of select is invalid - removing a from the selection, as you should NEVER fetch-join results over an association that was filtered.

…-condition-2.5' into 2.5 Backport #6464 Backport #6475

…lable

Ocramius · 2017-08-11T19:47:20Z

Manually merged, thanks @stesie!

master: 6d428c9
2.5: d89d238

ivok · 2020-12-03T20:01:22Z

Why this issue is not fixed yet?

For our new project we use Doctrine 2 and MSSQL and with first join query we encountered the same problem.

When I dump the sql build from Doctrine it wraps the joined table name with parenthesis "JOIN (table tb) ON...." which gives error for MSSQL

In our case, we have Users table which should be joined to the table we are quering. The User entity is softedeletable, which adds condition in the join clause.

But becouse of this condition the SqlWalker wraps the table users with parenthesis here:1084

// If we have WITH condition, we need to build nested joins for target class table and cti joins if ($withCondition) { $sql .= '(' . $targetTableJoin['table'] . $ctiJoins . ') ON ' . $targetTableJoin['condition']; } else { $sql .= $targetTableJoin['table'] . ' ON ' . $targetTableJoin['condition'] . $ctiJoins; }

stesie added 2 commits July 16, 2017 15:50

doctrine#6464 add test

4ec20c7

generate nested join sql for CTIs, closes doctrine#6464

eaa3b97

stesie force-pushed the issue-6464 branch from 0200395 to eaa3b97 Compare July 16, 2017 14:44

Ocramius suggested changes Jul 22, 2017

View reviewed changes

Ocramius added the Bug label Jul 22, 2017

doctrine#6464 code review updates

86ef212

Ocramius added this to the 2.5.7 milestone Jul 31, 2017

Ocramius added the Requires merge/review label Jul 31, 2017

Ocramius changed the title ~~Fix invalid sql generated for CTI JOINs when INNER JOIN'ing~~ Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a WITH clause Aug 11, 2017

Ocramius reviewed Aug 11, 2017

View reviewed changes

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6464 #6475 cleaning up test - removed invalid fetch join, CS

2337b7a

Ocramius added a commit that referenced this pull request Aug 11, 2017

Merge branch 'fix/#6464-#6475-correct-SQL-generated-with-JTI-and-WITH…

d89d238

…-condition-2.5' into 2.5 Backport #6464 Backport #6475

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6464 #6475 cleaning up test - removed invalid fetch join, CS

91a5091

Ocramius added a commit that referenced this pull request Aug 11, 2017

#6464 #6475 using PHP 7.1 void and string return hints where avai…

9ad91dd

…lable

Ocramius closed this in 6d428c9 Aug 11, 2017

Ocramius self-assigned this Aug 11, 2017

Ocramius removed the Requires merge/review label Aug 11, 2017

Ocramius mentioned this pull request Aug 13, 2017

Fixed usage of undefined variable in empty() expression in SqlWalker #6621

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a `WITH` clause #6475

Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a `WITH` clause #6475

stesie commented May 27, 2017

stesie commented May 27, 2017

Majkl578 commented Jul 15, 2017

stesie commented Jul 16, 2017

Ocramius Jul 22, 2017

Ocramius Jul 22, 2017

Ocramius Jul 22, 2017

Ocramius Jul 22, 2017

stesie Jul 29, 2017

Ocramius Jul 22, 2017

Ocramius Aug 11, 2017

Ocramius commented Aug 11, 2017

ivok commented Dec 3, 2020 •

edited

		@@ -891,7 +892,11 @@ public function walkRangeVariableDeclaration($rangeVariableDeclaration)
		);

		if ($class->isInheritanceTypeJoined()) {

Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a WITH clause #6475

Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a WITH clause #6475

Conversation

stesie commented May 27, 2017

stesie commented May 27, 2017

Majkl578 commented Jul 15, 2017

stesie commented Jul 16, 2017

Ocramius Jul 22, 2017

Choose a reason for hiding this comment

Ocramius Jul 22, 2017

Choose a reason for hiding this comment

Ocramius Jul 22, 2017

Choose a reason for hiding this comment

Ocramius Jul 22, 2017

Choose a reason for hiding this comment

stesie Jul 29, 2017

Choose a reason for hiding this comment

Ocramius Jul 22, 2017

Choose a reason for hiding this comment

Ocramius Aug 11, 2017

Choose a reason for hiding this comment

Ocramius commented Aug 11, 2017

ivok commented Dec 3, 2020 • edited

Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a `WITH` clause #6475

Fix invalid sql generated for CTI JOINs when INNER JOIN'ing with a `WITH` clause #6475

ivok commented Dec 3, 2020 •

edited