From 6d47d82ac9b1a5f2ced19df3008697cc5117f491 Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 1 Nov 2018 15:47:12 +0100 Subject: [PATCH] Bug 19893: Add code review fixes Sponsored-by: Gothenburg University Library Signed-off-by: Ere Maijala Signed-off-by: Martin Renvoize Signed-off-by: Nick Clemens --- Koha/Exceptions/Elasticsearch.pm | 17 +++++++ Koha/SearchEngine/Elasticsearch.pm | 71 ++++++++++++++++------------ Koha/SearchEngine/Elasticsearch/Indexer.pm | 2 +- admin/searchengine/elasticsearch/mappings.pl | 2 +- installer/data/mysql/sysprefs.sql | 2 + t/Koha/SearchEngine/Elasticsearch.t | 2 +- 6 files changed, 64 insertions(+), 32 deletions(-) create mode 100644 Koha/Exceptions/Elasticsearch.pm diff --git a/Koha/Exceptions/Elasticsearch.pm b/Koha/Exceptions/Elasticsearch.pm new file mode 100644 index 0000000000..d4fd589fbf --- /dev/null +++ b/Koha/Exceptions/Elasticsearch.pm @@ -0,0 +1,17 @@ +package Koha::Exceptions::Elasticsearch; + +use Modern::Perl; + +use Exception::Class ( + + 'Koha::Exceptions::Elasticsearch' => { + description => 'Something went wrong!', + }, + 'Koha::Exceptions::Elasticsearch::MARCFieldExprParseError' => { + isa => 'Koha::Exceptions::Elasticsearch', + description => 'Parse error while processing MARC field expression in mapping', + } + +); + +1; diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm index 1c5fc29d4b..bb9a1140a3 100644 --- a/Koha/SearchEngine/Elasticsearch.pm +++ b/Koha/SearchEngine/Elasticsearch.pm @@ -35,7 +35,6 @@ use Try::Tiny; use YAML::Syck; use List::Util qw( sum0 reduce ); -use Search::Elasticsearch; use MARC::File::XML; use MIME::Base64; use Encode qw(encode); @@ -69,7 +68,7 @@ sub new { my $class = shift @_; my $self = $class->SUPER::new(@_); # Check for a valid index - croak('No index name provided') unless $self->index; + Koha::Exceptions::MissingParameter->throw('No index name provided') unless $self->index; return $self; } @@ -312,17 +311,22 @@ sub sort_fields { =head2 _process_mappings($mappings, $data, $record_document) -Process all C<$mappings> targets operating on a specific MARC field C<$data> applied to C<$record_document> -Since we group all mappings by MARC field targets C<$mappings> will contain all targets for C<$data> -and thus we need to fetch the MARC field only once. + $self->_process_mappings($mappings, $marc_field_data, $record_document) + +Process all C<$mappings> targets operating on a specific MARC field C<$data>. +Since we group all mappings by MARC field targets C<$mappings> will contain +all targets for C<$data> and thus we need to fetch the MARC field only once. +C<$mappings> will be applied to C<$record_document> and new field values added. +The method has no return value. =over 4 =item C<$mappings> -Arrayref of mappings containing arrayrefs on the format [C<$taget>, C<$options>] where -C<$target> is the name of the target field and C<$options> is a hashref containing processing -directives for this particular mapping. +Arrayref of mappings containing arrayrefs in the format +[C<$taget>, C<$options>] where C<$target> is the name of the target field and +C<$options> is a hashref containing processing directives for this particular +mapping. =item C<$data> @@ -330,7 +334,8 @@ The source data from a MARC record field. =item C<$record_document> -Hashref representing the Elasticsearch document on which mappings should be applied. +Hashref representing the Elasticsearch document on which mappings should be +applied. =back @@ -465,7 +470,7 @@ sub marc_records_to_documents { # Suppress warnings if record length exceeded unless (substr($record->leader(), 0, 5) eq '99999') { foreach my $warning (@warnings) { - carp($warning); + carp $warning; } } $record_document->{'marc_data'} = $record->as_xml_record($marcflavour); @@ -482,15 +487,20 @@ sub marc_records_to_documents { =head2 _field_mappings($facet, $suggestible, $sort, $target_name, $target_type, $range) -Get mappings, an internal data structure later used by L<_process_mappings($mappings, $data, $record_document)> -to process MARC target data, for a MARC mapping. + my @mappings = _field_mappings($facet, $suggestible, $sort, $target_name, $target_type, $range) -The returned C<$mappings> is to to be confused with mappings provided by C<_foreach_mapping>, rather this -sub accepts properties from a mapping as provided by C<_foreach_mapping> and expands it to this internal -data stucture. In the caller context (C<_get_marc_mapping_rules>) the returned C<@mappings> is then -applied to each MARC target (leader, control field data, subfield or joined subfields) and -integrated into the mapping rules data structure used in C to -transform MARC records into Elasticsearch documents. +Get mappings, an internal data structure later used by +L<_process_mappings($mappings, $data, $record_document)> to process MARC target +data for a MARC mapping. + +The returned C<$mappings> is not to to be confused with mappings provided by +C<_foreach_mapping>, rather this sub accepts properties from a mapping as +provided by C<_foreach_mapping> and expands it to this internal data stucture. +In the caller context (C<_get_marc_mapping_rules>) the returned C<@mappings> +is then applied to each MARC target (leader, control field data, subfield or +joined subfields) and integrated into the mapping rules data structure used in +C to transform MARC records into Elasticsearch +documents. =over 4 @@ -516,14 +526,14 @@ Elasticsearch document target field type. =item C<$range> -An optinal range as a string on the format "-" or "", +An optional range as a string in the format "-" or "", where "" and "" are integers specifying a range that will be used -for extracting a substing from MARC data as Elasticsearch field target value. +for extracting a substring from MARC data as Elasticsearch field target value. The first character position is "1", and the range is inclusive, so "1-3" means the first three characters of MARC data. -If only "" is provided only one character as position "" will +If only "" is provided only one character at position "" will be extracted. =back @@ -593,7 +603,7 @@ rules keyed by MARC field tags holding all the mapping rules for that particular We can then iterate through all MARC fields for each record and apply all relevant rules once per fields instead of retreiving fields multiple times for each mapping rule -wich is terribly slow. +which is terribly slow. =cut @@ -604,10 +614,7 @@ wich is terribly slow. sub _get_marc_mapping_rules { my ($self) = @_; - my $marcflavour = lc C4::Context->preference('marcflavour'); - my @rules; - my $field_spec_regexp = qr/^([0-9]{3})([()0-9a-z]+)?(?:_\/(\d+(?:-\d+)?))?$/; my $leader_regexp = qr/^leader(?:_\/(\d+(?:-\d+)?))?$/; my $rules = { @@ -625,7 +632,7 @@ sub _get_marc_mapping_rules { if ($type eq 'sum') { push @{$rules->{sum}}, $name; } - elsif($type eq 'boolean') { + elsif ($type eq 'boolean') { # boolean gets special handling, if value doesn't exist for a field, # it is set to false $rules->{defaults}->{$name} = 'false'; @@ -644,7 +651,9 @@ sub _get_marc_mapping_rules { foreach my $token (split //, $2) { if ($token eq "(") { if ($open_group) { - die("Unmatched opening parenthesis for $marc_field"); + Koha::Exceptions::Elasticsearch::MARCFieldExprParseError->throw( + "Unmatched opening parenthesis for $marc_field" + ); } else { $open_group = 1; @@ -659,7 +668,9 @@ sub _get_marc_mapping_rules { $open_group = 0; } else { - die("Unmatched closing parenthesis for $marc_field"); + Koha::Exceptions::Elasticsearch::MARCFieldExprParseError->throw( + "Unmatched closing parenthesis for $marc_field" + ); } } elsif ($open_group) { @@ -699,7 +710,9 @@ sub _get_marc_mapping_rules { push @{$rules->{leader}}, @mappings; } else { - die("Invalid MARC field: $marc_field"); + Koha::Exceptions::Elasticsearch::MARCFieldExprParseError->throw( + "Invalid MARC field expression: $marc_field" + ); } }); return $rules; diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm index 0bc68f6435..4774d78019 100644 --- a/Koha/SearchEngine/Elasticsearch/Indexer.pm +++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm @@ -78,7 +78,7 @@ use constant { try { $self->update_index($biblionums, $records); } catch { - die("Something whent wrong trying to update index:" . $_[0]); + die("Something went wrong trying to update index:" . $_[0]); } Converts C C<$records> to Elasticsearch documents and performs diff --git a/admin/searchengine/elasticsearch/mappings.pl b/admin/searchengine/elasticsearch/mappings.pl index a2c1f66580..2c64db022e 100755 --- a/admin/searchengine/elasticsearch/mappings.pl +++ b/admin/searchengine/elasticsearch/mappings.pl @@ -48,7 +48,7 @@ my $schema = $database->schema; my $marc_type = lc C4::Context->preference('marcflavour'); -my @index_names = ('biblios', 'authorities'); +my @index_names = ($Koha::SearchEngine::Elasticsearch::BIBLIOS_INDEX, $Koha::SearchEngine::Elasticsearch::AUTHORITIES_INDEX); my $update_mappings = sub { for my $index_name (@index_names) { diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql index 077d8e0d96..69ea429a2e 100644 --- a/installer/data/mysql/sysprefs.sql +++ b/installer/data/mysql/sysprefs.sql @@ -153,6 +153,8 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('DumpTemplateVarsIntranet', '0', NULL , 'If enabled, dump all Template Toolkit variable to a comment in the html source for the staff intranet.', 'YesNo'), ('DumpTemplateVarsOpac', '0', NULL , 'If enabled, dump all Template Toolkit variable to a comment in the html source for the opac.', 'YesNo'), ('EasyAnalyticalRecords','0','','If on, display in the catalogue screens tools to easily setup analytical record relationships','YesNo'), +('ElasticsearchIndexStatus_authorities', '0', 'Authorities index status', NULL, NULL), +('ElasticsearchIndexStatus_biblios', '0', 'Biblios index status', NULL, NULL), ('emailLibrarianWhenHoldIsPlaced','0',NULL,'If ON, emails the librarian whenever a hold is placed','YesNo'), ('EnableAdvancedCatalogingEditor','0','','Enable the Rancor advanced cataloging editor','YesNo'), ('EnableBorrowerFiles','0',NULL,'If enabled, allows librarians to upload and attach arbitrary files to a borrower record.','YesNo'), diff --git a/t/Koha/SearchEngine/Elasticsearch.t b/t/Koha/SearchEngine/Elasticsearch.t index 323a3b0106..993cb1bf8e 100644 --- a/t/Koha/SearchEngine/Elasticsearch.t +++ b/t/Koha/SearchEngine/Elasticsearch.t @@ -220,7 +220,7 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests' } }); - my $see = Koha::SearchEngine::Elasticsearch->new({ index => 'biblios' }); + my $see = Koha::SearchEngine::Elasticsearch->new({ index => $Koha::SearchEngine::Elasticsearch::BIBLIOS_INDEX }); my $marc_record_1 = MARC::Record->new(); $marc_record_1->leader(' cam 22 a 4500'); -- 2.11.4.GIT