Bug 41097 - Deduping authorities script (dedup_authorities.pl) can die on duplicated ids
Summary: Deduping authorities script (dedup_authorities.pl) can die on duplicated ids
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 13706
Blocks:
  Show dependency treegraph
 
Reported: 2025-10-24 15:50 UTC by Nick Clemens (kidclamp)
Modified: 2026-01-29 20:44 UTC (History)
3 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the deduping authorities maintenance script (misc/maintenance/dedup_authorities.pl) so that it now works and displays the output from the merging of authority records as expected. Previously, it seemed to generate duplicate IDs, for example: Before ------ Processing authority 1660 (531/650 81.69%) Merging 1660,1662 into 1660. Updated 0 biblios Deleting 1662 Merge done. After ----- Processing authority 1660 (532/650 81.85%) Merging 1662 into 1660. Updated 0 biblios Deleting 1662 Merge done.
Version(s) released in:
Circulation function:


Attachments
Bug 41097: Don't use original record twice (1.74 KB, patch)
2025-10-24 15:54 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 41097: Don't use original record twice (1.78 KB, patch)
2025-10-25 00:48 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-10-24 15:50:27 UTC
When running the script it seems to generate duplicate ids:

Processing authority 1660 (531/650 81.69%)
    Merging 1660,1662 into 1660.
    Updated 0 biblios
    Deleting 1662
    Merge done.

This seems to be a product of _choose_records which gets all the candidates, but then adds the original record twice

318     my @records         = map { C4::AuthoritiesMarc::GetAuthority($_) } @recordids;
319     my @candidate_auths = @records;

and later: 
$choose_subs[2] ? $choose_subs[2]->($_) : 0 ] => ( $records[0], @candidate_auths );
Comment 1 Nick Clemens (kidclamp) 2025-10-24 15:54:55 UTC
Created attachment 188416 [details] [review]
Bug 41097: Don't use original record twice

This patch prevents us from passing the same record twice into the choose subs
to avoid a situation where the original record is not chosen as the one to keep
and we try to merge/delete the same record twice

To test:
1 - Run the script on a test system that you can reset
    perl misc/maintenance/dedup_authorities.pl -v -v --confirm -a="PERSO_NAME"
2 - Note duplication of entries in the output like:
    Merging 1660,1662 into 1660.
    Updated 0 biblios
    Deleting 1662
    Merge done.
3 - Note how many records are merged
4 - Run again to confirm nothing is merged, i.e. the script removed the dupes
5 - Reset your DB
6 - Apply patch
7 - Run the script again
8 - Note entries no longer have duplicated ids:
    Merging 1662 into 1660.
9 - Note the same number of records are merged
10 - Run again and confirm nothing is merged
11 - Sign off!
Comment 2 David Nind 2025-10-25 00:48:35 UTC
Created attachment 188437 [details] [review]
Bug 41097: Don't use original record twice

This patch prevents us from passing the same record twice into the choose subs
to avoid a situation where the original record is not chosen as the one to keep
and we try to merge/delete the same record twice

To test:
1 - Run the script on a test system that you can reset
    perl misc/maintenance/dedup_authorities.pl -v -v --confirm -a="PERSO_NAME"
2 - Note duplication of entries in the output like:
    Merging 1660,1662 into 1660.
    Updated 0 biblios
    Deleting 1662
    Merge done.
3 - Note how many records are merged
4 - Run again to confirm nothing is merged, i.e. the script removed the dupes
5 - Reset your DB
6 - Apply patch
7 - Run the script again
8 - Note entries no longer have duplicated ids:
    Merging 1662 into 1660.
9 - Note the same number of records are merged
10 - Run again and confirm nothing is merged
11 - Sign off!

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2025-10-25 00:57:01 UTC
Testing notes (using KTD) - just noting my experience, have signed off:

Before the patch
==================

First run
---------

...
Processing authority 748 (360/650 55.38%)
    Merging 748,748 into 403.
    Updated 0 biblios
    Deleting 748
    Updated 0 biblios
    Deleting 748
{UNKNOWN}: Can't call method "move_to_deleted" on an undefined value at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 754. at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 757

Second run, still get merges
----------------------------

...
Processing authority 1568 (514/597 86.10%)
    Merging 1568,1568 into 403.
    Updated 0 biblios
    Deleting 1568
    Updated 0 biblios
    Deleting 1568
{UNKNOWN}: Can't call method "move_to_deleted" on an undefined value at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 754. at /kohadevbox/koha/C4/AuthoritiesMarc.pm line 757

Third run
---------

End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 6 authorities
No biblios to update

Fourth run
----------

End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 0 authorities
No biblios to update


After the patch - with a reset_all
==================================

...
Processing authority 1662 (631/650 97.08%)
    Merging 1660 into 1662.
    Updated 0 biblios
    Deleting 1660
    Merge done.
....
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 50 authorities
No biblios to update

After the patch - with a shutdown and startup of KTD
====================================================

First run
---------

...
Processing authority 1660 (532/650 81.85%)
    Merging 1662 into 1660.
    Updated 0 biblios
    Deleting 1662
    Merge done.
...
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 104 authorities
No biblios to update

Second run
----------

...
End of deduping for authtype 'PERSO_NAME'
Updated 0 biblios
Deleted 0 authorities
No biblios to update
Comment 4 Marcel de Rooy 2025-11-17 10:15:23 UTC
Looking at the code:

This seems to be the only call:
my ( $recordid_to_keep, @recordids_to_merge ) = _choose_records( $authority->authid, @recordids );

The POD says: returns first the record to merge to and list of records to merge from

The sub makes no distinction between $authority->authid and @recordids.
Note btw that @recordids is a BAD name; it contains MARC records !
sub _choose_records {
    my @recordids = @_;

It returns candidate authids (picked from 001) but they are sorted. Why should the original first argument still be first?
Shouldnt we add the first parameter (which is NOT a MARC record), not include that one while sorting and push it in front when returning values?

This needs a bit more attention to clarify current obscureness while touching those lines here.
Comment 5 Nick Clemens (kidclamp) 2026-01-29 20:44:27 UTC
(In reply to Marcel de Rooy from comment #4)
> Looking at the code:
> 
> This seems to be the only call:
> my ( $recordid_to_keep, @recordids_to_merge ) = _choose_records(
> $authority->authid, @recordids );
> 
> The POD says: returns first the record to merge to and list of records to
> merge from
> 
> The sub makes no distinction between $authority->authid and @recordids.
> Note btw that @recordids is a BAD name; it contains MARC records !
> sub _choose_records {
>     my @recordids = @_;
> 
> It returns candidate authids (picked from 001) but they are sorted. Why
> should the original first argument still be first?

It shouldn't - I think that was just a mistake

> Shouldn't we add the first parameter (which is NOT a MARC record), not
> include that one while sorting and push it in front when returning values?

The whole point of sorting is to determine the best candidate to merge into, and we don't know which record that is before we sort.

Julian wrote the original, I just got it through QA, so I can't say why we originally wanted the first record, but I think the testing can demonstrate we don't want the same record id  and to end up trying to merge into the same record

> This needs a bit more attention to clarify current obscureness while
> touching those lines here.

I welcome any additions to clarify the code, but I do think this fix is pretty striaghtforward