Bug 5683 - link_bibs_to_authorities.pl can corrupt records
Summary: link_bibs_to_authorities.pl can corrupt records
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: 3.4
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Assignee: Jared Camins-Esakov
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 20522
  Show dependency treegraph
 
Reported: 2011-02-02 10:47 UTC by Jared Camins-Esakov
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

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


Attachments
Signed off Patch (1.17 KB, patch)
2011-02-02 16:55 UTC, Ian Walls
Details | Diff | Splinter Review
Follow-up patch (1 of 2) (1.99 KB, patch)
2011-04-08 12:05 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Follow-up patch (2 of 2) (1.94 KB, patch)
2011-04-08 12:05 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Signed off follow up 1 (1.96 KB, patch)
2011-05-13 02:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Signed off follow up 2 (2.01 KB, patch)
2011-05-13 02:10 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jared Camins-Esakov 2011-02-02 10:47:40 UTC
It seems that running link_bibs_to_authorities.pl can corrupt records in cases where there is a heading with only subfield 9 populated, since the script automatically removes all subfield 9s. The script should check that all fields it outputs have at least subfield, and delete and fields that don't have any subfields.
Comment 1 Ian Walls 2011-02-02 16:55:14 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-02-02 20:38:12 UTC
This passes its tests
perl t/db_dependent/Biblio.t 
1..6
ok 1 - use C4::Biblio;
ok 2 - Makes sure title field in biblio is empty.
ok 3 - uses ModBiblio to add a title to the previously created record and checks that its there.
ok 4 - Makes sure the isbn is still there after using ModBiblio.
ok 5 - First test of GetBiblioItemData to get same result of previous two GetBiblioData tests.
ok 6 - Second test checking it returns the correct isbn.

Pushed, and reclassified to be a bugfix not an enhancement

(otherwise it will not be pulled for 3.2.x)

