Bug 24586

Summary: Double quote escape doesn't work properly in C4::AuthoritiesMarc::SearchAuthorities with QueryParser enabled
Product: Koha Reporter: David Cook <dcook>
Component: Searching - ZebraAssignee: Bugs List <koha-bugs>
Status: CLOSED WONTFIX QA Contact:
Severity: normal    
Priority: P5 - low CC: katrin.fischer
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12742    
Attachments: Bug 24586: Gracefully fail authority lookup if PQF query invalid
Bug 24586: Add tests

Description David Cook 2020-02-05 04:23:33 UTC
Double quote escape doesn't work properly in C4::AuthoritiesMarc::SearchAuthorities with QueryParser enabled
Comment 1 David Cook 2020-02-05 04:28:21 UTC
QueryParser takes this as input:

(authtype:UNIF_TITLE) match-heading:"McGraw-Hill \\"Demystified\\" series" #AuthidAsc

And it outputs this:
 @or @attr 1=Local-Number @attr 7=1 0 @and @and @attr 1=authtype @attr 5=100 @attr 4=6 "UNIF_TITLE" @and @attr 1=Match-heading @attr 4=1 @attr 5=100 @attr 6=3 "Demystified\\\\"" @attr 1=Match-heading @attr 4=1 @attr 5=100 @attr 6=3 "series\\"" @attr 1=Match-heading @attr 4=1 @attr 5=100 @attr 6=3 "McGraw-Hill \\"

Which causes ZOOM::Query::PQF to throw a fatal error:
ZOOM error 20003 "can't set prefix query" (addinfo: "@or @attr 1=Local-Number @attr 7=1 0 @and @and @attr 1=authtype @attr 5=100 @attr 4=6 "UNIF_TITLE" @and @attr 1=Match-heading @attr 4=1 @attr 5=100 @attr 6=3 "Demystified\\\\"" @attr 1=Match-heading @attr 4=1 @attr 5=100 @attr 6=3 "series\\"" @attr 1=Match-heading @attr 4=1 @attr 5=100 @attr 6=3 "McGraw-Hill \\"")
Comment 2 David Cook 2020-02-05 04:55:42 UTC
Created attachment 98423 [details] [review]
Bug 24586: Gracefully fail authority lookup if PQF query invalid

If Koha's query building has produced an invalid PQF query, don't
throw a fatal error. Instead, trap the error, warn, and then skip
the authority lookup for that field.

Test Plan:

I don't really have a good test plan at the moment. Enable
AutoCreateAuthorities and BiblioAddsAuthorities maybe, and then
try to do a search for 830$a with a value of the following:

Bad "query" value
Comment 3 David Cook 2020-02-05 04:56:53 UTC
I've included a patch that I'm temporarily using as a workaround, but silently failing isn't perfect either. 

We should probably give the user a report after saving the record saying what happened with the authority linking.
Comment 4 Katrin Fischer 2020-02-05 07:08:05 UTC
I've added the omnibus bug for QueryParser - there is a ton of issues with it basically. I think we might deprecate it soon.
Comment 5 David Cook 2020-02-06 00:20:58 UTC
(In reply to Katrin Fischer from comment #4)
> I've added the omnibus bug for QueryParser - there is a ton of issues with
> it basically. I think we might deprecate it soon.

I am very much in favour of deprecating and removing the QueryParser. In the beginning, I was very supportive of the QueryParser, but I think there are just too many challenges trying to support it and the regular Koha query parsing.
Comment 6 David Cook 2020-02-06 00:22:07 UTC
For what it's worth, this patch would work for non-QueryParser as well. It's just about validating the PQF query before trying to search Zebra.
Comment 7 Jonathan Druart 2020-02-11 16:46:13 UTC
Created attachment 98721 [details] [review]
Bug 24586: Add tests

Not sure about this approach. Maybe we are hiding a problem coming from
somewhere else. Maybe better to handle the error from the callers.
Comment 8 David Cook 2020-02-11 23:08:39 UTC
(In reply to Jonathan Druart from comment #7)
> Created attachment 98721 [details] [review] [review]
> Bug 24586: Add tests
> 
> Not sure about this approach. Maybe we are hiding a problem coming from
> somewhere else. Maybe better to handle the error from the callers.

Agreed. The patch I've attached is a hot fix just to workaround the issue temporarily and I figured I'd share it, but I don't think that it's a complete solution.

For a long time, I've been thinking that we should catch errors from Zebra and Zebra libraries, and then display a user-friendly message to users from the calling script. Even something like "Something went wrong. Please contact your system administrator."

I think that could actually help surface a lot of existing problems.
Comment 9 David Cook 2020-02-11 23:09:55 UTC
(In reply to Jonathan Druart from comment #7)
> Created attachment 98721 [details] [review] [review]
> Bug 24586: Add tests
> 
> Not sure about this approach. Maybe we are hiding a problem coming from
> somewhere else. Maybe better to handle the error from the callers.

In fact, I forgot to mention it, but my patch will produce duplicate authorities. 

In the short term, I was ok with making authority duplicates rather than failing a bib import, but that would be a regression in Koha I think.

Better to fail rather than return nothing. I just haven't put the time into creating a complete solution as I have competing priorities :/.
Comment 10 Katrin Fischer 2020-02-24 20:54:19 UTC
We've voted to remove the QueryParser code (https://wiki.koha-community.org/wiki/Development_IRC_meeting_4_March_2020), so I am closing this WONTFIX.
Comment 11 David Cook 2020-02-25 06:35:56 UTC
That certainly makes life easier hehe