Bug 18131 - Matching staged records when using elastic search fails
Summary: Matching staged records when using elastic search fails
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 17546 (view as bug list)
Depends on: 19559 12478
Blocks:
  Show dependency treegraph
 
Reported: 2017-02-16 10:44 UTC by Stefan Berndtsson
Modified: 2017-11-26 16:25 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18131: ES - Fix matching staged records (6.24 KB, patch)
2017-02-27 15:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18131: ES - Fix matching staged records - Authority (2.30 KB, patch)
2017-03-23 17:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18131: ES - Fix matching staged records (6.32 KB, patch)
2017-04-12 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18131: ES - Fix matching staged records - Authority (2.35 KB, patch)
2017-04-12 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18131: Followup - Use data if biblionumber_tag < 10 (1.24 KB, patch)
2017-11-01 16:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18131: ES - Fix matching staged records (6.34 KB, patch)
2017-11-10 14:00 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18131: ES - Fix matching staged records - Authority (2.42 KB, patch)
2017-11-10 14:00 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18131: Followup - Use data if biblionumber_tag < 10 (1.31 KB, patch)
2017-11-10 14:00 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2017-02-16 10:44:35 UTC
There is a perl related bug in C4/Matcher.pm when collecting matched records.

"simple_search_compat" returns a reference to an array of matches. When using ElasticSearch, this array contains MARC::Record objects.

The score for these matches are then collected (currently line 692-694) into a hash "%matches" where the matched object is used as the key.

Perl converts all hash keys into strings, so the object is lost.

The hash "%matches" is used in a foreach (line 708) where it assumes the object can be recovered from the key, but this is not the case.

The error in the log is:
 stage-marc-import.pl: Can't locate object method "fields" via package "MARC::Record=HASH(0xb403da8)" (perhaps you forgot to load "MARC::Record=HASH(0xb403da8)"?) at /home/vagrant/kohaclone/C4/Biblio.pm line 2691., referer: http://localhost:8081/cgi-bin/koha/tools/stage-marc-import.pl



Steps to reproduce (given an instance using ElasticSearch as search engine):

1. Export a bibliographic record in MARC format.
2. Create a matching rule that matches on "999$c" with a score and threshold of 1000 (doesn't really matter as long as they're the same)
3. Edit the record in Koha so that they're slightly different (this is probably unnecessary).
4. Upload the exported file in Stage MARC import, and select the matching rule you created, then click on "Stage for import".


Expected outcome:
Redirected to the batch page with the matching record being shown.


Actual outcome:
Things seem to hang, but in reality the staging has crashed with the error shown above.
Comment 1 Jonathan Druart 2017-02-27 15:07:17 UTC
Created attachment 60716 [details] [review]
Bug 18131: ES - Fix matching staged records

The code in C4::Matches::get_matches is terrible and a bug has been introduced
by bug 12478 because of its way to handle uniqueness.

If search engine is elastic, simple_search_compat returns array ref of MARC::Record,
used as a string for the key of the matches hashref we get things like
"MARC::Record=HASH(0x8f76ab0)".
Yes, terrible...

The file is never staged and we get an internal server error:
    stage-marc-import.pl: Can't locate object method "fields" via package "MARC::Record=HASH(0x8f76ab0)" (perhaps you forgot to load "MARC::Record=HASH(0x8f76ab0)"?) at /home/vagrant/kohaclone/C4/Biblio.pm line 2691

To recreate the issue:
- Set SearchEngine == Elastic
- Create a matching rule on 999$c (you need to edit the existing one and specify
'Local-number' as search index, not 'local-number')
- Import a file with bibliographic records and use the matching rule you defined.

Test plan:
Import authority and bibliographic records with Zebra and Elastic using a matching rule.
Everything should work correctly.

Note: I found a bug when importing authorities using Elastic, see bug 17255 comment 38.
Comment 2 Nick Clemens 2017-03-23 13:15:26 UTC
Everything worked well for bibliogrpahic records, however, when I tried using an authority match rule I got:

