Bug 29284 - Koha dies when an analytics search fails in Elasticsearch
Summary: Koha dies when an analytics search fails in Elasticsearch
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
: 28682 (view as bug list)
Depends on: 28316
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-20 12:46 UTC by Nick Clemens
Modified: 2022-01-11 12:52 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,21.05.05


Attachments
Bug 29284: Don't die on analytics searchign error (3.70 KB, patch)
2021-10-20 12:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29284: Handle the case of an exclamation point in parentheses (1.44 KB, patch)
2021-10-20 12:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29284: Don't die on analytics searchign error (3.76 KB, patch)
2021-10-20 13:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29284: Handle the case of an exclamation point in parentheses (1.50 KB, patch)
2021-10-20 13:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29284: Unit test (1.21 KB, patch)
2021-10-21 10:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29284: (follow-up) Improve error logging to prevent warns (2.46 KB, patch)
2021-10-21 10:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29284: (follow-up) Fix code that I copied from too (1.38 KB, patch)
2021-10-21 10:33 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-10-20 12:46:03 UTC
Bug 28316 added some escaping of reserved ES characters, bug 28484 used the escaping on detail pages, however, it seems there may still be combinations that break the search. We should prevent a failure when loading these records

To recreate:
1 - Add a title to catalog, with 245a:
    Digger does it all (not really!)
2 - Set searchEngine preference to: Elasticsearch
3 - The record does not load
Comment 1 Nick Clemens 2021-10-20 12:54:31 UTC
Created attachment 126583 [details] [review]
Bug 29284: Don't die on analytics searchign error

This patch adds an eval around the call to search for analytic records
It pases a value to the template on the staff side, but logs the warning on
the opac

This seems similar to 'decoding_error' which is noted on staff side, but absent on OPAC

The eval follows the patter used during searching

To test:
1 - Add a title to catalog, with 245a:
    Digger does it all (not really!)
2 - Set searchEngine preference to: Elasticsearch
3 - The record does not load
4 - Apply patch
5 - The record loads, there is a note about analytics at the top fo the record
6 - View record in opac, no note
7 - Check logs on intranet and opac, searching error is logged
Comment 2 Nick Clemens 2021-10-20 12:54:34 UTC
Created attachment 126584 [details] [review]
Bug 29284: Handle the case of an exclamation point in parentheses

This expands the regex to handle this specific case

To test:
1 - Load record created for last patch
2 - Note analytics error
3 - Apply patch
4 - Restart and reload
5 - No more errorm also no Analytics link
6 - Add a 773$t to a record with title used before:
    Digger does it all (not really!)
7 - reload the initial record
8 - See 'Show analytics' link
9 - Click the link
10 - You should end up on the record you added the 773 to
Comment 3 Martin Renvoize 2021-10-20 13:24:17 UTC
*** Bug 28682 has been marked as a duplicate of this bug. ***
Comment 4 Martin Renvoize 2021-10-20 13:25:50 UTC
Created attachment 126586 [details] [review]
Bug 29284: Don't die on analytics searchign error

This patch adds an eval around the call to search for analytic records
It pases a value to the template on the staff side, but logs the warning on
the opac

This seems similar to 'decoding_error' which is noted on staff side, but absent on OPAC

The eval follows the patter used during searching

To test:
1 - Add a title to catalog, with 245a:
    Digger does it all (not really!)
2 - Set searchEngine preference to: Elasticsearch
3 - The record does not load
4 - Apply patch
5 - The record loads, there is a note about analytics at the top fo the record
6 - View record in opac, no note
7 - Check logs on intranet and opac, searching error is logged

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-10-20 13:25:54 UTC
Created attachment 126587 [details] [review]
Bug 29284: Handle the case of an exclamation point in parentheses

This expands the regex to handle this specific case

To test:
1 - Load record created for last patch
2 - Note analytics error
3 - Apply patch
4 - Restart and reload
5 - No more errorm also no Analytics link
6 - Add a 773$t to a record with title used before:
    Digger does it all (not really!)
7 - reload the initial record
8 - See 'Show analytics' link
9 - Click the link
10 - You should end up on the record you added the 773 to

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-10-20 13:26:57 UTC
Clear and concise patch that does what it says on the tin.

QA scripts are happy and testing shows it all works as expected.

Going straight for QA on this one as it's a major with a minimal complexity fix.

PQA
Comment 7 Jonathan Druart 2021-10-21 08:09:42 UTC
1.
Please improve the error handling

[2021/10/21 08:08:21] [WARN] Use of uninitialized value $err in concatenation (.) or string at /kohadevbox/koha/catalogue/detail.pl line 150.

+    if ($err || $@){
+        warn "Warning from simple_search_compat: $err.$@";

2. Second patch needs tests.
Comment 8 Nick Clemens 2021-10-21 10:33:41 UTC
Created attachment 126656 [details] [review]
Bug 29284: Unit test
Comment 9 Nick Clemens 2021-10-21 10:33:44 UTC
Created attachment 126657 [details] [review]
Bug 29284: (follow-up) Improve error logging to prevent warns
Comment 10 Nick Clemens 2021-10-21 10:33:48 UTC
Created attachment 126658 [details] [review]
Bug 29284: (follow-up) Fix code that I copied from too
Comment 11 Jonathan Druart 2021-10-25 13:59:16 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 12 Kyle M Hall 2021-10-29 13:38:32 UTC
Pushed to 21.05.x for 21.05.05
Comment 13 Martin Renvoize 2022-01-11 12:52:41 UTC
Doesn't 20.11 need this too?