Bug 25616 - Uppercase hard coded lower case boolean operators for Elasticsearch
Summary: Uppercase hard coded lower case boolean operators for Elasticsearch
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Gustafsson
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-28 12:28 UTC by David Gustafsson
Modified: 2022-03-20 15:23 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04


Attachments
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch (5.23 KB, patch)
2020-05-28 12:35 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch (6.12 KB, patch)
2022-02-01 00:15 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: Fix t/db_dependent/Search.t (2.95 KB, patch)
2022-02-15 07:19 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: (follow-up) Use uppercase in advanced search booleans (2.62 KB, patch)
2022-02-15 07:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch (6.18 KB, patch)
2022-02-19 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25616: Fix t/db_dependent/Search.t (3.01 KB, patch)
2022-02-19 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25616: (follow-up) Use uppercase in advanced search booleans (2.68 KB, patch)
2022-02-19 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch (6.24 KB, patch)
2022-02-22 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25616: Fix t/db_dependent/Search.t (3.07 KB, patch)
2022-02-22 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25616: (follow-up) Use uppercase in advanced search booleans (2.74 KB, patch)
2022-02-22 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25616: (QA follow-up) Catch a case in labels/label-item-search.pl (1.43 KB, patch)
2022-02-22 16:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25616: [21.11.x] Uppercase hard coded lower case boolean operators for Elasticsearch (6.07 KB, patch)
2022-02-28 06:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: [21.11.x] Fix t/db_dependent/Search.t (3.08 KB, patch)
2022-02-28 06:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: [21.11.x] (follow-up) Use uppercase in advanced search booleans (2.75 KB, patch)
2022-02-28 06:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: [21.11.x] (QA follow-up) Catch a case in labels/label-item-search.pl (1.44 KB, patch)
2022-02-28 06:06 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: (follow-up) Fix t/Search.t (1.20 KB, patch)
2022-03-02 18:54 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 25616: [21.11.x] (follow-up) Fix t/Search.t (1.21 KB, patch)
2022-03-02 18:56 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 David Gustafsson 2020-05-28 12:28:40 UTC
Since valid boolean operators in the Elasticsearch mini language for Query String Queries needs to be upper case, there some occurrences of queries in Koha left that needs changing. I think I found most of them.
Comment 1 David Gustafsson 2020-05-28 12:35:11 UTC
Created attachment 105411 [details] [review]
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch

Uppercase occurances of all (hopefully) lowercase "and"
used in ElasticSearch Query String Query contexts
Comment 2 David Gustafsson 2020-05-28 12:41:39 UTC
Regarding how to test. I have tested the fix in bulkmarimport.pl but regarding the others I am not all that familiar with Koha from a user-perspective and it would take me too long to write an how to test for all of them. One thing that could go wrong is that operator precedence for AND/OR is not what you expect and parentheses needs to be used for queries mixing them both. I tried to review all replacements and i THINK that AND/OR is not mixed in those places so should be ok and no extra precedence tweaking needed. Also think this will fix more than it could potentially break, but help in writing an how to test would otherwise be appreciated if this feels to risky to merge without one.
Comment 3 David Gustafsson 2021-09-24 14:04:47 UTC
The same issue has been address in bug 23322 och bug 24506, but some instances of where operator case need to be changed remains and is addressed in this patch.
Comment 4 Fridolin Somers 2022-01-27 22:45:07 UTC
Oh I've been searching hours on that bug.
Need to be fixed.
Comment 5 Fridolin Somers 2022-02-01 00:15:48 UTC
Created attachment 130029 [details] [review]
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch

Uppercase occurances of all (hopefully) lowercase "and"
used in ElasticSearch Query String Query contexts

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 6 Fridolin Somers 2022-02-01 00:16:21 UTC
I found one more in C4::Search::_build_initial_query
Comment 7 Katrin Fischer 2022-02-13 14:43:36 UTC
This looks all good, but I think we might need to fix tests:

t/db_dependent/Search.t .. 
    #   Failed test 'Built composed implicit-and CCL keyword query'
    #   at t/db_dependent/Search.t line 423.
    #                   'kw,wrdl=(rk=(salud)) AND kw,wrdl=(rk=(higiene)) '
    #     doesn't match '(?^u:kw\W.*salud\W.*and.*kw\W.*higiene)'
    # Looks like you failed 1 test of 90.
t/db_dependent/Search.t .. 1/3 
#   Failed test 'MARC21 + DOM'
#   at t/db_dependent/Search.t line 906.
t/db_dependent/Search.t .. 2/3 # Looks like you failed 1 test of 3.
t/db_dependent/Search.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 