Can't locate object method "field" via package "<?xml version="1.0" encoding="UTF-8"?>
<record xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://www.loc.gov/MARC21/slim" xsi:schemaLocation="http://www.loc.gov/MARC21/slim http://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd">

My rule was
Local-number
Field: 001
Comment 3 Jonathan Druart 2017-03-23 17:33:40 UTC
(In reply to Nick Clemens from comment #2)
> Everything worked well for bibliogrpahic records, however, when I tried
> using an authority match rule I got:

Yes but, as you might know, the authority searches are broken all around Koha.
Occurrences of C4::AuthoritiesMarc::SearchAuthorities must be replaced by search_auth_compat.
I would consider this outside the scope of this bug report...
Comment 4 Jonathan Druart 2017-03-23 17:47:56 UTC
Created attachment 61586 [details] [review]
Bug 18131: ES - Fix matching staged records - Authority

Occurrences of C4::AuthoritiesMarc::SearchAuthorities must be replaced
by search_auth_compat.

You need to define the search index of matching rule with one of the
values defined in %koha_to_index_name (from
Koha::SearchEngine::Elasticsearch::QueryBuilder::build_authorities_query_compat)
Comment 5 Jonathan Druart 2017-03-23 17:48:25 UTC
Nick, this patch may fix your issue.
Comment 6 Nick Clemens 2017-04-03 18:28:14 UTC
*** Bug 17546 has been marked as a duplicate of this bug. ***
Comment 7 Nick Clemens 2017-04-12 13:27:01 UTC
testing again, this does remove the fields error from authorities, but things are not fixed on authority side:

Matching rules can only use a small set of indexes in build_authorities_query_compat:
Options are: mainmainentry, mainentry, match, match-heading, see-from, and thesaurus.
Tag in Matchpoint components seems to be useless?

Sorting is also broken and I had to force no sorting to get match results, and then they didn't match anything
Options are: mainmainentry, mainentry, match, match-heading, see-from, and
373 thesaurus.

As Joubu said in comment #3 this is all outside the scope of this bug report.

Signing off on patches as they offer great improvement and fix matching for biblios - authorities issues should be filed as a new ticket
Comment 8 Nick Clemens 2017-04-12 13:28:17 UTC
Created attachment 62087 [details] [review]
Bug 18131: ES - Fix matching staged records

The code in C4::Matches::get_matches is terrible and a bug has been introduced
by bug 12478 because of its way to handle uniqueness.

If search engine is elastic, simple_search_compat returns array ref of MARC::Record,
used as a string for the key of the matches hashref we get things like
"MARC::Record=HASH(0x8f76ab0)".
Yes, terrible...

The file is never staged and we get an internal server error:
    stage-marc-import.pl: Can't locate object method "fields" via package "MARC::Record=HASH(0x8f76ab0)" (perhaps you forgot to load "MARC::Record=HASH(0x8f76ab0)"?) at /home/vagrant/kohaclone/C4/Biblio.pm line 2691

To recreate the issue:
- Set SearchEngine == Elastic
- Create a matching rule on 999$c (you need to edit the existing one and specify
'Local-number' as search index, not 'local-number')
- Import a file with bibliographic records and use the matching rule you defined.

Test plan:
Import authority and bibliographic records with Zebra and Elastic using a matching rule.
Everything should work correctly.

Note: I found a bug when importing authorities using Elastic, see bug 17255 comment 38.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2017-04-12 13:28:22 UTC
Created attachment 62088 [details] [review]
Bug 18131: ES - Fix matching staged records - Authority

Occurrences of C4::AuthoritiesMarc::SearchAuthorities must be replaced
by search_auth_compat.

You need to define the search index of matching rule with one of the
values defined in %koha_to_index_name (from
Koha::SearchEngine::Elasticsearch::QueryBuilder::build_authorities_query_compat)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Julian Maurice 2017-06-01 14:59:59 UTC
Comment on attachment 62087 [details] [review]
Bug 18131: ES - Fix matching staged records

Review of attachment 62087 [details] [review]:
-----------------------------------------------------------------

::: C4/Matcher.pm
@@ +673,5 @@
> +            else {
> +                if ( C4::Context->preference('SearchEngine') eq 'Elasticsearch' ) {
> +                    foreach my $matched ( @{$searchresults} ) {
> +                        my ( $biblionumber_tag, $biblionumber_subfield ) = C4::Biblio::GetMarcFromKohaField( "biblio.biblionumber", $marcframework_used );
> +                        my $id = $matched->field($biblionumber_tag)->subfield($biblionumber_subfield);

This fails on a UNIMARC setup where $biblionumber_tag is 001 (should use ->data())
Comment 11 Julian Maurice 2017-06-01 15:03:28 UTC
Also, this is weird that search_result_compat returns different result depending on the search engine. Shouldn't we fix one of Koha::SearchEngine::*::Search::search_result_compat ?

Failing QA for comment 10
Comment 12 Nick Clemens 2017-11-01 16:23:22 UTC
Created attachment 68884 [details] [review]
Bug 18131: Followup - Use data if biblionumber_tag < 10
Comment 13 Nick Clemens 2017-11-01 16:26:08 UTC
(In reply to Julian Maurice from comment #11)
> Also, this is weird that search_result_compat returns different result
> depending on the search engine. Shouldn't we fix one of
> Koha::SearchEngine::*::Search::search_result_compat ?

Agreed, it is odd, see the comment in the pod:
315 an arrayref of MARC::Records (note that this is different from the
316 L<C4::Search> version which will return plain XML, but too bad.)

I think beyond the scope here, let's get this fixed for now.
Comment 14 Julian Maurice 2017-11-10 14:00:43 UTC
Created attachment 69087 [details] [review]
Bug 18131: ES - Fix matching staged records

The code in C4::Matches::get_matches is terrible and a bug has been introduced
by bug 12478 because of its way to handle uniqueness.

If search engine is elastic, simple_search_compat returns array ref of MARC::Record,
used as a string for the key of the matches hashref we get things like
"MARC::Record=HASH(0x8f76ab0)".
Yes, terrible...

The file is never staged and we get an internal server error:
    stage-marc-import.pl: Can't locate object method "fields" via package "MARC::Record=HASH(0x8f76ab0)" (perhaps you forgot to load "MARC::Record=HASH(0x8f76ab0)"?) at /home/vagrant/kohaclone/C4/Biblio.pm line 2691

To recreate the issue:
- Set SearchEngine == Elastic
- Create a matching rule on 999$c (you need to edit the existing one and specify
'Local-number' as search index, not 'local-number')
- Import a file with bibliographic records and use the matching rule you defined.

Test plan:
Import authority and bibliographic records with Zebra and Elastic using a matching rule.
Everything should work correctly.

Note: I found a bug when importing authorities using Elastic, see bug 17255 comment 38.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 15 Julian Maurice 2017-11-10 14:00:48 UTC
Created attachment 69088 [details] [review]
Bug 18131: ES - Fix matching staged records - Authority

Occurrences of C4::AuthoritiesMarc::SearchAuthorities must be replaced
by search_auth_compat.

You need to define the search index of matching rule with one of the
values defined in %koha_to_index_name (from
Koha::SearchEngine::Elasticsearch::QueryBuilder::build_authorities_query_compat)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 16 Julian Maurice 2017-11-10 14:00:52 UTC
Created attachment 69089 [details] [review]
Bug 18131: Followup - Use data if biblionumber_tag < 10

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 17 Jonathan Druart 2017-11-26 16:25:43 UTC
Pushed to master for 17.11, thanks to everybody involved!