Bug 33404 - Authorities imported from Z39.50 in encodings other than UTF-8 are corrupted
Summary: Authorities imported from Z39.50 in encodings other than UTF-8 are corrupted
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on: 19436
Blocks: 33835
  Show dependency treegraph
 
Reported: 2023-04-04 13:18 UTC by Nick Clemens (kidclamp)
Modified: 2023-11-07 14:25 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08,22.05.17


Attachments
Bug 33404: (bug 19436 follow-up) Save the original encoded record (1.94 KB, patch)
2023-05-22 11:39 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33404: (bug 19436 follow-up) Save the original encoded record (2.04 KB, patch)
2023-05-24 11:23 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33404: Alternative approach (2.23 KB, patch)
2023-05-24 14:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33404: Alternative approach (2.24 KB, patch)
2023-05-24 15:30 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Sample record (2.23 KB, text/plain)
2023-05-24 15:32 UTC, Nick Clemens (kidclamp)
Details
Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding (4.55 KB, patch)
2023-05-25 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33404: Fix serverhost and init $page (2.15 KB, patch)
2023-05-25 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33404: Add unit tests (10.83 KB, patch)
2023-05-25 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding (4.57 KB, patch)
2023-05-31 12:50 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33404: Fix serverhost and init $page (2.17 KB, patch)
2023-05-31 12:50 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33404: Add unit tests (10.85 KB, patch)
2023-05-31 12:50 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding (4.64 KB, patch)
2023-07-04 12:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33404: Fix serverhost and init $page (2.24 KB, patch)
2023-07-04 12:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33404: Add unit tests (11.47 KB, patch)
2023-07-04 12:22 UTC, Jonathan Druart
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) 2023-04-04 13:18:46 UTC
Bug 19436 altered the call for storing imported auths in the DB:

before-
my ($notmarcrecord, $alreadyindb, $alreadyinfarm, $imported, $breedingid)= ImportBreedingAuth( $marcdata, 2, $serverhost[$k], $encoding[$k], $random, 'z3950' );

after-
my ($notmarcrecord, $alreadyindb, $alreadyinfarm, $imported, $breedingid)= ImportBreedingAuth( $marcrecord, 2, $serverhost[$k], $encoding[$k], $random);

Note the record passed changes from "marcdata" to "marcrecord"

Marcdata is the original raw record, so saving with original encoding makes sense

Marcrecord is the raw record converted to UTF-8 - but we save the original encoding
Comment 1 Nick Clemens (kidclamp) 2023-05-22 11:39:51 UTC
Created attachment 151510 [details] [review]
Bug 33404: (bug 19436 follow-up) Save the original encoded record

This patch restores previous behavior of storing the record as received
from the source with the original encoding. We store the encoding in the DB
as well, but save the record converted to UTF8 - this means that when we try
to display and convert to UTF8 again, we mangle the record

While providing a test plan, it requires a MARC8 Authority source, and I cannot locate
an open one. This change can be verified by reading the code.

There are no unit tests for Z3950SearchAuth, but it does too much and requires many mocks,
this is a small fix to a reversion and should be able to move forward.

To test:
1 - Search a Z39 authority server that has MARC8 (or non utf8) records
2 - Find a record with diacritics in the record
3 - Import it
4 - Note diacritics are mangled
5 - Apply patch, restart_all
6 - Perform a new Z3950 search (to ensure record is not already in reservoir)
7 - Find a record with diacritics
8 - Import it
9 - Note diacritics correctly encoded
Comment 2 David Nind 2023-05-22 20:39:01 UTC
Added assignee 8-)
Comment 3 Marcel de Rooy 2023-05-23 14:16:17 UTC
Looking here
Comment 4 Marcel de Rooy 2023-05-23 14:19:03 UTC
Can't locate object method "field" via package "00554cz  a2200157n  4500001001300000003000400013005001700017008004100034010001700075040001800092100006800110400006200178400005600240400005000296670005000346n  00027419 DLC20000914083028.0000914n| acannaabn          |a aaa        an  00027419   aDLCbengcDLC1 aHacikyan, A. J.q(Agop Jack),d1931-tEté sans aube.lEnglish1 aHacikyan, A. J.q(Agop Jack),d1931-tSummer without dawn1 aSoucy, Jean Yves,d1945-tEté sans aube.lEnglish1 aSoucy, Jean Yves,d1945-tSummer without dawn  aHacikyan, A. J. A summer without dawn, c2000." (perhaps you forgot to load "00554cz  a2200157n  4500001001300000003000400013005001700017008004100034010001700075040001800092100006800110400006200178400005600240400005000296670005000346n  00027419 DLC20000914083028.0000914n| acannaabn          |a aaa        an  00027419   aDLCbengcDLC1 aHacikyan, A. J.q(Agop Jack),d1931-tEté sans aube.lEnglish1 aHacikyan, A. J.q(Agop Jack),d1931-tSummer without dawn1 aSoucy, Jean Yves,d1945-tEté sans aube.lEnglish1 aSoucy, Jean Yves,d1945-tSummer without dawn  aHacikyan, A. J. A summer without dawn, c2000."?) at /usr/share/koha/C4/Breeding.pm line 479
    in C4::Breeding::ImportBreedingAuth at /usr/share/koha/C4/Breeding.pm line 479

