Bug 30883 - Authorities merge is limited to 100 biblio with Elasticsearch
Summary: Authorities merge is limited to 100 biblio with Elasticsearch
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Fridolin Somers
QA Contact:
URL:
Keywords:
: 30522 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-06-02 07:13 UTC by Fridolin Somers
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the hard-coded limit of 100 when merging authorities (when Elasticsearch is the search engine). When merging authorities where the term is used over 100 times, only the first 100 authorities would be merged and the old term deleted, irrespective of the value set in the AuthorityMergeLimit system preference.
Version(s) released in:
22.11.00, 22.05.01, 21.11.10, 21.05.17


Attachments
Bug 30883: Authorities merge is limited to 100 biblio with Elasticsearch (2.71 KB, patch)
2022-06-02 07:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30883: Authorities merge is limited to 100 biblio with Elasticsearch (2.75 KB, patch)
2022-06-04 23:24 UTC, David Nind
Details | Diff | Splinter Review
Bug 30883: Authorities merge is limited to 100 biblio with Elasticsearch (2.82 KB, patch)
2022-06-13 08:32 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2022-06-02 07:13:18 UTC
In C4::AuthoritiesMarc::merge, we search all biblio records using an authority with Koha::Authorities->linked_biblionumbers().
This method uses simple_search_compat() with no results limit, even saying in comment :
# if max_results is undefined, we will get all results
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/Authorities.pm#L80

With Zebra searchengin it is correct.

But not with Elasticsearch, there is an hardcoded limit at 100 in case no limit is given :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/SearchEngine/Elasticsearch/Search.pm#L346

This means authorities links are wrong after a merge or an edit in case the authority is used in more than 100 biblio records.

:(
Comment 1 Fridolin Somers 2022-06-02 07:23:00 UTC
I propose to fix by using by default the real server max given by Koha::SearchEngine::Elasticsearch::Search::max_result_window().

This will allow a huge limit nearly impossible to reach.

See Bug 30882 to how increase this limit.
Comment 2 Fridolin Somers 2022-06-02 07:36:13 UTC
Created attachment 135573 [details] [review]
Bug 30883: Authorities merge is limited to 100 biblio with Elasticsearch

In C4::AuthoritiesMarc::merge, we search all biblio records using an authority with Koha::Authorities->linked_biblionumbers().
This method uses simple_search_compat() with no results limit, even saying in comment :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/Authorities.pm#L80

With Zebra searchengin it is correct.

But not with Elasticsearch, there is an hardcoded limit at 100 in case no limit is given :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/SearchEngine/Elasticsearch/Search.pm#L346

This means authorities links are wrong after a merge or an edit in case the authority is used in more than 100 biblio records.

:(

I propose to fix by using by default the real server max given by Koha::SearchEngine::Elasticsearch::Search::max_result_window().
This will allow a huge limit nearly impossible to reach.
See Bug 30882 to how increase this limit.

Test plan :
1) Use Elasticsearch search engine
2) Use an authority id=111 linked to 200 biblio records
3) Perform a search 'an:111', you get 200 results
4) Create a new authority id=222 linked to 2 biblio records
5) Perform a search 'an:222', you get 2 results
6) Perform a merge of the two authorties, keeping id=222
7) Perform a search 'an:111'
   without patch you get 100 results
   with patch you get no results
8) Perform a search 'an:222'
   without patch you get 102 results
   with patch you get 202 results
Comment 3 Fridolin Somers 2022-06-02 08:52:28 UTC
BTW we should think of making a background job of this action
Comment 4 David Nind 2022-06-04 23:24:27 UTC
Created attachment 135699 [details] [review]
Bug 30883: Authorities merge is limited to 100 biblio with Elasticsearch

In C4::AuthoritiesMarc::merge, we search all biblio records using an authority with Koha::Authorities->linked_biblionumbers().
This method uses simple_search_compat() with no results limit, even saying in comment :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/Authorities.pm#L80

With Zebra searchengin it is correct.

But not with Elasticsearch, there is an hardcoded limit at 100 in case no limit is given :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/SearchEngine/Elasticsearch/Search.pm#L346

This means authorities links are wrong after a merge or an edit in case the authority is used in more than 100 biblio records.

:(

I propose to fix by using by default the real server max given by Koha::SearchEngine::Elasticsearch::Search::max_result_window().
This will allow a huge limit nearly impossible to reach.
See Bug 30882 to how increase this limit.

Test plan :
1) Use Elasticsearch search engine
2) Use an authority id=111 linked to 200 biblio records
3) Perform a search 'an:111', you get 200 results
4) Create a new authority id=222 linked to 2 biblio records
5) Perform a search 'an:222', you get 2 results
6) Perform a merge of the two authorties, keeping id=222
7) Perform a search 'an:111'
   without patch you get 100 results
   with patch you get no results
