Fix EZP-27416: Error 500 on search when trying to search space or single quote #2029

adamwojs · 2017-06-20T20:13:20Z

JIRA: https://jira.ez.no/browse/EZP-27416

Description

This PR provide patch to incorrect generated SQL in Legacy Search Engine when user is trying to search a query that contains only a word separators characters e.g single quote character.

Steps to reproduce

Setup Legacy Search Engine
In Platform UI go to Content > Content structure > Search
Try to search: ' (single quote)
Backend will return "Internal server error" with the following description:

The expression ' OR ' expected at least 1 argument but none provided.

TODO

Patch
Tests

andrerom · 2017-06-20T20:28:31Z

Will you also reuse $wordIdSubquery below? ;)

adamwojs · 2017-06-20T20:33:39Z

@andrerom Fixed :-)

adamwojs · 2017-06-20T20:51:46Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/FullText.php

+
+        $wordIdSubquery = $this->getWordIdSubquery($subSelect, $criterion->value);
+        if ($wordIdSubquery === null) {
+            return $query->expr->neq(1, 1);


This is still not compatible with declared return type, and I think it's require few words of comment .

This is still not compatible with declared return type, and I think it's require few words of comment .

I looked over returned values in other criterions and I actually think that @return in getWordIdSubquery PhpDoc is just wrong - it should be @return string because all methods of the eZ\Publish\Core\Persistence\Database\Expression interface return string as well.

adamwojs · 2017-06-20T20:52:40Z

Updated with PostgreSQL compatible version.

alongosz · 2017-06-21T13:43:00Z

I gave it a little bit of thought and I think we should consider two separate cases here, applied on SQL Search layer, so not affecting other search engines:

In eZ\Publish\Core\Search\Legacy\Content\Handler::findContent at the beginning we should check empty value:

if (trim($query->query->value) === '') {
    return new SearchResult(['time' => 0, 'totalCount' => 0]);
}

because search for an empty value should always return 0 count in 0 time (other SearchResult fields already have proper default values). Note on 0 time - it will be accurate with respect to current code if the above block gets placed as described, at the beginning of findContent

When we get empty-ish value, e.g. containing quotes, we actually should throw API InvalidArgumentException (instead of returning null in getWordIdSubquery). Meaning: when tokenizer returns empty result - something went wrong, because we already checked one of the expected states in point 1. ^
This exception is already hinted on findContent PhpDoc and is AFAIR close to what @adamwojs suggested when we initially discussed this :-)

WDYT @andrerom ?

alongosz

Some minor comments below:

alongosz · 2017-06-21T13:45:05Z

eZ/Publish/API/Repository/Tests/SearchServiceTest.php

+            array('searchQuery' => ''),
+            array('searchQuery' => '\''),
+            array('searchQuery' => ' '),
+        );


Nitpick: please use short array syntax.

alongosz · 2017-06-21T13:46:46Z

eZ/Publish/API/Repository/Tests/SearchServiceTest.php

+     *
+     * @dataProvider getFullTextQueriesWithoutWords
+     */
+    public function testFullTextOnQueryWithoutWords($searchQuery)


Please hint also @param in PhpDoc

alongosz · 2017-06-21T13:47:29Z

eZ/Publish/API/Repository/Tests/SearchServiceTest.php

