Bug 20596 - Authority record matching rule causes staging failure when MARC record contains multiple tag values for a match point
Summary: Authority record matching rule causes staging failure when MARC record contai...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-17 15:54 UTC by Marjorie Barry-Vila
Modified: 2023-12-28 20:44 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.01,22.05.09, 21.11.16


Attachments
authority to test (322 bytes, text/plain)
2018-04-17 15:54 UTC, Marjorie Barry-Vila
Details
Marcxml file with two entries 035a (6.42 KB, application/xml)
2022-12-13 08:20 UTC, Jan Kissig
Details
Screenshot of matching rule (58.26 KB, image/png)
2022-12-13 08:21 UTC, Jan Kissig
Details
Bug 20596: Join authority queries together with OR (3.31 KB, patch)
2022-12-14 05:18 UTC, David Cook
Details | Diff | Splinter Review
Bug 20596: Add unit test for multiple values in authority search (1.63 KB, patch)
2022-12-14 05:18 UTC, David Cook
Details | Diff | Splinter Review
Bug 20596: Join authority queries together with OR (3.36 KB, patch)
2022-12-14 18:40 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20596: Add unit test for multiple values in authority search (1.68 KB, patch)
2022-12-14 18:41 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 20596: Join authority queries together with OR (3.42 KB, patch)
2022-12-15 06:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 20596: Add unit test for multiple values in authority search (1.74 KB, patch)
2022-12-15 06:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 20596: (QA follow-up) perltidy (2.05 KB, patch)
2022-12-15 06:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 20596: Join authority queries together with OR (3.43 KB, patch)
2022-12-15 08:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20596: Add unit test for multiple values in authority search (1.74 KB, patch)
2022-12-15 08:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20596: (QA follow-up) perltidy (2.05 KB, patch)
2022-12-15 08:19 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 Marjorie Barry-Vila 2018-04-17 15:54:13 UTC
Created attachment 74361 [details]
authority to test

Hi,

We have a Zoom error when we apply a matching rule on a field (035) that appears twice in the authority record.

ZOOM error 20003 "can't set prefix query" (addinfo: "@or @attr 7=1 @attr 4=109 @attr 1=Local-Number 0  @attr 1=Any  @attr 4=1  @attr 5=100 @attr 6=3 "F000001" @attr 1=Any  @attr 4=1  @attr 5=100 @attr 6=3 "44444"") from diag-set 'ZOOM'

Test plan :
- Add a record matching rule for field 035$a (index control-number)
- upload a authority record file in tools/stage-marc-import.pl (attached file)
- go to Manage staged MARC records and open authority file
- apply matching rule for 035
- see error
Comment 1 Katrin Fischer 2021-03-04 09:49:56 UTC
Hi Marjorie,
have you checked that control-number exists for authority indexing? It does exist for bibliographic, but the index sets don't match up.

https://git.koha-community.org/Koha-community/Koha/src/branch/master/etc/zebradb/marc_defs/marc21/authorities/authority-koha-indexdefs.xml

