Browse Source

Bug 19893: Add code review fixes

Sponsored-by: Gothenburg University Library
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
18.11.x
David Gustafsson 3 years ago
committed by Nick Clemens
parent
commit
6d47d82ac9
  1. 17
      Koha/Exceptions/Elasticsearch.pm
  2. 71
      Koha/SearchEngine/Elasticsearch.pm
  3. 2
      Koha/SearchEngine/Elasticsearch/Indexer.pm
  4. 2
      admin/searchengine/elasticsearch/mappings.pl
  5. 2
      installer/data/mysql/sysprefs.sql
  6. 2
      t/Koha/SearchEngine/Elasticsearch.t

17
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;

71
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<marc_records_to_documents> 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<marc_records_to_documents> 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 "<START>-<END>" or "<START>",
An optional range as a string in the format "<START>-<END>" or "<START>",
where "<START>" and "<END>" 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 "<START>" is provided only one character as position "<START>" will
If only "<START>" is provided only one character at position "<START>" 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;

2
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<MARC::Records> C<$records> to Elasticsearch documents and performs

2
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) {

2
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'),

2
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');

Loading…
Cancel
Save