From 5ca295738a2928afeadfe3bea3749c1c742528cc Mon Sep 17 00:00:00 2001 From: Galen Charlton Date: Thu, 24 Oct 2013 20:25:55 +0000 Subject: [PATCH 1/2] Bug 11137: regression tests for QP search field alias bug This patch adds regression tests to verify that Boolean searches using QueryParser function correctly. This patch also ensures that QP is correctly initalized when Search.t is run. To test: [1] Apply this patch and the following patch. [2] Verify that prove -v t/QueryParser.t works [3] Verify that prove -v t/db_dependent/Search.t works [4] (optional) instead of applying both patches at the same time, apply only the regression test patch and run the tests listed in steps 2 and 3. The following tests should fail: t/db_dependent/Search.t (Wstat: 512 Tests: 198 Failed: 2) Failed tests: 42, 71 Non-zero exit status: 2 t/QueryParser.t (Wstat: 256 Tests: 28 Failed: 1) Failed test: 12 Non-zero exit status: 1 Signed-off-by: Galen Charlton Signed-off-by: Katrin Fischer Signed-off-by: Jonathan Druart Signed-off-by: Galen Charlton --- t/QueryParser.t | 1 + t/db_dependent/Search.t | 15 ++++++++++++++- t/db_dependent/zebra_config.pl | 4 +++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/t/QueryParser.t b/t/QueryParser.t index 01e898a64e..f0935ec739 100644 --- a/t/QueryParser.t +++ b/t/QueryParser.t @@ -25,6 +25,7 @@ is($QParser->target_syntax('biblioserver', 'keyword|publisher:smith'), '@attr 1= is($QParser->target_syntax('biblioserver', 'ti:"little engine that could"'), '@attr 1=4 @attr 4=1 "little engine that could"', 'phrase query'); is($QParser->target_syntax('biblioserver', 'keyword|titlekw:smith'), '@attr 1=4 @attr 9=20 @attr 2=102 @attr 4=6 "smith"', 'relevance-bumped query'); is($QParser->target_syntax('biblioserver', 'au:smith && johnson'), '@and @attr 1=1003 @attr 4=6 "smith" @attr 1=1003 @attr 4=6 "johnson"', 'query with boolean &&'); +is($QParser->target_syntax('biblioserver', 'au:smith && ti:johnson'), '@and @attr 1=1003 @attr 4=6 "smith" @attr 1=4 @attr 4=6 "johnson"', 'query with boolean &&'); is($QParser->target_syntax('biblioserver', 'au:smith pubdate(-2008)'), '@and @attr 1=1003 @attr 4=6 "smith" @attr 4=4 @attr 1=pubdate @attr 2=2 "2008"', 'keyword search with pubdate limited to -2008'); is($QParser->target_syntax('biblioserver', 'au:smith pubdate(2008-)'), '@and @attr 1=1003 @attr 4=6 "smith" @attr 4=4 @attr 1=pubdate @attr 2=4 "2008"', 'keyword search with pubdate limited to 2008-'); is($QParser->target_syntax('biblioserver', 'au:smith pubdate(2008)'), '@and @attr 1=1003 @attr 4=6 "smith" @attr 4=4 @attr 1=pubdate "2008"', 'keyword search with pubdate limited to 2008'); diff --git a/t/db_dependent/Search.t b/t/db_dependent/Search.t index d4bb05bc2f..66d4259d24 100644 --- a/t/db_dependent/Search.t +++ b/t/db_dependent/Search.t @@ -12,7 +12,7 @@ use YAML; use C4::Debug; require C4::Context; -use Test::More tests => 196; +use Test::More tests => 198; use Test::MockModule; use MARC::Record; use File::Spec; @@ -458,6 +458,19 @@ sub run_marc21_search_tests { is($results_hashref->{biblioserver}->{hits}, 12, 'search using index whose name contains "ns" returns expected results (bug 10271)'); + $UseQueryParser = 1; + ( $error, $query, $simple_query, $query_cgi, + $query_desc, $limit, $limit_cgi, $limit_desc, + $stopwords_removed, $query_type ) = buildQuery([], [ 'book' ], [ 'kw' ], [], [], 0, 'en'); + ($error, $results_hashref, $facets_loop) = getRecords($query,$simple_query,[ ], [ 'biblioserver' ],20,0,undef,\%branches,\%itemtypes,$query_type,0); + is($results_hashref->{biblioserver}->{hits}, 101, "Search for 'book' with index set to 'kw' returns 101 hits"); + ( $error, $query, $simple_query, $query_cgi, + $query_desc, $limit, $limit_cgi, $limit_desc, + $stopwords_removed, $query_type ) = buildQuery([ 'and' ], [ 'book', 'another' ], [ 'kw', 'kw' ], [], [], 0, 'en'); + ($error, $results_hashref, $facets_loop) = getRecords($query,$simple_query,[ ], [ 'biblioserver' ],20,0,undef,\%branches,\%itemtypes,$query_type,0); + is($results_hashref->{biblioserver}->{hits}, 1, "Search for 'kw:book && kw:another' returns 1 hit"); + $UseQueryParser = 0; + # FIXME: the availability limit does not actually work, so for the moment we # are just checking that it behaves consistently ( $error, $query, $simple_query, $query_cgi, diff --git a/t/db_dependent/zebra_config.pl b/t/db_dependent/zebra_config.pl index 9a4f4f5085..1395508153 100755 --- a/t/db_dependent/zebra_config.pl +++ b/t/db_dependent/zebra_config.pl @@ -44,7 +44,9 @@ make_path("$destination/var/run/zebradb"); $ENV{'INSTALL_BASE'} = $destination; $ENV{'__INSTALL_BASE__'} = $destination; -my @files = ( "$source/etc/koha-conf.xml" ); +my @files = ( "$source/etc/koha-conf.xml", + "$source/etc/searchengine/queryparser.yaml", + ); find(sub { push @files, $File::Find::name if ( -f $File::Find::name ); }, "$source/etc/zebradb"); From dd36a1ffa141e49daa60a20b55dcfcc1eccad857 Mon Sep 17 00:00:00 2001 From: Galen Charlton Date: Thu, 24 Oct 2013 20:42:38 +0000 Subject: [PATCH 2/2] Bug 11137: fix certain types of searches run using QueryParser QueryParser was loading redundant search aliases, which in turn resulted in certain queries involving two or more clauses joined by a Boolean operator getting parsed in correctly. To test: [1] Enable QueryParser. [2] Arrange your database so that some some bib records contain the keyword 'history' while a smaller subset contain both 'history' and 'earth'. (The exact words used don't matter.) [3] Run the following search from the OPAC global search bar: kw:history && kw:earth [4] Observe that the records that are returned include *all* bibs in the database with the keyword 'history'. In other words, the restriction that the records must also contain 'earth' is not observed. [5] Apply the patch and rerun the search. [6] This time, the results that are returned should include only those that contain both 'history' and 'earth'. [7] Verify that prove -v t/QueryParser.t passes. Signed-off-by: Galen Charlton Signed-off-by: Katrin Fischer Passes test plans, all tests and QA script. Also checked the db_dependent/Search.t tests. Tried some other searches, all seems to be working correctly. Signed-off-by: Jonathan Druart I reproduce the bug and I confirm this patch fixes it. Without the patch, kw:history && kw:earth, I got: Search biblios OK 4 1 1+0 RPN @attrset Bib-1 @attr 4=6 @attr 1=1016 history With the path, I get: Search biblios OK 2 1 1+0 RPN @attrset Bib-1 @and @attr 4=6 @attr 1=1016 history @attr 4=6 @attr 1=1016 earth Signed-off-by: Galen Charlton --- Koha/QueryParser/Driver/PQF.pm | 6 ------ t/QueryParser.t | 4 +--- t/db_dependent/Search.t | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/Koha/QueryParser/Driver/PQF.pm b/Koha/QueryParser/Driver/PQF.pm index cd3b994679..8181cd3dd1 100644 --- a/Koha/QueryParser/Driver/PQF.pm +++ b/Koha/QueryParser/Driver/PQF.pm @@ -111,7 +111,6 @@ sub add_bib1_field_map { my ($self, $class, $field, $server, $attributes) = @_; $self->add_search_field( $class => $field ); - $self->add_search_field_alias( $class => $field => $field ); return $self->_add_field_mapping($self->bib1_field_map, $class, $field, $server, $attributes); } @@ -542,14 +541,9 @@ sub initialize { $self->add_bib1_field_map( $class => $field => $server => $bib1_mapping ); } - $self->add_search_field_alias( $class => $field => - $field_mappings->{$class}->{$field}->{'index'} ); foreach my $alias ( @{ $field_mappings->{$class}->{$field}->{'aliases'} } ) { - next - if ( $alias eq - $field_mappings->{$class}->{$field}->{'index'} ); $self->add_search_field_alias( $class => $field => $alias ); } } diff --git a/t/QueryParser.t b/t/QueryParser.t index f0935ec739..41782f2fe4 100644 --- a/t/QueryParser.t +++ b/t/QueryParser.t @@ -44,9 +44,7 @@ my $desired_config = { 'field_mappings' => { 'author' => { 'personal' => { - 'aliases' => [ - 'personal' - ], + 'aliases' => [ ], 'bib1_mapping' => { 'biblioserver' => { '1' => '1004' diff --git a/t/db_dependent/Search.t b/t/db_dependent/Search.t index 66d4259d24..378fc2d266 100644 --- a/t/db_dependent/Search.t +++ b/t/db_dependent/Search.t @@ -12,7 +12,7 @@ use YAML; use C4::Debug; require C4::Context; -use Test::More tests => 198; +use Test::More tests => 200; use Test::MockModule; use MARC::Record; use File::Spec;