Test Summary Report
-------------------
t/db_dependent/Search.t (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Comment 8 Fridolin Somers 2022-02-15 07:19:33 UTC
Created attachment 130588 [details] [review]
Bug 25616: Fix t/db_dependent/Search.t
Comment 9 Fridolin Somers 2022-02-15 07:22:12 UTC
Created attachment 130589 [details] [review]
Bug 25616: (follow-up) Use uppercase in advanced search booleans
Comment 10 Fridolin Somers 2022-02-15 07:23:24 UTC
Seems logical to use uppercase in advanced search page OPAC and staff
Comment 11 Katrin Fischer 2022-02-19 21:11:30 UTC
Created attachment 130914 [details] [review]
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch

Uppercase occurances of all (hopefully) lowercase "and"
used in ElasticSearch Query String Query contexts

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2022-02-19 21:11:35 UTC
Created attachment 130915 [details] [review]
Bug 25616: Fix t/db_dependent/Search.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2022-02-19 21:11:40 UTC
Created attachment 130916 [details] [review]
Bug 25616: (follow-up) Use uppercase in advanced search booleans

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Martin Renvoize 2022-02-22 16:23:29 UTC
Created attachment 131019 [details] [review]
Bug 25616: Uppercase hard coded lower case boolean operators for Elasticsearch

Uppercase occurances of all (hopefully) lowercase "and"
used in ElasticSearch Query String Query contexts

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-02-22 16:23:33 UTC
Created attachment 131020 [details] [review]
Bug 25616: Fix t/db_dependent/Search.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-02-22 16:23:37 UTC
Created attachment 131021 [details] [review]
Bug 25616: (follow-up) Use uppercase in advanced search booleans

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-02-22 16:23:42 UTC
Created attachment 131022 [details] [review]
Bug 25616: (QA follow-up) Catch a case in labels/label-item-search.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-02-22 16:24:51 UTC
Good catch, no regressions found and QA scripts are happy.

I found another obvious case, and there are lots more `' and '` occurrences in the codebase that a possible cases of this. I think we should push this one as is and handle follow-ups in more bugs.
Comment 19 Fridolin Somers 2022-02-25 00:36:08 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 20 Kyle M Hall 2022-02-25 14:44:40 UTC
Patches need rebased for 21.11.x if backport is needed.
Comment 21 Fridolin Somers 2022-02-28 06:06:28 UTC
Created attachment 131161 [details] [review]
Bug 25616: [21.11.x] Uppercase hard coded lower case boolean operators for Elasticsearch

Uppercase occurances of all (hopefully) lowercase "and"
used in ElasticSearch Query String Query contexts

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Fridolin Somers 2022-02-28 06:06:33 UTC
Created attachment 131162 [details] [review]
Bug 25616: [21.11.x] Fix t/db_dependent/Search.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Fridolin Somers 2022-02-28 06:06:37 UTC
Created attachment 131163 [details] [review]
Bug 25616: [21.11.x] (follow-up) Use uppercase in advanced search booleans

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Fridolin Somers 2022-02-28 06:06:42 UTC
Created attachment 131164 [details] [review]
Bug 25616: [21.11.x] (QA follow-up) Catch a case in labels/label-item-search.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Fridolin Somers 2022-02-28 06:08:15 UTC
(In reply to Kyle M Hall from comment #20)
> Patches need rebased for 21.11.x if backport is needed.

Here we go.

Conflict solved on Koha::Biblio::get_marc_authors still in C4::Biblio::GetMarcAuthors
Comment 26 Fridolin Somers 2022-03-02 08:51:56 UTC
Ah wait, t/Search.t/ is failling :

    #   Failed test '$query built correctly (no operator)'
    #   at t/Search.t line 90.
    #          got: 'query AND parsed_operand'
    #     expected: 'query and parsed_operand'

    #   Failed test '$query_cgi built correctly (no operator)'
    #   at t/Search.t line 92.
    #          got: 'query_cgi&op=%20AND%20&idx=index&q=original_operand'
    #     expected: 'query_cgi&op=%20and%20&idx=index&q=original_operand'

    #   Failed test '$query_desc build correctly (no operator)'
    #   at t/Search.t line 94.
    #          got: 'query_desc AND index_plus original_operand'
    #     expected: 'query_desc and index_plus original_operand'
    # Looks like you failed 3 tests of 20.
Comment 27 Fridolin Somers 2022-03-02 18:54:46 UTC
Created attachment 131292 [details] [review]
Bug 25616: (follow-up) Fix t/Search.t
Comment 28 Fridolin Somers 2022-03-02 18:56:03 UTC
Created attachment 131293 [details] [review]
Bug 25616: [21.11.x] (follow-up) Fix t/Search.t
Comment 29 Fridolin Somers 2022-03-02 18:56:35 UTC
Bug 25616: (follow-up) Fix t/Search.t
pushed to master
Comment 30 Kyle M Hall 2022-03-04 14:22:47 UTC
Pushed to 21.11.x for 21.11.04
Comment 31 Andrew Fuerste-Henry 2022-03-20 15:23:19 UTC
I'm getting these failures on the qa script when applying the 21.11 patches to 21.05:
 FAIL	t/Search.t
   FAIL	  valid
			Test2::Hub::plan(Test2::Hub=HASH(0x55c009a7ad60), "SKIP") called 
			Test2::Hub::process(Test2::Hub=HASH(0x55c009a7ad60), Test2::Event::Plan=HASH(0x55c00c82a630)) called 
			Test2::Hub::send(Test2::Hub=HASH(0x55c009a7ad60), Test2::Event::Plan=HASH(0x55c00c82a630)) called 
			Test2::API::Context::send_event(Test2::API::Context=HASH(0x55c00c831440), "Plan", "max", 0, "directive", "SKIP", "reason", "Skipping remaining tests since we don't have a schema") called 
			Test2::API::Context::plan(Test2::API::Context=HASH(0x55c00c831440), 0, "SKIP", "Skipping remaining tests since we don't have a schema") called 
			Test::Builder::skip_all(Test::Builder=HASH(0x55c0095c7af0), "Skipping remaining tests since we don't have a schema") called 
			Test::DBIx::Class::_initialize_schema("Test::DBIx::Class", HASH(0x55c00a915a80)) called