Bug 24823: Remove unused parameters

Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'.
Refactor so that get_elasticsearch_params only returns parameters used
by Search::Elasticsearch, add class accessor for index_name.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Koha/SearchEngine/Elasticsearch.pm

@@ -45,7 +45,7 @@ use Encode qw(encode);
 use Business::ISBN;
 use Scalar::Util qw(looks_like_number);
 
-__PACKAGE__->mk_ro_accessors(qw( index ));
+__PACKAGE__->mk_ro_accessors(qw( index index_name ));
 __PACKAGE__->mk_accessors(qw( sort_fields ));
 
 # Constants to refer to the standard index names
@@ -66,15 +66,27 @@ The name of the index to use, generally 'biblios' or 'authorities'.
 
 =back
 
+=item index_name
+
+The Elasticsearch index name with Koha instance prefix.
+
+=back
+
+
 =head1 FUNCTIONS
 
 =cut
 
 sub new {
     my $class = shift @_;
-    my $self = $class->SUPER::new(@_);
+    my ($params) = @_;
+
     # Check for a valid index
-    Koha::Exceptions::MissingParameter->throw('No index name provided') unless $self->index;
+    Koha::Exceptions::MissingParameter->throw('No index name provided') unless $params->{index};
+    my $config = _read_configuration();
+    $params->{index_name} = $config->{index_name} . '_' . $params->{index};
+
+    my $self = $class->SUPER::new(@_);
     return $self;
 }
 
@@ -122,36 +134,21 @@ This is configured by the following in the C<config> block in koha-conf.xml:
 sub get_elasticsearch_params {
     my ($self) = @_;
 
-    # Copy the hash so that we're not modifying the original
-    my $conf = C4::Context->config('elasticsearch');
-    die "No 'elasticsearch' block is defined in koha-conf.xml.\n" if ( !$conf );
-    my $es = { %{ $conf } };
+    my $conf;
+    try {
+        $conf = _read_configuration();
+    } catch {
+        if ( ref($_) eq 'Koha::Exceptions::Config::MissingEntry' ) {
+            croak($_->message);
+        }
+    };
+    # Extract relevant parts of configuration
+    my $params = {
+        nodes => $conf->{nodes}
+    };
+    $params->{cxn_pool} //= 'Static';
 
-    # Helpfully, the multiple server lines end up in an array for us anyway
-    # if there are multiple ones, but not if there's only one.
-    my $server = $es->{server};
-    delete $es->{server};
-    if ( ref($server) eq 'ARRAY' ) {
-
-        # store it called 'nodes' (which is used by newer Search::Elasticsearch)
-        $es->{nodes} = $server;
-    }
-    elsif ($server) {
-        $es->{nodes} = [$server];
-    }
-    else {
-        die "No elasticsearch servers were specified in koha-conf.xml.\n";
-    }
-    die "No elasticsearch index_name was specified in koha-conf.xml.\n"
-      if ( !$es->{index_name} );
-    # Append the name of this particular index to our namespace
-    $es->{index_name} .= '_' . $self->index;
-
-    $es->{key_prefix} = 'es_';
-    $es->{cxn_pool} //= 'Static';
-    $es->{request_timeout} //= 60;
-
-    return $es;
+    return $params;
 }
 
 =head2 get_elasticsearch_settings
