Project

General

Profile

Actions

Bug #93337

closed

Querybuilder builds conditions for foreign_table_field and/or foreign_match_fields without "OR ...uid IS NULL" for joined Table

Added by Harald Witt over 3 years ago. Updated 11 months ago.

Status:
Resolved
Priority:
Must have
Assignee:
-
Category:
Extbase
Target version:
-
Start date:
2021-01-21
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
12
PHP Version:
7.4
Tags:
Complexity:
hard
Is Regression:
Sprint Focus:

Description

BTW: Unfortunately I cannot change the Priority to must have .
QueryBuilder creates wrong query when using foreign_table_field and/or foreign_match_fields in a 1:n inline relation.
The conditions for foreign_table_field and/or foreign_match_fields for the joined table are build, but the "OR ...uid IS NULL" is missing.
So all records of the main-table which do not have any child record in the joined table are excluded from the query result.

Here's my detailled demonstration. I'll use my own table names here ...
ext_tables.sql:

#
# Table structure for table 'tx_intranet_domain_model_childtable'
#
CREATE TABLE tx_intranet_domain_model_childtable (
    uid int(11) NOT NULL auto_increment,
    pid int(11) DEFAULT '0' NOT NULL,
    tstamp int(11) DEFAULT '0' NOT NULL,
    crdate int(11) DEFAULT '0' NOT NULL,
    cruser_id int(11) DEFAULT '0' NOT NULL,
    parent_table varchar(255) DEFAULT '' NOT NULL,
    parent_uid int(11) DEFAULT '0' NOT NULL,
    type smallint(5) unsigned DEFAULT '0' NOT NULL,
    child_content varchar(255) DEFAULT '' NOT NULL,
    PRIMARY KEY (uid),
    KEY `parent` (`parent_table`,`parent_uid`)
);

#
# Table structure for table 'tx_intranet_domain_model_parenttable'
#
CREATE TABLE tx_intranet_domain_model_parenttable (
    uid int(11) NOT NULL auto_increment,
    pid int(11) DEFAULT '0' NOT NULL,
    tstamp int(11) DEFAULT '0' NOT NULL,
    crdate int(11) DEFAULT '0' NOT NULL,
    cruser_id int(11) DEFAULT '0' NOT NULL,
    children int(11) DEFAULT '0' NOT NULL,
    parent_content varchar(255) DEFAULT '' NOT NULL,
    PRIMARY KEY (uid)
);

The important TCA-part of the column children of the table tx_intranet_domain_model_parenttable
'children' => [
    'exclude' => false,
    'label' => 'Child records',
    'config' => [
        'type' => 'inline',
        'foreign_table' => 'tx_intranet_domain_model_childtable',
        'foreign_field' => 'parent_uid',
        'foreign_table_field' => 'parent_table',
        'foreign_match_fields' => [ 'type' => 3, ],
        'overrideChildTca' => [
            'columns' => [ 'type' => [ 'config' => [ 'default' => 3, ], ], ],
            'types' => [ '3' =>     [ 'showitem' => '--div--;General,parent_content,child_content' ], ], // only an example here
        ],
        'appearance' => [
            'newRecordLinkTitle' => 'New child record',
        ]
    ],
],

Now we have to create the domain models, a Controller with an action. It's not shown here.

Please create a parent record in the backend with "who is who" in the field parent_content.
You MAY NOT create child-records for this demonstration purpose.

Now I'd like to search for the word "who" which could be in both, the field parent_content and/or the field child_content.
So I build this query in a method of my repositoty:
class ParenttableRepository extends \TYPO3\CMS\Extbase\Persistence\Repository
{
    /**
    * @var \TYPO3\CMS\Extbase\Persistence\Generic\Typo3QuerySettings
    */
    protected $defaultQuerySettings;

    public function initializeObject()
    {
        $this->defaultQuerySettings = $this->objectManager->get(\TYPO3\CMS\Extbase\Persistence\Generic\Typo3QuerySettings::class);
        $this->defaultQuerySettings->setRespectStoragePage(false);
    }

