Bug 28736

Summary: Better error message when ES fails to understand the syntax of the search query
Product: Koha Reporter: Peter Vashchuk <stalkernoid>
Component: Searching - ElasticsearchAssignee: Peter Vashchuk <stalkernoid>
Status: CLOSED FIXED QA Contact: Andrii Nugged <nugged>
Severity: enhancement    
Priority: P5 - low CC: e.betemps, jonathan.druart, kyle, nugged, stalkernoid
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28316
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
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
Bug 28736: fix requirements for the correct error message to show up
Bug 28736: fix requirements for the correct error message to show up
Bug 28736: fix requirements for the correct error message to show up
Bug 28736: fix requirements for the correct error message to show up
Bug 28736: (QA follow-up) hints in explanation about broken syntax
Bug 28736: (QA follow-up) hints in explanation about broken syntax
Bug 28736: (QA follow-up) hints in explanation about broken syntax

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 Andrii 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 Andrii Nugged 2021-08-21 08:56:25 UTC Comment hidden (obsolete)
Comment 7 Andrii 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 Andrii Nugged 2021-08-21 18:48:07 UTC Comment hidden (obsolete)
Comment 9 Andrii Nugged 2021-08-21 19:20:54 UTC Comment hidden (obsolete)
Comment 10 Andrii Nugged 2021-08-21 20:02:01 UTC Comment hidden (obsolete)
Comment 11 Andrii 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!