From b92742bcdd77a8156bd4e0b484e280c699275725 Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 12 Mar 2020 11:52:18 +0100 Subject: [PATCH] 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. --- Koha/SearchEngine/Elasticsearch.pm | 75 ++++++++++--------- Koha/SearchEngine/Elasticsearch/Browse.pm | 3 +- Koha/SearchEngine/Elasticsearch/Indexer.pm | 24 +++--- Koha/SearchEngine/Elasticsearch/Search.pm | 13 ++-- t/Koha/SearchEngine/Elasticsearch.t | 6 +- .../Koha/SearchEngine/Elasticsearch/Indexer.t | 10 +-- .../Koha/SearchEngine/Elasticsearch/Search.t | 2 +- 7 files changed, 62 insertions(+), 71 deletions(-) diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm index 6b64f97c59..3eaabca5b6 100644 --- a/Koha/SearchEngine/Elasticsearch.pm +++ b/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 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 } }; - - # 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; + 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'; - return $es; + return $params; } =head2 get_elasticsearch_settings @@ -1231,9 +1228,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 entry in koha-conf.xml" + ); + } if ( $conf && $conf->{server} ) { my $nodes = $conf->{server}; @@ -1246,7 +1245,8 @@ sub _read_configuration { } else { Koha::Exceptions::Config::MissingEntry->throw( - "Missing 'server' entry in config file for elasticsearch"); + "Missing / entry in koha-conf.xml" + ); } if ( defined $conf->{index_name} ) { @@ -1254,7 +1254,8 @@ sub _read_configuration { } else { Koha::Exceptions::Config::MissingEntry->throw( - "Missing 'index_name' entry in config file for elasticsearch"); + "Missing / entry in koha-conf.xml", + ); } return $configuration; diff --git a/Koha/SearchEngine/Elasticsearch/Browse.pm b/Koha/SearchEngine/Elasticsearch/Browse.pm index 1153e68ac5..85070730a7 100644 --- a/Koha/SearchEngine/Elasticsearch/Browse.pm +++ b/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 ); diff --git a/Koha/SearchEngine/Elasticsearch/Indexer.pm b/Koha/SearchEngine/Elasticsearch/Indexer.pm index aa50bc38ca..8f70817a6c 100644 --- a/Koha/SearchEngine/Elasticsearch/Indexer.pm +++ b/Koha/SearchEngine/Elasticsearch/Indexer.pm @@ -96,8 +96,6 @@ Arrayref of Cs. 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; @@ -113,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 ); @@ -236,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} @@ -252,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", ); }; } @@ -288,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, ); @@ -322,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(); } } @@ -337,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 } @@ -358,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, ); } diff --git a/Koha/SearchEngine/Elasticsearch/Search.pm b/Koha/SearchEngine/Elasticsearch/Search.pm index 60af359227..ff56ad1174 100644 --- a/Koha/SearchEngine/Elasticsearch/Search.pm +++ b/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; } diff --git a/t/Koha/SearchEngine/Elasticsearch.t b/t/Koha/SearchEngine/Elasticsearch.t index a4608f66f2..fede84ca34 100644 --- a/t/Koha/SearchEngine/Elasticsearch.t +++ b/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 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 / 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 / entry in koha-conf.xml", 'Exception message is correct' ); diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t index 4ce73d8875..24f377df40 100644 --- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t +++ b/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; diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t index 82ab287aba..e7d3b2ae62 100644 --- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/Search.t +++ b/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, -- 2.20.1