Bug 28676 - AutoCreateAuthorities can repeatedly generate authority records when using Default linker and heading is cached
Summary: AutoCreateAuthorities can repeatedly generate authority records when using De...
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on: 25189
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-07 14:44 UTC by Nick Clemens
Modified: 2021-10-18 02:36 UTC (History)
7 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,20.11.11,20.05.17,19.11.23


Attachments
Bug 28676: Unit test (1.42 KB, patch)
2021-07-07 15:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28676: Cache and retrieve match_count when searching a cached heading (1.92 KB, patch)
2021-07-07 15:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28676: Unit test (1.47 KB, patch)
2021-09-02 16:46 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 28676: Cache and retrieve match_count when searching a cached heading (1.97 KB, patch)
2021-09-02 16:46 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 28676: Cache and retrieve match_count when searching a cached heading (2.12 KB, patch)
2021-09-18 13:19 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28676: Unit test (1.62 KB, patch)
2021-09-18 13:29 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28676: Cache and retrieve match_count when searching a cached heading (2.12 KB, patch)
2021-09-18 13:29 UTC, Joonas Kylmälä
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-07-07 14:44:21 UTC
Bug 25189 avoided creating a new auth if the match_count was greater than 1, however, when we cache auth results we don't cache match_count, so if the same heading is searched, we will create a new record again:

C4/Linker/Default.pm
 38     if ( $self->{'cache'}->{$search_form.$auth_type}->{'cached'} ) {
 39         $authid = $self->{'cache'}->{$search_form.$auth_type}->{'authid'};
 40         $fuzzy  = $self->{'cache'}->{$search_form.$auth_type}->{'fuzzy'};
 41     }
Comment 1 Nick Clemens 2021-07-07 15:44:28 UTC
Created attachment 122653 [details] [review]
Bug 28676: Unit test
Comment 2 Nick Clemens 2021-07-07 15:44:31 UTC
Created attachment 122654 [details] [review]
Bug 28676: Cache and retrieve match_count when searching a cached heading

We use match_count to determine if a new authority record should be created, however,
we were not adding this count to the cache, so if a record returned too many matches on first
lookup, we would create a new record on the second lookup

To test:
1 - Set Linker Module to 'Default'
2 - Enable  AutoCreateAuthorities  and  BiblioAddsAuthorities and  CatalogModuleRelink and LinkerRelink
3 - Add two copies of a single authority via Z39
4 - Add two headings for that authority to a bib record (e.g. a 610 and 710)
5 - Save the record and note a new authority is generated
6 - Repeat and see another is generated
7 - Apply patch
8 - Restart all the things
9 - Save the record again, no new authority created
Comment 3 Phil Ringnalda 2021-09-02 16:46:41 UTC
Created attachment 124430 [details] [review]
Bug 28676: Unit test

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 4 Phil Ringnalda 2021-09-02 16:46:45 UTC
Created attachment 124431 [details] [review]
Bug 28676: Cache and retrieve match_count when searching a cached heading

We use match_count to determine if a new authority record should be created, however,
we were not adding this count to the cache, so if a record returned too many matches on first
lookup, we would create a new record on the second lookup

To test:
1 - Set Linker Module to 'Default'
2 - Enable  AutoCreateAuthorities  and  BiblioAddsAuthorities and  CatalogModuleRelink and LinkerRelink
3 - Add two copies of a single authority via Z39
4 - Add two headings for that authority to a bib record (e.g. a 610 and 710)
5 - Save the record and note a new authority is generated
6 - Repeat and see another is generated
7 - Apply patch
8 - Restart all the things
9 - Save the record again, no new authority created

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 5 Joonas Kylmälä 2021-09-18 13:19:42 UTC
Created attachment 125026 [details] [review]
Bug 28676: Cache and retrieve match_count when searching a cached heading

We use match_count to determine if a new authority record should be created, however,
we were not adding this count to the cache, so if a record returned too many matches on first
lookup, we would create a new record on the second lookup

To test:
1 - Set Linker Module to 'Default'
2 - Enable  AutoCreateAuthorities  and  BiblioAddsAuthorities and  CatalogModuleRelink and LinkerRelink
3 - Add two copies of a single authority via Z39
4 - Add two headings for that authority to a bib record (e.g. a 610 and 710)
5 - Save the record and note a new authority is generated
6 - Repeat and see another is generated
7 - Apply patch
8 - Restart all the things
9 - Save the record again, no new authority created

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 6 Joonas Kylmälä 2021-09-18 13:24:32 UTC
Fixes the problem, also as well for link_bibs_to_authorities.pl where the same issue occurred. This patch doesn't update the match_count value to cache in the update_cache() function even though it should, but the function was already buggy before this (it didn't update correctly the fuzzy value), and I think what we should do there is call the get_link() with a parameter to tell ignore cache so it will correctly then populate the cache values – but it's out of scope for this bug.

Passing QA.
Comment 7 Joonas Kylmälä 2021-09-18 13:29:08 UTC
Created attachment 125027 [details] [review]
Bug 28676: Unit test

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 8 Joonas Kylmälä 2021-09-18 13:29:11 UTC
Created attachment 125028 [details] [review]
Bug 28676: Cache and retrieve match_count when searching a cached heading

We use match_count to determine if a new authority record should be created, however,
we were not adding this count to the cache, so if a record returned too many matches on first
lookup, we would create a new record on the second lookup

To test:
1 - Set Linker Module to 'Default'
2 - Enable  AutoCreateAuthorities  and  BiblioAddsAuthorities and  CatalogModuleRelink and LinkerRelink
3 - Add two copies of a single authority via Z39
4 - Add two headings for that authority to a bib record (e.g. a 610 and 710)
5 - Save the record and note a new authority is generated
6 - Repeat and see another is generated
7 - Apply patch
8 - Restart all the things
9 - Save the record again, no new authority created

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Comment 9 Joonas Kylmälä 2021-09-18 13:30:13 UTC
(In reply to Joonas Kylmälä from comment #6)
> Fixes the problem, also as well for link_bibs_to_authorities.pl where the
> same issue occurred. This patch doesn't update the match_count value to
> cache in the update_cache() function even though it should, but the function
> was already buggy before this (it didn't update correctly the fuzzy value),
> and I think what we should do there is call the get_link() with a parameter
> to tell ignore cache so it will correctly then populate the cache values –
> but it's out of scope for this bug.
> 
> Passing QA.

Forgot to attach the other patch, re-attached now with my sign-off again.
Comment 10 Jonathan Druart 2021-09-20 12:47:42 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 11 Kyle M Hall 2021-10-01 17:31:15 UTC
Pushed to 21.05.x for 21.05.05
Comment 12 Fridolin Somers 2021-10-06 20:06:36 UTC
Pushed to 20.11.x for 20.11.11
Comment 13 Victor Grousset/tuxayo 2021-10-15 23:49:15 UTC
Backported: Pushed to 20.05.x branch for 20.05.17
Comment 14 wainuiwitikapark 2021-10-18 02:36:43 UTC
Backported to 19.11.x for 19.11.23