From e453c9ec3173bf73177f9365bc3b5b1ec9e4f5e7 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 Signed-off-by: Katrin Fischer --- C4/AuthoritiesMarc.pm | 2 +- C4/Biblio.pm | 11 +-- Koha/SearchEngine/Elasticsearch.pm | 30 ++++---- Koha/SearchEngine/Elasticsearch/QueryBuilder.pm | 2 +- admin/searchengine/elasticsearch/mappings.yaml | 24 ------- misc/search_tools/rebuild_elasticsearch.pl | 8 ++- .../Koha/SearchEngine/Elasticsearch.t | 81 +++++++--------------- 7 files changed, 54 insertions(+), 104 deletions(-) rename t/{ => db_dependent}/Koha/SearchEngine/Elasticsearch.t (94%) diff --git a/C4/AuthoritiesMarc.pm b/C4/AuthoritiesMarc.pm index bd25837c36..94fefcdd3c 100644 --- a/C4/AuthoritiesMarc.pm +++ b/C4/AuthoritiesMarc.pm @@ -628,7 +628,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 da86e62fdd..e92f76c4f0 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -2497,8 +2497,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 @@ -2518,13 +2518,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 a43b864145..f3e663e54a 100644 --- a/Koha/SearchEngine/Elasticsearch.pm +++ b/Koha/SearchEngine/Elasticsearch.pm @@ -234,10 +234,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}; } @@ -527,8 +527,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, { @@ -580,13 +594,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}; @@ -609,13 +616,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 8e2adf8136..d49de32eea 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 fede84ca34..6ae3d392fa 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.11.0