Bugzilla – Attachment 95001 Details for
Bug 23719
Record matching for authorities using defined fields is broken under ES
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23719: (follow-up) Add warn when passed invalid search field in marclist
Bug-23719-follow-up-Add-warn-when-passed-invalid-s.patch (text/plain), 5.15 KB, created by
Nick Clemens (kidclamp)
on 2019-11-04 12:33:53 UTC
(
hide
)
Description:
Bug 23719: (follow-up) Add warn when passed invalid search field in marclist
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2019-11-04 12:33:53 UTC
Size:
5.15 KB
patch
obsolete
>From 97304a392663fdbe21c64c9cb9f041197745c5a5 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Thu, 31 Oct 2019 14:34:53 +0000 >Subject: [PATCH] Bug 23719: (follow-up) Add warn when passed invalid search > field in marclist > >Note: I also remove warnings for undefined operation in this patch, is a trivial fix > >To test: >prove -v t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >--- > Koha/SearchEngine/Elasticsearch/QueryBuilder.pm | 10 ++++--- > .../Koha/SearchEngine/Elasticsearch/QueryBuilder.t | 31 ++++++++++++++++++---- > 2 files changed, 33 insertions(+), 8 deletions(-) > >diff --git a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm >index daf7e65688..4a7cede380 100644 >--- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm >+++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm >@@ -285,7 +285,7 @@ sub build_authorities_query { > > foreach my $s ( @{ $search->{searches} } ) { > my ( $wh, $op, $val ) = @{$s}{qw(where operator value)}; >- if ( $op eq 'is' || $op eq '=' || $op eq 'exact') { >+ if ( defined $op && ($op eq 'is' || $op eq '=' || $op eq 'exact') ) { > if ($wh) { > # Match the whole field, case insensitive, UTF normalized. > push @query_parts, { term => { "$wh.ci_raw" => $val } }; >@@ -304,7 +304,7 @@ sub build_authorities_query { > }; > } > } >- elsif ( $op eq 'start') { >+ elsif ( defined $op && $op eq 'start') { > # Match the prefix within a field for all searchable fields. > # Given that field data is "The quick brown fox" > # "The quick bro" will match, but not "quick bro" >@@ -464,6 +464,7 @@ sub build_authorities_query_compat { > # This turns the old-style many-options argument form into a more > # extensible hash form that is understood by L<build_authorities_query>. > my @searches; >+ my $mappings = $self->get_elasticsearch_mappings(); > > # Convert to lower case > $marclist = [map(lc, @{$marclist})]; >@@ -472,7 +473,10 @@ sub build_authorities_query_compat { > my @indexes; > # Make sure everything exists > foreach my $m (@$marclist) { >- push @indexes, exists $koha_to_index_name->{$m} ? $koha_to_index_name->{$m} : $m; >+ >+ $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 ''); > } > for ( my $i = 0 ; $i < @$value ; $i++ ) { > next unless $value->[$i]; #clean empty form values, ES doesn't like undefined searches >diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >index ccde4d911b..d55fdd3b9e 100644 >--- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >+++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >@@ -19,6 +19,7 @@ use Modern::Perl; > > use C4::Context; > use Test::Exception; >+use Test::Warn; > use t::lib::Mocks; > use t::lib::TestBuilder; > use Test::More tests => 6; >@@ -50,6 +51,10 @@ $se->mock( 'get_elasticsearch_mappings', sub { > type => 'text', > facet => 1 > }, >+ 'subject-heading-thesaurus' => { >+ type => 'text', >+ facet => 1 >+ }, > itemnumber => { > type => 'integer' > }, >@@ -59,12 +64,24 @@ $se->mock( 'get_elasticsearch_mappings', sub { > sortablenumber__sort => { > type => 'integer' > }, >- Heading => { >+ heading => { > type => 'text' > }, >- Heading__sort => { >+ 'heading-main' => { > type => 'text' >- } >+ }, >+ heading__sort => { >+ type => 'text' >+ }, >+ match => { >+ type => 'text' >+ }, >+ 'match-heading' => { >+ type => 'text' >+ }, >+ 'match-heading-see-from' => { >+ type => 'text' >+ }, > } > } > }; >@@ -92,7 +109,7 @@ my $clear_search_fields_cache = sub { > > subtest 'build_authorities_query_compat() tests' => sub { > >- plan tests => 56; >+ plan tests => 57; > > my $qb; > >@@ -186,7 +203,11 @@ subtest 'build_authorities_query_compat() tests' => sub { > ); > > # Authorities marclist check >- $query = $qb->build_authorities_query_compat( [ 'tomas','mainentry' ], undef, undef, ['contains'], [$search_term,$search_term], 'AUTH_TYPE', 'asc' ); >+ warning_like { >+ $query = $qb->build_authorities_query_compat( [ 'tomas','mainentry' ], undef, undef, ['contains'], [$search_term,$search_term], 'AUTH_TYPE', 'asc' ) >+ } >+ qr/Unknown search field tomas/, >+ "Warning for unknown field in marclist"; > is_deeply( > $query->{query}->{bool}->{must}[0]->{query_string}->{default_field}, > 'tomas', >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23719
:
93414
|
94680
|
94913
|
95001
|
95090
|
95091
|
99714