8) Perform a search 'an:222'
   without patch you get 102 results
   with patch you get 202 results

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2022-06-04 23:25:01 UTC
I finally managed to test this and get things to work (using koha-testing-docker).

Initially, both before and after applying the patch, my results were:
1. Merged authority record deleted, it no longer existed.
2. Searches were the same before and after, that is, there were no changes in the number of results.
3. None of the records that had the old term were updated with the new term.

The issue with merged authority records not updating the bibliographic records was the system preference AuthorityMergeLimit- this is  set to 50 in koha-testing-docker.

I'm pretty sure there are easier ways to add a new term to multiple records, but this is what I did:

1. Enage Elasticsearch as the search engine and reindexed: koha-elasticsearch --rebuild -d -b -a kohadev

2. AuthorityMergeLimit: set to 50, changed to 1,000.

3. Added two new topical terms:

- Term to merge ==> id = 1708
- Term to keep (merge) ==> id = 1709

4. Manually added "Term to keep (merge)" (1709) to two records
   ==> in authority search, search for "merge" shows as 2 records "Term to keep (merge)" and 0 for "Term to merge"

5. Created a MARC modification template (Tools > Catalog) with two actions:
   3.1 Add new field(s) 650$a with value Term to merge
   3.2 Update existing or add new field(s) 650$9 with value 1708
       if field 650$a matches Term to merge
   
6. Created a report to get a list of record ids, exported as a spreadsheet.
   SQL = select * from biblio;

7. Set up and ran a batch record modification:
   - Record type: Biblios
   - Enter a list of record numbers: copy and paste in 200 record ids from the exported report
   - MARC modification template: choose the template that you created
   - Click "Contine"
   - Check a record to see if it will add a 650$a and 650$9
   - Click "Modifiy selected records'
   ==> in authority search, search for "merge":
   . "Term to merge" shows as 200 records
   . "Term to keep (merge)" shows as 2 records
Comment 6 Katrin Fischer 2022-06-06 21:54:14 UTC
*** Bug 30522 has been marked as a duplicate of this bug. ***
Comment 7 Martin Renvoize 2022-06-13 08:32:31 UTC
Created attachment 135997 [details] [review]
Bug 30883: Authorities merge is limited to 100 biblio with Elasticsearch

In C4::AuthoritiesMarc::merge, we search all biblio records using an authority with Koha::Authorities->linked_biblionumbers().
This method uses simple_search_compat() with no results limit, even saying in comment :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/Authorities.pm#L80

With Zebra searchengin it is correct.

But not with Elasticsearch, there is an hardcoded limit at 100 in case no limit is given :
https://git.koha-community.org/Koha-community/Koha/src/commit/44d6528b566e5c16eba9b15f8aa24667293766c3/Koha/SearchEngine/Elasticsearch/Search.pm#L346

This means authorities links are wrong after a merge or an edit in case the authority is used in more than 100 biblio records.

:(

I propose to fix by using by default the real server max given by Koha::SearchEngine::Elasticsearch::Search::max_result_window().
This will allow a huge limit nearly impossible to reach.
See Bug 30882 to how increase this limit.

Test plan :
1) Use Elasticsearch search engine
2) Use an authority id=111 linked to 200 biblio records
3) Perform a search 'an:111', you get 200 results
4) Create a new authority id=222 linked to 2 biblio records
5) Perform a search 'an:222', you get 2 results
6) Perform a merge of the two authorties, keeping id=222
7) Perform a search 'an:111'
   without patch you get 100 results
   with patch you get no results
8) Perform a search 'an:222'
   without patch you get 102 results
   with patch you get 202 results

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2022-06-13 08:35:28 UTC
This looks great, and the QA scripts are happy.

One minor though is should we have a unit test for this as it affects a module..  but I wasn't entirely sure how best to approach that so thought PQA and leave it for the RM to decide whether to require one.

PQA
Comment 9 Tomás Cohen Arazi 2022-06-13 13:38:32 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 10 Lucas Gass 2022-06-24 16:05:53 UTC
Backported to 22.05.x for 22.05.01
Comment 11 Arthur Suzuki 2022-06-30 08:05:07 UTC
Pushed to 21.11.x for 21.11.10, thx!
Comment 12 Victor Grousset/tuxayo 2022-07-14 19:26:54 UTC
Can't reproduce on 21.05.x

7) Perform a search 'an:111'
   without patch you get 100 results
   with patch you get no results
8) Perform a search 'an:222'
   without patch you get 102 results
   with patch you get 202 results


With and without the patch, I always get the original number of results. Even if the 1st auth doesn't exist anymore.
Note: I used preexisting authorities on sample data. respectively 1 (used in 152 records) and 101 (used in 4 records)
Any idea? Does this show merge is completely broken in 21.05.x ? Maybe I used it badly.

Anyway, the code very simple and safe so I don't think it's related:
Backported: Pushed to 21.05.x branch for 21.05.17
Nothing to document, marking resolved.