Bug 35591 - bulkmarcimport.pl overwrites data if controlnumber eq authid
Summary: bulkmarcimport.pl overwrites data if controlnumber eq authid
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Thomas Klausner
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-12-18 15:54 UTC by Thomas Klausner
Modified: 2024-04-02 23:07 UTC (History)
2 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
handcrafted auth marcxml with controlnumber 1402 (5.68 KB, text/xml)
2024-01-23 15:54 UTC, Thomas Klausner
Details
auth_1402_pre_import.png - screenshot before import (78.72 KB, image/png)
2024-01-23 15:55 UTC, Thomas Klausner
Details
auth_1402_post_import.png - screenshot after import (112.10 KB, image/png)
2024-01-23 15:55 UTC, Thomas Klausner
Details
Bug 35591: Prevent bulkmarcimport from replacing local auth_header (3.72 KB, patch)
2024-01-23 16:37 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 35591: Prevent bulkmarcimport from replacing local auth_header (3.92 KB, patch)
2024-02-25 17:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Klausner 2023-12-18 15:54:04 UTC
We had a lot of auth_header (~700.000). Then we decided to import a lot more (10.000.000). We used --update together with a match-rule (based on 024) to prevent duplicates. Which all worked well.

Until...

We discovered that ~30k of our old auths where overwritten with "random" data (some other auths). What used to be "Graz" was now "Centro Editoriale Valtortiano". After a bit of panic we were able to restore our old auths from a backup.

But what happend?

The problem is that eg "Graz" had authid=777317. But "Valtortiano" has 001=00777317X. If you squint a bit (and ignore the leading 0s and the X at the end) that's the same number!

And it seems that C4::AuthoritiesMarc::GuessAuthId($record) returns the value of 001, which somehow causes the confusion. I'm not entirely sure yet, where the X is stripped, but the effect is that auth-record 777317 is overwritten.

An easy fix would be to add a new command line param to bulkmarcimport to either disable the call to GuessAuthId (which keeps with backwards compat, but makes it easy for other people to also run into this problem); or to only call GuessAuthId if the new param is set (saver, but changes behaviour)

If you agree that this is a problem, I can prepare a patch.

If we did something stupid, please enlighten us!
Comment 1 Katrin Fischer 2023-12-30 20:20:43 UTC
Overall this doesn't sound like intended behavior. 

Koha did not always replace 001 with the authid, in the distant past it kept the imported 001. I could imagine the behavior made more sense back then, but does much less now.

I see 2 different bugs here:
* It breaks importing for everyone that imports data with an existing 001 that is somewhat numerical and can clash with the authids in Koha that are added to 001 on import.
* The guess is not even exact. I assume this is because of using the search index, but maybe there should be an additional Perl check if they are really equivalent before overlay?

I would not be opposed to change the behavior here or make it a legacy switch that needs to be chosen actively. The current behavior appears quite dangerous.

Some other thoughts:
Could it make sense to forgo GuessAuthId when another match-rule is used? 
And maybe have a separate bug for making the match more exact/smarter.
Comment 2 Thomas Klausner 2024-01-23 15:54:16 UTC
Created attachment 161303 [details]
handcrafted auth marcxml with controlnumber 1402
Comment 3 Thomas Klausner 2024-01-23 15:55:00 UTC
Created attachment 161304 [details]
auth_1402_pre_import.png - screenshot before import
Comment 4 Thomas Klausner 2024-01-23 15:55:23 UTC
Created attachment 161305 [details]
auth_1402_post_import.png - screenshot after import
Comment 5 Thomas Klausner 2024-01-23 15:55:45 UTC
Here's some more info and tests for this bug:

* twin_040682714.xml is this GND record http://d-nb.info/gnd/4068271-7 ("Zwilling", i.e. twin), but modified to have <controlfield tag="001">1402</controlfield>, which matches the authid 1402 "Aristotle" in koha-testing-docker

* auth_1402_pre_import.png is a screenshot of authid 1402 from koha-testing-docker

* copy twin_040682714.xml to the root of your koha-testing-docker (so it will be available inside the container)

* run: misc/migration_tools/bulkmarcimport.pl -m MARCXML -c utf8 -a --file twin_040682714.xml --all --match isbn,001

(you can also run with --match, makes no difference, as long as --match does not actually hit a match)

