From 41daae5c723c027c6bed8652b478d937d26cb423 Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 47185db980..d54d1de4d4 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -1295,12 +1295,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 2c41c132bc..cfb50b1131 100644 --- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm +++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ -1062,14 +1062,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 e551a29a22..311ba11bad 100755 --- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t +++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t @@ -760,7 +760,7 @@ subtest 'build_query_compat() SearchLimitLibrary tests' => sub { }; subtest "Handle search filters" => sub { - plan tests => 4; + plan tests => 7; my $qb; @@ -772,15 +772,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 . + +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