Bug 10961 - Error in GetMarcBiblio can cause severe data loss
Summary: Error in GetMarcBiblio can cause severe data loss
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 14596 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-09-27 16:53 UTC by Kyle M Hall
Modified: 2016-12-05 21:23 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 10961 - Error in GetMarcBiblio can cause severe data loss (2.08 KB, patch)
2013-09-27 16:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10961 - (Followup) Error in GetMarcBiblio can cause severe data loss (3.32 KB, patch)
2015-07-23 14:49 UTC, stephane.delaune
Details | Diff | Splinter Review
Bug 10961 - Error in GetMarcBiblio can cause severe data loss (2.16 KB, patch)
2015-07-24 22:27 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10961 - (Followup) Error in GetMarcBiblio can cause severe data loss (3.96 KB, patch)
2015-07-24 22:27 UTC, M. Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 10961: Error in GetMarcBiblio can cause severe data loss (2.23 KB, patch)
2015-07-28 13:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[PASSED QA] Bug 10961: (followup) Make query fields explicit and add regression tests (4.07 KB, patch)
2015-07-28 13:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-09-27 16:53:28 UTC
A bug in GetMarcBiblio can cause severe data loss if your database has records where the biblionumber and biblioitemnumber do not match and the script misc/batchRebuildBiblioTables.pl is run. 

The Biblio::GetMarcBiblio makes a kall to  C4::Biblio::_koha_marc_update_bib_ids which passes the biblionumber as both the biblionumber *and the biblioitemnumber*.

Thus, if your biblio and biblioitem numbers are not always equal, you will end up with a record where the biblioitemnumber is incorrect in the record!

This is usually not a severe issue, but since batchRebuildBiblioTables uses that record to update the database tables, it ends up updating the wrong biblioitem row!
Comment 1 Kyle M Hall 2013-09-27 16:54:14 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-09-27 17:11:31 UTC
Comment on attachment 21520 [details] [review]
Bug 10961 - Error in GetMarcBiblio can cause severe data loss

Review of attachment 21520 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +1252,4 @@
>      my $biblionumber = shift;
>      my $embeditems   = shift || 0;
>      my $dbh          = C4::Context->dbh;
> +    my $sth          = $dbh->prepare("SELECT * FROM biblioitems WHERE biblionumber=? ");

You don't need * here, just "marcxml, biblioitemnumber".  Since biblioitems also contains the marc column, select * just causes unnecessary network traffic for remote DB servers.
Comment 3 Galen Charlton 2013-09-27 17:16:39 UTC
Also, as with any change to a routine in the C4 or Koha namespace, please provide a regression test.
Comment 4 Galen Charlton 2013-09-27 17:23:32 UTC
Also, while the bug in GetMarcBiblio() is real, consideration should be given to removing misc/batchRebuildBiblioTables.pl for the following reasons:

[1] It calls internal routines in C4::Biblio directly in the name of performance, but doing so is the direct cause of the bug.
[2] misc/maintenance/touch_all_biblios.pl will do the same thing, but does not call private routines in C4::Biblio.
Comment 5 stephane.delaune 2015-07-23 14:49:52 UTC Comment hidden (obsolete)
Comment 6 stephane.delaune 2015-07-23 14:51:40 UTC
*** Bug 14596 has been marked as a duplicate of this bug. ***
Comment 7 M. Tompsett 2015-07-24 22:27:20 UTC Comment hidden (obsolete)
Comment 8 M. Tompsett 2015-07-24 22:27:28 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2015-07-28 13:10:17 UTC
Created attachment 41214 [details] [review]
[PASSED QA] Bug 10961: Error in GetMarcBiblio can cause severe data loss

A bug in GetMarcBiblio can cause severe data loss if your database has
records where the biblionumber and biblioitemnumber do not match and the
script misc/batchRebuildBiblioTables.pl is run.

The Biblio::GetMarcBiblio makes a kall to
C4::Biblio::_koha_marc_update_bib_ids which passes the biblionumber as
both the biblionumber *and the biblioitemnumber*.

Thus, if your biblio and biblioitem numbers are not always equal, you
will end up with a record where the biblioitemnumber is incorrect in the
record!

This is usually not a severe issue, but since batchRebuildBiblioTables
uses that record to update the database tables, it ends up updating the
wrong biblioitem row!

NOTE: What a horrible, horrible typo that was. Tested this with the
      second patch.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 10 Tomás Cohen Arazi 2015-07-28 13:10:26 UTC
Created attachment 41215 [details] [review]
[PASSED QA] Bug 10961: (followup) Make query fields explicit and add regression tests

add correct frameworkcode to _koha_marc_update_bib_ids parameters

add test, prove with : prove t/db_dependent/Biblio.t

TEST PLAN
---------
1) git checkout -b bug_10961 origin/master
2) git bz apply 10961
3) git checkout origin/master -- C4/Biblio.pm
4) prove t/db_dependent/Biblio.t
   -- was expecting failure, got failure.
5) git reset --hard origin/master
6) git bz apply 10961
7) prove t/db_dependent/Biblio.t
   -- success as expected.
8) Read over code.
   -- Noted it also grabs the framework code for the biblio, rather than uses default.
      And it also corrects an indentation issue.
      Test case looks like it attempts to cover the biblionumber!=biblioitemnumber case
      by adding 1.
9) run koha qa test tools.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 11 Tomás Cohen Arazi 2015-07-28 13:37:56 UTC
Patches pushed to master.

Thanks Kyle and Stephane!
Comment 12 Chris Cormack 2015-08-10 21:08:38 UTC
Pushed to 3.20.x will be in 3.20.3
Comment 13 Liz Rea 2015-08-11 02:06:43 UTC
Pushed to 3.18.x, will be in 3.18.10.
Comment 14 Mason James 2015-08-28 06:05:03 UTC
Pushed to 3.16.x, will be in 3.16.14