    public function bugDemo() {
        $query = $this->createQuery();
        $query->matching(
                $query->logicalOr(
                    $query->like('parent_content', '%who%'),
                    $query->like('children.child_content', '%who%')
                )
            )
            ->execute();
        // BEGIN: Let's debug
        \TYPO3\CMS\Core\Utility\DebugUtility::debug($result);
        $queryParser = $this->objectManager->get(\TYPO3\CMS\Extbase\Persistence\Generic\Storage\Typo3DbQueryParser::class);
        echo $queryParser->convertQueryToDoctrineQueryBuilder($query)->getSQL();
        // END: Let's debug
        return $result;
    }
}

The result is empty and we got the following SQL-Query:
SELECT `tx_intranet_domain_model_parenttable`.* 
FROM `tx_intranet_domain_model_parenttable` `tx_intranet_domain_model_parenttable` 
LEFT JOIN `tx_intranet_domain_model_childtable` `tx_intranet_domain_model_childtable` 
ON `tx_intranet_domain_model_parenttable`.`uid` = `tx_intranet_domain_model_childtable`.`parent_uid` 
WHERE 
(
    (`tx_intranet_domain_model_childtable`.`type` = :dcValue2) 
    AND 
    (`tx_intranet_domain_model_childtable`.`parent_table` = :dcValue3)
) 
AND 
(
    (`tx_intranet_domain_model_parenttable`.`parent_content` LIKE :dcValue1) 
    OR 
    (`tx_intranet_domain_model_childtable`.`child_content` LIKE :dcValue4)
) 

As you can see, the existence of a child record is a must have for this conditions in a LEFt JOIN.
The correct SQL-Statement should be:
SELECT `tx_intranet_domain_model_parenttable`.* 
FROM `tx_intranet_domain_model_parenttable` `tx_intranet_domain_model_parenttable` 
LEFT JOIN `tx_intranet_domain_model_childtable` `tx_intranet_domain_model_childtable` 
ON `tx_intranet_domain_model_parenttable`.`uid` = `tx_intranet_domain_model_childtable`.`parent_uid` 
WHERE 
(
    (
        (`tx_intranet_domain_model_childtable`.`type` = :dcValue2) 
        AND 
        (`tx_intranet_domain_model_childtable`.`parent_table` = :dcValue3)
    )
    OR
    (`tx_intranet_domain_model_childtable`.`uid` IS NULL)
) 
AND 
(
    (`tx_intranet_domain_model_parenttable`.`parent_content` LIKE :dcValue1) 
    OR 
    (`tx_intranet_domain_model_childtable`.`child_content` LIKE :dcValue4)
) 

I see only two workarounds:
  1. I could built the complete statement manually. But that would be very complicated because there are lots of other fields in my case.
  2. I don't use parent_table and type anymore. In that case I'd have to maintain 8 tables instead of one.

So I hope you'll accept this as a bug and will fix it ASAP.

Greetings
Harald

Actions #1

Updated by Harald Witt over 3 years ago

  • Description updated (diff)
Actions #2

Updated by Harald Witt over 3 years ago

  • Description updated (diff)
Actions #3

Updated by Harald Witt over 3 years ago

  • Subject changed from Querybuilder builds conditions for foreign_table_field and/or foreign_match_fields with no propper nesting (always outside all other nested conditions) to Querybuilder builds conditions for foreign_table_field and/or foreign_match_fields without "OR ...uid IS NULL" for joined Table
  • Description updated (diff)
Actions #4

Updated by Harald Witt over 3 years ago

