Bug 33406 - Searching for authority with hyphen surrounded by spaces causes error 500 (with ES)
Summary: Searching for authority with hyphen surrounded by spaces causes error 500 (wi...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: unspecified
Hardware: All All
: P3 normal
Assignee: Jonathan Druart
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-04-04 14:01 UTC by Janusz Kaczmarek
Modified: 2024-07-04 20:40 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.11
Circulation function:


Attachments
Bug 33406: Handle ES search errors for authorities (3.47 KB, patch)
2023-05-11 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33406: Handle ES search errors for authorities (3.53 KB, patch)
2023-08-08 08:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33406: Filter out some known isolated trouble makers (1.25 KB, patch)
2023-08-08 08:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33406: Add tests (4.27 KB, patch)
2023-09-04 12:40 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33406: Handle ES search errors for authorities (3.53 KB, patch)
2023-09-04 12:40 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33406: Filter out some known isolated trouble makers (1.25 KB, patch)
2023-09-04 12:40 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33406: Add tests (4.33 KB, patch)
2023-09-14 13:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33406: Handle ES search errors for authorities (3.58 KB, patch)
2023-09-14 13:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33406: Filter out some known isolated trouble makers (1.30 KB, patch)
2023-09-14 13:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33406: (QA follow-up) Adjust tests and tidy (3.16 KB, patch)
2023-09-14 13:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33406: (follow-up) Mock SearchEngine to ensure using ES (827 bytes, patch)
2023-09-18 13:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2023-04-04 14:01:36 UTC
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 (+).
Comment 1 Jonathan Druart 2023-05-11 10:41:14 UTC
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
Comment 2 Janusz Kaczmarek 2023-05-11 15:11:41 UTC
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).
Comment 3 Pedro Amorim 2023-08-08 08:51:20 UTC
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>
Comment 4 Pedro Amorim 2023-08-08 08:51:22 UTC
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>
Comment 5 Pedro Amorim 2023-08-08 08:52:44 UTC
(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.
Comment 6 Pedro Amorim 2023-09-01 09:32:49 UTC
Hey, anyone interested in taking a look at this one?
Comment 7 Jonathan Druart 2023-09-01 10:05:09 UTC
Second patch is missing tests.
Comment 8 Pedro Amorim 2023-09-04 12:40:48 UTC
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
Comment 9 Pedro Amorim 2023-09-04 12:40:50 UTC
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>
Comment 10 Pedro Amorim 2023-09-04 12:40:53 UTC
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>
Comment 11 Nick Clemens (kidclamp) 2023-09-14 13:25:16 UTC
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>
Comment 12 Nick Clemens (kidclamp) 2023-09-14 13:25:19 UTC
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>
Comment 13 Nick Clemens (kidclamp) 2023-09-14 13:25:22 UTC
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>
Comment 14 Nick Clemens (kidclamp) 2023-09-14 13:25:25 UTC
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>
Comment 15 Nick Clemens (kidclamp) 2023-09-14 13:28:38 UTC
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
Comment 16 Tomás Cohen Arazi 2023-09-15 18:50:20 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 17 Fridolin Somers 2023-09-18 08:53:37 UTC
Pushed to 23.05.x for 23.05.04
Comment 18 Tomás Cohen Arazi 2023-09-18 12:46:50 UTC
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/
Comment 19 Nick Clemens (kidclamp) 2023-09-18 13:25:19 UTC
Created attachment 155818 [details] [review]
Bug 33406: (follow-up) Mock SearchEngine to ensure using ES
Comment 20 Fridolin Somers 2023-09-18 20:05:20 UTC
follow-up pushed to 23.05.x for 23.05.04
Comment 21 Pedro Amorim 2023-09-25 09:33:40 UTC
(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!
Comment 22 Matt Blenkinsop 2023-10-12 15:46:07 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x