This suggests 035 is not indexed at all for authorities :(
Comment 2 Marjorie Barry-Vila 2021-03-30 17:14:05 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Marjorie,
> have you checked that control-number exists for authority indexing? It does
> exist for bibliographic, but the index sets don't match up.
> 
> https://git.koha-community.org/Koha-community/Koha/src/branch/master/etc/
> zebradb/marc_defs/marc21/authorities/authority-koha-indexdefs.xml
> 
> This suggests 035 is not indexed at all for authorities :(

Hi Katrin,
Thank you for your reply and sorry for my delay.

I am amazed that zone 035 does not appear in the list of indexed zones.
 Also I am surprised that zone 016 does not appear in the authority-koha-indexdefs.xml file either, whereas for this field the search with the index control-number works.
Is there another file that lists potentially indexed zones for authorities where zone 016 would be found?

Regards,
Marjorie
Comment 3 Marjorie Barry-Vila 2021-03-30 17:14:05 UTC
(In reply to Katrin Fischer from comment #1)
> Hi Marjorie,
> have you checked that control-number exists for authority indexing? It does
> exist for bibliographic, but the index sets don't match up.
> 
> https://git.koha-community.org/Koha-community/Koha/src/branch/master/etc/
> zebradb/marc_defs/marc21/authorities/authority-koha-indexdefs.xml
> 
> This suggests 035 is not indexed at all for authorities :(

Hi Katrin,
Thank you for your reply and sorry for my delay.

I am amazed that zone 035 does not appear in the list of indexed zones.
 Also I am surprised that zone 016 does not appear in the authority-koha-indexdefs.xml file either, whereas for this field the search with the index control-number works.
Is there another file that lists potentially indexed zones for authorities where zone 016 would be found?

Regards,
Marjorie
Comment 4 Katrin Fischer 2021-04-02 13:36:00 UTC
Hi Marjorie, this should be the only file listing all indexes for the authorities. You might have local changes of course. And maybe it falls back to a keyword search if the index is unknown or so? I have no proof of this.
Comment 5 Jan Kissig 2022-12-12 13:26:49 UTC
hi,
since 21.11 the mentioned field 035(az) is indexed as Other-control-number both in biblios and authorities. 
The described error still exists when the record to be imported contains 2 datafields 035a.

Do record-match-rules rely on zoom, zebradb or would switching to elastic be a possible solution?

Jan
Comment 6 Katrin Fischer 2022-12-12 13:35:43 UTC
This is an interesting one - I am adding David Cook and Frido as our Zebra experts.
Comment 7 Katrin Fischer 2022-12-12 13:37:37 UTC
As far as I know the search index is used for matching. Switching to Elasticsearch might be an option to investigate, but fixing this would still be very worthwhile. Matching on 035 is an obvious choice.
Comment 8 David Cook 2022-12-12 23:24:28 UTC
At a glance, I think that there's a syntax error in the query being generated.

If you decompose the following:
@or @attr 7=1 @attr 4=109 @attr 1=Local-Number 0  @attr 1=Any  @attr 4=1  @attr 5=100 @attr 6=3 "F000001" @attr 1=Any  @attr 4=1  @attr 5=100 @attr 6=3 "44444"

You get this:

@or 
@attr 7=1 @attr 4=109 @attr 1=Local-Number 0  
@attr 1=Any  @attr 4=1  @attr 5=100 @attr 6=3 "F000001" 
@attr 1=Any  @attr 4=1  @attr 5=100 @attr 6=3 "44444"

There are 3 queries there but only 1 operator.

Marjorie, can you provide a screenshot of the record matching rule being used?
Comment 9 Jan Kissig 2022-12-13 08:18:44 UTC
I would like to update information as the index field to look at changed:


error:
==> /var/log/koha/hsbwildau/worker-output.log <==
ZOOM error 20003 "can't set prefix query" (addinfo: "@or @attr 7=1 @attr 4=109 @attr 1=Local-Number 0  @attr 1=Other-control-number  @attr 4=1  @attr 5=100 @attr 6=3 "(DE-101)04079184X" @attr 1=Other-control-number  @attr 4=1  @attr 5=100 @attr 6=3 "(DE-588)4079184-1"") from diag-set 'ZOOM' at /usr/share/koha/lib/Koha/BackgroundJob/StageMARCForImport.pm line 162.


Test plan: 
 - add matching rule as described in screenshot
 - stage gnd.xml for import with following settings
 - marcxml
 - record type: Authority 
 - format: marcxml
 - record matching rule as above
Comment 10 Jan Kissig 2022-12-13 08:20:32 UTC
Created attachment 144550 [details]
Marcxml file with two entries 035a
Comment 11 Jan Kissig 2022-12-13 08:21:23 UTC
Created attachment 144551 [details]
Screenshot of matching rule
Comment 12 David Cook 2022-12-14 03:53:53 UTC
Reproduced the error following Jan Kissig's instructions:

ZOOM error 20003 "can't set prefix query" (addinfo: "@or @attr 7=1 @attr 4=109 @attr 1=Local-Number 0  @attr 1=Other-control-number  @attr 4=1  @attr 5=100 @attr 6=3 "(DE-101)04079184X" @attr 1=Other-control-number  @attr 4=1  @attr 5=10
0 @attr 6=3 "(DE-588)4079184-1"") from diag-set 'ZOOM' at /kohadevbox/koha/Koha/BackgroundJob/StageMARCForImport.pm line 162.

It looks like C4::AuthoritiesMarc::SearchAuthorities is where the issue is.

The "$orderstring" is being tacked on the front which is OK: '@attr 7=1 @attr 4=109 @attr 1=Local-Number 0'

    227     $query="\@or $orderstring $query" if $orderstring;

That leaves the two actual queries:

@attr 1=Other-control-number  @attr 4=1  @attr 5=100 @attr 6=3 "(DE-101)04079184X" 
@attr 1=Other-control-number  @attr 4=1  @attr 5=100 @attr 6=3 "(DE-588)4079184-1"

As I suspected, Koha isn't adding the operator to join these two queries together. 

The query building code is frightful...
Comment 13 David Cook 2022-12-14 04:17:16 UTC
I'm curious about how the Elasticsearch handles it, since the "and_or" in C4::Matcher::get_matches makes no sense. 

Aha, it does something practical but annoying:

Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
 287         # Merge the indexes in with the search terms and the operands so that
 288         # each search thing is a handy unit.
 289         unshift @$operators, undef;    # The first one can't have an op

By comparison, C4::AuthoritiesMarc::SearchAuthorities doesn't actually use the real operators passed into the function...
Comment 14 David Cook 2022-12-14 04:30:38 UTC
Yeah... C4::AuthoritiesMarc::SearchAuthorities is fundamentally broken. 

(In reply to David Cook from comment #13)
> Koha/SearchEngine/Elasticsearch/QueryBuilder.pm
>  287         # Merge the indexes in with the search terms and the operands
> so that
>  288         # each search thing is a handy unit.
>  289         unshift @$operators, undef;    # The first one can't have an op

Actually, that is unrelated. You can ignore that comment...

--

If I understand the Elasticsearch code correctly, it's just hard-coding OR for build_authorities_query_compat().

Amusingly, many parts of Koha pass "and" into build_authorities_query_compat().
Comment 15 David Cook 2022-12-14 04:47:36 UTC
Ok so I'm working on a patch...

I tried both "Other-control-number" and "control-number", but neither worked. Now that's because "control-number" is not a valid authority index, and "Other-control-number" was probably just empty, since I do see it in ./etc/zebradb/marc_defs/marc21/authorities/authority-zebra-indexdefs.xsl
Comment 16 David Cook 2022-12-14 05:18:48 UTC
Created attachment 144561 [details] [review]
Bug 20596: Join authority queries together with OR

This patch joins authority queries together with OR like Elasticsearch.

To the best of the author's knowledge, this code only gets triggered
when using record matching rules, but there may be other situations
where it's used.

Authority search in cataloguing plugins use a different path, and
authority search in authority home doesn't allow multiple queries,
so they'll never trigger this OR joining of queries.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Create a record matching rule with the following:
Matching rule code: NID
Description: NormdatenID
Match threshold: 100
Record type: Authority record
Search index: Any
Score: 101
Tag: 035
Subfields: a
Offset: 0
Length: 0
Normalization rule: None

Click "Remove this match check"

2. Stage gnd.xml (from BZ 20596 attachments)
- Choose "MARCXML" for "Format"
- Choose "Authority" for "Record type"
- Choose "NID" for "Record matching rule"
- Click "Stage for import"
3. Note the job gets to "100% Finished"
(Before this patch, it would say "0% Failed")

4. View the batch
5. Import the record
6. Repeat steps 2-4 and note that the record matching rule
worked

7. Go to http://localhost:8081/cgi-bin/koha/authorities/authorities-home.pl
8. Search for "e"
9. Note that the search works

10. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode=
11. Click tab 6
12. Click on the tag editor next to "a Personal Name"
13. Type "e" into each box and click "Search"
14. Note that the search works
Comment 17 David Cook 2022-12-14 05:18:54 UTC
Created attachment 144562 [details] [review]
Bug 20596: Add unit test for multiple values in authority search

This patch adds a unit test which returns 2 authority records
via an implicit OR when searching for strings from two different
authority records.

Test plan:
1. prove t/db_dependent/Search.t
Comment 18 David Cook 2022-12-14 05:20:36 UTC
I think working on C4::AuthoritiesMarc just now may have taken years off my life, but I think those patches should do the trick.

Please give it a test and sign off if you're happy with it.

Perhaps Fridolin can QA this one...
Comment 19 Fridolin Somers 2022-12-14 07:08:57 UTC
> 
> Perhaps Fridolin can QA this one...

I will ;)
Comment 20 Katrin Fischer 2022-12-14 14:11:35 UTC
Jan, can you do the sign-off? You should be able to use a sandbox for it, if you don't have the dev env yet: https://wiki.koha-community.org/wiki/Sandboxes
Let us know, if we can help.
Comment 21 Katrin Fischer 2022-12-14 14:12:08 UTC
And thx to David and Frido :)
Comment 22 Jan Kissig 2022-12-14 15:14:09 UTC
Tested as described in comment #16 with
Search index: Any
and also with 
Search index: Other-control-number
and everything (staging and matching) worked as as expected!

Thanks for that fast response
Comment 23 PTFS Europe Sandboxes 2022-12-14 18:40:59 UTC
Created attachment 144590 [details] [review]
Bug 20596: Join authority queries together with OR

This patch joins authority queries together with OR like Elasticsearch.

To the best of the author's knowledge, this code only gets triggered
when using record matching rules, but there may be other situations
where it's used.

Authority search in cataloguing plugins use a different path, and
authority search in authority home doesn't allow multiple queries,
so they'll never trigger this OR joining of queries.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Create a record matching rule with the following:
Matching rule code: NID
Description: NormdatenID
Match threshold: 100
Record type: Authority record
Search index: Any
Score: 101
Tag: 035
Subfields: a
Offset: 0
Length: 0
Normalization rule: None

Click "Remove this match check"

2. Stage gnd.xml (from BZ 20596 attachments)
- Choose "MARCXML" for "Format"
- Choose "Authority" for "Record type"
- Choose "NID" for "Record matching rule"
- Click "Stage for import"
3. Note the job gets to "100% Finished"
(Before this patch, it would say "0% Failed")

4. View the batch
5. Import the record
6. Repeat steps 2-4 and note that the record matching rule
worked

7. Go to http://localhost:8081/cgi-bin/koha/authorities/authorities-home.pl
8. Search for "e"
9. Note that the search works

10. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode=
11. Click tab 6
12. Click on the tag editor next to "a Personal Name"
13. Type "e" into each box and click "Search"
14. Note that the search works

Signed-off-by: Katrin <katrin.fischer.83@web.de>
Comment 24 PTFS Europe Sandboxes 2022-12-14 18:41:05 UTC
Created attachment 144591 [details] [review]
Bug 20596: Add unit test for multiple values in authority search

This patch adds a unit test which returns 2 authority records
via an implicit OR when searching for strings from two different
authority records.

Test plan:
1. prove t/db_dependent/Search.t

Signed-off-by: Katrin <katrin.fischer.83@web.de>
Comment 25 Fridolin Somers 2022-12-15 06:38:07 UTC
Created attachment 144598 [details] [review]
Bug 20596: Join authority queries together with OR

This patch joins authority queries together with OR like Elasticsearch.

To the best of the author's knowledge, this code only gets triggered
when using record matching rules, but there may be other situations
where it's used.

Authority search in cataloguing plugins use a different path, and
authority search in authority home doesn't allow multiple queries,
so they'll never trigger this OR joining of queries.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Create a record matching rule with the following:
Matching rule code: NID
Description: NormdatenID
Match threshold: 100
Record type: Authority record
Search index: Any
Score: 101
Tag: 035
Subfields: a
Offset: 0
Length: 0
Normalization rule: None

Click "Remove this match check"

2. Stage gnd.xml (from BZ 20596 attachments)
- Choose "MARCXML" for "Format"
- Choose "Authority" for "Record type"
- Choose "NID" for "Record matching rule"
- Click "Stage for import"
3. Note the job gets to "100% Finished"
(Before this patch, it would say "0% Failed")

4. View the batch
5. Import the record
6. Repeat steps 2-4 and note that the record matching rule
worked

7. Go to http://localhost:8081/cgi-bin/koha/authorities/authorities-home.pl
8. Search for "e"
9. Note that the search works

10. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode=
11. Click tab 6
12. Click on the tag editor next to "a Personal Name"
13. Type "e" into each box and click "Search"
14. Note that the search works

Signed-off-by: Katrin <katrin.fischer.83@web.de>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 26 Fridolin Somers 2022-12-15 06:38:13 UTC
Created attachment 144599 [details] [review]
Bug 20596: Add unit test for multiple values in authority search

This patch adds a unit test which returns 2 authority records
via an implicit OR when searching for strings from two different
authority records.

Test plan:
1. prove t/db_dependent/Search.t

Signed-off-by: Katrin <katrin.fischer.83@web.de>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 27 Fridolin Somers 2022-12-15 06:38:19 UTC
Created attachment 144600 [details] [review]
Bug 20596: (QA follow-up) perltidy

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 28 Fridolin Somers 2022-12-15 06:39:27 UTC
Great work everyone on this tricky bug
Comment 29 Katrin Fischer 2022-12-15 08:00:16 UTC
(In reply to Jan Kissig from comment #22)
> Tested as described in comment #16 with
> Search index: Any
> and also with 
> Search index: Other-control-number
> and everything (staging and matching) worked as as expected!
> 
> Thanks for that fast response

It was actually not me testing, but Jan. There is a bug in the sandbox system where it uses the data of the one who created the sandbox instead of the information given on the sign-off form. To give Jan credit, can we please fix the sign-off lines before push?

I can do it, but it might take me too long, so leaving the comment as this is already PQA - thx Frido!
Comment 30 Martin Renvoize 2022-12-15 08:18:58 UTC
Created attachment 144605 [details] [review]
Bug 20596: Join authority queries together with OR

This patch joins authority queries together with OR like Elasticsearch.

To the best of the author's knowledge, this code only gets triggered
when using record matching rules, but there may be other situations
where it's used.

Authority search in cataloguing plugins use a different path, and
authority search in authority home doesn't allow multiple queries,
so they'll never trigger this OR joining of queries.

Test plan:
0. Apply patch and koha-plack --restart kohadev
1. Create a record matching rule with the following:
Matching rule code: NID
Description: NormdatenID
Match threshold: 100
Record type: Authority record
Search index: Any
Score: 101
Tag: 035
Subfields: a
Offset: 0
Length: 0
Normalization rule: None

Click "Remove this match check"

2. Stage gnd.xml (from BZ 20596 attachments)
- Choose "MARCXML" for "Format"
- Choose "Authority" for "Record type"
- Choose "NID" for "Record matching rule"
- Click "Stage for import"
3. Note the job gets to "100% Finished"
(Before this patch, it would say "0% Failed")

4. View the batch
5. Import the record
6. Repeat steps 2-4 and note that the record matching rule
worked

7. Go to http://localhost:8081/cgi-bin/koha/authorities/authorities-home.pl
8. Search for "e"
9. Note that the search works

10. Go to http://localhost:8081/cgi-bin/koha/cataloguing/addbiblio.pl?frameworkcode=
11. Click tab 6
12. Click on the tag editor next to "a Personal Name"
13. Type "e" into each box and click "Search"
14. Note that the search works

Signed-off-by: Jan Kissig <jan.kissig@th-wildau.de>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 31 Martin Renvoize 2022-12-15 08:19:04 UTC
Created attachment 144606 [details] [review]
Bug 20596: Add unit test for multiple values in authority search

This patch adds a unit test which returns 2 authority records
via an implicit OR when searching for strings from two different
authority records.

Test plan:
1. prove t/db_dependent/Search.t

Signed-off-by: Jan Kissig <jan.kissig@th-wildau.de>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 32 Martin Renvoize 2022-12-15 08:19:09 UTC
Created attachment 144607 [details] [review]
Bug 20596: (QA follow-up) perltidy

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 33 Martin Renvoize 2022-12-15 08:19:46 UTC
Signoff lines fixed at Katrins request.. great to have you aboard with testing Jan :)
Comment 34 Tomás Cohen Arazi 2022-12-15 12:58:23 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 35 Martin Renvoize 2022-12-15 14:05:49 UTC
Nice work everyone!

Pushed to 22.11.x for the next release
Comment 36 Lucas Gass 2023-01-19 23:27:44 UTC
Backported to 22.05.x for upcoming 22.05.09
Comment 37 Arthur Suzuki 2023-01-26 10:45:57 UTC
applied to 21.11.x for 21.11.16
Comment 38 wainuiwitikapark 2023-03-15 01:32:26 UTC
Not backported to 21.05.x