From 9dcc4da146e6d042e13d2ddae43e3284ba076f30 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 28 Apr 2020 12:19:56 +0000 Subject: [PATCH] Bug 25273: Make match-heading rely on authority type configuration The match-heading field is a special field used only by the linker, not accessible to staff or patrons via the interface. This field is used to store the constructed 'search form' used for matching bib headings to authority fields. In bug 24269 I attempted to use the mappings defined in the inferface and also inject the search term. This did not work as too many subfields were indexed on their own and leading to false matches. In this bug we remove the mappings for this field, and create it ourselves during the indexing process. The C4::Headings module is still used to generate the correct form, however, the mappings are set based on the authority types in the system. This gives the user the ability to add new typoes, but prevents mapping changes from breaking linker functionality To test: 1 - Start form a sample database with ElasticSearch working 2 - Download via Z39.50 2 authorities, one of which is a narrower heading of the other, e.g.: Waterworks Waterworks - Costs 3 - Place a heading for the broader term in a record. e.g. Waterworks In 650$a, without the cataloguing authority plugin. We don't want the link created now. You need syspref BiblioAddsAuthorities => allow 4 - Make sure linker is set to default 5 - Attempt to link the records misc/link_bibs_to_authorities.pl 6 - Linking fails 7 - Apply patch 8 - refresh index settings (if using a custom file, remove 'match-heading') You can reset mappings in the UI or run this: misc/search_tools/rebuild_elasticsearch.pl -v -d -r 9 - Reindex ES 10 - Try to link again 11 - It succeeds! 12 - Run the tests prove t/db_dependent/Koha/SearchEngine/Elasticsearch.t Signed-off-by: Victor Grousset/tuxayo --- C4/AuthoritiesMarc.pm | 2 +- C4/Biblio.pm | 11 +-- Koha/SearchEngine/Elasticsearch.pm | 31 +++---- .../Elasticsearch/QueryBuilder.pm | 2 +- .../searchengine/elasticsearch/mappings.yaml | 24 ------ misc/search_tools/rebuild_elasticsearch.pl | 8 +- .../Koha/SearchEngine/Elasticsearch.t | 81 ++++++------------- 7 files changed, 55 insertions(+), 104 deletions(-) rename t/{ => db_dependent}/Koha/SearchEngine/Elasticsearch.t (94%) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index 9d059a0266..a1c877d240 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -629,7 +629,7 @@ sub AddAuthority { $record->insert_fields_ordered( MARC::Field->new( '001', $authid ) ); # Update $dbh->do( "UPDATE auth_header SET authtypecode=?, marc=?, marcxml=? WHERE authid=?", undef, $authtypecode, $record->as_usmarc, $record->as_xml_record($format), $authid ) or die $DBI::errstr; - ModZebra( $authid, 'specialUpdate', 'authorityserver', $record ); + ModZebra( $authid, 'specialUpdate', 'authorityserver', { record => $record, authtypecode => $authtypecode } ); return ( $authid ); } diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 9eb3696a54..2e991427b8 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -2546,8 +2546,8 @@ $op is specialUpdate or recordDelete, and is used to know what we want to do $server is the server that we want to update -$record is the update MARC record if it's available. If it's not supplied -and is needed, it'll be loaded from the database. +$record is a hash including the authtypecode and updated MARC record. This is only used for +Elasticsearch authorities. If it's not supplied a biblio will be loaded from the database. =cut @@ -2567,13 +2567,14 @@ sub ModZebra { } ); if ( $op eq 'specialUpdate' ) { - unless ($record) { + if ($record) { + $indexer->update_index_background( [$biblionumber], [$record] ); + } else { $record = GetMarcBiblio({ biblionumber => $biblionumber, embed_items => 1 }); + $indexer->update_index_background( [$biblionumber], [{ record => $record }] ); } - my $records = [$record]; - $indexer->update_index_background( [$biblionumber], [$record] ); } elsif ( $op eq 'recordDelete' ) { $indexer->delete_index_background( [$biblionumber] ); diff --git a/Koha/SearchEngine/Elasticsearch.pm b/Koha/SearchEngine/Elasticsearch.pm index 6616671325..890713b540 100644 --- a/Koha/SearchEngine/Elasticsearch.pm +++ b/Koha/SearchEngine/Elasticsearch.pm @@ -75,6 +75,7 @@ sub new { my $self = $class->SUPER::new(@_); # Check for a valid index Koha::Exceptions::MissingParameter->throw('No index name provided') unless $self->index; + return $self; } @@ -238,10 +239,10 @@ sub get_elasticsearch_mappings { } } ); + $mappings->{data}{properties}{ 'match-heading' } = _get_elasticsearch_field_config('search', 'text') if $self->index eq 'authorities'; $all_mappings{$self->index} = $mappings; } $self->sort_fields(\%{$sort_fields{$self->index}}); - return $all_mappings{$self->index}; } @@ -531,8 +532,22 @@ sub marc_records_to_documents { my @record_documents; + my %auth_match_headings; + if( $self->index eq 'authorities' ){ + my @auth_types = Koha::Authority::Types->search(); + %auth_match_headings = map { $_->authtypecode => $_->auth_tag_to_report } @auth_types; + } + foreach my $record (@{$records}) { my $record_document = {}; + + if ( defined $record->{authtypecode} ){ + my $field = $record->{record}->field( $auth_match_headings{ $record->{authtypecode} } ); + my $heading = C4::Heading->new_from_field( $field, undef, 1 ); #new auth heading + push @{$record_document->{'match-heading'}}, $heading->search_form if $heading; + } + $record = $record->{record}; + my $mappings = $rules->{leader}; if ($mappings) { $self->_process_mappings($mappings, $record->leader(), $record_document, { @@ -584,13 +599,6 @@ sub marc_records_to_documents { } ); } - if ( @{$mappings} && grep { $_->[0] eq 'match-heading'} @{$mappings} ){ - # Used by the authority linker the match-heading field requires a specific syntax - # that is specified in C4/Heading - my $heading = C4::Heading->new_from_field( $field, undef, 1 ); #new auth heading - next unless $heading; - push @{$record_document->{'match-heading'}}, $heading->search_form; - } } my $subfields_join_mappings = $data_field_rules->{subfields_join}; @@ -613,13 +621,6 @@ sub marc_records_to_documents { } ); } - if ( grep { $_->[0] eq 'match-heading' } @{$subfields_join_mappings->{$subfields_group}} ){ - # Used by the authority linker the match-heading field requires a specific syntax - # that is specified in C4/Heading - my $heading = C4::Heading->new_from_field( $field, undef, 1 ); #new auth heading - next unless $heading; - push @{$record_document->{'match-heading'}}, $heading->search_form; - } } } } diff --git a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm index 1bbbf079b8..285400550e 100644 --- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm +++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ -476,7 +476,7 @@ sub build_authorities_query_compat { $m = exists $koha_to_index_name->{$m} ? $koha_to_index_name->{$m} : $m; push @indexes, $m; - warn "Unknown search field $m in marclist" unless (defined $mappings->{data}->{properties}->{$m} || $m eq ''); + warn "Unknown search field $m in marclist" unless (defined $mappings->{data}->{properties}->{$m} || $m eq '' || $m eq 'match-heading'); } for ( my $i = 0 ; $i < @$value ; $i++ ) { next unless $value->[$i]; #clean empty form values, ES doesn't like undefined searches diff --git a/admin/searchengine/elasticsearch/mappings.yaml b/admin/searchengine/elasticsearch/mappings.yaml index b72fe18be4..847647e0fd 100644 --- a/admin/searchengine/elasticsearch/mappings.yaml +++ b/admin/searchengine/elasticsearch/mappings.yaml @@ -695,30 +695,6 @@ authorities: sort: ~ suggestible: '' type: '' - Match-heading: - label: Match-heading - mappings: - - facet: '' - marc_field: 100(abcdefghjklmnopqrstvxyz) - marc_type: marc21 - sort: ~ - suggestible: '' - - facet: '' - marc_field: 111(acdefghjklnpqstvxyz) - marc_type: marc21 - sort: ~ - suggestible: '' - - facet: '' - marc_field: 100(abcdefghjklmnopqrstvxyz) - marc_type: normarc - sort: ~ - suggestible: '' - - facet: '' - marc_field: 111(acdefghjklnpqstvxyz) - marc_type: normarc - sort: ~ - suggestible: '' - type: '' Match-heading-see-from: label: Match-heading-see-from mappings: diff --git a/misc/search_tools/rebuild_elasticsearch.pl b/misc/search_tools/rebuild_elasticsearch.pl index 6cff2eae2e..8bb6110ba1 100755 --- a/misc/search_tools/rebuild_elasticsearch.pl +++ b/misc/search_tools/rebuild_elasticsearch.pl @@ -275,8 +275,10 @@ sub _do_reindex { my $commit_count = $commit; my ( @id_buffer, @commit_buffer ); while ( my $record = $next->() ) { - my $id = $record->id // $record->authid; - my $record = $record->record; + my $id = $record->id; + my $record_prepped; + $record_prepped->{authtypecode} = $record->authtypecode if ref $record eq 'Koha::MetadataRecord::Authority'; + $record_prepped->{record} = $record->record; $count++; if ( $verbose == 1 ) { _log( 1, "$count records processed\n" ) if ( $count % 1000 == 0); @@ -285,7 +287,7 @@ sub _do_reindex { } push @id_buffer, $id; - push @commit_buffer, $record; + push @commit_buffer, $record_prepped; if ( !( --$commit_count ) ) { _log( 1, "Committing $commit records...\n" ); my $response = $indexer->update_index( \@id_buffer, \@commit_buffer ); diff --git a/t/Koha/SearchEngine/Elasticsearch.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch.t similarity index 94% rename from t/Koha/SearchEngine/Elasticsearch.t rename to t/db_dependent/Koha/SearchEngine/Elasticsearch.t index a4608f66f2..8a4cb8f1d8 100644 --- a/t/Koha/SearchEngine/Elasticsearch.t +++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch.t @@ -21,6 +21,7 @@ use Test::More tests => 6; use Test::Exception; use t::lib::Mocks; +use t::lib::TestBuilder; use Test::MockModule; @@ -31,6 +32,9 @@ use List::Util qw( any ); use Koha::SearchEngine::Elasticsearch; use Koha::SearchEngine::Elasticsearch::Search; +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; + subtest '_read_configuration() tests' => sub { plan tests => 10; @@ -336,7 +340,10 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests' MARC::Field->new('999', '', '', c => '1234568'), MARC::Field->new('952', '', '', 0 => 1, g => 'string where should be numeric', o => $long_callno), ); - my $records = [$marc_record_1, $marc_record_2]; + my $records = [ + { record => $marc_record_1 }, + { record => $marc_record_2 }, + ]; $see->get_elasticsearch_mappings(); #sort_fields will call this and use the actual db values unless we call it first @@ -503,7 +510,7 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () tests' $large_marc_record->append_fields($item_field); } - $docs = $see->marc_records_to_documents([$large_marc_record]); + $docs = $see->marc_records_to_documents([{ record => $large_marc_record }]); is($docs->[0]->{marc_format}, 'MARCXML', 'For record exceeding max record size marc_format should be set correctly'); @@ -616,7 +623,10 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents_array () t MARC::Field->new('999', '', '', c => '1234568'), MARC::Field->new('952', '', '', 0 => 1, g => 'string where should be numeric'), ); - my $records = [$marc_record_1, $marc_record_2]; + my $records = [ + { record => $marc_record_1 }, + { record => $marc_record_2 }, + ]; $see->get_elasticsearch_mappings(); #sort_fields will call this and use the actual db values unless we call it first @@ -642,6 +652,12 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () authori t::lib::Mocks::mock_preference('marcflavour', 'MARC21'); t::lib::Mocks::mock_preference('ElasticsearchMARCFormat', 'ISO2709'); + my $builder = t::lib::TestBuilder->new; + my $auth_type = $builder->build_object({ class => 'Koha::Authority::Types', value =>{ + auth_tag_to_report => '150' + } + }); + my @mappings = ( { name => 'match', @@ -652,57 +668,7 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () authori sort => 0, marc_type => 'marc21', marc_field => '150(ae)', - }, - { - name => 'heading', - type => 'string', - facet => 0, - suggestible => 0, - searchable => 1, - sort => 0, - marc_type => 'marc21', - marc_field => '150a', - }, - { - name => 'heading', - type => 'string', - facet => 0, - suggestible => 0, - searchable => 1, - sort => 0, - marc_type => 'marc21', - marc_field => '150(ae)', - }, - { - name => 'heading-main', - type => 'string', - facet => 0, - suggestible => 0, - searchable => 1, - sort => 0, - marc_type => 'marc21', - marc_field => '150a', - }, - { - name => 'heading', - type => 'string', - facet => 0, - suggestible => 0, - searchable => 1, - sort => 0, - marc_type => 'marc21', - marc_field => '150', - }, - { - name => 'match-heading', - type => 'string', - facet => 0, - suggestible => 0, - searchable => 1, - sort => 0, - marc_type => 'marc21', - marc_field => '150', - }, + } ); my $se = Test::MockModule->new('Koha::SearchEngine::Elasticsearch'); @@ -735,7 +701,10 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () authori $marc_record_2->append_fields( MARC::Field->new('150', '', '', a => 'Subject', v => 'Genresubdiv', z => 'Geosubdiv', x => 'Generalsubdiv', e => 'wrongsubdiv' ), ); - my $records = [$marc_record_1, $marc_record_2]; + my $records = [ + { record => $marc_record_1, authtypecode=> $auth_type->authtypecode }, + { record => $marc_record_2, authtypecode=> $auth_type->authtypecode }, + ]; $see->get_elasticsearch_mappings(); #sort_fields will call this and use the actual db values unless we call it first @@ -752,3 +721,5 @@ subtest 'Koha::SearchEngine::Elasticsearch::marc_records_to_documents () authori "Second record match-heading should contain the correctly formatted heading without wrong subfield" ); }; + +$schema->storage->txn_rollback; -- 2.26.2