@@ -1233,9 +1230,11 @@ sub _read_configuration {
     my $configuration;
 
     my $conf = C4::Context->config('elasticsearch');
-    Koha::Exceptions::Config::MissingEntry->throw(
-        "Missing 'elasticsearch' block in config file")
-      unless defined $conf;
+    unless ( defined $conf ) {
+        Koha::Exceptions::Config::MissingEntry->throw(
+            "Missing <elasticsearch> entry in koha-conf.xml"
+        );
+    }
 
     if ( $conf && $conf->{server} ) {
         my $nodes = $conf->{server};
@@ -1248,7 +1247,8 @@ sub _read_configuration {
     }
     else {
         Koha::Exceptions::Config::MissingEntry->throw(
-            "Missing 'server' entry in config file for elasticsearch");
+            "Missing <elasticsearch>/<server> entry in koha-conf.xml"
+        );
     }
 
     if ( defined $conf->{index_name} ) {
@@ -1256,7 +1256,8 @@ sub _read_configuration {
     }
     else {
         Koha::Exceptions::Config::MissingEntry->throw(
-            "Missing 'index_name' entry in config file for elasticsearch");
+            "Missing <elasticsearch>/<index_name> entry in koha-conf.xml",
+        );
     }
 
     return $configuration;

Koha/SearchEngine/Elasticsearch/Browse.pm

@@ -106,9 +106,8 @@ sub browse {
 
     my $query = $self->_build_query($prefix, $field, $options);
     my $elasticsearch = $self->get_elasticsearch();
-    my $conf = $self->get_elasticsearch_params();
     my $results = $elasticsearch->search(
-        index => $conf->{index_name},
+        index => $self->index_name,
         body => $query
     );
 

Koha/SearchEngine/Elasticsearch/Indexer.pm

@@ -95,8 +95,7 @@ Arrayref of C<MARC::Record>s.
 
 sub update_index {
     my ($self, $biblionums, $records) = @_;
-    my $conf = $self->get_elasticsearch_params();
-    my $elasticsearch = $self->get_elasticsearch();
+
     my $documents = $self->marc_records_to_documents($records);
     my @body;
 
@@ -112,8 +111,9 @@ sub update_index {
     }
     my $response;
     if (@body) {
+        my $elasticsearch = $self->get_elasticsearch();
         $response = $elasticsearch->bulk(
-            index => $conf->{index_name},
+            index => $self->index_name,
             type => 'data', # is just hard coded in Indexer.pm?
             body => \@body
         );
@@ -235,14 +235,13 @@ failes.
 
 sub update_mappings {
     my ($self) = @_;
-    my $conf = $self->get_elasticsearch_params();
     my $elasticsearch = $self->get_elasticsearch();
     my $mappings = $self->get_elasticsearch_mappings();
 
     foreach my $type (keys %{$mappings}) {
         try {
             my $response = $elasticsearch->indices->put_mapping(
-                index => $conf->{index_name},
+                index => $self->index_name,
                 type => $type,
                 body => {
                     $type => $mappings->{$type}
@@ -251,8 +250,9 @@ sub update_mappings {
         } catch {
             $self->set_index_status_recreate_required();
             my $reason = $_[0]->{vars}->{body}->{error}->{reason};
+            my $index_name = $self->index_name;
             Koha::Exceptions::Exception->throw(
-                error => "Unable to update mappings for index \"$conf->{index_name}\". Reason was: \"$reason\". Index needs to be recreated and reindexed",
+                error => "Unable to update mappings for index \"$index_name\". Reason was: \"$reason\". Index needs to be recreated and reindexed",
             );
         };
     }
@@ -287,11 +287,9 @@ sub delete_index {
     my ($self, $biblionums) = @_;
 
     my $elasticsearch = $self->get_elasticsearch();
-    my $conf  = $self->get_elasticsearch_params();
-
     my @body = map { { delete => { _id => $_ } } } @{$biblionums};
     my $result = $elasticsearch->bulk(
-        index => $conf->{index_name},
+        index => $self->index_name,
         type => 'data',
         body => \@body,
     );
@@ -321,9 +319,8 @@ Drops the index from the Elasticsearch server.
 sub drop_index {
     my ($self) = @_;
     if ($self->index_exists) {
-        my $conf = $self->get_elasticsearch_params();
         my $elasticsearch = $self->get_elasticsearch();
-        $elasticsearch->indices->delete(index => $conf->{index_name});
+        $elasticsearch->indices->delete(index => $self->index_name);
         $self->set_index_status_recreate_required();
     }
 }
@@ -336,11 +333,10 @@ Creates the index (including mappings) on the Elasticsearch server.
 
 sub create_index {
     my ($self) = @_;
-    my $conf = $self->get_elasticsearch_params();
     my $settings = $self->get_elasticsearch_settings();
     my $elasticsearch = $self->get_elasticsearch();
     $elasticsearch->indices->create(
-        index => $conf->{index_name},
+        index => $self->index_name,
         body => {
             settings => $settings
         }
@@ -357,10 +353,9 @@ empty string to indicate whether index exists or not.
 
 sub index_exists {
     my ($self) = @_;
-    my $conf = $self->get_elasticsearch_params();
     my $elasticsearch = $self->get_elasticsearch();
     return $elasticsearch->indices->exists(
-        index => $conf->{index_name},
+        index => $self->index_name,
     );
 }
 

Koha/SearchEngine/Elasticsearch/Search.pm

@@ -82,7 +82,6 @@ Returns
 sub search {
     my ($self, $query, $page, $count, %options) = @_;
 
-    my $params = $self->get_elasticsearch_params();
     # 20 is the default number of results per page
     $query->{size} = $count // 20;
     # ES doesn't want pages, it wants a record to start from.
@@ -95,7 +94,7 @@ sub search {
     my $elasticsearch = $self->get_elasticsearch();
     my $results = eval {
         $elasticsearch->search(
-            index => $params->{index_name},
+            index => $self->index_name,
             body => $query
         );
     };
@@ -117,12 +116,11 @@ faster than pulling all the data in, usually.
 sub count {
     my ( $self, $query ) = @_;
     my $elasticsearch = $self->get_elasticsearch();
-    my $conf = $self->get_elasticsearch_params();
 
     # TODO: Probably possible to exclude results
     # and just return number of hits
     my $result = $elasticsearch->search(
-        index => $conf->{index_name},
+        index => $self->index_name,
         body => $query
     );
 
@@ -407,16 +405,15 @@ sub max_result_window {
     my ($self) = @_;
 
     my $elasticsearch = $self->get_elasticsearch();
-    my $conf = $self->get_elasticsearch_params();
 
     my $response = $elasticsearch->indices->get_settings(
-        index => $conf->{index_name},
+        index => $self->index_name,
         flat_settings => 'true',
         include_defaults => 'true'
     );
 
-    my $max_result_window = $response->{$conf->{index_name}}->{settings}->{'index.max_result_window'};
-    $max_result_window //= $response->{$conf->{index_name}}->{defaults}->{'index.max_result_window'};
+    my $max_result_window = $response->{$self->index_name}->{settings}->{'index.max_result_window'};
+    $max_result_window //= $response->{$self->index_name}->{defaults}->{'index.max_result_window'};
 
     return $max_result_window;
 }

t/Koha/SearchEngine/Elasticsearch.t

@@ -46,7 +46,7 @@ subtest '_read_configuration() tests' => sub {
       'Configuration problem, exception thrown';
     is(
         $@->message,
-        "Missing 'elasticsearch' block in config file",
+        "Missing <elasticsearch> entry in koha-conf.xml",
         'Exception message is correct'
     );
 
@@ -59,7 +59,7 @@ subtest '_read_configuration() tests' => sub {
       'Configuration problem, exception thrown';
     is(
         $@->message,
-        "Missing 'server' entry in config file for elasticsearch",
+        "Missing <elasticsearch>/<server> entry in koha-conf.xml",
         'Exception message is correct'
     );
 
@@ -72,7 +72,7 @@ subtest '_read_configuration() tests' => sub {
       'Configuration problem, exception thrown';
     is(
         $@->message,
-        "Missing 'index_name' entry in config file for elasticsearch",
+        "Missing <elasticsearch>/<index_name> entry in koha-conf.xml",
         'Exception message is correct'
     );
 

t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t

@@ -32,12 +32,12 @@ use_ok('Koha::SearchEngine::Elasticsearch::Indexer');
 subtest 'create_index() tests' => sub {
     plan tests => 5;
     my $se = Test::MockModule->new( 'Koha::SearchEngine::Elasticsearch' );
-    $se->mock( 'get_elasticsearch_params', sub {
+    $se->mock( '_read_configuration', sub {
             my ($self, $sub ) = @_;
-            my $method = $se->original( 'get_elasticsearch_params' );
-            my $params = $method->( $self );
-            $params->{index_name} .= '__test';
-            return $params;
+            my $method = $se->original( '_read_configuration' );
+            my $conf = $method->( $self );
+            $conf->{index_name} .= '__test';
+            return $conf;
         });
 
     my $indexer;

t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t

@@ -117,7 +117,7 @@ SKIP: {
     is ($searcher->max_result_window, 10000, 'By default, max_result_window is 10000');
 
     $searcher->get_elasticsearch()->indices->put_settings(
-        index => $searcher->get_elasticsearch_params()->{index_name},
+        index => $searcher->index_name,
         body => {
             'index' => {
                 'max_result_window' => 12000,