$marcdata is no marc record ! It is a marc blob.

How did you manage to test it?
Comment 5 Marcel de Rooy 2023-05-23 14:21:34 UTC
The changes do not look good to me. We should not save in other encodings. Everywhere we convert to UTF-8. Here we should too.

When I compare BIB with AUTH in Z3950Search, we also save bib records in UTF8.
Note that there are some subtle differences. For instance the moment when we call SetUTF8Flag.

This needs more attention.
Comment 6 Marcel de Rooy 2023-05-23 14:23:10 UTC
(In reply to Nick Clemens from comment #1)
> While providing a test plan, it requires a MARC8 Authority source, and I
> cannot locate
> an open one. This change can be verified by reading the code.

This sounds problematic.

> There are no unit tests for Z3950SearchAuth, but it does too much and
> requires many mocks,
> this is a small fix to a reversion and should be able to move forward.

And this too :)
Comment 7 Nick Clemens (kidclamp) 2023-05-24 11:23:45 UTC
Created attachment 151624 [details] [review]
Bug 33404: (bug 19436 follow-up) Save the original encoded record

This patch restores previous behavior of storing the record as received
from the source with the original encoding. We store the encoding in the DB
as well, but save the record converted to UTF8 - this means that when we try
to display and convert to UTF8 again, we mangle the record

While providing a test plan, it requires a MARC8 Authority source, and I cannot locate
an open one. This change can be verified by reading the code.

There are no unit tests for Z3950SearchAuth, but it does too much and requires many mocks,
this is a small fix to a reversion and should be able to move forward.

