Bug #93337: Querybuilder builds conditions for foreign_table_field and/or foreign_match_fields without "OR ...uid IS NULL" for joined Table - TYPO3 Core - TYPO3 Forge

Actions

Copy link

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 almost 4 years ago. Updated 5 months ago.

Status:

Closed

Priority:

Must have

Assignee:

Category:

Extbase

Target version:

Start date:

2021-01-21

Due date:

% Done:

100%

Estimated time:

TYPO3 Version:

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:

I could built the complete statement manually. But that would be very complicated because there are lots of other fields in my case.
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

Copy link

Updated by Harald Witt almost 4 years ago

Description updated (diff)

Actions

Copy link

Updated by Harald Witt almost 4 years ago

Description updated (diff)

Actions

Copy link

Updated by Harald Witt almost 4 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

Copy link

Updated by Harald Witt almost 4 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

Copy link

Updated by Harald Witt almost 4 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

Copy link

Updated by Markus Klein almost 2 years 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

Copy link

Updated by Harald Witt almost 2 years 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

Copy link

Updated by Gerrit Code Review almost 2 years ago

Status changed from New to Under Review

Actions

Copy link

#15

Updated by Harald Witt over 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

Copy link

#16

Updated by Harald Witt over 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

Copy link

#17

Updated by Harald Witt over 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.

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

Copy link

#28

Updated by Harald Witt over 1 year ago

Status changed from Under Review to Resolved
% Done changed from 0 to 100

Applied in changeset 2ce9df23e76f7315a334ffa381e7aa0e6e0e037a.

Actions

Copy link

#29

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

Copy link

#30

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

Copy link

#31

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

Copy link

#32

Updated by Harald Witt over 1 year ago

Status changed from Under Review to Resolved

Applied in changeset 3037bc3748f367dfa5daa9b9b77e76f78b5c6099.

Actions

Copy link

#33

Updated by Benni Mack 5 months ago

Status changed from Resolved to Closed

Actions

Copy link

Also available in: Atom PDF

Project

General

Profile

TYPO3 Core

Custom queries

Watchers (2)

Bug #93337

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

Updated by Harald Witt almost 4 years ago

Updated by Harald Witt almost 4 years ago

Updated by Harald Witt almost 4 years ago

Updated by Harald Witt almost 4 years ago

Updated by Harald Witt almost 4 years ago

Updated by Markus Klein almost 2 years ago

Updated by Harald Witt almost 2 years ago

Updated by Gerrit Code Review almost 2 years ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Harald Witt over 1 year ago

Updated by Harald Witt over 1 year ago

Updated by Harald Witt over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Harald Witt over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Gerrit Code Review over 1 year ago

Updated by Harald Witt over 1 year ago

Updated by Benni Mack 5 months ago