From ea988f1bd1115ed2df27b0154e6b605b91ebce27 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Thu, 19 Mar 2020 09:38:52 +0100 Subject: [PATCH] Bug 24902: Join different mc- limits with AND (elasticsearch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the advanced search form, you can enable several limits using syspref AdvancedSearchTypes (namely itemtypes, shelving locations, collections) When used, the resulting query parts end up being joined with OR, even if the field is different. That means that if you pick "Book" under itemtypes tab, and "Fiction" under collection tab, it will search "itype:BOOK OR ccode:FIC". It should be AND. For instance, if you select: Itemtypes: ✓ Book ✓ DVD Location: ✓ Child ✓ Adult it should search: itype:(Book OR DVD) AND location:(Child OR Adult) Test plan: 0. Do not apply the patch yet 1. Enable elasticsearch 2. Set syspref AdvancedSearchTypes = 'itemtypes|loc|ccode' 3. Create a new itemtype and a new authorised value for categories LOC and CCODE 4. Create a biblio with the new itemtype, another biblio with the new location, another biblio with the new collection, and again another biblio with the new itemtype, location and collection 5. Verify that you can find these new biblio records using only the "advanced search types" in the advanced search form 6. In the advanced search form, pick all 3 limits (itemtype, location, collection) and verify that it returns the 4 records. 7. Apply the patch 8. Repeat step 6, it should now return only the biblio that satisfies all criteria 9. Verify that if you select more than one {itemtype|location|collection} it still returns results that satisfies any selected criteria 10. prove t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t --- .../Elasticsearch/QueryBuilder.pm | 26 ++++++++++------ .../SearchEngine/ElasticSearch/QueryBuilder.t | 31 ++++++++++++++++++- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm index 166121fa11..1bbbf079b8 100644 --- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm +++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ -853,16 +853,22 @@ sub _join_queries { map { s/^mc-//r } grep { defined($_) && $_ ne '' && $_ =~ /^mc-/ } @parts; return () unless @norm_parts + @mc_parts; return ( @norm_parts, @mc_parts )[0] if @norm_parts + @mc_parts == 1; - my $grouped_mc = - @mc_parts ? '(' . ( join ' OR ', map { "($_)" } @mc_parts ) . ')' : (); - - # Handy trick: $x || () inside a join means that if $x ends up as an - # empty string, it gets replaced with (), which makes join ignore it. - # (bad effect: this'll also happen to '0', this hopefully doesn't matter - # in this case.) - join( ' AND ', - join( ' AND ', map { "($_)" } @norm_parts ) || (), - $grouped_mc || () ); + + # Group limits by field, so they can be OR'ed together + my %mc_limits; + foreach my $mc_part (@mc_parts) { + my ($field, $value) = split /:/, $mc_part, 2; + $mc_limits{$field} //= []; + push @{ $mc_limits{$field} }, $value; + } + + @mc_parts = map { + sprintf('%s:(%s)', $_, join (' OR ', @{ $mc_limits{$_} })); + } sort keys %mc_limits; + + @norm_parts = map { "($_)" } @norm_parts; + + return join( ' AND ', @norm_parts, @mc_parts); } =head2 _make_phrases diff --git a/t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t b/t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t index 3bd5e4244a..2057138431 100644 --- a/t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t +++ b/t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 6; use t::lib::Mocks; use_ok('Koha::SearchEngine::Elasticsearch::QueryBuilder'); @@ -223,4 +223,33 @@ subtest '_clean_search_term() tests' => sub { is($res, 'test another part', 'unbalanced curly brackets replaced correctly'); }; +subtest '_join_queries' => sub { + plan tests => 6; + + my $params = { + index => $Koha::SearchEngine::Elasticsearch::BIBLIOS_INDEX, + }; + my $qb = Koha::SearchEngine::Elasticsearch::QueryBuilder->new($params); + + my $query; + + $query = $qb->_join_queries('foo'); + is($query, 'foo', 'should work with a single param'); + + $query = $qb->_join_queries(undef, '', 'foo', '', undef); + is($query, 'foo', 'should ignore undef or empty queries'); + + $query = $qb->_join_queries('foo', 'bar'); + is($query, '(foo) AND (bar)', 'should join queries with an AND'); + + $query = $qb->_join_queries('homebranch:foo', 'onloan:false'); + is($query, '(homebranch:foo) AND (onloan:false)', 'should also work when field is specified'); + + $query = $qb->_join_queries('homebranch:foo', 'mc-itype:BOOK', 'mc-itype:EBOOK'); + is($query, '(homebranch:foo) AND itype:(BOOK OR EBOOK)', 'should join with OR when using an "mc-" field'); + + $query = $qb->_join_queries('homebranch:foo', 'mc-itype:BOOK', 'mc-itype:EBOOK', 'mc-location:SHELF'); + is($query, '(homebranch:foo) AND itype:(BOOK OR EBOOK) AND location:(SHELF)', 'should join "mc-" parts with AND if not the same field'); +}; + 1; -- 2.20.1