[DDC-2235] Fix for using a LEFT JOIN onto an entity with single table inheritance #656

Closed
wants to merge 7 commits into
from

Projects

None yet

4 participants

@tarnfeld

Possible fix for the bug DDC-2235. I'd love to hear some opinions on whether this is the right way to go about this issue. I'm not particularly familiar with the internals of doctrine so there may be a better solution.


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.

I've added some regression tests to ensure this bug never creeps back in. They fail on master (highlighting the bug) and pass after these commits have been applied. I've also included a couple of other queries as tests to be sure only this one case has been affected.

tarnfeld added some commits Apr 28, 2013
@tarnfeld tarnfeld 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`.
6877610
@tarnfeld tarnfeld Adjusted whitespace and added unit tests 615c25a
@tarnfeld tarnfeld Spelling 080c3e3
@Ocramius Ocramius and 1 other commented on an outdated diff Apr 29, 2013
lib/Doctrine/ORM/Query/SqlWalker.php
@@ -1026,6 +1040,10 @@ public function walkJoin($join)
? ' LEFT JOIN '
: ' INNER JOIN ';
+ if ($joinType == AST\Join::JOIN_TYPE_LEFT) {
@Ocramius
Ocramius Apr 29, 2013

use strict comparison and yoda conditionals

@Ocramius Ocramius and 1 other commented on an outdated diff Apr 29, 2013
.../Doctrine/Tests/ORM/Query/SelectSqlGenerationTest.php
@@ -1459,6 +1459,30 @@ public function testInheritanceTypeSingleTableInLeafClassWithEnabledForcePartial
}
/**
+ * @group DDC-2235
+ */
+ public function testInheritanceTypeSingleTableInLeftJoinIncludesOrIsNull()
+ {
+ // Regression test for best-case
+ $this->assertSqlGeneration(
@Ocramius
Ocramius Apr 29, 2013

what happens outside the context of arbitrary joins?

such as select f from foo f join f.bar b with f.baz = ?

@tarnfeld
tarnfeld Apr 29, 2013

Good point, those shouldn't be affected at all - however i'll add some tests just to be sure!

@Ocramius
Ocramius Apr 29, 2013

Can you add them to the tests then please? :)

@Ocramius
Doctrine member

By the way, this doesn't seem to solve the problem, rather than patching up the symptoms... Shouldn't all the IN be applied in the ON clause instead?

@tarnfeld

I'd much prefer that, but not quite sure where the other parts of the ON clause are generated, and whether its best to just arbitrarily put an AND IN() on the end of that string, or if there is a better way to go about it.

If you could point me in the right direction I'll gladly implement it. :)

@tarnfeld

Got it - will push a new commit with that implemented shortly.

@tarnfeld tarnfeld commented on an outdated diff Apr 29, 2013
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) . ')';
@tarnfeld
tarnfeld Apr 29, 2013

Gah, will fix this whitespace.

@tarnfeld

@Ocramius Any news on this being reviewed again / merged into master?

@eugene1g

The patch worked for my use-case (similar situation, left-joining a model with a discriminator column)

@Ocramius
Doctrine member

@tarnfeld merges of such major changes are not up to me - I generally agree with what you have implemented here though

@Ocramius Ocramius commented on an outdated diff May 23, 2013
lib/Doctrine/ORM/Query/SqlWalker.php
. $condExprConjunction . '(' . $this->walkConditionalExpression($join->conditionalExpression) . ')';
+
+ $discrSql = $this->_generateDiscriminatorColumnConditionSQL(array($joinAlias));
+ if ( ! empty($discrSql)) {
@Ocramius
Ocramius May 23, 2013

Missing newline before this line

@tarnfeld

@Ocramius Thanks for the heads up, i've pushed a fix for the new line.

@guilhermeblanco
Doctrine member

I'm working on this issue.

@guilhermeblanco guilhermeblanco referenced this pull request Aug 16, 2013
Merged

Fixed DDC-2235. #758

@guilhermeblanco
Doctrine member

I replaced this PR with a more in depth solution as of #758
I reused the tests available here but also fixed the filter support which was missing on this patch.

@tarnfeld

Nice one, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment