Project

General

Profile

Actions

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 almost 3 years ago.

Status:
Closed
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 5 (0 open5 closed)

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 disabledClosedMarkus 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
Actions #1

Updated by Manuel Selbach almost 5 years ago

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

Updated by Matthias Meusburger almost 5 years ago

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

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

Actions #4

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

Actions #5

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

Actions #6

Updated by Tobias D over 4 years ago

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

Updated by Tobias D over 4 years ago

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

Updated by Susanne Moog over 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
Actions #9

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

Actions #10

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

Actions #11

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

Actions #12

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

Actions #15

Updated by Markus Klein about 4 years ago

  • Status changed from Under Review to Closed

Solved with #87776

Actions #16

Updated by Markus Klein about 4 years ago

  • Status changed from Closed to Accepted

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

Actions #17

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

Actions #18

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

Actions #19

Updated by Markus Klein about 4 years ago

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

Actions #20

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

Actions #21

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

Actions #22

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

Actions #23

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

Actions #24

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

Actions #25

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

Actions #26

Updated by Gerrit Code Review almost 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/+/64937

Actions #27

Updated by Gerrit Code Review almost 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/+/64937

Actions #28

Updated by Gerrit Code Review almost 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/+/64937

Actions #29

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

Actions #30

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

Actions #31

Updated by Markus Klein about 3 years ago

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

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

Actions #33

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

Actions #34

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

Actions #35

Updated by Nikita Hovratov about 3 years ago

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

Updated by Markus Klein about 3 years ago

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

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

Actions #38

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

Actions #39

Updated by Christian Kuhn almost 3 years ago

  • Status changed from Under Review to Resolved

resolved in v10 and v11, will not be merged to v9 anymore.

Actions #40

Updated by Benni Mack almost 3 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF