[Bug 33406] New: Searching for authority with hyphen surrounded by spaces causes error 500 (with ES)
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Bug ID: 33406 Summary: Searching for authority with hyphen surrounded by spaces causes error 500 (with ES) Change sponsored?: --- Product: Koha Version: 22.11 Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Searching - Elasticsearch Assignee: koha-bugs@lists.koha-community.org Reporter: januszop@gmail.com 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 (+). -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Janusz Kaczmarek <januszop@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P3 Severity|enhancement |normal -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #1 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 151067 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151067&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|22.11 |unspecified Assignee|koha-bugs@lists.koha-commun |jonathan.druart+koha@gmail. |ity.org |com CC| |jonathan.druart+koha@gmail. | |com -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #2 from Janusz Kaczmarek <januszop@gmail.com> --- 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151067|0 |1 is obsolete| | --- Comment #3 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 154313 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154313&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #4 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 154314 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154314&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #5 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl, | |magnus@libriotech.no, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com, | |tomascohen@gmail.com --- Comment #6 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Hey, anyone interested in taking a look at this one? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Second patch is missing tests. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #8 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 155198 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155198&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154313|0 |1 is obsolete| | --- Comment #9 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 155199 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155199&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154314|0 |1 is obsolete| | --- Comment #10 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 155200 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155200&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #155198|0 |1 is obsolete| | Attachment #155199|0 |1 is obsolete| | Attachment #155200|0 |1 is obsolete| | --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155611 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155611&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #12 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155612&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #13 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155613 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155613&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155614 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155614&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |nick@bywatersolutions.com See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=31213 --- Comment #15 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |23.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #16 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable CC| |fridolin.somers@biblibre.co | |m Version(s)|23.11.00 |23.11.00,23.05.04 released in| | --- Comment #17 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.05.x for 23.05.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #18 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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/ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 155818 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=155818&action=edit Bug 33406: (follow-up) Mock SearchEngine to ensure using ES -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #20 from Fridolin Somers <fridolin.somers@biblibre.com> --- follow-up pushed to 23.05.x for 23.05.04 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #21 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|23.11.00,23.05.04 |23.11.00,23.05.04,22.11.11 released in| | Status|Pushed to stable |Pushed to oldstable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33406 --- Comment #22 from Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> --- Nice work everyone! Pushed to oldstable for 22.11.x -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org