Bugzilla – Attachment 73899 Details for
Bug 20549
Avoid indexer race condition when moving item
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
WR 289187 - Avoid biblio index race condition after moving item
WR-289187---Avoid-biblio-index-race-condition-afte.patch (text/plain), 10.06 KB, created by
Alex Buckley
on 2018-04-10 04:32:24 UTC
(
hide
)
Description:
WR 289187 - Avoid biblio index race condition after moving item
Filename:
MIME Type:
Creator:
Alex Buckley
Created:
2018-04-10 04:32:24 UTC
Size:
10.06 KB
patch
obsolete
>From 88179dfb194067f1f151e62210cce798216a081e Mon Sep 17 00:00:00 2001 >From: Alex Buckley <alexbuckley@catalyst.net.nz> >Date: Mon, 9 Apr 2018 08:59:41 +0000 >Subject: [PATCH] WR 289187 - Avoid biblio index race condition after moving > item > >The moveitem.pl script loops through a 2 value array containing "new" >and "old". The first loop value "new" is handed as an optional argument >to the MoveItemFromBiblio() function in C4/Items.pm. This updates the >biblios and only reindexes the new biblio. > >If a 1 if returned from the call to MoveItemFromBilio() to moveitem.pl >then the loop iterates through the second value in the array "old" this >calls the MoveItemFromBiblio() function as well but this time the >aforementioned function only does one thing which is add the old biblio >to the zebraqueue. > >Due to calling the ModItemFromBiblio() function twice from two different >iterations of the loop in moveitem.pl there is a 1 second gap between >the new biblio and old biblio being added to the zebraqueue table. > >This reduces the chances of a race condition of the old biblio being >indexed before the new biblio. > >https://bugs.koha-community.org/show_bug.cgi?id=20549 >--- > C4/Items.pm | 93 +++++++++++++++++++++---------- > cataloguing/moveitem.pl | 50 ++++++++++------- > t/db_dependent/Items/MoveItemFromBiblio.t | 10 ++-- > 3 files changed, 101 insertions(+), 52 deletions(-) > >diff --git a/C4/Items.pm b/C4/Items.pm >index e7279ff..7abb74b 100644 >--- a/C4/Items.pm >+++ b/C4/Items.pm >@@ -1863,7 +1863,7 @@ sub _koha_new_item { > > =head2 MoveItemFromBiblio > >- MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio); >+ MoveItemFromBiblio($itenumber, $frombiblio, $tobiblio, $bibliotoindex); > > Moves an item from a biblio to another > >@@ -1879,36 +1879,73 @@ sub MoveItemFromBiblio { > FROM biblioitems > WHERE biblionumber = ? > |, undef, $tobiblio ); >- my $return = $dbh->do(q| >- UPDATE items >- SET biblioitemnumber = ?, >- biblionumber = ? >- WHERE itemnumber = ? >- AND biblionumber = ? >- |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio ); >- if ($return == 1) { >- ModZebra( $tobiblio, "specialUpdate", "biblioserver" ); >- ModZebra( $frombiblio, "specialUpdate", "biblioserver" ); >- # Checking if the item we want to move is in an order >- require C4::Acquisition; >- my $order = C4::Acquisition::GetOrderFromItemnumber($itemnumber); >- if ($order) { >- # Replacing the biblionumber within the order if necessary >- $order->{'biblionumber'} = $tobiblio; >- C4::Acquisition::ModOrder($order); >- } > >- # Update reserves, hold_fill_targets, tmp_holdsqueue and linktracker tables >- for my $table_name ( qw( reserves hold_fill_targets tmp_holdsqueue linktracker ) ) { >- $dbh->do( qq| >- UPDATE $table_name >- SET biblionumber = ? >+ if ( defined $_[3]) { >+ my $bibliotoindex = $_[3]; >+ if ($bibliotoindex eq "new") { >+ my $return = $dbh->do(q| >+ UPDATE items >+ SET biblioitemnumber = ?, >+ biblionumber = ? > WHERE itemnumber = ? >- |, undef, $tobiblio, $itemnumber ); >+ AND biblionumber = ? >+ |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio ); >+ if ($return == 1) { >+ ModZebra( $tobiblio, "specialUpdate", "biblioserver" ); >+ # Checking if the item we want to move is in an order >+ require C4::Acquisition; >+ my $order = C4::Acquisition::GetOrderFromItemnumber($itemnumber); >+ if ($order) { >+ # Replacing the biblionumber within the order if necessary >+ $order->{'biblionumber'} = $tobiblio; >+ C4::Acquisition::ModOrder($order); >+ } >+ >+ # Update reserves, hold_fill_targets, tmp_holdsqueue and linktracker tables >+ for my $table_name ( qw( reserves hold_fill_targets tmp_holdsqueue linktracker ) ) { >+ $dbh->do( qq| >+ UPDATE $table_name >+ SET biblionumber = ? >+ WHERE itemnumber = ? >+ |, undef, $tobiblio, $itemnumber ); >+ } >+ return $tobiblio; >+ } >+ } elsif ($bibliotoindex eq "old") { >+ ModZebra( $frombiblio, "specialUpdate", "biblioserver" ); >+ return 1; > } >- return $tobiblio; >- } >- return; >+ } else { >+ my $return = $dbh->do(q| >+ UPDATE items >+ SET biblioitemnumber = ?, >+ biblionumber = ? >+ WHERE itemnumber = ? >+ AND biblionumber = ? >+ |, undef, $tobiblioitem, $tobiblio, $itemnumber, $frombiblio ); >+ if ($return == 1) { >+ ModZebra( $tobiblio, "specialUpdate", "biblioserver" ); >+ ModZebra( $frombiblio, "specialUpdate", "biblioserver" ); >+ # Checking if the item we want to move is in an order >+ require C4::Acquisition; >+ my $order = C4::Acquisition::GetOrderFromItemnumber($itemnumber); >+ if ($order) { >+ # Replacing the biblionumber within the order if necessary >+ $order->{'biblionumber'} = $tobiblio; >+ C4::Acquisition::ModOrder($order); >+ } >+ # Update reserves, hold_fill_targets, tmp_holdsqueue and linktracker tables >+ for my $table_name ( qw( reserves hold_fill_targets tmp_holdsqueue linktracker ) ) { >+ $dbh->do( qq| >+ UPDATE $table_name >+ SET biblionumber = ? >+ WHERE itemnumber = ? >+ |, undef, $tobiblio, $itemnumber ); >+ } >+ return $tobiblio; >+ } >+ return; >+ } > } > > =head2 ItemSafeToDelete >diff --git a/cataloguing/moveitem.pl b/cataloguing/moveitem.pl >index 5ef4ffe..2a3e72c 100755 >--- a/cataloguing/moveitem.pl >+++ b/cataloguing/moveitem.pl >@@ -65,7 +65,7 @@ $template->param(bibliotitle => $biblio->title); > $template->param(biblionumber => $biblionumber); > > # If we already have the barcode of the item to move and the biblionumber to move the item to >-if ($barcode && $biblionumber) { >+if ($barcode && $biblionumber) { > > my $itemnumber; > my $item = Koha::Items->find({ barcode => $barcode }); >@@ -74,25 +74,35 @@ if ($barcode && $biblionumber) { > > $itemnumber = $item->itemnumber; > my $frombiblionumber = $item->biblionumber; >- >- my $moveresult = MoveItemFromBiblio($itemnumber, $frombiblionumber, $biblionumber); >- if ($moveresult) { >- $template->param(success => 1); >- } else { >- $template->param(error => 1, >- errornonewitem => 1); >- } >- >- >- } else { >- $template->param(error => 1, >- errornoitem => 1); >- } >- $template->param( >- barcode => $barcode, >- itemnumber => $itemnumber, >- ); >- >+ my @bibliostoindex = ("new", "old"); >+ my $moveresult; >+ foreach my $bibliotoindex (@bibliostoindex) { >+ if ($bibliotoindex eq "new") { >+ $moveresult = MoveItemFromBiblio($itemnumber, $frombiblionumber, $biblionumber, $bibliotoindex); >+ if ($moveresult) { >+ $template->param(success => 1); >+ } else { >+ $template->param(error => 1, >+ errornonewitem => 1); >+ } >+ } elsif ($bibliotoindex eq "old" && $moveresult) { >+ my $moveresult = MoveItemFromBiblio($itemnumber, $frombiblionumber, $biblionumber, $bibliotoindex); >+ if ($moveresult) { >+ $template->param(success => 1); >+ } else { >+ $template->param(error => 1, >+ errornonewitem => 1); >+ } >+ } >+ } >+ } else { >+ $template->param(error => 1, >+ errornoitem => 1); >+ } >+ $template->param( >+ barcode => $barcode, >+ itemnumber => $itemnumber, >+ ); > } else { > $template->param(missingparameter => 1); > if (!$barcode) { $template->param(missingbarcode => 1); } >diff --git a/t/db_dependent/Items/MoveItemFromBiblio.t b/t/db_dependent/Items/MoveItemFromBiblio.t >index 21855d9..b71e47d 100644 >--- a/t/db_dependent/Items/MoveItemFromBiblio.t >+++ b/t/db_dependent/Items/MoveItemFromBiblio.t >@@ -16,7 +16,7 @@ > # along with Koha; if not, see <http://www.gnu.org/licenses>. > > use Modern::Perl; >-use Test::More tests => 8; >+use Test::More tests => 9; > > use C4::Items; > use C4::Reserves; >@@ -71,11 +71,13 @@ my $item_level_hold_to_move = $builder->build( > } > ); > >-my $to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber} ); >+my $to_biblionumber_index_new_biblio = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber}, "new" ); >+my $to_biblionumber_index_old_biblio = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber}, "old" ); > >-is( $to_biblionumber_after_moved, $to_biblio->{biblionumber}, 'MoveItemFromBiblio should return the to_biblionumber if success' ); >+is( $to_biblionumber_index_new_biblio, $to_biblio->{biblionumber}, 'MoveItemFromBiblio should return the to_biblionumber if success' ); >+is( $to_biblionumber_index_old_biblio, 1 , 'MoveItemFromBiblio should return the value 1 if the old biblio has been indexed if success' ); > >-$to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber} ); >+my $to_biblionumber_after_moved = C4::Items::MoveItemFromBiblio( $item2->{itemnumber}, $from_biblio->{biblionumber}, $to_biblio->{biblionumber} ); > > is( $to_biblionumber_after_moved, undef, 'MoveItemFromBiblio should return undef if the move has failed. If called twice, the item is not attached to the first biblio anymore' ); > >-- >2.1.4
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 20549
: 73899