With ES, an authority search in 'contains'-mode (both: OPAC & intra), where search term contains a hyphen surrounded by spaces generates Error 500. Is is due to ES parse exception while parsing query like: "(term_1*) AND (-) AND (term_2*)" resulting from build_authorities_query. The same effect for some other (less common) characters like caret (^), backslash (\), tilde (~), plus (+).
Created attachment 151067 [details] [review] Bug 33406: Handle ES search errors for authorities We are dealing ok with ES exceptions for biblio records search, catching them and raising them to the end user. But we don't for authorities, where we explode with an ugly 500. Test plan: Search for "(term_1*) AND (-) AND (term_2*)" in the authority search and notice that you don't get a 500 but an error instead saying that you should try again
Jonathan, thank you for the patch. I'll test it and it most probably will do its job. At the same time, I'm not quite sure if all we can do is to catch error. IMHO it will be rather surprising for an librarian to see information about an error and a prompt to try again while searching (with default settings) for an existing authority (and hyphens surrounded by spaces normally occur e.g. in corporate names or in series titles). Couldn't we, retaining the catching mechanism, also add somehow a correction like this: --- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm +++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ -1228,6 +1228,8 @@ sub _split_query { # Filter out empty values @tokens = grep( /\S/, @tokens ); + # Filter out some known isolated trouble makers: -, ^, \, ~, + + @tokens = grep( !/^[-\^\\~\+]*$/, @tokens ); return @tokens; } [The auth 'contains' query goes always through _split_query which is called by _truncate_terms] This would prevent most known issues to emerge. (I was unable, due to lack of time, to prepare the official patch by myself).
Created attachment 154313 [details] [review] Bug 33406: Handle ES search errors for authorities We are dealing ok with ES exceptions for biblio records search, catching them and raising them to the end user. But we don't for authorities, where we explode with an ugly 500. Test plan: Search for "(term_1*) AND (-) AND (term_2*)" in the authority search and notice that you don't get a 500 but an error instead saying that you should try again Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 154314 [details] [review] Bug 33406: Filter out some known isolated trouble makers Test plan: 1) Launch k-t-d with ES: -- ktd --es7 pull -- ktd --es7 up 2) Visit /cgi-bin/koha/authorities/authorities-home.pl and search for 'tim' 3) Edit that, add a dash '-' to 100$a, like 'Bunce, Tim - name'" 4) Do a authority search for 'tim - name', notice it errors out 5) Apply patch 6) Perform the same search, notice it doesn't error and returns the correct record Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
(In reply to Janusz Kaczmarek from comment #2) > > This would prevent most known issues to emerge. (I was unable, due to lack > of time, to prepare the official patch by myself). Janusz I've tested your fix proposal and it works. I've submitted the patch with your name on it.
Hey, anyone interested in taking a look at this one?
Second patch is missing tests.
Created attachment 155198 [details] [review] Bug 33406: Add tests Test plan: 1) Apply this tests patch only 2) Run: prove t/Koha/SearchEngine/ElasticSearch/Search.t 3) Notice it fails with nasty ES "Failed to parse query" error 4) Apply the other patches 5) Repeat 2) - notice theres no nasty error
Created attachment 155199 [details] [review] Bug 33406: Handle ES search errors for authorities We are dealing ok with ES exceptions for biblio records search, catching them and raising them to the end user. But we don't for authorities, where we explode with an ugly 500. Test plan: Search for "(term_1*) AND (-) AND (term_2*)" in the authority search and notice that you don't get a 500 but an error instead saying that you should try again Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 155200 [details] [review] Bug 33406: Filter out some known isolated trouble makers Test plan: 1) Launch k-t-d with ES: -- ktd --es7 pull -- ktd --es7 up 2) Visit /cgi-bin/koha/authorities/authorities-home.pl and search for 'tim' 3) Edit that, add a dash '-' to 100$a, like 'Bunce, Tim - name'" 4) Do a authority search for 'tim - name', notice it errors out 5) Apply patch 6) Perform the same search, notice it doesn't error and returns the correct record Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Created attachment 155611 [details] [review] Bug 33406: Add tests Test plan: 1) Apply this tests patch only 2) Run: prove t/Koha/SearchEngine/ElasticSearch/Search.t 3) Notice it fails with nasty ES "Failed to parse query" error 4) Apply the other patches 5) Repeat 2) - notice theres no nasty error Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 155612 [details] [review] Bug 33406: Handle ES search errors for authorities We are dealing ok with ES exceptions for biblio records search, catching them and raising them to the end user. But we don't for authorities, where we explode with an ugly 500. Test plan: Search for "(term_1*) AND (-) AND (term_2*)" in the authority search and notice that you don't get a 500 but an error instead saying that you should try again Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 155613 [details] [review] Bug 33406: Filter out some known isolated trouble makers Test plan: 1) Launch k-t-d with ES: -- ktd --es7 pull -- ktd --es7 up 2) Visit /cgi-bin/koha/authorities/authorities-home.pl and search for 'tim' 3) Edit that, add a dash '-' to 100$a, like 'Bunce, Tim - name'" 4) Do a authority search for 'tim - name', notice it errors out 5) Apply patch 6) Perform the same search, notice it doesn't error and returns the correct record Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 155614 [details] [review] Bug 33406: (QA follow-up) Adjust tests and tidy Rather than test that nothing is returned, we want to test that the terms are filtered as expected. This also avoids the possibility of the tests failing in a db where there is a record for Donald Duck Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I updated the tests to check the query as that is what is adjusted by these patches This will actually affect biblio searches as well (_split_query is called by _truncate_terms which is used in build query compat) but I think this is something people have been asking for. See bug 31213 for searches that are affected. I'd recommend that backport is delayed on this one until some further testing has been done, but it is a minor change and works well. Passing QA
Pushed to master for 23.11. Nice work everyone, thanks!
Pushed to 23.05.x for 23.05.04
This broke some regression tests. Please take care of that ASAP as this is pushed to stable now: https://jenkins.koha-community.org/job/Koha_Master/lastCompletedBuild/
Created attachment 155818 [details] [review] Bug 33406: (follow-up) Mock SearchEngine to ensure using ES
follow-up pushed to 23.05.x for 23.05.04
(In reply to Nick Clemens from comment #15) > I updated the tests to check the query as that is what is adjusted by these > patches > > This will actually affect biblio searches as well (_split_query is called by > _truncate_terms which is used in build query compat) but I think this is > something people have been asking for. See bug 31213 for searches that are > affected. > > I'd recommend that backport is delayed on this one until some further > testing has been done, but it is a minor change and works well. Passing QA Thanks for the follow-up on the tests Nick!
Nice work everyone! Pushed to oldstable for 22.11.x