From a4c5bd58361900983f870f1c0e7f497ae3618c85 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 22 Jun 2022 13:19:03 +0000 Subject: [PATCH] Bug 31013: Quote branchcodes in Elasticsearch limits This patch adds quoting when handling branchcodes in searching in order to prevent errors when branchcodes are reserved words in ES To test: 0 - Be using Elasticsearch with Koha 1 - Add a new branch code:OR name:Orly 2 - Add an item to this branch 3 - Use advanced search to limit search to only Orly 4 - Oh really bad, the search fails! 5 - Apply patch 6 - Repeat search 7 - Oh really good, the search succeeds 8 - prove t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- .../Elasticsearch/QueryBuilder.pm | 9 ++++--- .../SearchEngine/Elasticsearch/QueryBuilder.t | 26 +++++++++---------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm index dba475eb3c..b6a6b9a786 100644 --- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm +++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm @@ -1080,16 +1080,17 @@ sub _fix_limit_special_cases { } if (@branchcodes) { + # We quote the branchcodes here to prevent issues when codes are reserved words in ES, e.g. OR, AND, NOT, etc. if ( $branchfield eq "homebranch" ) { - push @new_lim, sprintf "(%s)", join " OR ", map { 'homebranch: ' . $_ } @branchcodes; + push @new_lim, sprintf "(%s)", join " OR ", map { 'homebranch: "' . $_ . '"' } @branchcodes; } elsif ( $branchfield eq "holdingbranch" ) { - push @new_lim, sprintf "(%s)", join " OR ", map { 'holdingbranch: ' . $_ } @branchcodes; + push @new_lim, sprintf "(%s)", join " OR ", map { 'holdingbranch: "' . $_ . '"' } @branchcodes; } else { push @new_lim, sprintf "(%s OR %s)", - join( " OR ", map { 'homebranch: ' . $_ } @branchcodes ), - join( " OR ", map { 'holdingbranch: ' . $_ } @branchcodes ); + join( " OR ", map { 'homebranch: "' . $_ . '"' } @branchcodes ), + join( " OR ", map { 'holdingbranch: "' . $_ . '"' } @branchcodes ); } } } diff --git a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t index b4e2de938c..c032bf8c91 100755 --- a/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t +++ b/t/db_dependent/Koha/SearchEngine/Elasticsearch/QueryBuilder.t @@ -725,37 +725,37 @@ subtest 'build_query_compat() SearchLimitLibrary tests' => sub { t::lib::Mocks::mock_preference('SearchLimitLibrary', 'both'); my ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc, undef ) = $query_builder->build_query_compat( undef, undef, undef, [ "branch:CPL" ], undef, undef, undef, undef ); - is( $limit, "(homebranch: CPL OR holdingbranch: CPL)", "Branch limit expanded to home/holding branch"); - is( $limit_desc, "(homebranch: CPL OR holdingbranch: CPL)", "Limit description correctly expanded"); - is( $limit_cgi, "&limit=branch%3ACPL", "Limit cgi does not get expanded"); + is( $limit, '(homebranch: "CPL" OR holdingbranch: "CPL")', "Branch limit expanded to home/holding branch"); + is( $limit_desc, '(homebranch: "CPL" OR holdingbranch: "CPL")', "Limit description correctly expanded"); + is( $limit_cgi, '&limit=branch%3ACPL', "Limit cgi does not get expanded"); ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc, undef ) = $query_builder->build_query_compat( undef, undef, undef, [ "multibranchlimit:$groupid" ], undef, undef, undef, undef ); - is( $limit, "(homebranch: $branchcodes[0] OR homebranch: $branchcodes[1] OR holdingbranch: $branchcodes[0] OR holdingbranch: $branchcodes[1])", "Multibranch limit expanded to home/holding branches"); - is( $limit_desc, "(homebranch: $branchcodes[0] OR homebranch: $branchcodes[1] OR holdingbranch: $branchcodes[0] OR holdingbranch: $branchcodes[1])", "Multibranch limit description correctly expanded"); + is( $limit, "(homebranch: \"$branchcodes[0]\" OR homebranch: \"$branchcodes[1]\" OR holdingbranch: \"$branchcodes[0]\" OR holdingbranch: \"$branchcodes[1]\")", "Multibranch limit expanded to home/holding branches"); + is( $limit_desc, "(homebranch: \"$branchcodes[0]\" OR homebranch: \"$branchcodes[1]\" OR holdingbranch: \"$branchcodes[0]\" OR holdingbranch: \"$branchcodes[1]\")", "Multibranch limit description correctly expanded"); is( $limit_cgi, "&limit=multibranchlimit%3A$groupid", "Multibranch limit cgi does not get expanded"); t::lib::Mocks::mock_preference('SearchLimitLibrary', 'homebranch'); ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc, undef ) = $query_builder->build_query_compat( undef, undef, undef, [ "branch:CPL" ], undef, undef, undef, undef ); - is( $limit, "(homebranch: CPL)", "branch limit expanded to home branch"); - is( $limit_desc, "(homebranch: CPL)", "limit description correctly expanded"); + is( $limit, "(homebranch: \"CPL\")", "branch limit expanded to home branch"); + is( $limit_desc, "(homebranch: \"CPL\")", "limit description correctly expanded"); is( $limit_cgi, "&limit=branch%3ACPL", "limit cgi does not get expanded"); ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc, undef ) = $query_builder->build_query_compat( undef, undef, undef, [ "multibranchlimit:$groupid" ], undef, undef, undef, undef ); - is( $limit, "(homebranch: $branchcodes[0] OR homebranch: $branchcodes[1])", "branch limit expanded to home branch"); - is( $limit_desc, "(homebranch: $branchcodes[0] OR homebranch: $branchcodes[1])", "limit description correctly expanded"); + is( $limit, "(homebranch: \"$branchcodes[0]\" OR homebranch: \"$branchcodes[1]\")", "branch limit expanded to home branch"); + is( $limit_desc, "(homebranch: \"$branchcodes[0]\" OR homebranch: \"$branchcodes[1]\")", "limit description correctly expanded"); is( $limit_cgi, "&limit=multibranchlimit%3A$groupid", "Limit cgi does not get expanded"); t::lib::Mocks::mock_preference('SearchLimitLibrary', 'holdingbranch'); ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc, undef ) = $query_builder->build_query_compat( undef, undef, undef, [ "branch:CPL" ], undef, undef, undef, undef ); - is( $limit, "(holdingbranch: CPL)", "branch limit expanded to holding branch"); - is( $limit_desc, "(holdingbranch: CPL)", "Limit description correctly expanded"); + is( $limit, "(holdingbranch: \"CPL\")", "branch limit expanded to holding branch"); + is( $limit_desc, "(holdingbranch: \"CPL\")", "Limit description correctly expanded"); is( $limit_cgi, "&limit=branch%3ACPL", "Limit cgi does not get expanded"); ( undef, undef, undef, undef, undef, $limit, $limit_cgi, $limit_desc, undef ) = $query_builder->build_query_compat( undef, undef, undef, [ "multibranchlimit:$groupid" ], undef, undef, undef, undef ); - is( $limit, "(holdingbranch: $branchcodes[0] OR holdingbranch: $branchcodes[1])", "branch limit expanded to holding branch"); - is( $limit_desc, "(holdingbranch: $branchcodes[0] OR holdingbranch: $branchcodes[1])", "Limit description correctly expanded"); + is( $limit, "(holdingbranch: \"$branchcodes[0]\" OR holdingbranch: \"$branchcodes[1]\")", "branch limit expanded to holding branch"); + is( $limit_desc, "(holdingbranch: \"$branchcodes[0]\" OR holdingbranch: \"$branchcodes[1]\")", "Limit description correctly expanded"); is( $limit_cgi, "&limit=multibranchlimit%3A$groupid", "Limit cgi does not get expanded"); }; -- 2.39.5