Bugzilla – Attachment 70185 Details for
Bug 19884
Improve performance of GetItem
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 19884 - Improve performance of GetItem
Bug-19884---Improve-performance-of-GetItem.patch (text/plain), 16.45 KB, created by
David Gustafsson
on 2017-12-28 13:25:49 UTC
(
hide
)
Description:
Bug 19884 - Improve performance of GetItem
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2017-12-28 13:25:49 UTC
Size:
16.45 KB
patch
obsolete
>From 218ce95e23462530ce9796f5bcd75c07e6a715f6 Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >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 > >Sponsored-by: Gothenburg University Library >--- > 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<C4::Items> > > 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 <kyle@bywatersolutions.com> >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
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 19884
:
70172
|
70182
|
70183
|
70184
|
70185
|
70247
|
70248
|
70251
|
70284
|
70285
|
70286
|
70606
|
82870
|
82871
|
82979
|
83111
|
83112
|
93949