+        $contentService->publishVersion(
+            $contentService->createContent(
+                $contentCreateStruct,
+                array($locationService->newLocationCreateStruct(2))


Nitpick: please use short array syntax.

alongosz · 2017-06-21T13:47:36Z

eZ/Publish/API/Repository/Tests/SearchServiceTest.php

+        $query = new Query(
+            array(
+                'query' => new Criterion\FullText($searchQuery),
+            )


Nitpick: please use short array syntax.

alongosz · 2017-06-21T13:51:20Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/FullText.php

+
+        $wordIdSubquery = $this->getWordIdSubquery($subSelect, $criterion->value);
+        if ($wordIdSubquery === null) {
+            return $query->expr->neq(1, 1);


This is still not compatible with declared return type, and I think it's require few words of comment .

I looked over returned values in other criterions and I actually think that @return in getWordIdSubquery PhpDoc is just wrong - it should be @return string because all methods of the eZ\Publish\Core\Persistence\Database\Expression interface return string as well.

adamwojs · 2017-08-03T08:51:47Z

@andrerom WDYT about @alongosz idea? IMHO is a cleaner solution than generating a query that always returns an empty result set.

andrerom · 2017-08-03T10:00:41Z

@adamwojs I kind of likes part of the proposal, but I had and have doubts about hardcoding logic about full text in the handler, as value can be anything here right?

alongosz · 2017-08-04T12:51:03Z

I had and have doubts about hardcoding logic about full text in the handler, as value can be anything here right?

@adamwojs: @andrerom has a good point here, I looked at this from the FullText Criterion perspective, not the whole scope of all possible Criterions - my mistake.

To determine if the value is correct should be a responsibility of eZ\Publish\Core\Search\Legacy\Content\Common\Gateway\CriterionHandler\FullText::handle.
While I'd prefer in case of empty value to get 0 count result, due do the way query expression is processed and built we cannot do it without a major refactoring.
In this case I'd go with InvalidArgumentException also for "pure" empty value ''.

@andrerom can we expect from API consumer not to allow such queries or be prepared for this exception?
If not, then the last thing, I could think of, we could do to solve this is to introduce our own exception thrown from FullText::handle and caught/handled in \eZ\Publish\Core\Search\Legacy\Content\Gateway\DoctrineDatabase::find or even in getResultCount by returning 0 result count.

andrerom · 2017-08-04T13:09:30Z

If not, then the last thing

I was thinking in similar way, adding exception that we handle internally, however how do we then tell UI to tell the user the query was invalid and this is why there is empty result?

alongosz · 2017-08-04T13:51:04Z

I was thinking in similar way, adding exception that we handle internally, however how do we then tell UI to tell the user the query was invalid and this is why there is empty result?

True. So I'd say the only one valid solution is to throw InvalidArgumentException for empty values as well. SearchService::findContent hints it, so my previous concern about API consumer expectations seems to be resolved.

…in valid token in Legacy Search Engine

adamwojs · 2017-08-08T09:41:28Z

I've updated PR according to our consensus about solution

andrerom · 2017-08-08T11:21:05Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/FullText.php

     */
    protected function getWordIdSubquery(SelectQuery $query, $string)
    {
        $subQuery = $query->subSelect();
        $tokens = $this->tokenizeString(
            $this->processor->transform($string, $this->configuration['commands'])
        );
+
+        if (empty($tokens)) {
+            throw new InvalidArgumentException('string', 'Search query does not contains any searchable token');


should rather be something that actually is 1. concrete enoug so devs know what is wrong, 2. user friendly enough so user knows what he did wrong.

Unsure, but something like :
Search query does not contain a valid FullText search string

Would also be great if we can create a new exception that extends InvalidArgumentException, so UI code can detect it better in case they want to provide a better user feedback then this.

andrerom · 2017-08-08T11:21:44Z

eZ/Publish/Core/Search/Legacy/Content/Common/Gateway/CriterionHandler/FullText.php

@@ -178,14 +178,21 @@ protected function getWordExpression(SelectQuery $query, $token)
     * @param \eZ\Publish\Core\Persistence\Database\SelectQuery $query
     * @param string $string
     *
-     * @return \eZ\Publish\Core\Persistence\Database\SelectQuery
+     * @return \eZ\Publish\Core\Persistence\Database\SelectQuery|null


when does it return null? On exceptions nothing is returned at all ;)

*Also note that handle() should add phpdoc that it uses getWordIdSubquery so the throw part gets reflected there to.

andrerom

Think this is better, some nitpicks added

…t contain valid token in Legacy Search Engine

…id token

adamwojs · 2017-08-08T12:50:49Z

eZ/Publish/Core/Search/Common/Exception/InvalidFullTextSearchString.php

@@ -0,0 +1,21 @@
+<?php
+
+namespace eZ\Publish\Core\Search\Common\Exception;


I'm not sure about namespace, but for me is too specific for \eZ\Publish\Core\Base\Exceptions and too generic for concrete search engine impl.

adamwojs · 2017-08-08T12:52:34Z

eZ/Publish/Core/Search/Common/Exception/InvalidFullTextSearchString.php

+/**
+ * Exception thrown FullText search string is invalid.
+ */
+class InvalidFullTextSearchString extends InvalidArgumentException


Maybe InvalidFullTextQuery will be better a name ?

I'd say either keep it as is or InvalidFullTextSearchQuery.

Side thing: if we intend for external bundle to be able to catch it (and thus refer to it), shouldn't it be a part of API, not Core? Or maybe SPI to avoid cluttering API namespace? POV @andrerom?

…t contain valid token in Legacy Search Engine

adamwojs · 2022-04-19T14:01:42Z

Closing PR as obsolete. eZ Platform 2.5 has reached EOM, so please reopen PR in ezsystems/ezplatform-kernel (bug fixes) or ibexa/core (features/improvements) if issue is still valid for v3.3 / v4.x and you are willing to work on it.

adamwojs changed the base branch from master to 6.7 June 20, 2017 20:13

adamwojs requested review from andrerom and alongosz June 20, 2017 20:33

adamwojs commented Jun 20, 2017

View reviewed changes

adamwojs changed the title ~~[WIP] Fix EZP-27416: Error 500 on search when trying to search space or single quote~~ Fix EZP-27416: Error 500 on search when trying to search space or single quote Jun 21, 2017

alongosz requested changes Jun 21, 2017

View reviewed changes

ezsystems deleted a comment from ezrobot Jun 21, 2017

Fix EZP-27416: Prevent internal server error when query doesn't conta…

d2f9bb3

…in valid token in Legacy Search Engine

adamwojs force-pushed the fix-EZP-27416-500_on_search_when_trying_to_search_space_or_single_quote branch from 57d1ddc to 39d37e8 Compare August 8, 2017 09:37

adamwojs force-pushed the fix-EZP-27416-500_on_search_when_trying_to_search_space_or_single_quote branch from 39d37e8 to 6d864c9 Compare August 8, 2017 09:42

Fix EZP-27416: Integration test for search queries without valid token

c1c11a6

adamwojs force-pushed the fix-EZP-27416-500_on_search_when_trying_to_search_space_or_single_quote branch from 6d864c9 to c1c11a6 Compare August 8, 2017 10:38

ezsystems deleted a comment from ezrobot Aug 8, 2017

andrerom reviewed Aug 8, 2017

View reviewed changes

adamwojs added 2 commits August 8, 2017 14:40

fixup! Fix EZP-27416: Prevent internal server error when query doesn'…

8e0af82

…t contain valid token in Legacy Search Engine

fixup! Fix EZP-27416: Integration test for search queries without val…

0bc4ef7

…id token

adamwojs commented Aug 8, 2017

View reviewed changes

fixup! Fix EZP-27416: Prevent internal server error when query doesn'…

9b0ce76

…t contain valid token in Legacy Search Engine

ezsystems deleted a comment from ezrobot Aug 8, 2017

adamwojs mentioned this pull request Nov 3, 2017

EZP-28055: As a v2 Editor I want to search for content items by entering a search keyword ezsystems/ezplatform-admin-ui#33

Merged

andrerom added the Needs Change label Feb 26, 2018

adamwojs closed this Apr 19, 2022

adamwojs deleted the fix-EZP-27416-500_on_search_when_trying_to_search_space_or_single_quote branch April 19, 2022 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EZP-27416: Error 500 on search when trying to search space or single quote #2029

Fix EZP-27416: Error 500 on search when trying to search space or single quote #2029

adamwojs commented Jun 20, 2017 •

edited

Loading

andrerom commented Jun 20, 2017

adamwojs commented Jun 20, 2017

adamwojs Jun 20, 2017

alongosz Jun 21, 2017

adamwojs commented Jun 20, 2017

alongosz commented Jun 21, 2017

alongosz left a comment

alongosz Jun 21, 2017

alongosz Jun 21, 2017

alongosz Jun 21, 2017

alongosz Jun 21, 2017

alongosz Jun 21, 2017

adamwojs commented Aug 3, 2017

andrerom commented Aug 3, 2017

alongosz commented Aug 4, 2017

andrerom commented Aug 4, 2017 •

edited

Loading

alongosz commented Aug 4, 2017

adamwojs commented Aug 8, 2017

andrerom Aug 8, 2017

andrerom Aug 8, 2017

andrerom Aug 8, 2017

andrerom left a comment

adamwojs Aug 8, 2017

adamwojs Aug 8, 2017

alongosz Aug 8, 2017

adamwojs commented Apr 19, 2022

		@@ -0,0 +1,21 @@
		<?php

		namespace eZ\Publish\Core\Search\Common\Exception;

Fix EZP-27416: Error 500 on search when trying to search space or single quote #2029

Fix EZP-27416: Error 500 on search when trying to search space or single quote #2029

Conversation

adamwojs commented Jun 20, 2017 • edited Loading

Description

Steps to reproduce

TODO

andrerom commented Jun 20, 2017

adamwojs commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwojs commented Jun 20, 2017

alongosz commented Jun 21, 2017

alongosz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwojs commented Aug 3, 2017

andrerom commented Aug 3, 2017

alongosz commented Aug 4, 2017

andrerom commented Aug 4, 2017 • edited Loading

alongosz commented Aug 4, 2017

adamwojs commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwojs commented Apr 19, 2022

adamwojs commented Jun 20, 2017 •

edited

Loading

andrerom commented Aug 4, 2017 •

edited

Loading