To test:
1 - Search a Z39 authority server that has MARC8 (or non utf8) records
2 - Find a record with diacritics in the record
3 - Import it
4 - Note diacritics are mangled
5 - Apply patch, restart_all
6 - Perform a new Z3950 search (to ensure record is not already in reservoir)
7 - Find a record with diacritics
8 - Import it
9 - Note diacritics correctly encoded
Comment 8 Nick Clemens (kidclamp) 2023-05-24 11:28:05 UTC
(In reply to Marcel de Rooy from comment #5)
> The changes do not look good to me. We should not save in other encodings.
> Everywhere we convert to UTF-8. Here we should too.

I am happy to go this direction if preferred


(In reply to Marcel de Rooy from comment #4)
> $marcdata is no marc record ! It is a marc blob.
> 
> How did you manage to test it?

I forgot to commit changes after testing :-) New patch retested and verified against OCLC MARC8 records

(In reply to Marcel de Rooy from comment #6)
> (In reply to Nick Clemens from comment #1)
> > While providing a test plan, it requires a MARC8 Authority source, and I
> > cannot locate
> > an open one. This change can be verified by reading the code.
> 
> This sounds problematic.

If you can find a source I would appreciate it

> > There are no unit tests for Z3950SearchAuth, but it does too much and
> > requires many mocks,
> > this is a small fix to a reversion and should be able to move forward.
> 
> And this too :)

Yes, but we should not allow bugs to remain when we have a fix, but not the time to pay technical debts.
Comment 9 Marcel de Rooy 2023-05-24 14:58:55 UTC
Created attachment 151636 [details] [review]
Bug 33404: Alternative approach

Work in progress
Comment 10 Nick Clemens (kidclamp) 2023-05-24 15:30:46 UTC
Created attachment 151641 [details] [review]
Bug 33404: Alternative approach

Work in progress

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Nick Clemens (kidclamp) 2023-05-24 15:32:07 UTC
This (In reply to Marcel de Rooy from comment #9)
> Created attachment 151636 [details] [review] [review]
> Bug 33404: Alternative approach
> 
> Work in progress

This works better - sometimes mine worked, and then sometimes I was losing the heading fields, not sure why.

I would attach a sample MARC-8 - but I can't get one out of the system without it being mangled - I exported the record I used for testing as marc 8 and will attach if that helps
Comment 12 Nick Clemens (kidclamp) 2023-05-24 15:32:57 UTC
Created attachment 151644 [details]
Sample record
Comment 13 Marcel de Rooy 2023-05-25 05:33:05 UTC
(In reply to Nick Clemens from comment #12)
> Created attachment 151644 [details]
> Sample record

Thx. Will try to incorporate in a test.
Comment 14 Marcel de Rooy 2023-05-25 12:27:30 UTC
Created attachment 151672 [details] [review]
Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding

Test plan:
[1] If you have access to a Z3950 MARC8 auth server, search
    for an authority record and import it.
[2] If you have access to a Z3950 UTF8 auth server, search
    for an authority record and import it.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2023-05-25 12:27:33 UTC
Created attachment 151673 [details] [review]
Bug 33404: Fix serverhost and init $page

The array serverhost is not filled. Should be replaced with values
from servers array.

Test plan:
Nothing exciting here. Read the patch.
Note that we will test in the next patch if the hostname is saved
correctly in the import batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2023-05-25 12:27:36 UTC
Created attachment 151674 [details] [review]
Bug 33404: Add unit tests

Test plan:
Run t/db_dependent/Breeding.t
Run t/db_dependent/Breeding_Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2023-05-25 12:32:26 UTC
Note for QA: The Z3950SearchAuth test can be extended further later on. And used as a basis for Z3950Search too. We need to consolidate the duplicated code.
Comment 18 Marcel de Rooy 2023-05-25 12:33:20 UTC
(In reply to Marcel de Rooy from comment #13)
> (In reply to Nick Clemens from comment #12)
> > Created attachment 151644 [details]
> > Sample record
> 
> Thx. Will try to incorporate in a test.

Nick, there is an issue with this sample record. I could import it but I could not use it in my test. I have the impression that there is an issue with exporting authority records from Koha in MARC8?
Comment 19 Marcel de Rooy 2023-05-26 06:04:14 UTC
Raising sev to major
Comment 20 Nick Clemens (kidclamp) 2023-05-31 12:50:02 UTC
Created attachment 151868 [details] [review]
Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding

Test plan:
[1] If you have access to a Z3950 MARC8 auth server, search
    for an authority record and import it.
[2] If you have access to a Z3950 UTF8 auth server, search
    for an authority record and import it.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens (kidclamp) 2023-05-31 12:50:04 UTC
Created attachment 151869 [details] [review]
Bug 33404: Fix serverhost and init $page

The array serverhost is not filled. Should be replaced with values
from servers array.

Test plan:
Nothing exciting here. Read the patch.
Note that we will test in the next patch if the hostname is saved
correctly in the import batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens (kidclamp) 2023-05-31 12:50:07 UTC
Created attachment 151870 [details] [review]
Bug 33404: Add unit tests

Test plan:
Run t/db_dependent/Breeding.t
Run t/db_dependent/Breeding_Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Jonathan Druart 2023-07-04 12:22:29 UTC
Created attachment 153014 [details] [review]
Bug 33404: Z3950SearchAuth: Save in UTF-8 encoding

Test plan:
[1] If you have access to a Z3950 MARC8 auth server, search
    for an authority record and import it.
[2] If you have access to a Z3950 UTF8 auth server, search
    for an authority record and import it.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2023-07-04 12:22:33 UTC
Created attachment 153015 [details] [review]
Bug 33404: Fix serverhost and init $page

The array serverhost is not filled. Should be replaced with values
from servers array.

Test plan:
Nothing exciting here. Read the patch.
Note that we will test in the next patch if the hostname is saved
correctly in the import batch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2023-07-04 12:22:36 UTC
Created attachment 153016 [details] [review]
Bug 33404: Add unit tests

Test plan:
Run t/db_dependent/Breeding.t
Run t/db_dependent/Breeding_Auth.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Amended patch: perltidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Tomás Cohen Arazi 2023-07-05 15:19:25 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 27 Martin Renvoize 2023-07-17 15:35:44 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 28 Matt Blenkinsop 2023-07-18 13:28:13 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 29 Lucas Gass 2023-11-07 14:25:26 UTC
Backported to 22.05.x for upcoming 22.05.17 release.