Bug 30152 - Elasticsearch - queries with OR don't work with limits
Summary: Elasticsearch - queries with OR don't work with limits
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Kevin Carnes
QA Contact: Fridolin Somers
URL:
Keywords:
: 30173 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-02-22 13:27 UTC by Kevin Carnes
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.04, 21.11.11


Attachments
When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct. (1.91 KB, patch)
2022-02-22 13:58 UTC, Kevin Carnes
Details | Diff | Splinter Review
When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct. (1.95 KB, patch)
2022-02-22 14:00 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (2.01 KB, patch)
2022-02-22 15:22 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (1.96 KB, patch)
2022-02-22 16:09 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (2.72 KB, patch)
2022-05-25 08:46 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (13.32 KB, patch)
2022-05-26 08:38 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (13.57 KB, patch)
2022-05-30 06:34 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (13.62 KB, patch)
2022-05-31 00:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (3.71 KB, patch)
2022-06-08 10:34 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (3.75 KB, patch)
2022-06-27 11:10 UTC, David Nind
Details | Diff | Splinter Review
Bug 30152: Elasticsearch - queries with OR don't work with limits (3.82 KB, patch)
2022-06-30 05:46 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Carnes 2022-02-22 13:27:12 UTC
When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.
Comment 1 Kevin Carnes 2022-02-22 13:58:31 UTC
Created attachment 131010 [details] [review]
When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Title)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeats the search
13) Observe that results with both keywords are in the results
14) Sign off
Comment 2 Kevin Carnes 2022-02-22 14:00:28 UTC
Created attachment 131011 [details] [review]
When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Title)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 3 Kevin Carnes 2022-02-22 15:22:45 UTC
Created attachment 131017 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 4 Kevin Carnes 2022-02-22 16:02:39 UTC
Comment on attachment 131017 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

>From b4cfe66eea7d100d319a5ee256061e8eea15bd99 Mon Sep 17 00:00:00 2001
>From: Kevin Carnes <kevin.carnes@ub.lu.se>
>Date: Tue, 22 Feb 2022 14:29:15 +0100
>Subject: [PATCH] Bug 30152: Elasticsearch - queries with OR don't work with
> limits
>
>When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.
>
>To test:
>1) Set SearchEngine to Elasticsearch
>2) Index records in Elasticsearch
>3) Do an advanced search
>4) Select More options
>5) Enter a value for the first Keyword (e.g. Novels)
>6) Change "and" before the second Keyword to "or"
>7) Enter another value for the second Keyword (e.g. Prose)
>8) Limit the search (e.g. Item type Books)
>9) Do the search
>10) Observe that records with the first keyword are not in the results
>11) Apply the patch
>12) Repeat the search
>13) Observe that results with both keywords are in the results
>14) Sign off
>
>Sponsored-by: Lund University Library
>---
> Koha/SearchEngine/Elasticsearch/QueryBuilder.pm | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
>index 3e2cb1b784..b2ef8aa8d9 100644
>--- a/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
>+++ b/Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
>@@ -306,9 +306,7 @@ sub build_query_compat {
>         # them into a structured ES query itself. Maybe later, though that'd be
>         # more robust.
>         $search_param_query_str = join( ' ', $self->_create_query_string(@search_params) );
>-        $query_str = join( ' AND ',
>-            $search_param_query_str || (),
>-            $self->_join_queries( $self->_convert_index_strings(@$limits) ) || () );
>+        $query_str = join( ' AND ', map { "($_)" } grep { $_ } ( $search_param_query_str, $self->_join_queries( $self->_convert_index_strings(@$limits) ) ) );
> 
>         # If there's no query on the left, let's remove the junk left behind
>         $query_str =~ s/^ AND //;
>-- 
>2.17.1
Comment 5 Kevin Carnes 2022-02-22 16:09:39 UTC
Created attachment 131018 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 6 Kevin Carnes 2022-02-24 07:56:28 UTC
*** Bug 30173 has been marked as a duplicate of this bug. ***
Comment 7 Fridolin Somers 2022-05-24 22:25:44 UTC
Good catch.
This will need a unit test.
Comment 8 Fridolin Somers 2022-05-24 22:38:52 UTC
In my opinion this code looks better (if it fixes the same way) :
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30141#c1
Comment 9 Kevin Carnes 2022-05-25 08:46:21 UTC
Created attachment 135334 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 10 Kevin Carnes 2022-05-25 08:49:18 UTC
(In reply to Fridolin Somers from comment #8)
> In my opinion this code looks better (if it fixes the same way) :
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30141#c1

Ok, it now uses that code and has a simple test to make sure that the query looks correct, but doesn't actually test searching.
Comment 11 Kevin Carnes 2022-05-26 08:38:03 UTC
Created attachment 135360 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 12 Kevin Carnes 2022-05-30 06:34:27 UTC
Created attachment 135446 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 13 David Nind 2022-05-31 00:27:40 UTC
Created attachment 135454 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 Fridolin Somers 2022-06-03 10:35:19 UTC
Looks good but I propose we had those parenthesis only when there are limits.
Comment 15 Kevin Carnes 2022-06-08 10:34:07 UTC
Created attachment 135793 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library
Comment 16 Fridolin Somers 2022-06-09 06:44:27 UTC
Looks great
Comment 17 Fridolin Somers 2022-06-09 06:44:43 UTC
Looks great Kevin :D
Comment 18 David Nind 2022-06-27 11:10:41 UTC
Created attachment 136569 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Fridolin Somers 2022-06-30 05:46:16 UTC
Created attachment 136766 [details] [review]
Bug 30152: Elasticsearch - queries with OR don't work with limits

When a query with "OR" is combined with a limit in Elasticsearch, the precedence is not preserved and the results are not correct.

To test:
1) Set SearchEngine to Elasticsearch
2) Index records in Elasticsearch
3) Do an advanced search
4) Select More options
5) Enter a value for the first Keyword (e.g. Novels)
6) Change "and" before the second Keyword to "or"
7) Enter another value for the second Keyword (e.g. Prose)
8) Limit the search (e.g. Item type Books)
9) Do the search
10) Observe that records with the first keyword are not in the results
11) Apply the patch
12) Repeat the search
13) Observe that results with both keywords are in the results
14) Sign off

Sponsored-by: Lund University Library

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 20 Tomás Cohen Arazi 2022-07-01 19:58:16 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 21 Lucas Gass 2022-08-09 22:04:17 UTC
Backported to 22.05.x for 22.05.04
Comment 22 Arthur Suzuki 2022-08-12 07:30:03 UTC
pushed to 21.11.x for 21.11.11
Comment 23 Victor Grousset/tuxayo 2022-08-17 19:45:45 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document, marking resolved.