The same issue already exists in Typo3 9.5.
Seems to be very seldom to find things in parent- OR child-records while parents without children exists :-(

Actions #5

Updated by Harald Witt over 3 years ago

As already mentioned this was hard, but I got it.
The class \TYPO3\CMS\Extbase\Persistence\Generic\Storage\Typo3DbQueryParser contains a class called getAdditionalMatchFieldsStatement() which produces the slightly buggy statement. Here's my purposed bugfix:
All interesting lines are marked as /*NEW*/ or /*OLD*/ at the end.
$isNullExpr is used to find at least one field which can be compared with IS NULL.

protected function getAdditionalMatchFieldsStatement($exprBuilder, $columnMap, $childTableAlias, $parentTable = null)
{
    $additionalWhereForMatchFields = [];
    $relationTableMatchFields = $columnMap->getRelationTableMatchFields();
    if (is_array($relationTableMatchFields) && !empty($relationTableMatchFields)) {
        foreach ($relationTableMatchFields as $fieldName => $value) {
            $additionalWhereForMatchFields[] = $exprBuilder->eq($childTableAlias . '.' . $fieldName, $this->queryBuilder->createNamedParameter($value));
            if (!$isNullExpr) { /*NEW*/
                $isNullExpr = $exprBuilder->isNull($childTableAlias . '.' . $fieldName); /*NEW*/
            } /*NEW*/
        }
    }

    if (isset($parentTable)) {
        $parentTableFieldName = $columnMap->getParentTableFieldName();
        if (!empty($parentTableFieldName)) {
            $additionalWhereForMatchFields[] = $exprBuilder->eq($childTableAlias . '.' . $parentTableFieldName, $this->queryBuilder->createNamedParameter($p
            if (!$isNullExpr) { /*NEW*/
                $isNullExpr = $exprBuilder->isNull($childTableAlias . '.' . $parentTableFieldName); /*NEW*/
            } /*NEW*/
        }
    }

    if (!empty($additionalWhereForMatchFields)) {
        return $exprBuilder->orX($isNullExpr, $exprBuilder->andX(...$additionalWhereForMatchFields)); /*NEW*/
//        return $exprBuilder->andX(...$additionalWhereForMatchFields); /*OLD*/
    }
    return '';
}

There are only exact 4 calls to this protected function. Tree of them are in function addUnionStatement(...) for each relation type: RELATION_HAS_ONE, RELATION_HAS_MANY and RELATION_HAS_AND_BELONGS_TO_MANY.
The fourth call is in function parseComparison(...).
So it should not be a big task to review this bugfix purpose.

Harald

Actions #6

Updated by Markus Klein over 1 year ago

  • Category changed from Database API (Doctrine DBAL) to Extbase

Hi Harald,

thanks for this report. I stumbled over this by accident. I think this is still valid.

Would you mind pushing your proposed solution to our review system?
You can find the full blown guide here: https://docs.typo3.org/m/typo3/guide-contributionworkflow/main/en-us/

Actions #7

Updated by Harald Witt about 1 year ago

Hi Markus,
Thank you for taking the time to read this long contibution.
And yes you are right. It ist still valid. The method getAdditionalMatchFieldsStatement() is unchaged for years, also in Version 12.
So I'll follow the contribution workflow and submit a patch soon.

But before I wrote this answer, I was thinking about other usecases.

My original usecase was, that the two conditions (parent_content and child_content) are ORed (disjunction).
This unfortunally evaluated to false whenever a parent record had no child and is solved with my patch.

Another usecase would be, that the two conditions are ANDed (conjunction). What will happen?
Nothing changes with my patch! Before my patch the parent records were excluded because of missing childs.
After my patch they will also be excluded, but now because child_content is NULL and the condition will therefore evaluate to false.

But how does it work, if no additional match fields are present in TCA?
Nothing will happen. The method getAdditionalMatchFieldsStatement() will return an empty string as it already does in the past. The Join will be done as usual and all parent records with no child will appear with NULL in the child record fields.

So I think there's nothing to worry about.

Greetings
Harald

Actions #8

Updated by Gerrit Code Review about 1 year ago

  • Status changed from New to Under Review

Patch set 1 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #9

Updated by Gerrit Code Review about 1 year ago

Patch set 2 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #10

Updated by Gerrit Code Review about 1 year ago

Patch set 3 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #11

Updated by Gerrit Code Review about 1 year ago

Patch set 4 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #12

Updated by Gerrit Code Review about 1 year ago

Patch set 5 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #13

Updated by Gerrit Code Review about 1 year ago

Patch set 6 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #14

Updated by Gerrit Code Review about 1 year ago

Patch set 7 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #15

Updated by Harald Witt about 1 year ago

Hi, meanwhile I think there is a very much smarter way to solve the problem.
Instead of fetching some field inside getAdditionalMatchFieldsStatement() and append a "OR field IS NULL" to the WHERE clause we could do the following:
Simply add the additional match fileds as condition to the ON clause of the JOIN. This way all parent records will be joined. If no child record are present, the result will contain a bunch of child records containing a NULL value. But these records are subject to the rest of the WHERE clause and are not excluded in general before.

SELECT parent.* FROM parent LEFT JOIN child
ON (parent.uid=child.parent AND child.parent_table='parent' AND child.type='3')
WHERE (parent.title LIKE '%w/o%' OR child.title LIKE '%w/o%');

Actions #16

Updated by Harald Witt about 1 year ago

Hi again.
As I realized the last purpose to solve the problem is the only way to do this. The first purpose to add a "OR field IS NULL" to the WHERE clause would lead to problems. And this is the reason:

Have a look at the method parseComparison() inside Typo3DbQueryParser. There you can see a special case:
If a (1st) field is subject of a comarison of type "CONTAINS" and (2nd) the relation of that field is of type "RELATION_HAS_AND_BELONGS_TO_MANY" a Sub-SELECT is made. This Sub-Select also uses getAdditionalMatchFieldsStatement() and needs the statement as it is, i. e. without my first purpose "OR field IS NULL". If I would make it that way, we would have another problem ... the following:

What if the mentioned field has (3rd) not a "NOT NULL" in ext_tables.sql and (4th) the selected field for "OR field IS NULL" really contains the value NULL and (5th) the WHERE clause would normally lead to a FALSE? In ths case my "OR field IS NULL" could lead to a TRUE, what's obviousely wrong.

So, lets use sure way, i. e. my second purpose.

Actions #17

Updated by Harald Witt about 1 year ago

  • Priority changed from Should have to Must have
  • TYPO3 Version changed from 10 to 12
  • PHP Version changed from 7.3 to 7.4

Hi again.
As I saw for the relation type "RELATION_HAS_AND_BELONGS_TO_MANY" it was already done in the right way I suggested.
So I realized the same only with the relation types "RELATION_HAS_ONE" AND "RELATION_HAS_MANY".
Here an example for the relation type "RELATION_HAS_MANY":

Old code snippet:

        } elseif ($columnMap->getTypeOfRelation() === ColumnMap::RELATION_HAS_MANY) {
            // @todo: no tests for this part yet
            if (isset($parentKeyFieldName)) {
                $joinConditionExpression = $this->queryBuilder->expr()->eq(
                    $tableName . '.uid',
                    $this->queryBuilder->quoteIdentifier($childTableAlias . '.' . $parentKeyFieldName)
                );
            } else {
                $joinConditionExpression = $this->queryBuilder->expr()->inSet(
                    $tableName . '.' . $columnName,
                    $this->queryBuilder->quoteIdentifier($childTableAlias . '.uid'),
                    true
                );
            }
            $this->queryBuilder->leftJoin($tableName, $childTableName, $childTableAlias, $joinConditionExpression);
            $this->unionTableAliasCache[] = $childTableAlias;
            $this->queryBuilder->andWhere(
                $this->getAdditionalMatchFieldsStatement($this->queryBuilder->expr(), $columnMap, $childTableAlias, $realTableName)
            );
            $this->suggestDistinctQuery = true;

This was resulting in the following SQL statement:

SELECT `tx_relations_domain_model_relation`.* FROM `tx_relations_domain_model_relation` `tx_relations_domain_model_relation` 
LEFT JOIN `tx_relations_domain_model_rel_one_many` `tx_relations_domain_model_rel_one_many` 
ON `tx_relations_domain_model_relation`.`uid` = `tx_relations_domain_model_rel_one_many`.`parent_uid` 

WHERE (((`tx_relations_domain_model_rel_one_many`.`record_type` = :dcValue1) 
AND (`tx_relations_domain_model_rel_one_many`.`parent_table` = :dcValue2))) 
AND (((`tx_relations_domain_model_rel_one_many`.`title` LIKE :dcValue3) OR (`tx_relations_domain_model_relation`.`uid` = :dcValue4))) 
ORDER BY `tx_relations_domain_model_relation`.`uid` ASC

My new code is now:

        } elseif ($columnMap->getTypeOfRelation() === Relation::HAS_MANY) {
            if (isset($parentKeyFieldName)) {
                $basicJoinCondition = $this->queryBuilder->expr()->eq(
                    $tableName . '.uid',
                    $this->queryBuilder->quoteIdentifier($childTableAlias . '.' . $parentKeyFieldName)
                );
            } else {
                $basicJoinCondition = $this->queryBuilder->expr()->inSet(
                    $tableName . '.' . $columnName,
                    $this->queryBuilder->quoteIdentifier($childTableAlias . '.uid'),
                    true
                );
            }
            $joinConditionExpression = $this->queryBuilder->expr()->and(
                $basicJoinCondition,
                $this->getAdditionalMatchFieldsStatement($this->queryBuilder->expr(), $columnMap, $childTableAlias, $realTableName)
            );
            $this->queryBuilder->leftJoin($tableName, $childTableName, $childTableAlias, $joinConditionExpression);
            $this->unionTableAliasCache[] = $childTableAlias;
            $this->suggestDistinctQuery = true;

And now the SQL statement looks like this:

SELECT `tx_relations_domain_model_relation`.* FROM `tx_relations_domain_model_relation` `tx_relations_domain_model_relation` 
LEFT JOIN `tx_relations_domain_model_rel_one_many` `tx_relations_domain_model_rel_one_many` 
ON ((`tx_relations_domain_model_relation`.`uid` = `tx_relations_domain_model_rel_one_many`.`parent_uid`) 
AND (((`tx_relations_domain_model_rel_one_many`.`record_type` = :dcValue2) 
AND (`tx_relations_domain_model_rel_one_many`.`parent_table` = :dcValue3)))) 

WHERE ((`tx_relations_domain_model_relation`.`title` LIKE :dcValue1) OR (`tx_relations_domain_model_rel_one_many`.`title` LIKE :dcValue4)) 
ORDER BY `tx_relations_domain_model_relation`.`uid` ASC

Perfect! This is exactly what I wanted. And the Bug is gone now. Matching parent records with no childs are now found as expected.
I also removed the annotation "// @todo: no tests for this part yet" because Markus Klein wrote a test for this. Many thanks to Markus and Benni for the huge support and their patience.
Last but not least I'll now create a patch (main branch) for Gerrit. Hope that will take not too much brainwax :-)
After that this should be backported to at least Typo3 11.5, maybe also for Typo3 10.4. The bug is really very old.

Greetings from Harald

Actions #18

Updated by Gerrit Code Review about 1 year ago

Patch set 8 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #19

Updated by Gerrit Code Review about 1 year ago

Patch set 9 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #20

Updated by Gerrit Code Review about 1 year ago

Patch set 10 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #21

Updated by Gerrit Code Review about 1 year ago

Patch set 11 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #22

Updated by Gerrit Code Review about 1 year ago

Patch set 12 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #23

Updated by Gerrit Code Review about 1 year ago

Patch set 13 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #24

Updated by Gerrit Code Review about 1 year ago

Patch set 14 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #25

Updated by Gerrit Code Review about 1 year ago

Patch set 15 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #26

Updated by Gerrit Code Review about 1 year ago

Patch set 16 for branch main of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/77899

Actions #27

Updated by Gerrit Code Review 12 months ago

Patch set 1 for branch 12.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/+/79118

Actions #28

Updated by Harald Witt 12 months ago

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

Updated by Gerrit Code Review 12 months ago

  • Status changed from Resolved to Under Review

Patch set 1 for branch 11.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/+/79145

Actions #30

Updated by Gerrit Code Review 12 months ago

Patch set 2 for branch 11.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/+/79145

Actions #31

Updated by Gerrit Code Review 12 months ago

Patch set 3 for branch 11.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/+/79145

Actions #32

Updated by Harald Witt 11 months ago

  • Status changed from Under Review to Resolved
Actions

Also available in: Atom PDF