Bug 28736 - Better error message when ES fails to understand the syntax of the search query
Summary: Better error message when ES fails to understand the syntax of the search query
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Peter Vashchuk
QA Contact: Andrew Nugged
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-22 12:54 UTC by Peter Vashchuk
Modified: 2021-09-03 14:59 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00


Attachments
Current "Unable to perform your search. Please try again." error (51.66 KB, image/png)
2021-07-22 13:00 UTC, Peter Vashchuk
Details
Bug 28736: fix requirements for the correct error message to show up (1.70 KB, patch)
2021-07-26 09:28 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 28736: fix requirements for the correct error message to show up (1.75 KB, patch)
2021-08-02 20:55 UTC, David Nind
Details | Diff | Splinter Review
Bug 28736: fix requirements for the correct error message to show up (1.80 KB, patch)
2021-08-20 04:14 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28736: fix requirements for the correct error message to show up (2.42 KB, patch)
2021-08-21 08:56 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 28736: (QA follow-up) hints in explanation about broken syntax (1.41 KB, patch)
2021-08-21 08:56 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 28736: (QA follow-up) hints in explanation about broken syntax (1.50 KB, patch)
2021-08-21 18:48 UTC, Andrew Nugged
Details | Diff | Splinter Review
Bug 28736: (QA follow-up) hints in explanation about broken syntax (1.59 KB, patch)
2021-08-21 19:20 UTC, Andrew Nugged
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Vashchuk 2021-07-22 12:54:56 UTC
"Unable to perform your search. Please try again." error message doesn't provide any information to the user about the problem and can be really confusing, it implies that by simply performing the search again the result may be different somehow, while in reality the search query probably has incorrect ES syntax which will never work no matter how many times you try, asking user to "Please try again" is redundant.
Comment 1 Peter Vashchuk 2021-07-22 13:00:28 UTC
Created attachment 123046 [details]
Current "Unable to perform your search. Please try again." error
Comment 2 Peter Vashchuk 2021-07-26 09:28:18 UTC
Created attachment 123181 [details] [review]
Bug 28736: fix requirements for the correct error message to show up

There is existing error message in the code stating:
"Unable to understand your search query, please rephrase and try again."
which fits perfectly but because it looks for "ParseException" in the
warning output it doesn't show up on this page as it's actually
"parse_exception".

This patch makes that it's also checked if "parse_exception" is present
in the warning output.

To reproduce:
1) using ES search for something like "// ^ ! { } [ ] .. , <>" that
will for sure break the syntax of ES.
2) after the search query fails note that the error is
"Unable to perform your search. Please try again."
3) apply the patch
4) search for the same thing again
5) error message should be "Unable to understand your search query,
please rephrase and try again." now.
Comment 3 David Nind 2021-08-02 20:55:17 UTC
Created attachment 123404 [details] [review]
Bug 28736: fix requirements for the correct error message to show up

There is existing error message in the code stating:
"Unable to understand your search query, please rephrase and try again."
which fits perfectly but because it looks for "ParseException" in the
warning output it doesn't show up on this page as it's actually
"parse_exception".

This patch makes that it's also checked if "parse_exception" is present
in the warning output.

To reproduce:
1) using ES search for something like "// ^ ! { } [ ] .. , <>" that
will for sure break the syntax of ES.
2) after the search query fails note that the error is
"Unable to perform your search. Please try again."
3) apply the patch
4) search for the same thing again
5) error message should be "Unable to understand your search query,
please rephrase and try again." now.

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Victor Grousset/tuxayo 2021-08-20 04:14:48 UTC
Created attachment 123985 [details] [review]
Bug 28736: fix requirements for the correct error message to show up

There is existing error message in the code stating:
"Unable to understand your search query, please rephrase and try again."
which fits perfectly but because it looks for "ParseException" in the
warning output it doesn't show up on this page as it's actually
"parse_exception".

This patch makes that it's also checked if "parse_exception" is present
in the warning output.

To reproduce:
1) using ES search for something like "// ^ ! { } [ ] .. , <>" that
will for sure break the syntax of ES.
2) after the search query fails note that the error is
"Unable to perform your search. Please try again."
3) apply the patch
4) search for the same thing again
5) error message should be "Unable to understand your search query,
please rephrase and try again." now.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 5 Andrew Nugged 2021-08-21 08:56:21 UTC
Created attachment 124005 [details] [review]
Bug 28736: fix requirements for the correct error message to show up

There is existing error message in the code stating:
"Unable to understand your search query, please rephrase and try again."
which fits perfectly but because it looks for "ParseException" in the
warning output it doesn't show up on this page as it's actually
"parse_exception".

This patch makes that it's also checked if "parse_exception" is present
in the warning output.

Side note:
"ParseException" reaction code was added here:
    e0f6c4dc Bug 12478: improve error reporting a bit
Search::Elasticsearch seems propagates clean ES JSON answer,
and in current ES version inside of $@ it contains "parse_exception"
string in dumped JSON answer ("'type' => 'parse_exception'").
Old seeked phrase "ParseException" wasn't reproduced, only in ES logs
("Caused by: org.apache.lucene.queryparser.classic.ParseException:
 Cannot parse ..."). Check for both phrases won't complicate future
changes, but this note added for reference and code cleanup if needed.

To reproduce:
1) using ES search for something like "// ^ ! { } [ ] .. , <>" that
will for sure break the syntax of ES.
2) after the search query fails note that the error is
"Unable to perform your search. Please try again."
3) apply the patch
4) search for the same thing again
5) error message should be "Unable to understand your search query,
please rephrase and try again." now.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Andrew Nugged <nugged@gmail.com>
Comment 6 Andrew Nugged 2021-08-21 08:56:25 UTC Comment hidden (obsolete)
Comment 7 Andrew Nugged 2021-08-21 08:59:36 UTC
This "sub process_error" code was added here:
    e0f6c4dc Bug 12478: improve error reporting a bit
And it expected to get 'ParseException' inside of $@.

I didn't found any traces and explanations when we have text with "ParseException" from ES, is it older ES versions that had such word? (Search::Elasticsearch seems just propagates clean ES answer though)

Nowadays we have "parse_exception" in "$@" contents, in dumped JSON answer from ES it contains answer body with "'type' => 'parse_exception'" value.

I don't think the "ParseException" presence will make future updates more complicated but let's have this history-of-thoughts noted in the commit message,


Part2: QA-follow-up: in my experience, I had a lot of users thinking that "nothing was found", when ES error happened. I changed the phrase to be wordier but to explain that there MIGHT be results, but the user needs to review his request.
Comment 8 Andrew Nugged 2021-08-21 18:48:07 UTC Comment hidden (obsolete)
Comment 9 Andrew Nugged 2021-08-21 19:20:54 UTC Comment hidden (obsolete)
Comment 10 Andrew Nugged 2021-08-21 20:02:01 UTC Comment hidden (obsolete)
Comment 11 Andrew Nugged 2021-08-25 15:10:42 UTC
I removed the last mine patch, I will create a separate ticket about that.
(it's about "Part2: QA-follow-up:" in my message above.

Putting back ticket to PQA
Comment 12 Jonathan Druart 2021-08-30 15:11:56 UTC
Pushed to master for 21.11, thanks to everybody involved!