Bug #88919
closed
QueryBuilder with DefaultRestrictionContainer effectivly transforms outer joins to inner joins because the restrictions are applied to all join tables unconditionally
Added by Stefan P almost 5 years ago. Updated over 2 years ago.
100%
Description
In an outer join (left or right join) one of the sides can be null.
But when using the QueryBuilder with it's DefaultRestrictionContainer it adds all conditions to all tables. Which is fine for inner joins but breaks outer joins (by efectivley kicking out rows that are fetched by the outer join, leaving one with teh same result as the inner join).
Consider this example (scenario: content inline in content):
SELECT c1.*
FROM tt_content AS c1
LEFT JOIN tt_content AS c2 on c1.parent_field = c2.uid
WHERE (c2 IS NULL OR c2.hidden = 0);
This would fetch all content rows but leave out the children whose parent is hidden.
Another example would be a join on pages to find pages by some parent page property.
Now you want this query with correctly respected TYPO3 fields (enable fields, deleted) and use the QueryBuilder. The QueryBuilder will add the DefaultRestriction container. So far so good. It will add the Restrictions to all join tables however, no matter if they're inner or outer joined. So it adds
c1.hidden = 0 AND c2.hidden = 0
which implcitly removes all join rows where the outer side is NULL. Leaving you implicitly with the inner join result.
As soon as an explicit outer join is requested the DefaultRestrictionContainer should only be applied to the from-table, not to the join-tables - or it should also add a OR join_table.uid IS NULL to the RestrictionContainer in case of outer joins.
For single-table or inner-join queries nothing has to change.
I found this on TYPO3 9, but assume this is true for 8 and 10 as well.
In case this is intended (can't believe this) this should at least be documented prominently - found this only by manually debugging the generated SQL statement to this that my left join condition was "short-circuited" by the DefaultRestrictionContainer.
Updated by Manuel Selbach almost 5 years ago
- Related to Feature #87776: Limit Restriction to table/s in QueryBuilder added
Updated by Matthias Meusburger over 4 years ago
- Related to Bug #86385: QueryBuilder restrictions break leftJoin added
Updated by Gerrit Code Review over 4 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61880
Updated by Gerrit Code Review over 4 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61880
Updated by Gerrit Code Review over 4 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61880
Updated by Tobias D about 4 years ago
- Related to deleted (Bug #86385: QueryBuilder restrictions break leftJoin)
Updated by Tobias D about 4 years ago
- Related to Bug #86385: QueryBuilder restrictions break leftJoin added
Updated by Susanne Moog about 4 years ago
- Has duplicate Bug #32539: Using a constraint or ordering on a property on the right side of a m:n relation causes records from the left side with no related elements (0 cardinality) to not show added
Updated by Gerrit Code Review about 4 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61880
Updated by Gerrit Code Review about 4 years ago
Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61880
Updated by Manuel Selbach about 4 years ago
The related task has been merged for version 10 and is available in 10.4.
Futhermore, there is a "workaround" that works already with version 9, that is described in a PR for the official documentation https://github.com/TYPO3-Documentation/TYPO3CMS-Reference-CoreApi/pull/840
Updated by MHO no-lastname-given almost 4 years ago
There is a new extension cc_dbal_leftjoin which fixes this behavior. Just install it and add true as additional parameter for leftJoin.
https://extensions.typo3.org/extension/cc_dbal_leftjoin/
//$queryBuilder->leftJoin($fromAlias, $join, $alias, $condition);
$queryBuilder->leftJoin($fromAlias, $join, $alias, $condition, true);
So SQL changes:
# SELECT * FROM table1 LEFT JOIN table2 ON table1.uid = table2.field WHERE table1.deleted = 0 AND table2.deleted = 0;
SELECT * FROM table1 LEFT JOIN table2 ON table1.uid = table2.field AND table2.deleted = 0 WHERE table1.deleted = 0
Updated by Markus Klein almost 4 years ago
- Status changed from Under Review to Closed
Solved with #87776
Updated by Markus Klein almost 4 years ago
- Status changed from Closed to Accepted
Sorry, not solved. Was only a feature for v10 with #87776
Updated by Gerrit Code Review almost 4 years ago
- Status changed from Accepted to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64937
Updated by Manuel Selbach almost 4 years ago
Markus Klein wrote:
Sorry, not solved. Was only a feature for v10 with #87776
Have you seen the "workaround" described in the documentation (still in review on Github https://github.com/TYPO3-Documentation/TYPO3CMS-Reference-CoreApi/pull/840).
Is there something missing?
Updated by Markus Klein almost 4 years ago
I don't want workarounds if there is a proper solution. I'm expecting the database API to work as intended.
Updated by Helmut Hummel almost 4 years ago
Markus Klein wrote:
I don't want workarounds if there is a proper solution. I'm expecting the database API to work as intended.
If #87776 solves the issue at hand, I don't understand why new API is introduced that does the same.
Besides that, I'm not sure whether the framework API can understand the intention of the user.
Instead the API should allow to build an SQL query that reflects the intention. As far as I can see,
this can be done with #87776
Updated by Markus Klein almost 4 years ago
My use case and why I think this must be fixed in Core (the default behaviour)
Table User -------------- | uid | name | -------------- | 1 | Max | | 2 | Sue | | 3 | Luke | -------------- Table Speaker ----------------------------------- | uid | userid | rating | deleted | ----------------------------------- | 10 | 1 | 5 | 0 | | 11 | 2 | 4 | 1 | -----------------------------------
$qb->select('u.*', 's.rating')
->from('user', 'u')
->leftJoin('u', speaker, 's', 's.userid = u.uid')
->orderBy('u.uid');
Expected result: ----------------------- | uid | name | rating | ----------------------- | 1 | Max | 5 | | 2 | Sue | null | | 3 | Luke | null | ----------------------- Actual result: ----------------------- | uid | name | rating | ----------------------- | 1 | Max | 5 | -----------------------
"Side note": The worst thing about the leftJoin() is that if the joined table has NO enableFields, it behaves like expected!
Updated by Stefan P almost 4 years ago
The worst thing is that this even needs discussion.
The API provides methods for outer joins. Commanding the API to do an outer join must do an outer join and no inner join. Currently it does an implicit inner join. Which is a bug. Period.
Any outer join, left or right, must either
1) not have any implicit conditions on the joined tables (only the ones explicitly requested by the calling code)
2) or having OR joined_table.field IS NULL added to all implicit field conditions
Clean code requires that the code does what it claims it's doing. If this can not be satisfied, drop the API at all to save everyone's time.
Sorry for the upcoming rant, but the core development more and more takes the direction into a state where I could post this rant under a lot of tickets, I have the impression.
The existance of a workaround does not free from being in the responsibility to provide a proper bugfix. Or else you could close down forge entirely, because there's a workaround for every bug ("just write your own patch").
As far as I'm concerned TYPO3 9 is still a LTS version and it's still in its support period and claims to be an enterprise CMS. And companies/people that are no core devs themselves still contribute with membership fees, by providing bug reports, spreading the word about TYPO3, helping others in Slack or even doing code submissions. So they can expect that bugs are treated as bugs and get fixed. Do all these words still mean anything these days?
Updated by Markus Klein almost 4 years ago
Stefan P wrote:
The worst thing is that this even needs discussion.
The API provides methods for outer joins. Commanding the API to do an outer join must do an outer join and no inner join. Currently it does an implicit inner join. Which is a bug. Period.
Any outer join, left or right, must either
1) not have any implicit conditions on the joined tables (only the ones explicitly requested by the calling code)
2) or havingOR joined_table.field IS NULLadded to all implicit field conditionsClean code requires that the code does what it claims it's doing. If this can not be satisfied, drop the API at all to save everyone's time.
Sorry for the upcoming rant, but the core development more and more takes the direction into a state where I could post this rant under a lot of tickets, I have the impression.
The existance of a workaround does not free from being in the responsibility to provide a proper bugfix. Or else you could close down forge entirely, because there's a workaround for every bug ("just write your own patch").
As far as I'm concerned TYPO3 9 is still a LTS version and it's still in its support period and claims to be an enterprise CMS. And companies/people that are no core devs themselves still contribute with membership fees, by providing bug reports, spreading the word about TYPO3, helping others in Slack or even doing code submissions. So they can expect that bugs are treated as bugs and get fixed. Do all these words still mean anything these days?
I am all with you!
Just two notes from my side:
1) Discussion is IMO always a good thing to prevent wrong decisions and to maybe find a better solution one didn't think of. But discussions must end at some point.
2) Core development - specifically bug fixing - can easily be described as "fear driven development". Fixing a bug, specifically one that was experienced by many, always causes problems for those having a workaround in place already, which the fix then breaks. I can tell you from my own years-long experience that no matter how unlikely it seems that some misbehaviour has caused a problem for someone, a few days after the release of the fix the first complaint rushes in on forge. That's kind of a natural law. So, we have been extremely careful in the past on what to fix, in which version and how.
Besides this carefulness, I strongly agree that this very problem MUST be fixed in all supported versions!
Updated by Stefan P almost 4 years ago
Fixing a bug, specifically one that was experienced by many, always causes problems for those having a workaround in place already, which the fix then breaks.
The way must always be: fix it to match the intention. Not fixing it with the reason "there are people smart enough to workaround" makes it just worse (especially for those not smart enough to workaround, but smart enough to trust the API). Any other way would introduce an ever growing "climate of carelessness" over time: if a discovered bug has no "consequences" (because one can "workaround" it) people fall into a state of not caring at all about feature being implemented correctly in the first place. This is not good for a development community (this even has nothing to do with paid or free or FOSS or closed source, but with a general attitude of how people think about code quality).
People who are able to workaround, are also able to drop their workaround soon enough.
Updated by Stephan Großberndt almost 4 years ago
Same issue here. leftJoin is simply broken when using it with tables having restrictions. The way EXT:cc_dbal_leftjoin solves it is the correct one in my opinion, those conditions must be added to the join condition.
Updated by Gerrit Code Review over 3 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64937
Updated by Gerrit Code Review over 3 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64937
Updated by Gerrit Code Review over 3 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64937
Updated by Gerrit Code Review over 3 years ago
Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/64937
Updated by Gerrit Code Review about 3 years ago
Patch set 1 for branch 10.4 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/68740
Updated by Markus Klein about 3 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 9bb6b3cb598087a4135b1e4ca0aaaf4c9faa38b2.
Updated by Gerrit Code Review about 3 years ago
- Status changed from Resolved to Under Review
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/68792
Updated by Gerrit Code Review about 3 years ago
Patch set 2 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/68792
Updated by Gerrit Code Review about 3 years ago
Patch set 3 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/68792
Updated by Nikita Hovratov almost 3 years ago
- Related to Bug #94141: Hidden records are taken into account in m:n relations but instantiated as empty objects added
Updated by Markus Klein almost 3 years ago
- Related to Bug #94119: Ghost record for field with mm relation when relation record is disabled added
Updated by Gerrit Code Review almost 3 years ago
Patch set 4 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/68792
Updated by Gerrit Code Review almost 3 years ago
Patch set 5 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/68792
Updated by Christian Kuhn over 2 years ago
- Status changed from Under Review to Resolved
resolved in v10 and v11, will not be merged to v9 anymore.
Updated by