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!
Created attachment 21520 [details] [review] 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!
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.
Also, as with any change to a routine in the C4 or Koha namespace, please provide a regression test.
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.
Created attachment 41130 [details] [review] Bug 10961 - (Followup) Error in GetMarcBiblio can cause severe data loss add correct frameworkcode to _koha_marc_update_bib_ids parameters add test, prove with : prove t/db_dependent/Biblio.t
*** Bug 14596 has been marked as a duplicate of this bug. ***
Created attachment 41182 [details] [review] 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>
Created attachment 41183 [details] [review] Bug 10961 - (Followup) Error in GetMarcBiblio can cause severe data loss 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>
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>
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>
Patches pushed to master. Thanks Kyle and Stephane!
Pushed to 3.20.x will be in 3.20.3
Pushed to 3.18.x, will be in 3.18.10.
Pushed to 3.16.x, will be in 3.16.14