Bug 24586

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 \\"")

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

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.

I've added the omnibus bug for QueryParser - there is a ton of issues with it basically. I think we might deprecate it soon.

(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.

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.

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.

(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.

(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 :/.

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.

That certainly makes life easier hehe