Koha's current query parsing code is very fragile, hard to understand, and generally quite buggy. By sharing Evergreen's general-purpose QueryParser module, we can eliminate a lot of that code and offer users a better experience.
Created attachment 14020 [details] [review] Bug 9239: Add QueryParser module to Koha Although the OpenILS::QueryParser class is going to be moved into a separate CPAN distribution, this hasn't been done yet, and for ease of testing it makes sense to include the class in Koha for the moment.
Created attachment 14021 [details] [review] Bug 9239: Introduce QueryParser driver for PQF Since the most expressive query language supported by Zebra is PQF, this patch adds a PQF driver for QueryParser which will translate QueryParser queries into standard PQF (guided by mappings which have been written to match Koha's existing Zebra configuration) which can then be sent to Zebra. This driver, Koha::QueryParser::Driver::PQF(::*) extends the OpenILS::QueryParser(::*) class(es), so as to preserve maximum interoperability between the various users of the QueryParser driver.
Created attachment 14022 [details] [review] Bug 9239: Add syspref to enable QueryParser In light of the experimental nature of the QueryParser code at this juncture, it behooves Koha to have the option of disabling QP, at least for the purpose of comparing results between QP-enabled and QP-disabled searches. This patch adds a UseQueryParser syspref to do just that. In cases where the user enables QP without having QP installed, however, there will be no dramatic failure, and Koha will simply fall back to using the non-QP search code. In order to reduce the overhead added by the introduction of QueryParser, this patch adds a C4::Context->queryparser accessor to a static QueryParser object which is initialized the first time it is required and then resed thereafter.
Created attachment 14023 [details] [review] Bug 9239: Allow the use of QueryParser for all queries With the inclusion of this patch, all searches will (try) to use QueryParser for handling queries for both the bibliographic and authority databases if UseQueryParser is enabled. If QueryParser is unavailable, UseQueryParser is disabled, or the search uses CCL indexes, the old search code will be used.
The four patches I have attached to this bug do not represent entirely finished work. In particular, I did not yet write test plans, and a reshuffling of the code hierarchies means that the unit tests I had do not work at present. However, rather than wait until I have completely finished everything, I wanted to post this work-in-progress so that people who were interested could apply the patches, try a few searches, and see what they think. One important point on testing: you will need to add <queryparser_config> element at the end of koha-conf.xml which points to etc/searchengine/queryparser.yaml. For example, <queryparser_config>/home/jcamins/kohaclone/etc/searchengine/queryparser.yaml</queryparser_config>
Created attachment 14172 [details] [review] Bug 9239: Add QueryParser module to Koha Although the OpenILS::QueryParser class is going to be moved into a separate CPAN distribution, this hasn't been done yet, and for ease of testing it makes sense to include the class in Koha for the moment.
Created attachment 14173 [details] [review] Bug 9239: Introduce QueryParser driver for PQF Since the most expressive query language supported by Zebra is PQF, this patch adds a PQF driver for QueryParser which will translate QueryParser queries into standard PQF (guided by mappings which have been written to match Koha's existing Zebra configuration) which can then be sent to Zebra. This driver, Koha::QueryParser::Driver::PQF(::*) extends the OpenILS::QueryParser(::*) class(es), so as to preserve maximum interoperability between the various users of the QueryParser driver. Initially, search syntax is as follows: * AND operator: && * OR operator: || * GROUPING operators: ( ) Fields can mostly be searched using the ccl prefixes they have now. The exception is the various date limits which are searched with a syntax like this: pubdate(2008) For sorting, you can simply add #title-sort-az (etc.) to your query.
Created attachment 14174 [details] [review] Bug 9239: Add syspref to enable QueryParser In light of the experimental nature of the QueryParser code at this juncture, it behooves Koha to have the option of disabling QP, at least for the purpose of comparing results between QP-enabled and QP-disabled searches. This patch adds a UseQueryParser syspref to do just that. In cases where the user enables QP without having QP installed, however, there will be no dramatic failure, and Koha will simply fall back to using the non-QP search code. In order to reduce the overhead added by the introduction of QueryParser, this patch adds a C4::Context->queryparser accessor to a static QueryParser object which is initialized the first time it is required and then reset thereafter.
Created attachment 14175 [details] [review] Bug 9239: Allow the use of QueryParser for all queries With the inclusion of this patch, all searches will (try) to use QueryParser for handling queries for both the bibliographic and authority databases if UseQueryParser is enabled. If QueryParser is unavailable, UseQueryParser is disabled, or the search uses CCL indexes, the old search code will be used. To test: 1) Apply patch. 2) Run the unit test with `prove t/QueryParser.t` 3) Enable the UseQueryParser syspref. 4) Try searches that should return results in the following places: * OPAC (simple search) * OPAC (advanced search) * OPAC (authorities) * Staff client (header search) * Staff client (advanced search) * Staff client (cataloging search) * Staff client (authorities) * Staff client (importing a batch using a match point) * Staff client (searching for an item for adding to a label) * Staff client (acquisitions) * Staff client (searching for a record to create a serial) * ANYWHERE ELSE I HAVE FORGOTTEN 5) Disable the UseQueryParser syspref. Repeat at least some of the searches you did above. 6) If all searches worked, sign off.
Tests all pass t/QueryParser.t .. ok All tests successful. Files=1, Tests=27, 0 wallclock secs ( 0.03 usr 0.01 sys + 0.14 cusr 0.01 csys = 0.19 CPU) Result: PASS Working through the rest of the test plan now
Created attachment 14180 [details] [review] Bug 9239: Add QueryParser module to Koha Although the OpenILS::QueryParser class is going to be moved into a separate CPAN distribution, this hasn't been done yet, and for ease of testing it makes sense to include the class in Koha for the moment. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 14181 [details] [review] Bug 9239: Introduce QueryParser driver for PQF Since the most expressive query language supported by Zebra is PQF, this patch adds a PQF driver for QueryParser which will translate QueryParser queries into standard PQF (guided by mappings which have been written to match Koha's existing Zebra configuration) which can then be sent to Zebra. This driver, Koha::QueryParser::Driver::PQF(::*) extends the OpenILS::QueryParser(::*) class(es), so as to preserve maximum interoperability between the various users of the QueryParser driver. Initially, search syntax is as follows: * AND operator: && * OR operator: || * GROUPING operators: ( ) Fields can mostly be searched using the ccl prefixes they have now. The exception is the various date limits which are searched with a syntax like this: pubdate(2008) For sorting, you can simply add #title-sort-az (etc.) to your query. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 14182 [details] [review] Bug 9239: Add syspref to enable QueryParser In light of the experimental nature of the QueryParser code at this juncture, it behooves Koha to have the option of disabling QP, at least for the purpose of comparing results between QP-enabled and QP-disabled searches. This patch adds a UseQueryParser syspref to do just that. In cases where the user enables QP without having QP installed, however, there will be no dramatic failure, and Koha will simply fall back to using the non-QP search code. In order to reduce the overhead added by the introduction of QueryParser, this patch adds a C4::Context->queryparser accessor to a static QueryParser object which is initialized the first time it is required and then reset thereafter. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 14183 [details] [review] Bug 9239: Allow the use of QueryParser for all queries With the inclusion of this patch, all searches will (try) to use QueryParser for handling queries for both the bibliographic and authority databases if UseQueryParser is enabled. If QueryParser is unavailable, UseQueryParser is disabled, or the search uses CCL indexes, the old search code will be used. To test: 1) Apply patch. 2) Run the unit test with `prove t/QueryParser.t` 3) Enable the UseQueryParser syspref. 4) Try searches that should return results in the following places: * OPAC (simple search) * OPAC (advanced search) * OPAC (authorities) * Staff client (header search) * Staff client (advanced search) * Staff client (cataloging search) * Staff client (authorities) * Staff client (importing a batch using a match point) * Staff client (searching for an item for adding to a label) * Staff client (acquisitions) * Staff client (searching for a record to create a serial) * ANYWHERE ELSE I HAVE FORGOTTEN 5) Disable the UseQueryParser syspref. Repeat at least some of the searches you did above. 6) If all searches worked, sign off. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 14847 [details] [review] Bug 9239 follow-up: POD and perlcritic fixes There were several perlcritic violations for returning explicit undef, and splitting up the various driver classes resulted in POD compliance errors, where the POD was starting with head2 rather than head1. This patch fixes those errors (but has no functional impact).
First tests look promising :)
Hi Jared, there is a little problem on updating the koha-conf.xml file for existing installations. I have tested this with 2 different installations now and updating the koha-conf.xml file to add the query parser line does not work for me. To reproduce: 1) Apply all patches 2) (Run database update) 3) Update koha-conf.xml: perl Makefile.PL --prev-install-log ../koha-dev/misc/koha-install-log make make test sudo make upgrade It prompted me for dom and icu (which I thought I had already activated). It completed successfully, but checking the generated file, the tag for the query parser is missing. Can you please take a look what's going wrong here? I will try to continue testing with a new installation.
I set up a new dev installation with Normarc and the conf file still doesn't show anything related to the query parser: Last lines of my koha-conf.xml file: <zebra_bib_index_mode>dom</zebra_bib_index_mode> <zebra_auth_index_mode>dom</zebra_auth_index_mode> </config> </yazgfs> When I added the lines you gave me on IRC I was able to successfully test some searches with the new query syntax, so I think this might be the only piece missing?
Created attachment 14949 [details] [review] [PAssed QA] Bug 9239: Add QueryParser module to Koha Although the OpenILS::QueryParser class is going to be moved into a separate CPAN distribution, this hasn't been done yet, and for ease of testing it makes sense to include the class in Koha for the moment. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com>
Created attachment 14950 [details] [review] [Passed QA] Bug 9239: Introduce QueryParser driver for PQF Since the most expressive query language supported by Zebra is PQF, this patch adds a PQF driver for QueryParser which will translate QueryParser queries into standard PQF (guided by mappings which have been written to match Koha's existing Zebra configuration) which can then be sent to Zebra. This driver, Koha::QueryParser::Driver::PQF(::*) extends the OpenILS::QueryParser(::*) class(es), so as to preserve maximum interoperability between the various users of the QueryParser driver. Initially, search syntax is as follows: * AND operator: && * OR operator: || * GROUPING operators: ( ) Fields can mostly be searched using the ccl prefixes they have now. The exception is the various date limits which are searched with a syntax like this: pubdate(2008) For sorting, you can simply add #title-sort-az (etc.) to your query. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Test Passed successfully after installing missing dep for Test::Deep
Created attachment 14951 [details] [review] [Passed QA] Bug 9239: Add syspref to enable QueryParser In light of the experimental nature of the QueryParser code at this juncture, it behooves Koha to have the option of disabling QP, at least for the purpose of comparing results between QP-enabled and QP-disabled searches. This patch adds a UseQueryParser syspref to do just that. In cases where the user enables QP without having QP installed, however, there will be no dramatic failure, and Koha will simply fall back to using the non-QP search code. In order to reduce the overhead added by the introduction of QueryParser, this patch adds a C4::Context->queryparser accessor to a static QueryParser object which is initialized the first time it is required and then reset thereafter. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Fixed merge conflict in sysprefs.sql and updatedatabase.sql
Created attachment 14952 [details] [review] [Passed QA] Bug 9239: Allow the use of QueryParser for all queries With the inclusion of this patch, all searches will (try) to use QueryParser for handling queries for both the bibliographic and authority databases if UseQueryParser is enabled. If QueryParser is unavailable, UseQueryParser is disabled, or the search uses CCL indexes, the old search code will be used. To test: 1) Apply patch. 2) Run the unit test with `prove t/QueryParser.t` 3) Enable the UseQueryParser syspref. 4) Try searches that should return results in the following places: * OPAC (simple search) * OPAC (advanced search) * OPAC (authorities) * Staff client (header search) * Staff client (advanced search) * Staff client (cataloging search) * Staff client (authorities) * Staff client (importing a batch using a match point) * Staff client (searching for an item for adding to a label) * Staff client (acquisitions) * Staff client (searching for a record to create a serial) * ANYWHERE ELSE I HAVE FORGOTTEN 5) Disable the UseQueryParser syspref. Repeat at least some of the searches you did above. 6) If all searches worked, sign off. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Searching still works as expected for variuos places. QueryParser syspref seemed to be enabled by default
Created attachment 14953 [details] [review] [Signed off] Bug 9239 follow-up: POD and perlcritic fixes There were several perlcritic violations for returning explicit undef, and splitting up the various driver classes resulted in POD compliance errors, where the POD was starting with head2 rather than head1. This patch fixes those errors (but has no functional impact). Signed-off-by: Elliott Davis <elliott@bywatersolions.com>
Applied to fix a couple of merge issues. Appears to work as advertised. Provided test plan passes. I didn't experience the issues that katrin did but I already had those fields in my koha-conf xml file.
I went back and removed a line from my conf file that I had from testing previously (which is why it worked for me initially) and re-ran make and got the same error as KF. Apologies for the mis-informed test. I think there should be a follow up to add this line to make but shouldn't be a reason to fail the test. (In reply to comment #24) > Applied to fix a couple of merge issues. Appears to work as advertised. > Provided test plan passes. I didn't experience the issues that katrin did > but I already had those fields in my koha-conf xml file.
Created attachment 15121 [details] [review] Bug 9239 follow-up: update koha-conf.xml The previous patches were missing the koha-conf.xml updates. This patch updates koha-conf.xml and makes the changes neccessary to include the QueryParser configuration file in the packages. To test: 1) Run Makefile and check generated koha-conf.xml to confirm that the line <queryparser_config>...</queryparser_config> is there with an absolute path. 2) That was it.
Created attachment 15437 [details] [review] Bug 9239 follow-up: update koha-conf.xml The previous patches were missing the koha-conf.xml updates. This patch updates koha-conf.xml and makes the changes neccessary to include the QueryParser configuration file in the packages. To test: 1) Run Makefile and check generated koha-conf.xml to confirm that the line <queryparser_config>...</queryparser_config> is there with an absolute path. 2) That was it. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Starting testing on this again...
Created attachment 15865 [details] [review] Bug 9239 QA follow-up: fix highlighting and merge error QP searches with && broke search highlighting on the OPAC details page. This patch corrects encoding of the query_desc parameter that is passed to the details page. My last attempt at rebasing also transposed the variable for index names with the variable for operators, meaning that the dropdown in the basic search did not work.
Created attachment 15867 [details] [review] Bug 9239 QA follow-up: fix highlighting and merge error QP searches with && broke search highlighting on the OPAC details page. This patch corrects encoding of the query_desc parameter that is passed to the details page. My last attempt at rebasing also transposed the variable for index names with the variable for operators, meaning that the dropdown in the basic search did not work. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Working on this again... we are getting close!
Created attachment 15929 [details] [review] Bug 9239 QA-follow-up: quiet warnings and fix problem with limits
Created attachment 15999 [details] [review] Bug 9239 QA follow-up: escape CGI input Koha was not previously escaping CGI input, which caused problems for highlighting and is a security issue.
Created attachment 16004 [details] [review] Bug 9239 QA follow-up: the last QA follow-up was missing a require This patch also corrects the definition of the an= index, which was missing exactness.
Created attachment 16051 [details] [review] Bug 9239 QA follow-up: don't fall back to kw if field is specified
Created attachment 16125 [details] [review] Bug 9239 QA follow-up: better error handling * Fix a long-standing bug in the linker that could crash the linker when run against odd data. * Sanitize input to SimpleSearch. * Correctly handle CCL indexes with QP.
Created attachment 16130 [details] [review] Bug 9239 QA follow-up: remove stray debug code Remove a line of debug code from EG, provide better error handling when presented with weird data in the authority linker, and correct queryparser configuration to reflect the correct configuration for Zebra.
Created attachment 16131 [details] [review] Bug 9239 QA follow-up: escape quotes in searches This patch escapes quotes in searches for those rare instances where there is a literal quote inside a search term.
Created attachment 16132 [details] [review] Bug 9239 QA follow-up: amend error handling
Created attachment 16133 [details] [review] Bug 9239 QA follow-up: amend error handling
Created attachment 16136 [details] [review] Bug 9239 QA follow-up: fix overeager quote escaping The quote escaping added two follow-ups ago was a little too eager, and escaped perfectly valid quotes in some instances. This patch moves the escaping deeper into the loop so that no needed quotes will be escaped.
Created attachment 16140 [details] [review] [PASSED QA] Bug 9239: Add QueryParser module to Koha Although the OpenILS::QueryParser class is going to be moved into a separate CPAN distribution, this hasn't been done yet, and for ease of testing it makes sense to include the class in Koha for the moment. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16141 [details] [review] [PASSED QA] Bug 9239: Introduce QueryParser driver for PQF Since the most expressive query language supported by Zebra is PQF, this patch adds a PQF driver for QueryParser which will translate QueryParser queries into standard PQF (guided by mappings which have been written to match Koha's existing Zebra configuration) which can then be sent to Zebra. This driver, Koha::QueryParser::Driver::PQF(::*) extends the OpenILS::QueryParser(::*) class(es), so as to preserve maximum interoperability between the various users of the QueryParser driver. Initially, search syntax is as follows: * AND operator: && * OR operator: || * GROUPING operators: ( ) Fields can mostly be searched using the ccl prefixes they have now. The exception is the various date limits which are searched with a syntax like this: pubdate(2008) For sorting, you can simply add #title-sort-az (etc.) to your query. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Test Passed successfully after installing missing dep for Test::Deep Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16142 [details] [review] [PASSED QA] Bug 9239: Add syspref to enable QueryParser In light of the experimental nature of the QueryParser code at this juncture, it behooves Koha to have the option of disabling QP, at least for the purpose of comparing results between QP-enabled and QP-disabled searches. This patch adds a UseQueryParser syspref to do just that. In cases where the user enables QP without having QP installed, however, there will be no dramatic failure, and Koha will simply fall back to using the non-QP search code. In order to reduce the overhead added by the introduction of QueryParser, this patch adds a C4::Context->queryparser accessor to a static QueryParser object which is initialized the first time it is required and then reset thereafter. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Fixed merge conflict in sysprefs.sql and updatedatabase.sql Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed merge conflict in sysprefs.sql und updatedatabase.pl
Created attachment 16143 [details] [review] [PASSED QA] Bug 9239: Allow the use of QueryParser for all queries With the inclusion of this patch, all searches will (try) to use QueryParser for handling queries for both the bibliographic and authority databases if UseQueryParser is enabled. If QueryParser is unavailable, UseQueryParser is disabled, or the search uses CCL indexes, the old search code will be used. To test: 1) Apply patch. 2) Run the unit test with `prove t/QueryParser.t` 3) Enable the UseQueryParser syspref. 4) Try searches that should return results in the following places: * OPAC (simple search) * OPAC (advanced search) * OPAC (authorities) * Staff client (header search) * Staff client (advanced search) * Staff client (cataloging search) * Staff client (authorities) * Staff client (importing a batch using a match point) * Staff client (searching for an item for adding to a label) * Staff client (acquisitions) * Staff client (searching for a record to create a serial) * ANYWHERE ELSE I HAVE FORGOTTEN 5) Disable the UseQueryParser syspref. Repeat at least some of the searches you did above. 6) If all searches worked, sign off. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Searching still works as expected for variuos places. QueryParser syspref seemed to be enabled by default Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16144 [details] [review] [PASSED QA] Bug 9239 follow-up: POD and perlcritic fixes There were several perlcritic violations for returning explicit undef, and splitting up the various driver classes resulted in POD compliance errors, where the POD was starting with head2 rather than head1. This patch fixes those errors (but has no functional impact). Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16145 [details] [review] [PASSED QA] Bug 9239 follow-up: update koha-conf.xml The previous patches were missing the koha-conf.xml updates. This patch updates koha-conf.xml and makes the changes neccessary to include the QueryParser configuration file in the packages. To test: 1) Run Makefile and check generated koha-conf.xml to confirm that the line <queryparser_config>...</queryparser_config> is there with an absolute path. 2) That was it. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tested update successfully - new line now shows up and query parser is used.
Created attachment 16146 [details] [review] [PASSED QA] Bug 9239 QA follow-up: fix highlighting and merge error QP searches with && broke search highlighting on the OPAC details page. This patch corrects encoding of the query_desc parameter that is passed to the details page. My last attempt at rebasing also transposed the variable for index names with the variable for operators, meaning that the dropdown in the basic search did not work. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixes some problems raised during QA successfully.
Created attachment 16147 [details] [review] [PASSED QA] Bug 9239 QA-follow-up: quiet warnings and fix problem with limits Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16148 [details] [review] [PASSED QA] Bug 9239 QA follow-up: escape CGI input Koha was not previously escaping CGI input, which caused problems for highlighting and is a security issue. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Thx for fixing this.
Created attachment 16149 [details] [review] [PASSED QA] Bug 9239 QA follow-up: the last QA follow-up was missing a require This patch also corrects the definition of the an= index, which was missing exactness. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16150 [details] [review] [PASSED QA] Bug 9239 QA follow-up: don't fall back to kw if field is specified Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16151 [details] [review] [PASSED QA] Bug 9239 QA follow-up: remove stray debug code Remove a line of debug code from EG, provide better error handling when presented with weird data in the authority linker, and correct queryparser configuration to reflect the correct configuration for Zebra. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16152 [details] [review] [PASSED QA] Bug 9239 QA follow-up: better error handling * Fix a long-standing bug in the linker that could crash the linker when run against odd data. * Sanitize input to SimpleSearch. * Correctly handle CCL indexes with QP. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16153 [details] [review] [PASSED QA] Bug 9239 QA follow-up: escape quotes in searches This patch escapes quotes in searches for those rare instances where there is a literal quote inside a search term. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16154 [details] [review] [PASSED QA] Bug 9239 QA follow-up: amend error handling Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 16155 [details] [review] [PASSED QA] Bug 9239 QA follow-up: fix overeager quote escaping The quote escaping added two follow-ups ago was a little too eager, and escaped perfectly valid quotes in some instances. This patch moves the escaping deeper into the loop so that no needed quotes will be escaped. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Test plan included here, as this is the last patch in the series: All tests pass. OPAC Simple search - Keyword searches using boolean operators || and && - OK - Pubdate searches using pubdate() - OK - Using different search options: keyword, title, etc. - OK Facets - limit to available from a query parser simple search - OK - Other facets - OK Advanced search - Date range search - OK - Itemtype limits - OK - Language limit - OK - Combining various serach options with and without 'more' options and using boolean operators - OK Authorities - Searching for partial and complete names like "M", "Benedictus" works. - OK - Biblio count isc correct - OK - Links to linked biblios work. - OK. INTRANET Staged MARC import - Import records using various matching points - OK Serials - Search for a serial in subscription form - OK Acquisitions - Search for existing record from basket - OK Authorities - Running the linker script with QP on works. - OK - Searching for authorities works. - OK - Biblio count isc correct - OK - Links to linked biblios work. - OK. NOTE: As agreed with Jared in chat, query parser should be off by default for the next release.
Created attachment 16180 [details] [review] Bug 9239: Add QueryParser module to Koha Although the OpenILS::QueryParser class is going to be moved into a separate CPAN distribution, this hasn't been done yet, and for ease of testing it makes sense to include the class in Koha for the moment. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16181 [details] [review] Bug 9239: Introduce QueryParser driver for PQF Since the most expressive query language supported by Zebra is PQF, this patch adds a PQF driver for QueryParser which will translate QueryParser queries into standard PQF (guided by mappings which have been written to match Koha's existing Zebra configuration) which can then be sent to Zebra. This driver, Koha::QueryParser::Driver::PQF(::*) extends the OpenILS::QueryParser(::*) class(es), so as to preserve maximum interoperability between the various users of the QueryParser driver. Initially, search syntax is as follows: * AND operator: && * OR operator: || * GROUPING operators: ( ) Fields can mostly be searched using the ccl prefixes they have now. The exception is the various date limits which are searched with a syntax like this: pubdate(2008) For sorting, you can simply add #title-sort-az (etc.) to your query. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Test Passed successfully after installing missing dep for Test::Deep Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16182 [details] [review] Bug 9239: Add syspref to enable QueryParser In light of the experimental nature of the QueryParser code at this juncture, it behooves Koha to have the option of disabling QP, at least for the purpose of comparing results between QP-enabled and QP-disabled searches. This patch adds a UseQueryParser syspref to do just that. In cases where the user enables QP without having QP installed, however, there will be no dramatic failure, and Koha will simply fall back to using the non-QP search code. In order to reduce the overhead added by the introduction of QueryParser, this patch adds a C4::Context->queryparser accessor to a static QueryParser object which is initialized the first time it is required and then reset thereafter. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Fixed merge conflict in sysprefs.sql and updatedatabase.sql Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixed merge conflict in sysprefs.sql und updatedatabase.pl Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16183 [details] [review] Bug 9239: Allow the use of QueryParser for all queries With the inclusion of this patch, all searches will (try) to use QueryParser for handling queries for both the bibliographic and authority databases if UseQueryParser is enabled. If QueryParser is unavailable, UseQueryParser is disabled, or the search uses CCL indexes, the old search code will be used. To test: 1) Apply patch. 2) Run the unit test with `prove t/QueryParser.t` 3) Enable the UseQueryParser syspref. 4) Try searches that should return results in the following places: * OPAC (simple search) * OPAC (advanced search) * OPAC (authorities) * Staff client (header search) * Staff client (advanced search) * Staff client (cataloging search) * Staff client (authorities) * Staff client (importing a batch using a match point) * Staff client (searching for an item for adding to a label) * Staff client (acquisitions) * Staff client (searching for a record to create a serial) * ANYWHERE ELSE I HAVE FORGOTTEN 5) Disable the UseQueryParser syspref. Repeat at least some of the searches you did above. 6) If all searches worked, sign off. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Searching still works as expected for variuos places. QueryParser syspref seemed to be enabled by default Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16184 [details] [review] Bug 9239 follow-up: POD and perlcritic fixes There were several perlcritic violations for returning explicit undef, and splitting up the various driver classes resulted in POD compliance errors, where the POD was starting with head2 rather than head1. This patch fixes those errors (but has no functional impact). Signed-off-by: Elliott Davis <elliott@bywatersolions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16185 [details] [review] Bug 9239 follow-up: update koha-conf.xml The previous patches were missing the koha-conf.xml updates. This patch updates koha-conf.xml and makes the changes neccessary to include the QueryParser configuration file in the packages. To test: 1) Run Makefile and check generated koha-conf.xml to confirm that the line <queryparser_config>...</queryparser_config> is there with an absolute path. 2) That was it. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tested update successfully - new line now shows up and query parser is used. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16186 [details] [review] Bug 9239 QA follow-up: fix highlighting and merge error QP searches with && broke search highlighting on the OPAC details page. This patch corrects encoding of the query_desc parameter that is passed to the details page. My last attempt at rebasing also transposed the variable for index names with the variable for operators, meaning that the dropdown in the basic search did not work. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixes some problems raised during QA successfully. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16187 [details] [review] Bug 9239 QA-follow-up: quiet warnings and fix problem with limits Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16188 [details] [review] Bug 9239 QA follow-up: escape CGI input Koha was not previously escaping CGI input, which caused problems for highlighting and is a security issue. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Thx for fixing this. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16189 [details] [review] Bug 9239 QA follow-up: the last QA follow-up was missing a require This patch also corrects the definition of the an= index, which was missing exactness. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16190 [details] [review] Bug 9239 QA follow-up: don't fall back to kw if field is specified Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16191 [details] [review] Bug 9239 QA follow-up: remove stray debug code Remove a line of debug code from EG, provide better error handling when presented with weird data in the authority linker, and correct queryparser configuration to reflect the correct configuration for Zebra. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16192 [details] [review] Bug 9239 QA follow-up: better error handling * Fix a long-standing bug in the linker that could crash the linker when run against odd data. * Sanitize input to SimpleSearch. * Correctly handle CCL indexes with QP. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16193 [details] [review] Bug 9239 QA follow-up: escape quotes in searches This patch escapes quotes in searches for those rare instances where there is a literal quote inside a search term. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16194 [details] [review] Bug 9239 QA follow-up: amend error handling Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Created attachment 16195 [details] [review] Bug 9239 QA follow-up: fix overeager quote escaping The quote escaping added two follow-ups ago was a little too eager, and escaped perfectly valid quotes in some instances. This patch moves the escaping deeper into the loop so that no needed quotes will be escaped. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Test plan included here, as this is the last patch in the series: All tests pass. OPAC Simple search - Keyword searches using boolean operators || and && - OK - Pubdate searches using pubdate() - OK - Using different search options: keyword, title, etc. - OK Facets - limit to available from a query parser simple search - OK - Other facets - OK Advanced search - Date range search - OK - Itemtype limits - OK - Language limit - OK - Combining various serach options with and without 'more' options and using boolean operators - OK Authorities - Searching for partial and complete names like "M", "Benedictus" works. - OK - Biblio count isc correct - OK - Links to linked biblios work. - OK. INTRANET Staged MARC import - Import records using various matching points - OK Serials - Search for a serial in subscription form - OK Acquisitions - Search for existing record from basket - OK Authorities - Running the linker script with QP on works. - OK - Searching for authorities works. - OK - Biblio count isc correct - OK - Links to linked biblios work. - OK. NOTE: As agreed with Jared in chat, query parser should be off by default for the next release. Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
This patch has been pushed to master.
If you are using a git install and do not run the installer, you will need to update koha-conf.xml and add the following line (modified to point to your git clone) before "</config>" near the bottom of the koha-conf.xml file: <queryparser_config>/home/jcamins/kohaclone/etc/searchengine/queryparser.yaml</queryparser_config>