Bug #88919

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 2 years ago. Updated 7 days ago.

Status:
Under Review
Priority:
Should have
Assignee:
-
Category:
Database API (Doctrine DBAL)
Target version:
-
Start date:
2019-08-05
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
9
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:

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.


Related issues

Related to TYPO3 Core - Feature #87776: Limit Restriction to table/s in QueryBuilderClosedManuel Selbach2019-02-24

Actions
Related to TYPO3 Core - Bug #86385: QueryBuilder restrictions break leftJoinClosed2018-09-26

Actions
Related to TYPO3 Core - Bug #94141: Hidden records are taken into account in m:n relations but instantiated as empty objects Closed2021-05-14

Actions
Related to TYPO3 Core - Bug #94119: Ghost record for field with mm relation when relation record is disabledResolvedMarkus Klein2021-05-12

Actions
Has duplicate TYPO3 Core - 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 showClosed2011-12-14

Actions
#1

Updated by Manuel Selbach almost 2 years ago

  • Related to Feature #87776: Limit Restriction to table/s in QueryBuilder added
#2

Updated by Matthias Meusburger almost 2 years ago

  • Related to Bug #86385: QueryBuilder restrictions break leftJoin added
#3

Updated by Gerrit Code Review over 1 year 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

#4

Updated by Gerrit Code Review over 1 year 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

#5

Updated by Gerrit Code Review over 1 year 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

#6

Updated by Tobias Doll over 1 year ago

  • Related to deleted (Bug #86385: QueryBuilder restrictions break leftJoin)
#7

Updated by Tobias Doll over 1 year ago

  • Related to Bug #86385: QueryBuilder restrictions break leftJoin added
#8

Updated by Susanne Moog about 1 year 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
#9

Updated by Gerrit Code Review about 1 year 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

#10

Updated by Gerrit Code Review about 1 year 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

#11

Updated by Manuel Selbach about 1 year 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

#12

Updated by MHO no-lastname-given about 1 year 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

#15

Updated by Markus Klein 12 months ago

  • Status changed from Under Review to Closed

Solved with #87776

#16

Updated by Markus Klein 12 months ago

  • Status changed from Closed to Accepted

Sorry, not solved. Was only a feature for v10 with #87776

#17

Updated by Gerrit Code Review 12 months 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

#18

Updated by Manuel Selbach 12 months 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?

#19

Updated by Markus Klein 12 months ago

I don't want workarounds if there is a proper solution. I'm expecting the database API to work as intended.

#20

Updated by Helmut Hummel 12 months 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

#21

Updated by Markus Klein 12 months 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!

#22

Updated by Stefan P 12 months 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?

#23

Updated by Markus Klein 12 months 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 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?

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!

#24

Updated by Stefan P 12 months 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.

#25

Updated by Stephan Großberndt 11 months 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.

#26

Updated by Gerrit Code Review 8 months 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

#27

Updated by Gerrit Code Review 8 months 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

#28

Updated by Gerrit Code Review 8 months 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

#29

Updated by Gerrit Code Review 7 months 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

#30

Updated by Gerrit Code Review 2 months 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

#31

Updated by Markus Klein 2 months ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#32

Updated by Gerrit Code Review 2 months 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

#33

Updated by Gerrit Code Review 2 months 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

#34

Updated by Gerrit Code Review 2 months 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

#35

Updated by Nikita Hovratov about 1 month ago

  • Related to Bug #94141: Hidden records are taken into account in m:n relations but instantiated as empty objects added
#36

Updated by Markus Klein 29 days ago

  • Related to Bug #94119: Ghost record for field with mm relation when relation record is disabled added
#37

Updated by Gerrit Code Review 7 days 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

Also available in: Atom PDF