Fixed broken search in PostgreSQL

[alshenetsky]

See #6242
A recent change (which I shamefully commited) broke search for the PostgreSQL platform. This PR should fix the issue.

[janklan]

No need to lynch yourself :)

Do you think this is a real fix, though? If I understand things correctly, the initial problem was an incorrect query with an empty WHERE statement, because there are no any $queryTermConditions.

Adding a bogus condition just to make the $queryTermConditions to contain something doesn't seem right. If the WHERE can't be removed from the query, can't we instead add 1 = 1 when $queryTermConditions is empty?

[alshenetsky]

Adding a bogus condition just to make the $queryTermConditions to contain something doesn't seem right. If the WHERE can't be removed from the query, can't we instead add 1 = 1 when $queryTermConditions is empty?

That would be counter-intuitive. Imagine: you type text into the search box and see ALL entries as if they all contained that text (in fact none of them).

You could add an infeasible condition, like WHERE 1 = 2 😄 But that wouldn't be any better: some developer would go crazy once he uses the profiler and sees the queries.

[janklan]

Right, so the problem you have is when someone searches for a string in a controller that only deals with numbers. Then the array of conditions is empty, and the result is intended to be always empty as well as there can't be any matches.

Wouldn't in this case make sense to just return an empty iterator instead of running a SQL query that we know will yield no results?

---

Looking at the code, it's probably not as easy to do that. With that in mind, I think this would be a viable solution developers would not freak about.

Right under the massive if/else statement:

if ($queryTermConditions->count() > 0) {
    if (SearchMode::ALL_TERMS === $searchDto->getSearchMode()) {
        $queryBuilder->andWhere($queryTermConditions);
    } else {
        $queryBuilder->orWhere($queryTermConditions);
    }
} else {
    /**
     * When the above if/else block doesn't add any conditions to $queryTermsConditions, we know the search
     * will never return any results. Passing an empty Expr object to Doctrine's QueryBuilder produces an invalid
     * SQL query, causing the search function to fail.
     *
     * Adding the `1 = 'look-for-this-string-in-the-source-code-for-more-info'` condition creates a valid SQL
     * query that returns no results.
     *
     * @see https://github.com/EasyCorp/EasyAdminBundle/pull/6262
     */
    $queryBuilder->andWhere("1 = 'look-for-this-string-in-the-source-code-for-more-info'");
}

At least you can easily find the source of the query by searching for "look-for-this-string-in-the-source-code-for-more-info" in the code :D

[alshenetsky]

In general, you're right. But I'm pretty sure it's impossible without extensive refactoring, which will be backwards-incompatible. This QueryBuilder already exists in the user-space, and all we can/should do is refine its conditions.

---

Just saw the second part of your comment. I hope you're not serious :D

[alshenetsky]

The aesthetics of this if condition is a separate issue. However, I believe that queries should be generated in a predictable way.

[janklan]

I was dead serious.

Another thought: } elseif ($propertyConfig['is_text'] || ($propertyConfig['is_integer'] && !$isPostgreSql)) {- doesn't this keep the buggy behaviour in place for PostgreSQL?

[Seb33300]

I would suggest similar solution as suggested by @janklan but without imbricated conditions:

$queryTermConditions = new Orx();
foreach ($searchablePropertiesConfig as $propertyConfig) {
    // All conditions...
}

/** Add this */
// No searchable property => return empty result
if ($queryTermConditions->count() === 0) {
    $queryTermConditions->add('1 = 2');
}
/** End add this */

if (SearchMode::ALL_TERMS === $searchDto->getSearchMode()) {
	$queryBuilder->andWhere($queryTermConditions);
} else {
	$queryBuilder->orWhere($queryTermConditions);
}

[alshenetsky]

So be it. Please review :)

[alshenetsky]

Oh, no... I think someone already fixed this, but no one saw his PR
https://github.com/EasyCorp/EasyAdminBundle/pull/6227/files

I should close this.

[Seb33300]

Right, but you code still need to be reverted