Please test.
Comment 3 Jared Camins-Esakov 2011-02-24 15:29:55 UTC
This particular issue is fixed in Master.
Comment 4 Jared Camins-Esakov 2011-04-06 11:30:07 UTC
It turns out that this particular type of corruption is not limited to C4::Biblio. It is also possible to corrupt records in C4::Charset.
Comment 5 Jared Camins-Esakov 2011-04-06 14:16:25 UTC
There are two follow-up patches that need sign off. Both have been sent to the list.
Comment 6 Stéphane Delaune 2011-04-07 09:25:23 UTC
perl t/db_dependent/Biblio.t 
1..6
ok 1 - use C4::Biblio;
ok 2 - Makes sure title field in biblio is empty.
This does not passes its tests
not ok 3 - uses ModBiblio to add a title to the previously created record and checks that its there.
#   Failed test 'uses ModBiblio to add a title to the previously created record and checks that its there.'
#   at t/db_dependent/Biblio.t line 30.
#          got: undef
#     expected: 'Foundation'
not ok 4 - Makes sure the isbn is still there after using ModBiblio.
#   Failed test 'Makes sure the isbn is still there after using ModBiblio.'
#   at t/db_dependent/Biblio.t line 31.
#          got: undef
#     expected: '0590353403'
not ok 5 - First test of GetBiblioItemData to get same result of previous two GetBiblioData tests.
#   Failed test 'First test of GetBiblioItemData to get same result of previous two GetBiblioData tests.'
#   at t/db_dependent/Biblio.t line 34.
#          got: undef
#     expected: 'Foundation'
not ok 6 - Second test checking it returns the correct isbn.
#   Failed test 'Second test checking it returns the correct isbn.'
#   at t/db_dependent/Biblio.t line 35.
#          got: undef
#     expected: '0590353403'
# Looks like you failed 4 tests of 6.
Comment 7 Jared Camins-Esakov 2011-04-07 12:25:06 UTC
Which patch were you testing? The first follow-up? I can't duplicate the results of your test.
Comment 8 Stéphane Delaune 2011-04-07 14:52:02 UTC
(In reply to comment #7)
> Which patch were you testing? The first follow-up? I can't duplicate the
> results of your test.

I was test this patch : http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=3053
Comment 9 Jared Camins-Esakov 2011-04-07 14:56:35 UTC
Stephane,

That patch has already been pushed. The two follow-up patches that need signing off were sent to the list.

Regards,
Jared
Comment 10 Jared Camins-Esakov 2011-04-08 12:05:14 UTC Comment hidden (obsolete)
Comment 11 Jared Camins-Esakov 2011-04-08 12:05:49 UTC Comment hidden (obsolete)
Comment 12 Jared Camins-Esakov 2011-04-08 12:06:42 UTC
I have attached the two follow-up patches to the bug. Sorry about the confusion.
Comment 13 Chris Cormack 2011-04-11 21:54:22 UTC
Can someone please test the 2 follow up bugs, there are 3 critical bugs for 3.4 needing sign off, this is one of them
Comment 14 Jared Camins-Esakov 2011-04-15 21:25:44 UTC
For testing it may be better to apply the second follow-up patch first (and run perl t/db_dependent/Biblio.t), to confirm the incorrect behavior exists, and then apply the first follow-up patch to fix it (and rerun the test).
Comment 15 Chris Cormack 2011-05-13 02:08:24 UTC
Applied the patch containing the updated tests, ran the test

rorohiko:[git/5683-]:~/git/koha% perl t/db_dependent/Biblio.t
1..9
ok 1 - use C4::Biblio;
DBD::mysql::db do failed: Unknown or incorrect time zone: 'Pacific/Auckland' at C4/Context.pm line 700, <DATA> line 496.
ok 2 - Makes sure title field in biblio is empty.
ok 3 - uses ModBiblio to add a title to the previously created record and checks that its there.
ok 4 - Makes sure the isbn is still there after using ModBiblio.
ok 5 - First test of GetBiblioItemData to get same result of previous two GetBiblioData tests.
ok 6 - Second test checking it returns the correct isbn.
ok 7 - ModBiblio handles authority-linked 655
ok 8 - ModBiblio handles 655 with authority link but no heading
# Field 655 must have indicators (use ' ' for empty indicators) at C4/Charset.pm line 141
not ok 9 - ModBiblio handles 655 with no subfields
#   Failed test 'ModBiblio handles 655 with no subfields'
#   at t/db_dependent/Biblio.t line 70.
# Looks like you failed 1 test of 9.

Applied the patch to fix it
rorohiko:[git/5683-]:~/git/koha% perl t/db_dependent/Biblio.t
1..9                                                         
ok 1 - use C4::Biblio;
DBD::mysql::db do failed: Unknown or incorrect time zone: 'Pacific/Auckland' at C4/Context.pm line 700, <DATA> line 496.
ok 2 - Makes sure title field in biblio is empty.
ok 3 - uses ModBiblio to add a title to the previously created record and checks that its there.
ok 4 - Makes sure the isbn is still there after using ModBiblio.
ok 5 - First test of GetBiblioItemData to get same result of previous two GetBiblioData tests.
ok 6 - Second test checking it returns the correct isbn.
ok 7 - ModBiblio handles authority-linked 655
ok 8 - ModBiblio handles 655 with authority link but no heading
ok 9 - ModBiblio handles 655 with no subfield

Patch fixes the issue, signing off
Comment 16 Chris Cormack 2011-05-13 02:10:07 UTC
Created attachment 4093 [details] [review]
Signed off follow up 1
Comment 17 Chris Cormack 2011-05-13 02:10:40 UTC
Created attachment 4094 [details] [review]
Signed off follow up 2
Comment 18 Ian Walls 2011-06-01 14:14:04 UTC
Applied patch 1, tested:  tests failed.  Applied patch 2, tested:  tests succeeded.  Marking as Passed QA
Comment 19 Chris Cormack 2011-06-01 21:13:23 UTC
Pushed to master
Comment 20 Jared Camins-Esakov 2011-07-08 14:37:02 UTC
Seems to be working now. Closing.
Comment 21 Paul A 2012-01-06 20:46:55 UTC
This is NOT resolved; it can lead to *un-linking* all biblio-authority links under some circumstances, with only restore from backup as a work-around.

stdout:

./bin/link_bibs_to_authorities.pl
/.../
Bib authority heading linking report
------------------------------------
Number of bibs checked:       14911
Number of bibs modified:      14886
Number of bibs with errors:   0

"modified" == removed.
Comment 22 Jared Camins-Esakov 2012-01-06 20:49:03 UTC
(In reply to comment #21)
> This is NOT resolved; it can lead to *un-linking* all biblio-authority links
> under some circumstances, with only restore from backup as a work-around.
> 
> stdout:
> 
> ./bin/link_bibs_to_authorities.pl
> /.../
> Bib authority heading linking report
> ------------------------------------
> Number of bibs checked:       14911
> Number of bibs modified:      14886
> Number of bibs with errors:   0
> 
> "modified" == removed.

Please see bug 7284 for a fix for that.  This particular bug is about record corruption due to removing a subfield $9 when that is the only subfield in the field.