* auth_1402_post_import.png is a screenshot of authid 1402 after running bulkmarcimport, showing that the authid 1402 now contains the data from twin_040682714.xml
Comment 6 Thomas Klausner 2024-01-23 16:37:36 UTC
Created attachment 161306 [details] [review]
Bug 35591: Prevent bulkmarcimport from replacing local auth_header

If you run bulkmarcimport,pl on authorities with --update (or --all)
it will overwrite a local auth_header if the auth_id of a local
auth_header matches the control-number (field 001) of an imported MARC
record.

This patch adds a new option --guessauthid (which is still enabled by
default to keep the script backwards compatible, but broken), which can
turn off this behaviour if called as --noguessauthid.

To test with koha-testing-docker:

* Inspect cgi-bin/koha/authorities/detail.pl?authid=1402 which should be
  PERSO_NAME of Aristotles.
* get the file twin_040682714.xml from Bugzilla and make it available
  inside of koha-testing-docker
* run:
  misc/migration_tools/bulkmarcimport.pl -m MARCXML -c utf8 -a --file twin_040682714.xml --all --match isbn,001
* Check cgi-bin/koha/authorities/detail.pl?authid=1402, which now is a
  TOPIC_TERM "Zwilling" (twin...)

Reset koha-testing-docker, apply the patch

* run with the new flag
  misc/migration_tools/bulkmarcimport.pl -m MARCXML -c utf8 -a --file twin_040682714.xml --all --match isbn,001 --noguessauthid

* cgi-bin/koha/authorities/detail.pl?authid=1402 still is "Aristotle"

* "Zwilling" is cgi-bin/koha/authorities/detail.pl?authid=1708 (at least
  on my ktd, the actual id might vary)
Comment 7 Thomas Klausner 2024-01-23 16:38:35 UTC
Sponsored-by: Steiermärkische Landesbibliothek
Comment 8 Victor Grousset/tuxayo 2024-02-25 17:47:31 UTC
Created attachment 162409 [details] [review]
Bug 35591: Prevent bulkmarcimport from replacing local auth_header

If you run bulkmarcimport,pl on authorities with --update (or --all)
it will overwrite a local auth_header if the auth_id of a local
auth_header matches the control-number (field 001) of an imported MARC
record.

This patch adds a new option --guessauthid (which is still enabled by
default to keep the script backwards compatible, but broken), which can
turn off this behaviour if called as --noguessauthid.

To test with koha-testing-docker:

* Inspect cgi-bin/koha/authorities/detail.pl?authid=1402 which should be
  PERSO_NAME of Aristotles.
* get the file twin_040682714.xml from Bugzilla and make it available
  inside of koha-testing-docker
* run:
  misc/migration_tools/bulkmarcimport.pl -m MARCXML -c utf8 -a --file twin_040682714.xml --all --match isbn,001
* Check cgi-bin/koha/authorities/detail.pl?authid=1402, which now is a
  TOPIC_TERM "Zwilling" (twin...)

Reset koha-testing-docker, apply the patch

* run with the new flag
  misc/migration_tools/bulkmarcimport.pl -m MARCXML -c utf8 -a --file twin_040682714.xml --all --match isbn,001 --noguessauthid

* cgi-bin/koha/authorities/detail.pl?authid=1402 still is "Aristotle"

* "Zwilling" is cgi-bin/koha/authorities/detail.pl?authid=1708 (at least
  on my ktd, the actual id might vary)

Sponsored-by: Steiermärkische Landesbibliothek
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 9 Victor Grousset/tuxayo 2024-02-25 17:47:50 UTC
It works :)
Comment 10 Victor Grousset/tuxayo 2024-02-25 17:54:22 UTC
Well done, it's a nasty bug.

(In reply to Katrin Fischer from comment #1)
> I would not be opposed to change the behavior here or make it a legacy
> switch that needs to be chosen actively. The current behavior appears quite
> dangerous.

+1 for legacy switch. The current patch is a fix switch. Default is still dangerous.
Comment 11 Nick Clemens (kidclamp) 2024-04-01 11:37:29 UTC
I believe bug 29440 fixed or altered this - the patch doesn't apply, nor do I recreate the behaviour following the test plan - GuessAuthId is no longer used in the script
Comment 12 Victor Grousset/tuxayo 2024-04-02 23:07:47 UTC
Confirmed!

I can reproduce on the commit before bug 29440.
And I can't with the commits of bug 29440.

Is there a worry to have about the broken legacy behavior that is "lost".