Attachment 80970 Details for Bug 19893 – Bug 19893: Add pods, remove syspref, add tests for serialization format

[patch] Bug 19893: Add pods, remove syspref, add tests for serialization format

Bug-19893-Add-pods-remove-syspref-add-tests-for-se.patch (text/plain), 17.39 KB, created by David Gustafsson on 2018-10-22 11:37:18 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Gustafsson

Created: 2018-10-22 11:37:18 UTC

Size: 17.39 KB

patch

obsolete

>From 285c2611896517ef204424cc70c035828a9483bb Mon Sep 17 00:00:00 2001
>From: David Gustafsson <david.gustafsson@ub.gu.se>
>Date: Mon, 22 Oct 2018 11:30:17 +0200
>Subject: [PATCH] Bug 19893: Add pods, remove syspref, add tests for
> serialization format
>
>Add missing pods, remove obsolete syspref and add test for serialization format for records exceeding max record size
>---
> Koha/SearchEngine/Elasticsearch.pm          |  63 +++++++--
> Koha/SearchEngine/Elasticsearch/Indexer.pm  | 212 +++++++++++++++++++++-------
> misc/search_tools/rebuild_elastic_search.pl |   6 +-
> t/Koha/SearchEngine/Elasticsearch.t         |  27 +++-
> 4 files changed, 246 insertions(+), 62 deletions(-)
>
>diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm
>index bd1c5e0f6f..3719f8961d 100644
>--- a/Koha/SearchEngine/Elasticsearch.pm
>+++ b/Koha/SearchEngine/Elasticsearch.pm
>@@ -73,6 +73,15 @@ sub new {
>     return $self;
> }
> 
>+=head2 get_elasticsearch
>+
>+    my $elasticsearch_client = $self->get_elasticsearch();
>+
>+Returns a C<Search::Elasticsearch> client. The client is cached on a C<Koha::SearchEngine::ElasticSearch>
>+instance level and will be reused if method is called multiple times.
>+
>+=cut
>+
> sub get_elasticsearch {
>     my $self = shift @_;
>     unless (defined $self->{elasticsearch}) {
>@@ -145,8 +154,8 @@ sub get_elasticsearch_params {
> 
>     my $settings = $self->get_elasticsearch_settings();
> 
>-This provides the settings provided to elasticsearch when an index is created.
>-These can do things like define tokenisation methods.
>+This provides the settings provided to Elasticsearch when an index is created.
>+These can do things like define tokenization methods.
> 
> A hashref containing the settings is returned.
> 
>@@ -170,7 +179,7 @@ sub get_elasticsearch_settings {
> 
>     my $mappings = $self->get_elasticsearch_mappings();
> 
>-This provides the mappings that get passed to elasticsearch when an index is
>+This provides the mappings that get passed to Elasticsearch when an index is
> created.
> 
> =cut
>@@ -232,7 +241,7 @@ sub get_elasticsearch_mappings {
> 
> =head2 _get_elasticsearch_mapping
> 
>-Get the ES mappings for the given purpose and data type
>+Get the Elasticsearch mappings for the given purpose and data type.
> 
> $mapping = _get_elasticsearch_mapping('search', 'text');
> 
>@@ -301,13 +310,31 @@ sub sort_fields {
>     return $self->_sort_fields_accessor();
> }
> 
>+=head2 marc_records_to_documents($marc_records)
>+
>+    my @record_documents = $self->marc_records_to_documents($marc_records);
>+
>+Using mappings stored in databse convert C<$marc_records> to ElasticSearch documents.
>+
>+Returns array of hash references, representing ElasticSearch documents,
>+acceptable as body payload in C<Search::Elasticsearch> requests.
>+
>+=over 4
>+
>+=item C<$marc_documents>
>+
>+Reference to array of C<MARC::Record> objects to be converted to ElasticSearch documents.
>+
>+=back
>+
>+=cut
>+
> sub marc_records_to_documents {
>     my ($self, $records) = @_;
>-    my $rules = $self->get_marc_mapping_rules();
>+    my $rules = $self->_get_marc_mapping_rules();
>     my $control_fields_rules = $rules->{control_fields};
>     my $data_fields_rules = $rules->{data_fields};
>     my $marcflavour = lc C4::Context->preference('marcflavour');
>-    my $serialization_format = C4::Context->preference('ElasticsearchMARCSerializationFormat');
> 
>     my @record_documents;
> 
>@@ -426,8 +453,28 @@ sub marc_records_to_documents {
>     return \@record_documents;
> }
> 
>-# Provides the rules for marc to Elasticsearch JSON document conversion.
>-sub get_marc_mapping_rules {
>+=head2 _get_marc_mapping_rules
>+
>+    my $mapping_rules = $self->_get_marc_mapping_rules()
>+
>+Generates rules from mappings stored in databse for MARC records to Elasticsearch JSON document conversion.
>+
>+Since field retrieval is slow in C<MARC::Records> (all fields are itereted through for
>+each call to C<MARC::Record>->field) we create an optimized structure of mapping
>+rules keyed by MARC field tags holding all the mapping rules for that particular tag.
>+
>+We can then iterate through all MARC fields for each record and apply all relevant
>+rules once per fields instead of retreiving field multiple times for each mapping rule
>+wich is terribly slow.
>+
>+=cut
>+
>+# TODO: This structure can be used for processing multiple MARC::Records so is currently
>+# rebuilt for each batch. Since it is cacheable it could also be stored in an in
>+# memory cache which it is currently not. The performance gain of caching
>+# would probably be marginal, but to do this could be a further improvement.
>+
>+sub _get_marc_mapping_rules {
>     my ($self) = @_;
> 
>     my $marcflavour = lc C4::Context->preference('marcflavour');
>diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm
>index a6bd323d30..4646e0b94f 100644
>--- a/Koha/SearchEngine/Elasticsearch/Indexer.pm
>+++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm
>@@ -44,45 +44,76 @@ Koha::SearchEngine::Elasticsearch::Indexer - handles adding new records to the i
>     $indexer->drop_index();
>     $indexer->update_index(\@biblionumbers, \@records);
> 
>-=head1 FUNCTIONS
> 
>-=head2 $indexer->update_index($biblionums, $records);
>+=head1 CONSTANTS
> 
>-C<$biblionums> is an arrayref containing the biblionumbers for the records.
>+=over 4
> 
>-C<$records> is an arrayref containing the L<MARC::Record>s themselves.
>+=item C<Koha::SearchEngine::Elasticsearch::Indexer::INDEX_STATUS_OK>
> 
>-The values in the arrays must match up, and the 999$c value in the MARC record
>-will be rewritten using the values in C<$biblionums> to ensure they are correct.
>-If C<$biblionums> is C<undef>, this won't happen, but you should be sure that
>-999$c is correct on your own then.
>+Represents an index state where index is created and in a working state.
> 
>-Note that this will modify the original record if C<$biblionums> is supplied.
>-If that's a problem, clone them first.
>+=item C<Koha::SearchEngine::Elasticsearch::Indexer::INDEX_STATUS_REINDEX_REQUIRED>
>+
>+Not currently used, but could be useful later, for example if can detect when new field or mapping added.
>+
>+=item C<Koha::SearchEngine::Elasticsearch::Indexer::INDEX_STATUS_RECREATE_REQUIRED>
>+
>+Representings an index state where index needs to be recreated and is not in a working state.
>+
>+=back
> 
> =cut
> 
> use constant {
>     INDEX_STATUS_OK => 0,
>-    INDEX_STATUS_REINDEX_REQUIRED => 1, # Not currently used, but could be useful later, for example if can detect when new field or mapping added
>+    INDEX_STATUS_REINDEX_REQUIRED => 1,
>     INDEX_STATUS_RECREATE_REQUIRED => 2,
> };
> 
>+=head1 FUNCTIONS
>+
>+=head2 update_index($biblionums, $records)
>+
>+    try {
>+        $self->update_index($biblionums, $records);
>+    } catch {
>+        die("Something whent wrong trying to update index:" .  $_[0]);
>+    }
>+
>+Converts C<MARC::Records> C<$records> to Elasticsearch documents and perform
>+an update request for these records on the Elasticsearch index.
>+
>+The values in the arrays must match up, and the 999$c value in the MARC record
>+will be rewritten using the values in C<$biblionums> to ensure they are correct.
>+If C<$biblionums> is C<undef>, this won't happen, so in that case you should make
>+sure that 999$c is correct.
>+
>+Note that this will modify the original record if C<$biblionums> is supplied.
>+If that's a problem, clone them first.
>+
>+=over 4
>+
>+=item C<$biblionums>
>+
>+Arrayref of biblio numbers for the C<$records>, the order must be the same
>+as C<$records>.
>+
>+=item C<$records>
>+
>+Arrayref of C<MARC::Record>s.
>+
>+=back
>+
>+=cut
>+
> sub update_index {
>     my ($self, $biblionums, $records) = @_;
> 
>-    # TODO should have a separate path for dealing with a large number
>-    # of records at once where we use the bulk update functions in ES.
>     if ($biblionums) {
>         $self->_sanitise_records($biblionums, $records);
>     }
> 
>-    $self->bulk_index($records);
>-    return 1;
>-}
>-
>-sub bulk_index {
>-    my ($self, $records) = @_;
>     my $conf = $self->get_elasticsearch_params();
>     my $elasticsearch = $self->get_elasticsearch();
>     my $documents = $self->marc_records_to_documents($records);
>@@ -108,27 +139,88 @@ sub bulk_index {
>     return 1;
> }
> 
>-sub index_status_ok {
>-    my ($self, $set) = @_;
>-    return defined $set ?
>-        $self->index_status(INDEX_STATUS_OK) :
>-        $self->index_status == INDEX_STATUS_OK;
>+=head2 set_index_status_ok
>+
>+Convenience method for setting index status to C<INDEX_STATUS_OK>.
>+
>+=cut
>+
>+sub set_index_status_ok {
>+    my ($self) = @_;
>+    $self->index_status(INDEX_STATUS_OK);
> }
> 
>-sub index_status_reindex_required {
>-    my ($self, $set) = @_;
>-    return defined $set ?
>-        $self->index_status(INDEX_STATUS_REINDEX_REQUIRED) :
>-        $self->index_status == INDEX_STATUS_REINDEX_REQUIRED;
>+=head2 is_index_status_ok
>+
>+Convenience method for checking if index status is C<INDEX_STATUS_OK>.
>+
>+=cut
>+
>+sub is_index_status_ok {
>+    my ($self) = @_;
>+    return $self->index_status == INDEX_STATUS_OK;
> }
> 
>-sub index_status_recreate_required {
>-    my ($self, $set) = @_;
>-    return defined $set ?
>-        $self->index_status(INDEX_STATUS_RECREATE_REQUIRED) :
>-        $self->index_status == INDEX_STATUS_RECREATE_REQUIRED;
>+=head2 set_index_status_reindex_required
>+
>+Convenience method for setting index status to C<INDEX_REINDEX_REQUIRED>.
>+
>+=cut
>+
>+sub set_index_status_reindex_required {
>+    my ($self) = @_;
>+    $self->index_status(INDEX_STATUS_REINDEX_REQUIRED);
> }
> 
>+=head2 is_index_status_reindex_required
>+
>+Convenience method for checking if index status is C<INDEX_STATUS_REINDEX_REQUIRED>.
>+
>+=cut
>+
>+sub is_index_status_reindex_required {
>+    my ($self) = @_;
>+    return $self->index_status == INDEX_STATUS_REINDEX_REQUIRED;
>+}
>+
>+=head2 set_index_status_recreate_required
>+
>+Convenience method for setting index status to C<INDEX_STATUS_RECREATE_REQUIRED>.
>+
>+=cut
>+
>+sub set_index_status_recreate_required {
>+    my ($self) = @_;
>+    $self->index_status(INDEX_STATUS_RECREATE_REQUIRED);
>+}
>+
>+=head2 is_index_status_recreate_required
>+
>+Convenience method for checking if index status is C<INDEX_STATUS_RECREATE_REQUIRED>.
>+
>+=cut
>+
>+sub is_index_status_recreate_required {
>+    my ($self) = @_;
>+    return $self->index_status == INDEX_STATUS_RECREATE_REQUIRED;
>+}
>+
>+=head2 index_status($status)
>+
>+Will either set the current index status to C<$status> and return C<$status>,
>+or return the current index status if called with no arguments.
>+
>+=over 4
>+
>+=item C<$status>
>+
>+Optional argument. If passed will set current index status to C<$status> if C<$status> is
>+a valid status. See L</CONSTANTS>.
>+
>+=back
>+
>+=cut
>+
> sub index_status {
>     my ($self, $status) = @_;
>     my $key = 'ElasticsearchIndexStatus_' . $self->index;
>@@ -150,6 +242,15 @@ sub index_status {
>     }
> }
> 
>+=head2 update_mappings
>+
>+Generate Elasticsearch mappings from mappings stored in database and
>+perform a request to update Elasticsearch index mappings. Will throw an
>+error and set index status to C<INDEX_STATUS_RECREATE_REQUIRED> if update
>+failes.
>+
>+=cut
>+
> sub update_mappings {
>     my ($self) = @_;
>     my $conf = $self->get_elasticsearch_params();
>@@ -166,35 +267,35 @@ sub update_mappings {
>                 }
>             );
>         } catch {
>-            $self->index_status_recreate_required(1);
>+            $self->set_index_status_recreate_required();
>             my $reason = $_[0]->{vars}->{body}->{error}->{reason};
>             Koha::Exceptions::Exception->throw(
>                 error => "Unable to update mappings for index \"$conf->{index_name}\". Reason was: \"$reason\". Index needs to be recreated and reindexed",
>             );
>         };
>     }
>-    $self->index_status_ok(1);
>+    $self->set_index_status_ok();
> }
> 
>-=head2 $indexer->update_index_background($biblionums, $records)
>+=head2 update_index_background($biblionums, $records)
> 
>-This has exactly the same API as C<update_index_background> however it'll
>+This has exactly the same API as C<update_index> however it'll
> return immediately. It'll start a background process that does the adding.
> 
> If it fails to add to Elasticsearch then it'll add to a queue that will cause
> it to be updated by a regular index cron job in the future.
> 
>+=cut
>+
> # TODO implement in the future - I don't know the best way of doing this yet.
> # If fork: make sure process group is changed so apache doesn't wait for us.
> 
>-=cut
>-
> sub update_index_background {
>     my $self = shift;
>     $self->update_index(@_);
> }
> 
>-=head2 $indexer->delete_index($biblionums)
>+=head2 delete_index($biblionums)
> 
> C<$biblionums> is an arrayref of biblionumbers to delete from the index.
> 
>@@ -206,7 +307,7 @@ sub delete_index {
>     if ( !$self->store ) {
>         my $params  = $self->get_elasticsearch_params();
>         $self->store(
>-            Catmandu::Store::ElasticSearch->new(
>+            Catmandu::Store::Elasticsearch->new(
>                 %$params,
>                 index_settings => $self->get_elasticsearch_settings(),
>                 index_mappings => $self->get_elasticsearch_mappings(),
>@@ -217,23 +318,21 @@ sub delete_index {
>     $self->store->bag->commit;
> }
> 
>-=head2 $indexer->delete_index_background($biblionums)
>+=head2 delete_index_background($biblionums)
> 
>-Identical to L<delete_index>, this will return immediately and start a
>-background process to do the actual deleting.
>+Identical to L</delete_index($biblionums)>
> 
> =cut
> 
>-# TODO implement in the future
>-
>+# TODO: Should be made async
> sub delete_index_background {
>     my $self = shift;
>     $self->delete_index(@_);
> }
> 
>-=head2 $indexer->drop_index();
>+=head2 drop_index
> 
>-Drops the index from the elasticsearch server.
>+Drops the index from the Elasticsearch server.
> 
> =cut
> 
>@@ -243,10 +342,16 @@ sub drop_index {
>         my $conf = $self->get_elasticsearch_params();
>         my $elasticsearch = $self->get_elasticsearch();
>         $elasticsearch->indices->delete(index => $conf->{index_name});
>-        $self->index_status_recreate_required(1);
>+        $self->set_index_status_recreate_required();
>     }
> }
> 
>+=head2 create_index
>+
>+Creates the index (including mappings) on the Elasticsearch server.
>+
>+=cut
>+
> sub create_index {
>     my ($self) = @_;
>     my $conf = $self->get_elasticsearch_params();
>@@ -261,6 +366,13 @@ sub create_index {
>     $self->update_mappings();
> }
> 
>+=head2 index_exists
>+
>+Checks if index has been created on the ElasticSearch server. Returns C<1> or the
>+empty string to indicate whether index exists or not.
>+
>+=cut
>+
> sub index_exists {
>     my ($self) = @_;
>     my $conf = $self->get_elasticsearch_params();
>diff --git a/misc/search_tools/rebuild_elastic_search.pl b/misc/search_tools/rebuild_elastic_search.pl
>index bed6b5197c..caa9a3d870 100755
>--- a/misc/search_tools/rebuild_elastic_search.pl
>+++ b/misc/search_tools/rebuild_elastic_search.pl
>@@ -165,13 +165,13 @@ sub do_reindex {
>         $indexer->drop_index() if $indexer->index_exists();
>         $indexer->create_index();
>     }
>-    elsif (!$indexer->index_exists()) {
>+    elsif (!$indexer->index_exists) {
>         # Create index if does not exist
>         $indexer->create_index();
>-    } elsif ($indexer->index_status_ok) {
>+    } elsif ($indexer->is_index_status_ok) {
>         # Update mapping unless index is some kind of problematic state
>         $indexer->update_mappings();
>-    } elsif ($indexer->index_status_recreate_required) {
>+    } elsif ($indexer->is_index_status_recreate_required) {
>         warn qq/Index "$index_name" has status "recreate required", suggesting it should be recreated/;
>     }
> 
>diff --git a/t/Koha/SearchEngine/Elasticsearch.t b/t/Koha/SearchEngine/Elasticsearch.t
>index f4fe4355fa..323a3b0106 100644
>--- a/t/Koha/SearchEngine/Elasticsearch.t
>+++ b/t/Koha/SearchEngine/Elasticsearch.t
>@@ -115,7 +115,7 @@ subtest 'get_elasticsearch_mappings() tests' => sub {
> 
> subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests' => sub {
> 
>-    plan tests => 30;
>+    plan tests => 32;
> 
>     t::lib::Mocks::mock_preference('marcflavour', 'MARC21');
> 
>@@ -315,6 +315,8 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests'
> 
>     ok(defined $docs->[0][1]->{marc_format}, 'First document marc_format field should be set');
> 
>+    is($docs->[0][1]->{marc_format}, 'base64ISO2709', 'First document marc_format should be set correctly');
>+
>     is(scalar @{$docs->[0][1]->{type_of_record}}, 1, 'First document type_of_record field should have one value');
>     is_deeply(
>         $docs->[0][1]->{type_of_record},
>@@ -351,4 +353,27 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests'
> 
>     ok(!(defined $docs->[0][1]->{unimarc_title}), "No mapping when marc_type doesn't match marc flavour");
> 
>+    # Marc serialization format fallback for records exceeding ISO2709 max record size
>+
>+    my $large_marc_record = MARC::Record->new();
>+    $large_marc_record->leader('     cam  22      a 4500');
>+
>+    $large_marc_record->append_fields(
>+        MARC::Field->new('100', '', '', a => 'Author 1'),
>+        MARC::Field->new('110', '', '', a => 'Corp Author'),
>+        MARC::Field->new('210', '', '', a => 'Title 1'),
>+        MARC::Field->new('245', '', '', a => 'Title:', b => 'large record'),
>+        MARC::Field->new('999', '', '', c => '1234567'),
>+    );
>+
>+    my $item_field = MARC::Field->new('952', '', '', o => '123456789123456789123456789', p => '123456789', z => 'test');
>+    my $items_count = 1638;
>+    while(--$items_count) {
>+        $large_marc_record->append_fields($item_field);
>+    }
>+
>+    $docs = $see->marc_records_to_documents([$large_marc_record]);
>+
>+    is($docs->[0][1]->{marc_format}, 'MARCXML', 'For record exceeding max record size marc_format should be set correctly');
>+
> };
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 19893: 70199 | 70200 | 70201 | 70202 | 70230 | 70325 | 73153 | 75109 | 75271 | 75272 | 75353 | 75428 | 75455 | 75588 | 75658 | 75660 | 76612 | 76613 | 76614 | 76629 | 78091 | 78092 | 78093 | 78094 | 78524 | 78561 | 78587 | 78588 | 78589 | 78590 | 78591 | 78592 | 78593 | 78594 | 78690 | 78691 | 78692 | 78693 | 78694 | 78695 | 78696 | 80969 | 80970 | 80971 | 80983 | 81014 | 81015 | 81284 | 81822 | 81901 | 81903 | 81955 | 81957 | 81958 | 81959 | 81960 | 81961 | 81962 | 81963 | 81964 | 81965 | 81966 | 82143 | 82144 | 82145 | 82146 | 82147 | 82148 | 82149 | 82150 | 82151 | 82152 | 82256 | 82257 | 82258 | 82259 | 82260 | 82261 | 82262 | 82263 | 82264 | 82265 | 82266 | 82267