From 48219ae96dfe914786848b28ee998423b53d24b4 Mon Sep 17 00:00:00 2001 From: Ere Maijala Date: Fri, 21 Sep 2018 14:43:38 +0300 Subject: [PATCH] Bug 20664: Optimize retrieval of biblio and item data Optimizes embedding of item data in MARC and fixes several bottlenecks encountered while profiling OAI-PMH and exporting of records. There are two ways this is accomplished: 1.) Create optimized method for fetching item fields for MARC embedding. 2.) Use the cache service more and where repeated calls are made. Test plan: 1.) Before applying the patch, time an export_records.pl run for a set of biblios that also have items. Run it a couple of times to account for initial slowness and possible fluctuations. For example: time misc/export_records.pl --record-type bibs --starting_biblionumber 960000 --ending_biblionumber 965000 --format xml --filename unoptimized.xml 2.) Apply the patch. 3.) Time the export process again with a different output file: time misc/export_records.pl --record-type bibs --starting_biblionumber 960000 --ending_biblionumber 965000 --format xml --filename optimized.xml 4.) Verify that the optimized process is faster. 5.) Compare the resulting export files to make sure they're identical: --- C4/Biblio.pm | 87 ++++++++++++-------------- C4/Items.pm | 110 +++++++++++++++++++++++++++++---- misc/migration_tools/bulkmarcimport.pl | 2 +- t/db_dependent/Items.t | 9 +-- 4 files changed, 142 insertions(+), 66 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 153241f..4b89cea 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -310,7 +310,7 @@ sub ModBiblio { $frameworkcode = "" if !$frameworkcode || $frameworkcode eq "Default"; # XXX - _strip_item_fields($record, $frameworkcode); + _strip_item_fields($record); # update biblionumber and biblioitemnumber in MARC # FIXME - this is assuming a 1 to 1 relationship between @@ -344,7 +344,7 @@ sub ModBiblio { =head2 _strip_item_fields - _strip_item_fields($record, $frameworkcode) + _strip_item_fields($record) Utility routine to remove item tags from a MARC bib. @@ -353,9 +353,8 @@ MARC bib. sub _strip_item_fields { my $record = shift; - my $frameworkcode = shift; # get the items before and append them to the biblio before updating the record, atm we just have the biblio - my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); + my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber" ); # delete any item fields from incoming record to avoid # duplication or incorrect data - use AddItem() or ModItem() @@ -1089,14 +1088,22 @@ sub GetMarcSubfieldStructure { sub GetMarcFromKohaField { my ( $kohafield ) = @_; return unless $kohafield; - # The next call uses the Default framework since it is AUTHORITATIVE - # for all Koha to MARC mappings. - my $mss = GetMarcSubfieldStructure( '', { unsafe => 1 } ); # Do not change framework - my @retval; - foreach( @{ $mss->{$kohafield} } ) { - push @retval, $_->{tagfield}, $_->{tagsubfield}; - } - return wantarray ? @retval : ( @retval ? $retval[0] : undef ); + + my $cache = Koha::Caches->get_instance(); + my $cache_key = "MarcFromKohaField-$kohafield"; + my $cached = $cache->get_from_cache($cache_key); + if ( !defined $cached ) { + # The next call uses the Default framework since it is AUTHORITATIVE + # for all Koha to MARC mappings. + my $mss = GetMarcSubfieldStructure( '', { unsafe => 1 } ); # Do not change framework + my @retval; + foreach( @{ $mss->{$kohafield} } ) { + push @retval, $_->{tagfield}, $_->{tagsubfield}; + } + $cached = \@retval; + $cache->set_in_cache( $cache_key, $cached ); + } + return wantarray ? @$cached : ( @$cached ? $cached->[0] : undef ); } =head2 GetMarcSubfieldStructureFromKohaField @@ -2804,7 +2811,7 @@ override if necessary. sub EmbedItemsInMarcBiblio { my ($params) = @_; - my ($marc, $biblionumber, $itemnumbers, $opac, $borcat); + my ($marc, $biblionumber, $itemnumbers, $use_hidingrules, $borcat); $marc = $params->{marc_record}; if ( !$marc ) { carp 'EmbedItemsInMarcBiblio: No MARC record passed'; @@ -2812,49 +2819,33 @@ sub EmbedItemsInMarcBiblio { } $biblionumber = $params->{biblionumber}; $itemnumbers = $params->{item_numbers}; - $opac = $params->{opac}; + $use_hidingrules = $params->{opac}; $borcat = $params->{borcat} // q{}; $itemnumbers = [] unless defined $itemnumbers; - my $frameworkcode = GetFrameworkCode($biblionumber); - _strip_item_fields($marc, $frameworkcode); + _strip_item_fields($marc); - # ... and embed the current items - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare("SELECT itemnumber FROM items WHERE biblionumber = ?"); - $sth->execute($biblionumber); - my ( $itemtag, $itemsubfield ) = GetMarcFromKohaField( "items.itemnumber", $frameworkcode ); + my $hidingrules = {}; + if ($use_hidingrules && $borcat && !UseHidingRulesWithBorrowerCategory($borcat)) { + # don't hide anything from this borrower category + $use_hidingrules = 0; + } - my @item_fields; # Array holding the actual MARC data for items to be included. - my @items; # Array holding items which are both in the list (sitenumbers) - # and on this biblionumber + my $yaml = $use_hidingrules ? C4::Context->preference('OpacHiddenItems') : ''; + if ( $yaml =~ /\S/ ) { + $yaml = "$yaml\n\n"; # YAML is anal on ending \n. Surplus does not hurt + eval { + $hidingrules = YAML::Load($yaml); + }; + if ($@) { + carp "Unable to parse OpacHiddenItems syspref : $@"; + } + } - # Flag indicating if there is potential hiding. - my $opachiddenitems = $opac - && ( C4::Context->preference('OpacHiddenItems') !~ /^\s*$/ ); + my $item_fields = C4::Items::GetMarcItemFields( $biblionumber, $itemnumbers, $hidingrules ); - 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 @items2pass = map { $_->{item} } @items; - my @hiddenitems = - $opachiddenitems - ? C4::Items::GetHiddenItemnumbers({ - items => \@items2pass, - borcat => $borcat }) - : (); - # 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 ); - push @item_fields, $item_marc->field($itemtag); - } - $marc->append_fields(@item_fields); + $marc->append_fields(@$item_fields) if ( @$item_fields ); } =head1 INTERNAL FUNCTIONS diff --git a/C4/Items.pm b/C4/Items.pm index e46737d..66815af 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -74,6 +74,7 @@ BEGIN { GetHostItemsInfo get_hostitemnumbers_of GetHiddenItemnumbers + UseHidingRulesWithBorrowerCategory ItemSafeToDelete DelItemCheck MoveItemFromBiblio @@ -1299,12 +1300,10 @@ to be excluded sub GetHiddenItemnumbers { my $params = shift; my $items = $params->{items}; - if (my $exceptions = C4::Context->preference('OpacHiddenItemsExceptions') and $params->{'borcat'}){ - foreach my $except (split(/\|/, $exceptions)){ - if ($params->{'borcat'} eq $except){ - return; # we dont hide anything for this borrower category - } - } + + if ($params->{'borcat'} && !UseHidingRulesWithBorrowerCategory($params->{'borcat'})) { + # Don't hide anything from this borrower category + return; } my @resultitems; @@ -1350,6 +1349,27 @@ sub GetHiddenItemnumbers { return @resultitems; } +=head2 UseHidingRulesWithBorrowerCategory + + if (UseHidingRulesWithBorrowerCategory($category}) { ... } + +Checks if item hiding rules should be used with the given borrower category. + +=cut + +sub UseHidingRulesWithBorrowerCategory { + my ($borcat) = @_; + + if ($borcat && (my $exceptions = C4::Context->preference('OpacHiddenItemsExceptions'))) { + foreach my $except (split(/\|/, $exceptions)) { + if ($borcat eq $except) { + return 0; + } + } + } + return 1; +} + =head1 LIMITED USE FUNCTIONS The following functions, while part of the public API, @@ -1419,6 +1439,73 @@ sub Item2Marc { return $itemmarc; } +=head2 GetMarcItemFields + + my @marc_fields = GetMarcItemFields($biblionumber); + +Returns an array of MARC::Record objects of the items for the biblio. + +=cut + +sub GetMarcItemFields { + my ( $biblionumber, $itemnumbers, $hidingrules ) = @_; + + my $item_level_itype = C4::Context->preference('item-level_itypes'); + # This is so much faster than using Koha::Items->search that it makes sense even if it's ugly. + my $query = 'SELECT * FROM items WHERE biblionumber = ?'; + if (@$itemnumbers) { + $query .= ' AND itemnumber IN (' . join(',', @$itemnumbers) . ')'; + } + my $sth = C4::Context->dbh->prepare($query); + $sth->execute($biblionumber); + my $items = $sth->fetchall_arrayref({}); + $sth->finish(); + my @item_fields; + my ($itemtag, $itemsubfield) = GetMarcFromKohaField('items.itemnumber'); + + ITEMLOOP: foreach my $item (@$items) { + + # Check hiding rules + if (defined $hidingrules) { + foreach my $field (keys %$hidingrules) { + my $val = $item->{$field}; + $val = '' unless defined $val; + + # If the results matches the values in the hiding rules, skip the item + if (any { $val eq $_ } @{$hidingrules->{$field}}) { + next ITEMLOOP; + } + } + } + + # Set correct item type + if (!$item_level_itype || !$item->{itype}) { + warn 'item-level_itypes set but no itemtype set for item (' . $item->{itemnumber} . ')' if (!$item->{itype}); + my $biblioitem = Koha::Biblioitems->find($item->{biblioitemnumber}); + $item->{itype} = $biblioitem->itemtype(); + } + + my $mungeditem = { + map { + defined($item->{$_}) && $item->{$_} ne '' ? ("items.$_" => $item->{$_}) : () + } keys %{ $item } + }; + my $itemmarc = TransformKohaToMarc($mungeditem); + + my $unlinked_item_subfields = _parse_unlinked_item_subfields_from_xml($mungeditem->{'items.more_subfields_xml'}); + if (defined $unlinked_item_subfields and $#$unlinked_item_subfields > -1) { + foreach my $field ($itemmarc->field($itemtag)){ + $field->add_subfields(@$unlinked_item_subfields); + } + } + + push @item_fields, $itemmarc->field($itemtag); + } + + return \@item_fields; +} + + =head1 PRIVATE FUNCTIONS AND VARIABLES The following functions are not meant to be called @@ -1555,10 +1642,11 @@ sub _do_column_fixes_for_mod { (not defined $item->{'withdrawn'} or $item->{'withdrawn'} eq '')) { $item->{'withdrawn'} = 0; } - if (exists $item->{location} - and $item->{location} ne 'CART' - and $item->{location} ne 'PROC' - and not $item->{permanent_location} + if (exists $item->{'location'} + and defined $item->{'location'} + and $item->{'location'} ne 'CART' + and $item->{'location'} ne 'PROC' + and (not defined $item->{'permanent_location'} or not $item->{'permanent_location'}) ) { $item->{'permanent_location'} = $item->{'location'}; } @@ -2223,7 +2311,7 @@ sub _SearchItems_build_where_fragment { push @columns, Koha::Database->new()->schema()->resultset('Biblioitem')->result_source->columns; my @operators = qw(= != > < >= <= like); my $field = $filter->{field}; - if ( (0 < grep /^$field$/, @columns) or (substr($field, 0, 5) eq 'marc:') ) { + if ( defined $field and ((0 < grep /^$field$/, @columns) or (substr($field, 0, 5) eq 'marc:')) ) { my $op = $filter->{operator}; my $query = $filter->{query}; diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl index d820d3e..333b3da 100755 --- a/misc/migration_tools/bulkmarcimport.pl +++ b/misc/migration_tools/bulkmarcimport.pl @@ -486,7 +486,7 @@ RECORD: while ( ) { # Work on a clone so that if there are real errors, we can maybe # fix them up later. my $clone_record = $record->clone(); - C4::Biblio::_strip_item_fields($clone_record, ''); + C4::Biblio::_strip_item_fields($clone_record); # This sets the marc fields if there was an error, and also calls # defer_marc_save. ModBiblioMarc( $clone_record, $biblionumber, $framework ); diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index 6ae721a..0ed0422 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -690,7 +690,7 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub { Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => '9', kohafield => 'items.itemnumber' })->store; Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => 'p', kohafield => 'items.barcode' })->store; Koha::MarcSubfieldStructure->new({ frameworkcode => '', tagfield => '952', tagsubfield => 'y', kohafield => 'items.itype' })->store; - Koha::Caches->get_instance->clear_from_cache( "MarcSubfieldStructure-" ); + Koha::Caches->get_instance()->flush_all(); my $itemtype = $builder->build({ source => 'Itemtype' })->{itemtype}; @@ -731,9 +731,7 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub { Koha::MarcSubfieldStructures->search({ frameworkcode => '', tagfield => '952', tagsubfield => 'p' })->delete; # And make sure the caches are cleared - my $cache = Koha::Caches->get_instance(); - $cache->clear_from_cache("default_value_for_mod_marc-"); - $cache->clear_from_cache("MarcSubfieldStructure-"); + Koha::Caches->get_instance()->flush_all(); # Update the MARC field with another value $item_record->delete_fields( $barcode_field ); @@ -748,8 +746,7 @@ subtest 'C4::Items::_build_default_values_for_mod_marc' => sub { $item = GetItem($item_itemnumber); is ( $item->{barcode}, $a_barcode, 'items.barcode is not mapped anymore, so the DB column has not been updated' ); - $cache->clear_from_cache("default_value_for_mod_marc-"); - $cache->clear_from_cache( "MarcSubfieldStructure-" ); + Koha::Caches->get_instance()->flush_all(); $schema->storage->txn_rollback; }; -- 2.7.4