Bug 29284

Summary: Koha dies when an analytics search fails in Elasticsearch
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ElasticsearchAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: andrewfh, fridolin.somers, jonathan.druart+koha, kyle, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28682
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.14
Bug Depends on: 28316    
Bug Blocks:    
Attachments: Bug 29284: Don't die on analytics searchign error
Bug 29284: Handle the case of an exclamation point in parentheses
Bug 29284: Don't die on analytics searchign error
Bug 29284: Handle the case of an exclamation point in parentheses
Bug 29284: Unit test
Bug 29284: (follow-up) Improve error logging to prevent warns
Bug 29284: (follow-up) Fix code that I copied from too

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?
Comment 14 Victor Grousset/tuxayo 2022-01-30 23:53:23 UTC
Backported: Pushed to 20.11.x branch for 20.11.14