|
Description
Nick Clemens (kidclamp)
2021-11-04 18:00:37 UTC
Created attachment 127325 [details] [review] Bug 29418: Screen ending colons from search term Created attachment 127326 [details] [review] Bug 29418: Screen ending colons from search term This error message also seems to be appearing with ! and () and maybe addtional punctuation in the 245 field. And question marks (?) If you're not using analytics at all and want to just hide this error, you could put this in your intranetuserCSS:
/* Hide analytics error */
.analytics_error {
display: none;
}
*** Bug 30655 has been marked as a duplicate of this bug. *** Hmm, I started looking at adding unit tests for this and I see that there are a reasonable number for the same function in ElasticSearch.. wondering if we need to handle any other cases here that are already handled in the ES implementation..? Created attachment 134865 [details] [review] Bug 29418: Unit test Created attachment 134866 [details] [review] Bug 29418: Screen ending colons from search term Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> This improves things for me.. but I think it needs more eyes from someone more familiar with Zebra. Signing off with the Unit test I added for now to get more eyes. This goes some way towards resolving these errors: [WARN] CCL parsing error (10014) Unknown qualifier ZOOM for query: Host-item=( Utensili = ) at /home/koha/kohaclone/C4/Search.pm line 242. [2022/05/09 12:39:13] [WARN] Warning from simple_search_compat: 'CCL parsing error (10014) Unknown qualifier ZOOM' at /home/koha/kohaclone/Koha/Biblio.pm line 540. Looks good to me on itself. Would be helpful to include some pointers to recreate these warnings. My configuration does not show them at all. When should I expect a \: at the end of $a btw ? Normally, I see only a : (In reply to Marcel de Rooy from comment #12) > Looks good to me on itself. > Would be helpful to include some pointers to recreate these warnings. My > configuration does not show them at all. > > When should I expect a \: at the end of $a btw ? > Normally, I see only a : I think they only really appear if you are not using UseControlNumber... we tend to have it enabled so don't see it all that often. *** Bug 27575 has been marked as a duplicate of this bug. *** (In reply to Martin Renvoize from comment #11) > [WARN] CCL parsing error (10014) Unknown qualifier ZOOM for query: > Host-item=( Utensili = ) at /home/koha/kohaclone/C4/Search.pm line 242. > [2022/05/09 12:39:13] [WARN] Warning from simple_search_compat: 'CCL parsing > error (10014) Unknown qualifier ZOOM' at /home/koha/kohaclone/Koha/Biblio.pm > line 540. This warning actually reveals that we're going about this the wrong way. Take this code: $searchstr = "Host-item:($cleaned_title)"; We're just injecting an arbitrary string between those parentheses, and that's causing parsing errors. What we should be doing is this: $searchstr = qq#Host-item:("$cleaned_title")#; Observe the difference: Z> find Host-item=( Utensili = ) CCL ERROR: Unknown qualifier Z> find Host-item=(" Utensili = ") Sent searchRequest. Received SearchResponse. Search was a success. Number of hits: 0, setno 2 SearchResult-1: term=Utensili cnt=0 records returned: 0 Elapsed: 0.000349 In fact, an even better option would be this: $searchstr = qq#(Host-item:"$cleaned_title")#; This way, we're putting the key-value pair into its own little group. We can then add "and", "or", or whatever. (I really haven't gone down the rabbit hole of the purpose of $searchstr. (In reply to Martin Renvoize from comment #7) > Hmm, I started looking at adding unit tests for this and I see that there > are a reasonable number for the same function in ElasticSearch.. wondering > if we need to handle any other cases here that are already handled in the ES > implementation..? As Liz and Sheri have mentioned, there are other special characters in the CCL syntax. (See https://software.indexdata.com/yaz/doc/tools.html#ccl.syntax) However, so long as those are within double quotes, they should be treated as search data and not reserved characters. So I suppose clean_search_term() should really just escape double quotes. -- Note: We're in a fortunate position here of building the search string ourselves rather than having to parse user input, so we have so much control and ability to do this the way that works for the search engine. Ah, but as I look again at Koha/Biblio.pm, we have to factor in Elasticsearch. I don't know if Elasticsearch can handle double quotes around $cleaned_title. I suppose clean_search_term() could escape double quotes within 245$a using a backslash[1], plus it could return the clean_search_term wrapped in double quotes, so that Zebra processes it correctly. I'll leave that distinction up to someone more familiar with the Koha-ES integration... [1] Z> find (Any="shuffle "") CCL ERROR: ')' expected Z> find (Any="shuffle \"") Sent searchRequest. Received SearchResponse. Search was a success. Number of hits: 3, setno 28 SearchResult-1: term=shuffle cnt=3 records returned: 0 Elapsed: 0.000369 (In reply to Martin Renvoize from comment #10) > This improves things for me.. but I think it needs more eyes from someone > more familiar with Zebra. Thanks for adding me. Hopefully my comments were a bit helpful... Bumping into this again because of Bug 30865 but escaping a semicolon won't be enough. You'd have to escape question marks as well, since they're a special case which is only handled by "Special attribute combos" (see https://software.indexdata.com/yaz/doc/tools.html#ccl.qualifiers) There's % and ! as well as proximity operators. Also >=, <=, <>, >, and < as relational operators. In the end I think it's just easier to compose queries more carefully, and go the route I'm going with bug 30865. Please :-D Created attachment 137023 [details] [review] Bug 29418: Fix error searching for analytics in detail view This patch fixes the problem of searching for analytics when the title contains reserved characters like ? To test: 1. Have a record with ? in the title 2. Open the detail view (staff) => FAIL: tells there's an error checking for analytics 3. Open the detail view (OPAC) => FAIL: tells there's an error checking for analytics 4. Apply this patch and restart all 5. Repeat 2 and 3 => SUCCESS: Link shows correctly! 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I tried with Elasticsearch and Zebra on master, but the error was not shown, is this specific to 21.05? (In reply to Katrin Fischer from comment #23) > I tried with Elasticsearch and Zebra on master, but the error was not shown, > is this specific to 21.05? Yes. 21.11 got the Koha::Biblio->get_marc_components method with it's own bugfix. Created attachment 137048 [details] [review] Bug 29418: Fix error searching for analytics in detail view This patch fixes the problem of searching for analytics when the title contains reserved characters like ? To test: 1. Have a record with ? in the title 2. Open the detail view (staff) => FAIL: tells there's an error checking for analytics 3. Open the detail view (OPAC) => FAIL: tells there's an error checking for analytics 4. Apply this patch and restart all 5. Repeat 2 and 3 => SUCCESS: Link shows correctly! 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> This is now signed off by Katrin, but here are my testing notes for what they are worth!
I'm not very familiar with analytical records, and I wasn't sure whether the ? should be in the host record or the child/analytical record.
I tried combinations for the host record and the child/analytical record, and for both.
I haven't changed the bug status.
(Hopefully, I recorded my observations correctly!)
Testing notes using koha-testing-docker
=======================================
Starting 21.05.x instance:
a) For Koha repository, git checkout 21.05.x
b) Edit koha-testing-docker .env file and change IMAGE=master to IMAGE=21.05
Set up an analytical record:
a) Enable the EasyAnalyticalRecords system preference
b) For any record, select Edit > Link to host record - choose any other record (I tested with a record that had an item already attached to it)
c) For the record chosen as the host item, add a ? to the title
Used two records for testing:
- Host record: Programming Perl (biblionumber = 262)
- Child/analytical record: Perl best practices (biblionumber = 5)
Before the patch is applied
~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. If only the host record has a ? in the title:
- Viewing the host record in the staff interface - error displayed before the title: "There was an error searching for analytic records, please see the logs for details."
- Viewing the host record in the OPAC - no visible error message displayed (like in the staff interface), but there is an error message in the /var/log/koha/kohadev/plack-opac-error.log log file:
[2022/07/03 22:39:46] [WARN] CCL parsing error (10014) Embedded truncation not supported ZOOM for query: Host-item=(Programming ? Perl /) at /kohadevbox/koha/C4/Search.pm line 241.
[2022/07/03 22:39:46] [WARN] Warning from simple_search_compat: CCL parsing error (10014) Embedded truncation not supported ZOOM at /kohadevbox/koha/opac/opac-detail.pl line 208.
2. If only the child/analytical record has a ? in the title:
- Staff interface:
. Host record displays 'Analytics: Show analytics'
. Clicking on 'Show analytics' link displays the child/analytical record
. Error displayed before the title in the child/analytical record: "There was an error searching for analytic records, please see the logs for details."
- Viewing the host record in the OPAC
. no error displayed
. displays 'Analytics: Show analytics'
. clicking on show analytics: child/analytical record displayed with no error, but error in the logs as per 1
- Viewing the child/analytical record in the OPAC:
. no error displayed
. but error in the logs as per 1
3. Both the host and child/analytical record have a ? in the title:
. same as 1 for both the host and child/analytical record
After the patch is applied
~~~~~~~~~~~~~~~~~~~~~~~~~~
1. If only the host record has a ? in the title:
- Staff interface:
. Host record shows 'Analytics: Show analytics', no error messages
. Clicking on the 'Show analytics' link: No results match your search for 'Host-item:(Programming ? Perl )'.
. Error in the /var/log/koha/kohadev/plack-intranet-error.log log file:
[2022/07/03 23:40:01] [WARN] WARNING: query problem with Host-item=(Programming ? Perl ) ZOOM error 10014 "CCL parsing error" (addinfo: "Embedded truncation not supported") from diag-set 'ZOOM' at /kohadevbox/koha/C4/Search.pm line 349.
- OPAC:
. Host record shows 'Analytics: Show analytics'
. Error message in the /var/log/koha/kohadev/plack-opac-error.log log file:
[2022/07/03 23:41:25] [WARN] WARNING: query problem with Host-item=(Programming ? Perl ) ZOOM error 10014 "CCL parsing error" (addinfo: "Embedded truncation not supported") from diag-set 'ZOOM' at /kohadevbox/koha/C4/Search.pm line 349.
[2022/07/03 23:41:25] [WARN] Use of uninitialized value $times in subtraction (-) at /kohadevbox/koha/C4/Search.pm line 1709.
2. If only the child/analytical record has a ? in the title: works as expected, no errors in the log
3. Both the host and child/analytical record has a ?:
- Staff interface and OPAC:
. Host record shows 'Analytics: Show analytics'
. Clicking on the 'Show analytics' link: No results match your search for 'Host-item:(Programming ? Perl )'.
Hm, strange. The hos record having a ? case worked for me. Vut I had added the ? a the end of the string, maybe it could make a difference? (In reply to Katrin Fischer from comment #27) > Hm, strange. The hos record having a ? case worked for me. Vut I had added > the ? a the end of the string, maybe it could make a difference? I had added the ? in the middle of the title. (In reply to David Nind from comment #28) > (In reply to Katrin Fischer from comment #27) > > Hm, strange. The hos record having a ? case worked for me. Vut I had added > > the ? a the end of the string, maybe it could make a difference? > > I had added the ? in the middle of the title. Hi, the internal analytics search works correctly, but a follow-up is required for the XSLT to generate a good link. I overlooked it. Working on it now. Created attachment 137135 [details] [review] Bug 29418: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 137136 [details] [review] Bug 29418: Make clean_search_term escape double quotes We noticed that several characters will break Zebra queries. So search terms need to be quoted for things to work. In this context, double quotes inside search terms are problematic because double quotes are what we use for quoting strings. This patch makes the clean_search_term method escape double quotes. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t => FAIL: It doesn't work as it should! 3. Apply this patch 4. Repeat 2 => SUCCESS: It does the job! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 137137 [details] [review] Bug 29418: Make detail rendering use the cleaned-up title for searching This patch makes both staff and OPAC detail pages generate a valid search link so you can trace related records in the Show analytics link. To test: 1. Pick a bibliographic record (I chose 'Unconditional' from the sample data 2. Add " and ? to the title statement. I changed it to 'Uncond"itional?¿' 3. Add a child record to it 4. Open the detail page for the host record => FAIL: It doesn't show the 'Show analytics' link 5. Repeat 4 for the same record, in the OPAC => FAIL: It doesn't show the 'Show analytics' link 6. Apply this patch 7. Repeat 4 and 5. => SUCCESS: Links are shown! 8. Follow the links => SUCCESS: The links take you to the right resultset! 9. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 137138 [details] [review] Bug 29418: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 137139 [details] [review] Bug 29418: Make clean_search_term escape double quotes We noticed that several characters will break Zebra queries. So search terms need to be quoted for things to work. In this context, double quotes inside search terms are problematic because double quotes are what we use for quoting strings. This patch makes the clean_search_term method escape double quotes. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t => FAIL: It doesn't work as it should! 3. Apply this patch 4. Repeat 2 => SUCCESS: It does the job! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 137140 [details] [review] Bug 29418: Make detail rendering use the cleaned-up title for searching This patch makes both staff and OPAC detail pages generate a valid search link so you can trace related records in the Show analytics link. To test: 1. Pick a bibliographic record (I chose 'Unconditional' from the sample data 2. Add " and ? to the title statement. I changed it to 'Uncond"itional?¿' 3. Add a child record to it 4. Open the detail page for the host record => FAIL: It doesn't show the 'Show analytics' link 5. Repeat 4 for the same record, in the OPAC => FAIL: It doesn't show the 'Show analytics' link 6. Apply this patch 7. Repeat 4 and 5. => SUCCESS: Links are shown! 8. Follow the links => SUCCESS: The links take you to the right resultset! 9. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 137142 [details] [review] Bug 29418: (follow-up) Fix links generated in XSLTs This patch addresses the case of links that are generated on the XSLT side, those linking to host records using 773$t and 773$a. To test: 1. Repeat the test plan from the other patches 2. When sitting on the detail view of the host record, click on the link to open the child. => SUCCESS: The link works, because of the previous patches 3. Click on the 'In:' link so the search takes you to the host record => FAIL: The link leads no results 4. Apply this patch 5. Restart all 6. Reload the child record page and click on the link => SUCCESS: Voila! The host record is retrieved! 7. Bonus points: Retry the full test plan, but add a " or ? to the author in both host and child records => SUCCESS: Things don't work before this patch, and work afterwards. 8. Sign off :-D Sponsored-by: Theke Solutions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 137171 [details] [review] Bug 29418: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 137172 [details] [review] Bug 29418: Make clean_search_term escape double quotes We noticed that several characters will break Zebra queries. So search terms need to be quoted for things to work. In this context, double quotes inside search terms are problematic because double quotes are what we use for quoting strings. This patch makes the clean_search_term method escape double quotes. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/Koha/SearchEngine/Zebra/QueryBuilder.t => FAIL: It doesn't work as it should! 3. Apply this patch 4. Repeat 2 => SUCCESS: It does the job! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 137173 [details] [review] Bug 29418: Make detail rendering use the cleaned-up title for searching This patch makes both staff and OPAC detail pages generate a valid search link so you can trace related records in the Show analytics link. To test: 1. Pick a bibliographic record (I chose 'Unconditional' from the sample data 2. Add " and ? to the title statement. I changed it to 'Uncond"itional?¿' 3. Add a child record to it 4. Open the detail page for the host record => FAIL: It doesn't show the 'Show analytics' link 5. Repeat 4 for the same record, in the OPAC => FAIL: It doesn't show the 'Show analytics' link 6. Apply this patch 7. Repeat 4 and 5. => SUCCESS: Links are shown! 8. Follow the links => SUCCESS: The links take you to the right resultset! 9. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 137174 [details] [review] Bug 29418: (follow-up) Fix links generated in XSLTs This patch addresses the case of links that are generated on the XSLT side, those linking to host records using 773$t and 773$a. To test: 1. Repeat the test plan from the other patches 2. When sitting on the detail view of the host record, click on the link to open the child. => SUCCESS: The link works, because of the previous patches 3. Click on the 'In:' link so the search takes you to the host record => FAIL: The link leads no results 4. Apply this patch 5. Restart all 6. Reload the child record page and click on the link => SUCCESS: Voila! The host record is retrieved! 7. Bonus points: Retry the full test plan, but add a " or ? to the author in both host and child records => SUCCESS: Things don't work before this patch, and work afterwards. 8. Sign off :-D Sponsored-by: Theke Solutions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Nice work Tomas, this is a great solution for the 21.05 series. I think we need the follow-up in 21.11+ too however.. and the approach of passing through the analytics_query as a variable (although actually all we pass here is the escaped title) make sense to me for a master additional too.. DRY our code out by passing the constructed and escaped analytics query from our biblio method rather than re-constructing a version in the xslts. All in all.. PQA for this branch. (In reply to Martin Renvoize from comment #41) > Nice work Tomas, this is a great solution for the 21.05 series. > > I think we need the follow-up in 21.11+ too however.. and the approach of > passing through the analytics_query as a variable (although actually all we > pass here is the escaped title) make sense to me for a master additional > too.. DRY our code out by passing the constructed and escaped analytics > query from our biblio method rather than re-constructing a version in the > xslts. This problem is present in (at least) master, and it doesn't need just the follow-up. I'll file a bug for master to avoid mixing things. Backported: Pushed to 21.05.x branch for 21.05.17 (In reply to Victor Grousset/tuxayo from comment #43) > Backported: Pushed to 21.05.x branch for 21.05.17 Thanks! |