Bug 23322 - Elasticsearch - Record matching fails when multiple keys exist
Summary: Elasticsearch - Record matching fails when multiple keys exist
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
: 19197 22545 (view as bug list)
Depends on:
Blocks: 23324
  Show dependency treegraph
 
Reported: 2019-07-16 12:08 UTC by Nick Clemens
Modified: 2020-11-30 21:46 UTC (History)
5 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:
19.11.00,19.05.03


Attachments
Test record (1.17 KB, application/x-wine-extension-mrc)
2019-07-16 12:08 UTC, Nick Clemens
Details
Bug 23322: Correct case for boolean in ES search (1.62 KB, patch)
2019-07-16 12:11 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23322: Correct case for boolean in ES search (1.67 KB, patch)
2019-07-16 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23322: Correct case for boolean in ES search (1.74 KB, patch)
2019-08-08 11:48 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-07-16 12:08:19 UTC
For a record with multipel matchpoints C4/Matcher.pm combines terms using 'or' however, ES expects 'OR' so the search fails

To test:
 0 - Set SearchEngine to ElasticSearch
 1 - Import the example record (a version is included in sample data in devbox/testing docker so you can skip)
 2 - Stage the attached example record
 3 - Match using 'ISBN' rule
 4 - No matches found
Comment 1 Nick Clemens 2019-07-16 12:08:39 UTC
Created attachment 91547 [details]
Test record
Comment 2 Nick Clemens 2019-07-16 12:11:27 UTC
Created attachment 91548 [details] [review]
Bug 23322: Correct case for boolean in ES search

I did not include tests only because this is a very small reasonable change and routine has no tests.
It does not affect behaviour, it only touches syntax lightly.

To test:
  0 - Set SearchEngine to ElasticSearch
  1 - Import the example record (a version is included in sample data in devbox/testing docker so you can skip)
  2 - Stage the attached example record
  3 - Match using 'ISBN' rule
  4 - No matches found
  5 - Apply patch
  6 - Restart all the things
  7 - Reapply matching with no rule
  8 - Reapply with ISBN matcing
  9 - It matches!
 10 - Set SearchEngine to Zebra
 11 - Reapply matching with no rule
 12 - Reapply with ISBN matching
 13 - Matching works as before in Zebra
Comment 3 Nick Clemens 2019-07-16 16:38:05 UTC
Created attachment 91556 [details] [review]
Bug 23322: Correct case for boolean in ES search

I did not include tests only because this is a very small reasonable change and routine has no tests.
It does not affect behaviour, it only touches syntax lightly.

To test:
  0 - Set SearchEngine to ElasticSearch
  1 - Import the example record (a version is included in sample data in devbox/testing docker so you can skip)
  2 - Stage the attached example record
  3 - Match using 'ISBN' rule
  4 - No matches found
  5 - Apply patch
  6 - Restart all the things
  7 - Reapply matching with no rule
  8 - Reapply with ISBN matcing
  9 - It matches!
 10 - Set SearchEngine to Zebra
 11 - Reapply matching with no rule
 12 - Reapply with ISBN matching
 13 - Matching works as before in Zebra

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Comment 4 Martin Renvoize 2019-08-08 11:48:02 UTC
Created attachment 92067 [details] [review]
Bug 23322: Correct case for boolean in ES search

I did not include tests only because this is a very small reasonable change and routine has no tests.
It does not affect behaviour, it only touches syntax lightly.

To test:
  0 - Set SearchEngine to ElasticSearch
  1 - Import the example record (a version is included in sample data in devbox/testing docker so you can skip)
  2 - Stage the attached example record
  3 - Match using 'ISBN' rule
  4 - No matches found
  5 - Apply patch
  6 - Restart all the things
  7 - Reapply matching with no rule
  8 - Reapply with ISBN matcing
  9 - It matches!
 10 - Set SearchEngine to Zebra
 11 - Reapply matching with no rule
 12 - Reapply with ISBN matching
 13 - Matching works as before in Zebra

Signed-off-by: Ron Houk <rhouk@ottumwapubliclibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-08-08 11:48:41 UTC
Trivial patch, works as expected.. would have been nice to have a unit test for it.. but as it's ES dependant I'm not going to fail it for that.

Passing QA
Comment 6 Martin Renvoize 2019-08-08 12:07:05 UTC
Nice work!

Pushed to master for 19.11.00
Comment 7 Fridolin Somers 2019-08-09 14:05:29 UTC
Pushed to 19.05.x for 19.05.03
Comment 8 Lucas Gass 2019-08-28 14:41:44 UTC
backported to 18.11.x for 18.11.09
Comment 9 Katrin Fischer 2019-10-14 22:03:15 UTC
*** Bug 22545 has been marked as a duplicate of this bug. ***
Comment 10 David Gustafsson 2020-05-28 11:15:46 UTC
*** Bug 19197 has been marked as a duplicate of this bug. ***