Bug 25050 - Elasticsearch - authority indexing depends on mapping order
Summary: Elasticsearch - authority indexing depends on mapping order
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:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-03 12:12 UTC by Nick Clemens
Modified: 2020-11-30 21:47 UTC (History)
7 users (show)

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


Attachments
Bug 25050: Unit tests (3.12 KB, patch)
2020-04-03 12:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25050: Adjust marc_records_to_documents to search entire mapping array (1.61 KB, patch)
2020-04-03 12:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Mappings that uncovered the bug (81.74 KB, application/x-yaml)
2020-04-03 12:27 UTC, Nick Clemens
Details
Bug 25050: Unit tests (3.19 KB, patch)
2020-04-03 13:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25050: Adjust marc_records_to_documents to search entire mapping array (1.67 KB, patch)
2020-04-03 13:05 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25050: Unit tests (3.25 KB, patch)
2020-04-10 08:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25050: Adjust marc_records_to_documents to search entire mapping array (1.75 KB, patch)
2020-04-10 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-04-03 12:12:30 UTC
There is an assumption in the ES code that match-heading mappings will appear in a specified portion of the mappings array.

Certain mappings setups will not meet this assumption. We need to move our searching up one level
Comment 1 Nick Clemens 2020-04-03 12:22:42 UTC
Created attachment 102346 [details] [review]
Bug 25050: Unit tests

This patch simplay alters the data we use for the tests, doing so causes them to fail

To test:
1 - Apply only this patch
2 - prove -v t/Koha/SearchEngine/Elasticsearch.t
3 - It fails!
4 - Apply next patch
5 - It passes!
Comment 2 Nick Clemens 2020-04-03 12:22:45 UTC
Created attachment 102347 [details] [review]
Bug 25050: Adjust marc_records_to_documents to search entire mapping array

There was an assumption in the ES code that match-heading mappings will appear in
a specified portion of the mappings array.

Certain mappings setups will not meet this assumption.
We need to move our searching up one level

The key seems to be having a mapping for a complete field, say 150, in both the
match-heading and another field as well as having mappings for ungrouped fields like
150a 150ab etc.

The unit test coverage should be sufficient for testing
Comment 3 Nick Clemens 2020-04-03 12:27:49 UTC
Created attachment 102348 [details]
Mappings that uncovered the bug

You can test with this mapping file. 

Donwload and save the file.
Alter your koha-conf.xml to point to this file.
Reload your koha mappings.
Reset all the things
Reindex ES
Check an auth record:
    curl -XGET 'es:9200/koha_kohadev_authorities/data/568?pretty' | grep -A 5 match-heading
Note match heading contains the terms, but not a formatted heading string
Apply patch
Reindex ES
Check an auth record
You should now have a string with 'subdiv' terms e.g.:
Authors, Irish chronologicalsubdiv 20th century formsubdiv Biography
Comment 4 Andrew Fuerste-Henry 2020-04-03 13:05:35 UTC
Created attachment 102356 [details] [review]
Bug 25050: Unit tests

This patch simplay alters the data we use for the tests, doing so causes them to fail

To test:
1 - Apply only this patch
2 - prove -v t/Koha/SearchEngine/Elasticsearch.t
3 - It fails!
4 - Apply next patch
5 - It passes!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 Andrew Fuerste-Henry 2020-04-03 13:05:38 UTC
Created attachment 102357 [details] [review]
Bug 25050: Adjust marc_records_to_documents to search entire mapping array

There was an assumption in the ES code that match-heading mappings will appear in
a specified portion of the mappings array.

Certain mappings setups will not meet this assumption.
We need to move our searching up one level

The key seems to be having a mapping for a complete field, say 150, in both the
match-heading and another field as well as having mappings for ungrouped fields like
150a 150ab etc.

The unit test coverage should be sufficient for testing

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 6 Jonathan Druart 2020-04-10 08:59:59 UTC
Created attachment 102683 [details] [review]
Bug 25050: Unit tests

This patch simplay alters the data we use for the tests, doing so causes them to fail

To test:
1 - Apply only this patch
2 - prove -v t/Koha/SearchEngine/Elasticsearch.t
3 - It fails!
4 - Apply next patch
5 - It passes!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Jonathan Druart 2020-04-10 09:00:02 UTC
Created attachment 102684 [details] [review]
Bug 25050: Adjust marc_records_to_documents to search entire mapping array

There was an assumption in the ES code that match-heading mappings will appear in
a specified portion of the mappings array.

Certain mappings setups will not meet this assumption.
We need to move our searching up one level

The key seems to be having a mapping for a complete field, say 150, in both the
match-heading and another field as well as having mappings for ungrouped fields like
150a 150ab etc.

The unit test coverage should be sufficient for testing

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Martin Renvoize 2020-04-14 07:37:32 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Joy Nelson 2020-05-04 21:39:29 UTC
backported to 19.11.x for 19.11.06
Comment 10 Lucas Gass 2020-05-13 21:25:44 UTC
doesnt apply clean to 19.05.x, if needed please rebase