Bug 27597 - Searching "kw:term" does not work with Elasticsearch
Summary: Searching "kw:term" does not work with Elasticsearch
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-02 15:55 UTC by Fridolin Somers
Modified: 2021-03-23 23:01 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.04,20.05.10


Attachments
Bug 27597: Remove leading colon in ES query (2.27 KB, patch)
2021-02-03 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27597: Remove extra spaces in the generated query (1.98 KB, patch)
2021-02-03 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27597: Remove leading colon in ES query (2.33 KB, patch)
2021-03-05 13:56 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27597: Remove extra spaces in the generated query (2.04 KB, patch)
2021-03-05 13:56 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 27597: Remove leading colon in ES query (2.39 KB, patch)
2021-03-12 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2021-02-02 15:55:12 UTC
In Koha/SearchEngine/Elasticsearch/QueryBuilder.pm :

There is a map of convertion from CCL :

our %index_field_convert = (
    'kw' => '',
    'ab' => 'abstract',
    'au' => 'author',
...

You see that kw is replaced by empty.

See that in _convert_index_strings() :

        my ($conv) = $self->_convert_index_fields($field);
        unless ( defined($conv) ) {
            push @res, $s;
            next;
        }
        push @res, ($conv->{field} ? $conv->{field} . ':' : '')
            . $self->_modify_string_by_type( %$conv, operand => $term );

If Field is empty the character ":" is not added.

This behavior is missing from _convert_index_strings_freeform() :

    $search =~ s/($field_name_pattern)($multi_field_pattern):/(exists $index_field_convert{$1} ? $index_field_convert{$1} : $1)."$2:"/oge;

Query "kw:term" will be converted to ":term" :(
Comment 1 Jonathan Druart 2021-02-03 09:39:58 UTC
Created attachment 116241 [details] [review]
Bug 27597: Remove leading colon in ES query

If we are searching on kw there is a leading colon at the beginning of
the generated query:
  kw:foo becomes :foo

Note that it only happens when there is no other terms before.

Test plan:
0. Don't apply the patch
1. Search for kw:foo
2. Notice the error
  Error: Unable to perform your search. Please try again.
and the logs say
  Failed to parse query [(:foo*)]
3. Apply the patch
4. Repeat the search and notice that you know get:
   "12 result(s) found for 'kw:foo'."
Comment 2 Jonathan Druart 2021-02-03 09:40:01 UTC
Created attachment 116242 [details] [review]
Bug 27597: Remove extra spaces in the generated query

Look at the tests, sometime we have extra spaces.
With this patch they still pass and the expected queries are better.
Feel free to drop this patch if you have any concerns about it.
Comment 3 Fridolin Somers 2021-02-03 09:57:31 UTC
Super,

But we also need to fix in _convert_index_strings_freeform() rigth ?
Comment 4 Fridolin Somers 2021-02-03 09:59:11 UTC
And BTW is it in theory correct to replace "kw:" with "*:"  ?
Comment 5 Jonathan Druart 2021-02-03 12:33:42 UTC
(In reply to Fridolin Somers from comment #3)
> Super,
> 
> But we also need to fix in _convert_index_strings_freeform() rigth ?

I fixed it where we deal with colons, so it seems to be the right place.
_convert_index_strings_freeform is only called from _clean_search_term anyway.

(In reply to Fridolin Somers from comment #4)
> And BTW is it in theory correct to replace "kw:" with "*:"  ?

If it's what we want we should map is in %index_field_convert then.
Comment 6 Jonathan Druart 2021-02-17 08:42:02 UTC
It also breaks itemBarcodeFallbackSearch.

[2021/02/17 08:36:15] [WARN] [Request] ** [http://es:9200]-[400] [query_shard_exception] Failed to parse query [(:street*)], with: {"index_uuid":"YeYOreb1Rl2j-l52RVXJ4A","index":"koha_kohadev_biblios"}, called f
rom sub Search::Elasticsearch::Role::Client::Direct::__ANON__ at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch/Search.pm line 96. With vars: {'status_code' => 400,'body' => {'status' => 400,'error' => {'faile
d_shards' => [{'index' => 'koha_kohadev_biblios','reason' => {'type' => 'query_shard_exception','index_uuid' => 'YeYOreb1Rl2j-l52RVXJ4A','caused_by' => {'reason' => 'Cannot parse \'(:street*)\': Encountered " ":" ": "" at line 1, column 1.
Comment 7 Fridolin Somers 2021-03-01 09:31:28 UTC
     # Remove unquoted colons that have whitespace on either side of them
-    $term =~ s/(:+)(\s+)$lookahead/$2/g;
+    $term =~ s/(:+)(\s+)$lookahead//g;
     $term =~ s/(\s+)(:+)$lookahead/$1/g;

Could you explain this change please ?
Comment 8 Jonathan Druart 2021-03-01 10:31:45 UTC
(In reply to Fridolin Somers from comment #7)
>      # Remove unquoted colons that have whitespace on either side of them
> -    $term =~ s/(:+)(\s+)$lookahead/$2/g;
> +    $term =~ s/(:+)(\s+)$lookahead//g;
>      $term =~ s/(\s+)(:+)$lookahead/$1/g;
> 
> Could you explain this change please ?

Look at the unit tests, it removes extra spaces.
Comment 9 Fridolin Somers 2021-03-05 13:56:10 UTC
Created attachment 117856 [details] [review]
Bug 27597: Remove leading colon in ES query

If we are searching on kw there is a leading colon at the beginning of
the generated query:
  kw:foo becomes :foo

Note that it only happens when there is no other terms before.

Test plan:
0. Don't apply the patch
1. Search for kw:foo
2. Notice the error
  Error: Unable to perform your search. Please try again.
and the logs say
  Failed to parse query [(:foo*)]
3. Apply the patch
4. Repeat the search and notice that you know get:
   "12 result(s) found for 'kw:foo'."

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 10 Fridolin Somers 2021-03-05 13:56:25 UTC
Created attachment 117857 [details] [review]
Bug 27597: Remove extra spaces in the generated query

Look at the tests, sometime we have extra spaces.
With this patch they still pass and the expected queries are better.
Feel free to drop this patch if you have any concerns about it.

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 11 Fridolin Somers 2021-03-05 13:56:54 UTC
All is good ;)
Comment 12 Katrin Fischer 2021-03-06 12:21:21 UTC
I am stuck on this one as I can't get Elasticsearch to work :(

Error: Unable to perform your search. Please try again. 

Applied the patch, reset mappings, reindexed... status of Elasticsearch is green.
Comment 13 Nick Clemens 2021-03-12 13:30:20 UTC
Created attachment 118190 [details] [review]
Bug 27597: Remove leading colon in ES query

If we are searching on kw there is a leading colon at the beginning of
the generated query:
  kw:foo becomes :foo

Note that it only happens when there is no other terms before.

Test plan:
0. Don't apply the patch
1. Search for kw:foo
2. Notice the error
  Error: Unable to perform your search. Please try again.
and the logs say
  Failed to parse query [(:foo*)]
3. Apply the patch
4. Repeat the search and notice that you know get:
   "12 result(s) found for 'kw:foo'."

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2021-03-12 13:31:36 UTC
(In reply to Fridolin Somers from comment #10)
> Created attachment 117857 [details] [review] [review]
> Bug 27597: Remove extra spaces in the generated query
> 
> Look at the tests, sometime we have extra spaces.
> With this patch they still pass and the expected queries are better.
> Feel free to drop this patch if you have any concerns about it.
> 
> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

I obsoleted this, it causes a recreation of issues solved in bug 24567, tested with the comment from bug 24372:

Using an example record in the testing DB, try searching for:
Pictura murală din nordul Moldovei: modificari estetice si restarare = Mural painting in the north of Moldavia: aesthetic modification and restoration

In ES the search fails. If you look at the query we end up with terms like:
Moldoveimodificari AND Moldaviaaesthetic
Comment 15 Jonathan Druart 2021-03-16 15:10:56 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 16 Fridolin Somers 2021-03-19 15:14:04 UTC
Pushed to 20.11.x for 20.11.04
Comment 17 Andrew Fuerste-Henry 2021-03-23 14:45:49 UTC
Pushed to 20.05.x for 20.05.10
Comment 18 Victor Grousset/tuxayo 2021-03-23 23:01:10 UTC
Can't backport to 19.11.x: can't solve a conflict.
If there is an interest in having this backported, please submit a patch for 19.11.

<<<<<<< HEAD
    $term =~ s/(\:[:\s]+|[:\s]+:)$lookahead//g;
||||||| parent of 622a68f695 (Bug 27597: Remove leading colon in ES query)
    $term =~ s/(:+)(\s+)$lookahead/$2/g;
    $term =~ s/(\s+)(:+)$lookahead/$1/g;
=======
    $term =~ s/(:+)(\s+)$lookahead/$2/g;
    $term =~ s/(\s+)(:+)$lookahead/$1/g;
    $term =~ s/^://;
>>>>>>> 622a68f695 (Bug 27597: Remove leading colon in ES query)