Bugzilla – Attachment 133376 Details for
Bug 17170
Add the ability to create 'saved searches' for use as filters when searching the catalog
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 17170: (follow-up) Correct handling of limits in ES
Bug-17170-follow-up-Correct-handling-of-limits-in-.patch (text/plain), 12.13 KB, created by
Nick Clemens (kidclamp)
on 2022-04-18 14:57:28 UTC
(
hide
)
Description:
Bug 17170: (follow-up) Correct handling of limits in ES
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2022-04-18 14:57:28 UTC
Size:
12.13 KB
patch
obsolete
>From 9693736daa317763a8cbb4843c7fcb05f6687d1e Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Mon, 18 Apr 2022 14:51:36 +0000 >Subject: [PATCH] Bug 17170: (follow-up) Correct handling of limits in ES > >This patch alters the code in the ES QueryBuilder. Reflecting >how things are handled in build_query_compat we clean the query, >but not the limits. In Zebra we simply recursivly call buildQuery, >but the ES query structure is more complex, so we handle the pieces >individually > >Additionally I add more tests to cover other cases >--- > C4/Search.pm | 7 ++- > .../Elasticsearch/QueryBuilder.pm | 12 +++-- > Koha/SearchFilter.pm | 10 ++-- > .../SearchEngine/Elasticsearch/QueryBuilder.t | 21 ++++++-- > .../Koha/SearchEngine/Zebra/QueryBuilder.t | 21 ++++++-- > t/db_dependent/Koha/SearchFilters.t | 54 +++++++++++++++++++ > 6 files changed, 105 insertions(+), 20 deletions(-) > create mode 100755 t/db_dependent/Koha/SearchFilters.t > >diff --git a/C4/Search.pm b/C4/Search.pm >index ca522fa8b4..3ec2552ccf 100644 >--- a/C4/Search.pm >+++ b/C4/Search.pm >@@ -1510,12 +1510,15 @@ sub buildQuery { > $limit .= "$this_limit"; > $limit_desc .= " $this_limit"; > } elsif ( $this_limit =~ '^search_filter:' ) { >+ # Here we will get the query as a string, append to the limits, and pass through buildQuery >+ # again to clean the terms and handle nested silters > $limit_cgi .= "&limit=" . uri_escape_utf8($this_limit); > my ($filter_id) = ( $this_limit =~ /^search_filter:(.*)$/ ); > my $search_filter = Koha::SearchFilters->find( $filter_id ); > next unless $search_filter; >- my $expanded = $search_filter->expand_filter; >- my ( $error, undef, undef, undef, undef, $fixed_limit, undef, undef, undef ) = buildQuery ( undef, undef, undef, $expanded, undef, undef, $lang); >+ my ($expanded_lim, $query_lim) = $search_filter->expand_filter; >+ push @$expanded_lim, $query_lim; >+ my ( $error, undef, undef, undef, undef, $fixed_limit, undef, undef, undef ) = buildQuery ( undef, undef, undef, $expanded_lim, undef, undef, $lang); > $limit .= " and " if $limit || $query; > $limit .= "$fixed_limit"; > $limit_desc .= " $limit"; >diff --git a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm >index aefe17270b..f26f51a4bd 100644 >--- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm >+++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm >@@ -1060,14 +1060,18 @@ sub _fix_limit_special_cases { > push @new_lim, "date-of-publication:[$start TO $end]"; > } > elsif( $l =~ /^search_filter:/ ){ >+ # Here we are going to get the query as a string, clean it, and take care of the part of the limit >+ # Calling build_query_compat here is avoided because we generate more complex query structures > my ($filter_id) = ( $l =~ /^search_filter:(.*)$/ ); > my $search_filter = Koha::SearchFilters->find( $filter_id ); > next unless $search_filter; >- my $expanded = $search_filter->expand_filter; >- foreach my $e ( @{$self->_fix_limit_special_cases($expanded)} ) { >- push @new_lim, $self->clean_search_term( $e ); >+ my ($expanded_lim,$query_lim) = $search_filter->expand_filter; >+ # In the case of nested filters we need to expand them all >+ foreach my $el ( @{$self->_fix_limit_special_cases($expanded_lim)} ){ >+ push @new_lim, $el; > } >-# push @new_lim, @{$self->_fix_limit_special_cases($expanded)}; >+ # We need to clean the query part as we have built a string from the original search >+ push @new_lim, $self->clean_search_term( $query_lim ); > } > elsif ( $l =~ /^yr,st-numeric=/ ) { > my ($date) = ( $l =~ /^yr,st-numeric=(.*)$/ ); >diff --git a/Koha/SearchFilter.pm b/Koha/SearchFilter.pm >index 493e1fc1ee..547e89a448 100644 >--- a/Koha/SearchFilter.pm >+++ b/Koha/SearchFilter.pm >@@ -43,10 +43,10 @@ sub to_api_mapping { > > =head3 expand_filter > >- my $expanded_filter = $filter->expand_filter; >+ my ($expanded_limit, $query_limit) = $filter->expand_filter; > >- Returns the filter as an arrayref of limit queries suitable to >- be passed to QueryBuilder >+ Returns the filter as an arrayref of limit queries, and the query parts combined >+ into a string suitable to be passed to QueryBuilder > > =cut > >@@ -74,9 +74,7 @@ sub expand_filter { > $query_limit .= $limit; > } > >- push @$limits, "(".$query_limit.")" if $query_limit; >- >- return $limits; >+ return ($limits, $query_limit); > } > > =head2 Internal methods >diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >index 5e210f2b79..373487fed3 100755 >--- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >+++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t >@@ -762,7 +762,7 @@ subtest 'build_query_compat() SearchLimitLibrary tests' => sub { > }; > > subtest "Handle search filters" => sub { >- plan tests => 4; >+ plan tests => 7; > > my $qb; > >@@ -774,15 +774,28 @@ subtest "Handle search filters" => sub { > my $filter = Koha::SearchFilter->new({ > name => "test", > query => q|{"operands":["cat","bat","rat"],"indexes":["kw","ti","au"],"operators":["AND","OR"]}|, >- limits => q|{"limits":["mc-itype,phr:BK","available"]}|, >+ limits => q|{"limits":["mc-itype,phr:BK","mc-itype,phr:MU","available"]}|, > })->store; > my $filter_id = $filter->id; > > my ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc ) = $qb->build_query_compat( undef, undef, undef, ["search_filter:$filter_id"] ); > >- is( $limit,q{(itype:("BK")) AND (onloan:false) AND (((cat) AND title:(bat) OR author:(rat)))},"Limit correctly formed"); >+ is( $limit,q{(onloan:false) AND ((cat) AND title:(bat) OR author:(rat)) AND itype:(("BK") OR ("MU"))},"Limit correctly formed"); > is( $limit_cgi,"&limit=search_filter%3A$filter_id","CGI limit is not expanded"); >- is( $limit_desc,q{(itype:("BK")) AND (onloan:false) AND (((cat) AND title:(bat) OR author:(rat)))},"Limit description is correctly expanded"); >+ is( $limit_desc,q{(onloan:false) AND ((cat) AND title:(bat) OR author:(rat)) AND itype:(("BK") OR ("MU"))},"Limit description is correctly expanded"); >+ >+ $filter = Koha::SearchFilter->new({ >+ name => "test", >+ query => q|{"operands":["su:biography"],"indexes":[],"operators":[]}|, >+ limits => q|{"limits":[]}|, >+ })->store; >+ $filter_id = $filter->id; >+ >+ ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc ) = $qb->build_query_compat( undef, undef, undef, ["search_filter:$filter_id"] ); >+ >+ is( $limit,q{(subject:biography)},"Limit correctly formed for ccl type query"); >+ is( $limit_cgi,"&limit=search_filter%3A$filter_id","CGI limit is not expanded"); >+ is( $limit_desc,q{(subject:biography)},"Limit description is correctly handled for ccl type query"); > > }; > >diff --git a/t/db_dependent/Koha/SearchEngine/Zebra/QueryBuilder.t b/t/db_dependent/Koha/SearchEngine/Zebra/QueryBuilder.t >index 2653e45acd..5a8fdb8f50 100755 >--- a/t/db_dependent/Koha/SearchEngine/Zebra/QueryBuilder.t >+++ b/t/db_dependent/Koha/SearchEngine/Zebra/QueryBuilder.t >@@ -98,7 +98,7 @@ subtest 'build_query_compat() SearchLimitLibrary tests' => sub { > }; > > subtest "Handle search filters" => sub { >- plan tests => 4; >+ plan tests => 7; > > my $qb; > >@@ -110,14 +110,27 @@ subtest "Handle search filters" => sub { > my $filter = Koha::SearchFilter->new({ > name => "test", > query => q|{"operands":["cat","bat","rat"],"indexes":["kw","ti","au"],"operators":["AND","OR"]}|, >- limits => q|{"limits":["mc-itype,phr:BK","available"]}|, >+ limits => q|{"limits":["mc-itype,phr:BK","mc-itype,phr:MU","available"]}|, > })->store; > my $filter_id = $filter->id; > > my ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc ) = $qb->build_query_compat( undef, undef, undef, ["search_filter:$filter_id"] ); > >- is( $limit,q{(kw=(cat) AND ti=(bat) OR au=(rat)) and (mc-itype,phr=BK) and (( (allrecords,AlwaysMatches='') and (not-onloan-count,st-numeric >= 1) and (lost,st-numeric=0) ))},"Limit correctly formed"); >+ is( $limit,q{kw=(cat) AND ti=(bat) OR au=(rat) and (mc-itype,phr=BK or mc-itype,phr=MU) and (( (allrecords,AlwaysMatches='') and (not-onloan-count,st-numeric >= 1) and (lost,st-numeric=0) ))},"Limit correctly formed"); > is( $limit_cgi,"&limit=search_filter%3A$filter_id","CGI limit is not expanded"); >- is( $limit_desc,q{(kw=(cat) AND ti=(bat) OR au=(rat)) and (mc-itype,phr=BK) and (( (allrecords,AlwaysMatches='') and (not-onloan-count,st-numeric >= 1) and (lost,st-numeric=0) ))},"Limit description is correctly expanded"); >+ is( $limit_desc,q{kw=(cat) AND ti=(bat) OR au=(rat) and (mc-itype,phr=BK or mc-itype,phr=MU) and (( (allrecords,AlwaysMatches='') and (not-onloan-count,st-numeric >= 1) and (lost,st-numeric=0) ))},"Limit description is correctly expanded"); >+ >+ $filter = Koha::SearchFilter->new({ >+ name => "test", >+ query => q|{"operands":["su:biography"],"indexes":[],"operators":[]}|, >+ limits => q|{"limits":[]}|, >+ })->store; >+ $filter_id = $filter->id; >+ >+ ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc ) = $qb->build_query_compat( undef, undef, undef, ["search_filter:$filter_id"] ); >+ >+ is( $limit,q{(su=biography)},"Limit correctly formed for ccl type query"); >+ is( $limit_cgi,"&limit=search_filter%3A$filter_id","CGI limit is not expanded"); >+ is( $limit_desc,q{(su=biography)},"Limit description is correctly handled for ccl type query"); > > }; >diff --git a/t/db_dependent/Koha/SearchFilters.t b/t/db_dependent/Koha/SearchFilters.t >new file mode 100755 >index 0000000000..03f79fd1c5 >--- /dev/null >+++ b/t/db_dependent/Koha/SearchFilters.t >@@ -0,0 +1,54 @@ >+#!/usr/bin/perl >+ >+# Copyright 2022 Koha Development team >+# >+# This file is part of Koha >+# >+# Koha is free software; you can redistribute it and/or modify it >+# under the terms of the GNU General Public License as published by >+# the Free Software Foundation; either version 3 of the License, or >+# (at your option) any later version. >+# >+# Koha is distributed in the hope that it will be useful, but >+# WITHOUT ANY WARRANTY; without even the implied warranty of >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+# GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License >+# along with Koha; if not, see <http://www.gnu.org/licenses>. >+ >+use Modern::Perl; >+ >+use Test::More tests => 6; >+ >+use Koha::SearchFilters; >+use Koha::Database; >+ >+use t::lib::TestBuilder; >+ >+my $schema = Koha::Database->new->schema; >+$schema->storage->txn_begin; >+ >+my $builder = t::lib::TestBuilder->new; >+my $nb_of_filters = Koha::SearchFilters->search->count; >+my $new_filter_1 = Koha::SearchFilter->new({ >+ name => "Test 1", >+ query => q|{"operators":["OR"],"indexes":["kw","kw","kw"],"operands":["street","bruce",""]}|, >+ limits => q|{"limits":["mc-itype,phr:BK","mc-itype,phr:MU"]}| >+})->store; >+my $new_filter_2 = Koha::SearchFilter->new({ name => "Test 2" })->store; >+ >+like( $new_filter_1->id, qr|^\d+$|, 'Adding a new filter should have set the id'); >+is( Koha::SearchFilters->search->count, $nb_of_filters + 2, 'The 2 filters should have been added' ); >+ >+my $retrieved_filter_2 = Koha::SearchFilters->find( $new_filter_2->id ); >+is( $retrieved_filter_2->name, $new_filter_2->name, 'Find a filter by id should return the correct filter' ); >+ >+$retrieved_filter_2->delete; >+is( Koha::SearchFilters->search->count, $nb_of_filters + 1, 'Delete should have deleted the filter' ); >+ >+my ($expanded_lim,$expanded_query) = $new_filter_1->expand_filter; >+is_deeply( $expanded_lim, ["mc-itype,phr:BK","mc-itype,phr:MU"], "Expanded limits is an arrayref of limits"); >+is( $expanded_query,"kw=(street) OR kw=(bruce)","Expanded query is a search string"); >+ >+$schema->storage->txn_rollback; >-- >2.30.2
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 17170
:
133263
|
133264
|
133265
|
133266
|
133267
|
133268
|
133269
|
133270
|
133361
|
133376
|
137027
|
137028
|
137029
|
137030
|
137031
|
137032
|
137033
|
137034
|
137035
|
137036
|
137186
|
137187
|
137188
|
137189
|
137190
|
137191
|
137192
|
137193
|
137194
|
137195
|
137808
|
137809
|
137810
|
137811
|
137812
|
137813
|
137814
|
137815
|
137816
|
137817
|
138505
|
138506
|
138507
|
138508
|
138509
|
138510
|
138511
|
138512
|
138513
|
138514
|
139948
|
139949
|
139950
|
139951
|
139952
|
139953
|
139954
|
139955
|
139956
|
139957
|
141228
|
141229
|
141230
|
141231
|
141232
|
141233
|
141234
|
141235
|
141236
|
141237
|
141316
|
141317
|
141318
|
141319
|
141320
|
141321
|
141322
|
141323
|
141324
|
141325
|
141326
|
141327
|
141328
|
141348
|
141349
|
141350
|
141351
|
141352
|
141353
|
141354
|
141355
|
141356
|
141357
|
141358
|
141359
|
141360
|
141422
|
141423
|
141424
|
141425
|
141426
|
141427
|
141428
|
141429
|
141430
|
141431
|
141432
|
141433
|
141434
|
141435
|
141436
|
141437
|
141438
|
141461
|
141462
|
142365
|
142367
|
142368
|
142369
|
142370
|
142371
|
142372
|
142373
|
142374
|
142375
|
142376
|
142377
|
142378
|
142379
|
142380
|
142381
|
142382
|
142383
|
142384
|
142385
|
142494
|
142498