From 1be2429574002dcd32adb9863363c8453e850719 Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Fri, 15 Dec 2017 11:36:38 +0100 Subject: [PATCH] Bug 19884 - Improve performance of GetItem Provide optimized search_unblessed method for Koha::Items to increase performance of GetItem. Also extend GetMarcItem() to also accept an item record as second parameter. This way an extra call to GetItem() can be avoided by passing an item record directly instead of an item number. Also add new function GetItems() making it possible to load multiple items at once, which is more efficient than loading them one by one. How to test: 1) Run tests in t/db_dependent/Items.t --- C4/Biblio.pm | 49 ++++++++++++++++++----------- C4/Items.pm | 68 +++++++++++++++++++++++++++------------- Koha/Items.pm | 66 +++++++++++++++++++++++++++++++++++++++ t/db_dependent/Items.t | 85 +++++++++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 223 insertions(+), 45 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 03d9a605fb..b26c767ef8 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -2810,9 +2810,10 @@ sub EmbedItemsInMarcBiblio { carp 'EmbedItemsInMarcBiblio: No MARC record passed'; return; } - $itemnumbers = [] unless defined $itemnumbers; + # Could this be moved lower down and run only when items + # exists for improved performance? Probably.. my $frameworkcode = GetFrameworkCode($biblionumber); _strip_item_fields($marc, $frameworkcode); @@ -2820,26 +2821,36 @@ sub EmbedItemsInMarcBiblio { my $dbh = C4::Context->dbh; my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?"); $sth->execute($biblionumber); - my @item_fields; - my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); - my @items; + my @all_itemnumbers; + while ( my ($itemnumber) = $sth->fetchrow_array ) { + push @all_itemnumbers, $itemnumber; + } + # Remove invalid item numbers by intersecting with all valid item numbers + if(@$itemnumbers) { + my %h = map { $_ => undef } @{$itemnumbers}; + $itemnumbers = [grep { exists $h{$_} } @all_itemnumbers]; + } + else { + $itemnumbers = \@all_itemnumbers; + } + # If no item numbers then no point in continuing + return unless (@{$itemnumbers}); + my $opachiddenitems = $opac - && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); + && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); require C4::Items; - while ( my ($itemnumber) = $sth->fetchrow_array ) { - next if @$itemnumbers and not grep { $_ == $itemnumber } @$itemnumbers; - my $i = $opachiddenitems ? C4::Items::GetItem($itemnumber) : undef; - push @items, { itemnumber => $itemnumber, item => $i }; - } - my @hiddenitems = - $opachiddenitems - ? C4::Items::GetHiddenItemnumbers( map { $_->{item} } @items ) - : (); - # Convert to a hash for quick searching - my %hiddenitems = map { $_ => 1 } @hiddenitems; - foreach my $itemnumber ( map { $_->{itemnumber} } @items ) { - next if $hiddenitems{$itemnumber}; - my $item_marc = C4::Items::GetMarcItem( $biblionumber, $itemnumber ); + + my $items = C4::Items::GetItems($itemnumbers); + if ($opachiddenitems) { + my %hidden_items = map { $_ => undef } C4::Items::GetHiddenItemnumbers(@{$items}); + # Reduce items to non hidden items + $items = [grep { !(exists $hidden_items{$_->{itemnumber}}) } @{$items}]; + } + + my ($itemtag) = GetMarcFromKohaField("items.itemnumber", $frameworkcode); + my @item_fields; + foreach my $item (@{$items}) { + my $item_marc = C4::Items::GetMarcItem($biblionumber, $item); push @item_fields, $item_marc->field($itemtag); } $marc->append_fields(@item_fields); diff --git a/C4/Items.pm b/C4/Items.pm index a7154be82a..e66afeb3f2 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -144,34 +144,60 @@ of C Return item information, for a given itemnumber or barcode. The return value is a hashref mapping item column -names to values. If C<$serial> is true, include serial publication data. +names to values, or undef if item was not found. If C<$serial> is true, include serial publication data. =cut sub GetItem { - my ($itemnumber,$barcode, $serial) = @_; - my $dbh = C4::Context->dbh; + my ($itemnumber, $barcode, $serial) = @_; + my $items = GetItems($itemnumber ? [$itemnumber] : undef, $barcode ? [$barcode] : undef, $serial); + return @{$items} ? $items->[0] : undef; +} - my $item; - if ($itemnumber) { - $item = Koha::Items->find( $itemnumber ); - } else { - $item = Koha::Items->find( { barcode => $barcode } ); - } +=head2 GetItems - return unless ( $item ); + $item = GetItems($itemnumber,$barcode,$serial); - my $data = $item->unblessed(); - $data->{itype} = $item->effective_itemtype(); # set the correct itype +Return list of item information, for given itemnumbers or barcodes. +The return value is an arrayref with hashrefs mapping item column +names to values, or an empty arrayref if no items were found. +If C<$serial> is true, include serial publication data. +=cut + +sub GetItems { + my ($itemnumbers, $barcodes, $serial) = @_; + my $dbh = C4::Context->dbh; + my $items_data = []; + + # Important: If 'item-level_itypes' preference is not set Koha::Item::effective_itemtype() + # will be equal to $item->{itype} and it is possible to fetch the data directly from items table. + # This will have a much smaller footprint then unblessing the item objects fetched through + # find/search. If more calculated attributes are added in the future, this code will need + # to account for that. + if (C4::Context->preference('item-level_itypes')) { + $items_data = Koha::Items->search_unblessed($itemnumbers ? $itemnumbers : { barcode => $barcodes }); + } + else { + my @items = Koha::Items->search( + $itemnumbers ? { itemnumber => { IN => $itemnumbers } } : { barcode => { IN => $barcodes } } + ); + foreach my $item (@items) { + my $data = $item->unblessed(); + # Set the correct itype + $data->{itype} = $item->effective_itemtype(); + push @{$items_data}, $data; + } + } if ($serial) { - my $ssth = $dbh->prepare("SELECT serialseq,publisheddate from serialitems left join serial on serialitems.serialid=serial.serialid where serialitems.itemnumber=?"); - $ssth->execute( $data->{'itemnumber'} ); - ( $data->{'serialseq'}, $data->{'publisheddate'} ) = $ssth->fetchrow_array(); + foreach my $data (@{$items_data}) { + my $ssth = $dbh->prepare("SELECT serialseq,publisheddate from serialitems left join serial on serialitems.serialid=serial.serialid where serialitems.itemnumber=?"); + $ssth->execute( $data->{'itemnumber'} ); + ( $data->{'serialseq'}, $data->{'publisheddate'} ) = $ssth->fetchrow_array(); + } } - - return $data; -} # sub GetItem + return $items_data; +} =head2 CartToShelf @@ -1481,13 +1507,13 @@ sub GetMarcItem { # while the other treats the MARC representation as authoritative # under certain circumstances. - my $itemrecord = GetItem($itemnumber); + my $itemrecord = ref($itemnumber) ? $itemnumber : GetItem($itemnumber); # Tack on 'items.' prefix to column names so that C4::Biblio::TransformKohaToMarc will work. # Also, don't emit a subfield if the underlying field is blank. - - return Item2Marc($itemrecord,$biblionumber); + + return Item2Marc($itemrecord, $biblionumber); } sub Item2Marc { diff --git a/Koha/Items.pm b/Koha/Items.pm index 7b86c5a5e0..d6ab08b3b5 100644 --- a/Koha/Items.pm +++ b/Koha/Items.pm @@ -53,6 +53,72 @@ sub object_class { return 'Koha::Item'; } +=head2 search_unblessed + + $items = Koha::Items->search_unblessed($conditions); + +In scalar context, returns an array reference with hashes containing item data, in list context returns an array. +Calling search_unblessed should produce the same result as calling Koha::Items->search and iterating +though the result unblessing each item. In cases where you only need the item data using this optimized +method is much faster. The arguments accepted are a subset and approximation of those supported by +Koha::items->search, with only the "barcode" and "itemnumber" attributes supported as conditions. +If called with a non-hash conditions argument a search will be performed as if "itemnumber" was the +provided condition. Only one condition is accepted and if more are provided the method will die with +an error. + +=cut + +sub search_unblessed { + my ($self, $conditions) = @_; + my $items = []; + my $field_name; + if (ref($conditions) eq 'HASH') { + my %valid_condition_fields = ( + 'barcode' => undef, + 'itemnumber' => undef, + ); + my @fields = keys %{$conditions}; + if (@fields == 1) { + foreach my $field (@fields) { + die("Invalid condition: \"$field\"") unless exists $valid_condition_fields{$field}; + } + } + elsif(@fields > 1) { + die("Multiple conditions are not supported"); + } + else { + die("No conditions provided"); + } + } + else { + $conditions = { + 'itemnumber' => $conditions, + }; + } + # Only accepts one condition + my ($field, $values) = (%{$conditions}); + + if (ref($values) eq 'ARRAY' && @{$values} == 1) { + ($values) = @{$values}; + } + if (!ref($values)) { + my $item = C4::Context->dbh->selectrow_hashref(qq{SELECT * FROM items WHERE $field = ?}, undef, $values); + push @{$items}, $item; + } + elsif (ref($values) eq 'ARRAY') { + my $in = join ', ', (('?') x @{$values}); + my $sth = C4::Context->dbh->prepare(qq{SELECT * FROM items WHERE $field IN($in)}); + $sth->execute(@{$values}); + while (my $item = $sth->fetchrow_hashref) { + push @{$items}, $item; + } + } + else { + die("Invalid value for field: \"$field\""); + } + return wantarray ? @{$items} : $items; +} + =head1 AUTHOR Kyle M Hall diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index 8c85959bb7..e795049ab8 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -44,7 +44,9 @@ my $location = 'My Location'; subtest 'General Add, Get and Del tests' => sub { - plan tests => 16; + plan tests => 25; + + my $item_barcode = 123; $schema->storage->txn_begin; @@ -61,28 +63,96 @@ subtest 'General Add, Get and Del tests' => sub { my ($bibnum, $bibitemnum) = get_biblio(); # Add an item. - my ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $library->{branchcode}, holdingbranch => $library->{branchcode}, location => $location, itype => $itemtype->{itemtype} } , $bibnum); + my ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ + homebranch => $library->{branchcode}, + holdingbranch => $library->{branchcode}, + location => $location, + itype => $itemtype->{itemtype}, + barcode => $item_barcode + }, $bibnum + ); cmp_ok($item_bibnum, '==', $bibnum, "New item is linked to correct biblionumber."); cmp_ok($item_bibitemnum, '==', $bibitemnum, "New item is linked to correct biblioitemnumber."); # Get item. my $getitem = GetItem($itemnumber); cmp_ok($getitem->{'itemnumber'}, '==', $itemnumber, "Retrieved item has correct itemnumber."); + cmp_ok($getitem->{'barcode'}, '==', $item_barcode, "Retrieved item has correct barcode."); cmp_ok($getitem->{'biblioitemnumber'}, '==', $item_bibitemnum, "Retrieved item has correct biblioitemnumber."); is( $getitem->{location}, $location, "The location should not have been modified" ); is( $getitem->{permanent_location}, $location, "The permanent_location should have been set to the location value" ); + $getitem = GetItem(undef, $item_barcode); + cmp_ok($getitem->{'itemnumber'}, '==', $itemnumber, "Item retrieved by barcode has correct itemnumber."); + # Modify item; setting barcode. - ModItem({ barcode => '987654321' }, $bibnum, $itemnumber); + $item_barcode = '987654321'; + ModItem({ barcode => $item_barcode }, $bibnum, $itemnumber); my $moditem = GetItem($itemnumber); - cmp_ok($moditem->{'barcode'}, '==', '987654321', 'Modified item barcode successfully to: '.$moditem->{'barcode'} . '.'); + cmp_ok($moditem->{'barcode'}, '==', $item_barcode, 'Modified item barcode successfully to: ' . $moditem->{'barcode'} . '.'); + + # Update some more properties to get larger coverage + my $item_properties = { + 'dateaccessioned' => '2012-12-12', + 'datelastborrowed' => '2013-12-12', + 'datelastseen' => '2013-12-12', + 'notforloan' => '0', + 'damaged' => '0', + 'itemlost' => '0', + 'itemcallnumber' => '1234', + 'restricted' => '0', + }; + + # Clone item propreties since ModItem modifies this argument + # causing later tests to fail + ModItem({%{$item_properties}}, $bibnum, $itemnumber); + + # Search unblessed tests + my $search_conditions = { + 'itemnumber' => $itemnumber, + }; + my ($item_object) = Koha::Items->search($search_conditions); + my $item_object_data = $item_object->unblessed; + cmp_ok($item_object_data->{'itemnumber'}, '==', $itemnumber, "Item object retrieved by Koha::Items->search using \"itemnumber\" condition has correct itemnumber."); + + # Intersect with updated properties + my $updated_item_properties = { map { ($_ => $item_object_data->{$_}) } keys %{$item_properties} }; + + is_deeply($updated_item_properties, $item_properties, "Updated item properties have correct values."); + + my ($item_data) = Koha::Items->search_unblessed($search_conditions); + cmp_ok( + $item_data->{'itemnumber'}, + '==', + $itemnumber, + "Item data retrieved by Koha::Items->search_unblessed using \"itemnumber\" condition has correct itemnumber." + ); + + ($item_data) = Koha::Items->search_unblessed({ 'barcode' => $item_barcode }); + cmp_ok( + $item_data->{'itemnumber'}, + '==', + $itemnumber, + "Item data retrieved by Koha::Items->search_unblessed using \"barcode\" condition has correct itemnumber." + ); + + is_deeply($item_object_data, $item_data, "Item data retrieved by unblessing item object is identical to item data from Koha::Items->search_unblessed."); # Delete item. DelItem({ biblionumber => $bibnum, itemnumber => $itemnumber }); my $getdeleted = GetItem($itemnumber); is($getdeleted->{'itemnumber'}, undef, "Item deleted as expected."); - ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ homebranch => $library->{branchcode}, holdingbranch => $library->{branchcode}, location => $location, permanent_location => 'my permanent location', itype => $itemtype->{itemtype} } , $bibnum); + ($item_bibnum, $item_bibitemnum, $itemnumber) = AddItem({ + homebranch => $library->{branchcode}, + holdingbranch => $library->{branchcode}, + location => $location, + permanent_location => 'my permanent location', + itype => $itemtype->{itemtype}, + barcode => $item_barcode + }, + $bibnum + ); $getitem = GetItem($itemnumber); is( $getitem->{location}, $location, "The location should not have been modified" ); is( $getitem->{permanent_location}, 'my permanent location', "The permanent_location should not have modified" ); @@ -100,6 +170,11 @@ subtest 'General Add, Get and Del tests' => sub { t::lib::Mocks::mock_preference('item-level_itypes', '1'); $getitem = GetItem($itemnumber); is( $getitem->{itype}, $itemtype->{itemtype}, "Itemtype set correctly when using item-level_itypes" ); + # Good to test this since different code for retrieving items is run in GetItems() depending on item-level_itypes preference + is( $getitem->{itemnumber}, $itemnumber, "Item successfully retrieved by itemnumber when using item-level_itypes" ); + $getitem = GetItem(undef, $item_barcode); + is( $getitem->{itemnumber}, $itemnumber, "Item successfully retrieved by barcode when using item-level_itypes" ); + t::lib::Mocks::mock_preference('item-level_itypes', '0'); $getitem = GetItem($itemnumber); is( $getitem->{itype}, undef, "Itemtype set correctly when not using item-level_itypes" ); -- 2.11.0