When the Elasticsearch search engine is used one cannot search for biblios with titles that contain square brakets ([ or ]). You will get an error if you try to search with the title of the biblio as it is written fully, e.g. "The dog [home edition]"
It also make fail detail.pl and opac-detail.pl. You can't print those pages when there are brackets in the title (internal server error: reason' => "Failed to parse query [(host-item:Vietnam, un [plus] un [\x{e9}gal] un)]"). You can escape brackets in the $cleaned_title variable like this: $cleaned_title =~ s/(?:\[|\])//g; and then the pages are printable. But that doesn't solve the search problem.
This is a decision we need to make. Do we want to automatically escape these characters, and then we cannot use range searches in Koha, or do we want to allow for more advanced searching, but ask the user to escape things?
Perhaps we could try to reduce the problem by first trying without escaping and then escaping if the first attempt failed? I admit I haven't really thought this through, but it could allow the flexibility while still making the searches work.
(In reply to Nick Clemens from comment #2) > This is a decision we need to make. Do we want to automatically escape these > characters, and then we cannot use range searches in Koha, or do we want to > allow for more advanced searching, but ask the user to escape things? What if we introduce config variable (so Koha can behave in the old way for those who don't use this or not changed, because of proper default value we will set), and even we can have that variable more advanced state "extra checkbox" under the search field, so: "useAdvancedSearchSyntax" config var with three states: 1. always (current Koha style) 2. never (i.e. all special characters auto escaped) 3. show checkbox "on/off advanced search syntax" just under the search field third option I mean: [___________________] [Submit] [x] disable advanced syntax (syntax help link) so we sill have some URL like this: /cgi-bin/koha/catalogue/search.pl?q=test+book&disable_adv_syntax=1 and adding this +1 parameter to search form this also allows to generate externals search links if we need ?
The issue is a lot wider than just square brackets, please see the list of reserved characters that need to be escaped if we are not using them as operators: https://www.elastic.co/guide/en/elasticsearch/reference/2.1/query-dsl-query-string-query.html#_reserved_characters
Created attachment 121435 [details] [review] Bug 28316: screen square and curly brackets in the search query This patch screens all square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). Previously, there was old time fix for curly brackets that replaced them with quotes which made curly brackets range functionality not working, that code was also removed with this patch Also this patch adds tests for this behavior. To reproduce: 1) with ES enabled, search for a book with tittle like "test [test]", which will result in error. 2) apply the patch. 3) search for that book again, which should work now. 4) run test QueryBuilder.t to make sure that it runs successfully.
Created attachment 121443 [details] [review] Bug 28316: screen square and curly brackets in the search query This patch screens all square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). Previously, there was old time fix for curly brackets that replaced them with quotes which made curly brackets range functionality not working, that code was also removed with this patch Also this patch adds tests for this behavior. To reproduce: 1) with ES enabled, search for a book with tittle like "test [test]", which will result in error. 2) apply the patch. 3) search for that book again, which should work now. 4) run test QueryBuilder.t to make sure that it runs successfully. Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
This is a great improvement, I have been playing with this and bug 28326. I found most things work, but QueryAutoTruncate does play some role here. Title of record: The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} With QueryAutoTruncate = only if * is added: The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} => No results Quote it: "The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega}" => Results! With QueryAutoTrunacte = automatically: The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} => Error: Unable to perform your search Quote it: "The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} " => Results I find that with this patch, and then adding quotes around the search in bug 28326: diff --git a/catalogue/detail.pl b/catalogue/detail.pl index 8d06f0da51..b11346162c 100755 --- a/catalogue/detail.pl +++ b/catalogue/detail.pl @@ -132,7 +132,7 @@ if ( $xslfile ) { my $query = ( C4::Context->preference('UseControlNumber') and $record->field('001') ) ? 'rcn:'. $record->field('001')->data . ' AND (bib-level:a OR bib-level:b)' - : "Host-item:$cleaned_title"; + : 'Host-item:"'.$cleaned_title.'"'; Most things work. I am pinging Victor as he has been finding ways to fail all of these, but I think we are on a good track
When testing this I think it would be good idea to verify if the behaviour with QueryRegexEscapeOptions set to other than "Escape" value still works as expected. Although I have come across many problems in the past already if QueryRegexEscapeOptions doesn't have the "Escape" option set so I'm not sure how it is actually even supposed to work with the other values.
If the scope here widens, please remember to update the bug description! (still in release notes mode here... )
With the patch of comment 8 and services restarted. Retried the case listed in the comment 8: > With QueryAutoTruncate = only if * is added: > The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} => No results I still have no result with the patch from comment 8 I still have to quote it. > With QueryAutoTrunacte = automatically: > The ideas behind the chess openings "fox quick"~5 tag:{alpha TO omega} => Error: Unable to perform your search Still error With QueryAutoTruncate=both possible values For a search of the following and a record named the following: name:/joh?n(ath[oa]n)/ => no result With QueryAutoTruncate=only if * For a search of the following and a record named the following: count:[10 TO *] => no result With QueryAutoTruncate=auto For a search of the following and a record named the following: count:[10 TO *] => Error: Unable to perform your search With QueryAutoTruncate=only if * For a search of the following and a record named the following: age:(+>=10 +<20) => no result With QueryAutoTruncate=auto For a search of the following and a record named the following: age:(+>=10 +<20) => no result With QueryAutoTruncate=only if * For a search of the following and a record named the following: quick^2 fox => Error: Unable to perform your search With QueryAutoTruncate=only if * For a search of the following and a record named the following: The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / => Error: Unable to perform your search With QueryAutoTruncate=auto For a search of the following and a record named the following: The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / => Error: Unable to perform your search With QueryAutoTruncate=auto For a search of the following (no record) [chess TO *] => Error: Unable to perform your search With QueryAutoTruncate=only if * For a search of the following (no record) [chess TO *] => whole catalogue Maybe it's expected With QueryAutoTruncate=auto For a search of the following (no record) Nmap network scanning : <> >< => Error: Unable to perform your search Not sure what is expected in the above results.
With QueryAutoTruncate=both possible values nmap onfocus=JaVaSCript:alert(9) autofocus => Error: Unable to perform your search With QueryAutoTruncate=both possible values nmap " onfocus=JaVaSCript:alert(10) autofocus => Error: Unable to perform your search
Created attachment 121561 [details] [review] Bug 28316: different QueryRegexEscapeOptions' values work as expected This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still work as expected. It does so by storing the contents of regexes before escaping square and curly brackets and then restores the contents of regexes back to how it was before. Also this patch covers this with tests.
Created attachment 121562 [details] [review] Bug 28316: different QueryRegexEscapeOptions' values work as expected This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still work as expected. It does so by storing the contents of regexes before escaping square and curly brackets and then restores the contents of regexes back to how it was before. Also this patch covers this with tests.
Changed the scope of the ticket, now this ticket is about various punctuation characters, like [], {}, : and !, that cause internal server errors when you use Elastic Search.
Created attachment 121751 [details] [review] Bug 28316: remove unquoted semicolons and screen all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that strips the colons from the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now.
Created attachment 121752 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. 4) additionally perform a search using range, for example: "[1999 TO 2020]", ensure that it works correctly too.
Created attachment 121753 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book.
Created attachment 121754 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 121755 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled"
Created attachment 121756 [details] [review] Bug 28316: add tests
*** Bug 28292 has been marked as a duplicate of this bug. ***
Created attachment 121837 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that screens the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now.
Created attachment 121838 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. 4) additionally perform a search using range, for example: "[1999 TO 2020]", ensure that it works correctly too.
Created attachment 121839 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book.
Created attachment 121840 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 121841 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled"
Created attachment 121842 [details] [review] Bug 28316: add tests
Created attachment 121975 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now.
Created attachment 121976 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. 4) additionally perform a search using range, for example: "[1999 TO 2020]", ensure that it works correctly too.
Created attachment 121977 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book.
Created attachment 121978 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 121979 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 121980 [details] [review] Bug 28316: add tests
Created attachment 122103 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now.
Created attachment 122104 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. 4) additionally perform a search using range, for example: "[1999 TO 2020]", ensure that it works correctly too.
Created attachment 122105 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book.
Created attachment 122106 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 122107 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 122108 [details] [review] Bug 28316: add tests
> 4) additionally perform a search using range, for example: "[1999 TO 2020]", ensure that it works correctly too. What values should be in the record for the search to yield it? It doesn't say anymore "You did not specify any search criteria." So it's better. But it says that there are not matches. I'm not sure ranges actually work. (because i don't know how to use them ^^") I tried «date-of-publication:[1950:2020]» Is that a correct query for a record with [1984] or 1984 in 260$c
errata: «date-of-publication:[1950 TO 2020]»
Created attachment 122201 [details] [review] Bug 28316: add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122202 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122203 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning, while keeping ranges as they are, allowing ES to search for biblios with titles that contain square and curly brackets and at the same time allows the use of range searches ([1990 TO 2000]). To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. 4) additionally perform a search using range, for example: "[1999 TO 2020]", ensure that it works correctly too. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122204 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122205 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 122206 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
The test plan yielded good results :) Patches were reordered to make it easier to check the tests (they indeed fail when the implementation is missing) Do these commits need a test plan? Bug 28316: avoid messing up regexes in the search queries Bug 28316: avoid messing up regexes in the search queries (they have the same name and description, suspicious ^^)
You right, Victor, I see also that description messed up and diverged after lot of reworks, let's Petro fix this, Petro, I assume patch 122205 explanation should be without "QueryRegexEscapeOptions" paragraph, and patch 122206 related to: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28316#c27 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28316#c45 so it should be about "range", not regex, about hiding ranges from escaping actually, I see most description from 122206 appears in 122203, and 122203 is just about "replacing brackets which not have special meaning" only.
I forgot I tested this in the past. What should be expected to be different from comment 11 results? - Should I still have to quote stuff (the cases of comment 8) - Should some stuff return results instead of none? - No crash I guess? Speaking of that: With QueryAutoTruncate=only if * For a search of the following and a record named the following: The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / => Error: Unable to perform your search With QueryAutoTruncate=auto For a search of the following and a record named the following: The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / => Error: Unable to perform your search
Created attachment 122801 [details] [review] Bug 28316: add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122802 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122803 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning. To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122804 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 122805 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible.
Created attachment 122806 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled" 1) enable QueryAutoTruncate at your sysprefs. 2) perform a search using range, for example: "[1999 TO 2020]", it shouldn't work the way it's supposed to. 3) apply the patch. 4) perform the same search with range, ensure that it works correctly.
(In reply to Victor Grousset/tuxayo from comment #51) > With QueryAutoTruncate=only if * > For a search of the following and a record named the following: > The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / > => Error: Unable to perform your search > > With QueryAutoTruncate=auto > For a search of the following and a record named the following: > The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / > => Error: Unable to perform your search Hey Victor, thanks for the response, I think search query like that should never work as it will always cause ES syntax error, ElasticSearch is a mini-language after all and has it's own syntax and it will die on something like that. Though we do need to notify the user, in cases like that, that the search query is incorrect and that they need to correct it and search with it again. I will make a ticket for it a bit later if.
Pushed the patches with fixed descriptions and removed some unnecessary newlines that were left there.
Victor, please check this ticket: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28736 it will make sense why search queries like this one "+ - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /" fail :)
(In reply to Victor Grousset/tuxayo from comment #51) Victor, please can you again give-this-a-go? i.e. Re-Sign-off this ticket again according to that so we now might have proper "error messages" for the wrong syntax in ES requests (bug 28736), if that comforts you?
Created attachment 123601 [details] [review] Bug 28316: add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 123602 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 123603 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning. To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 123604 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 123605 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 123606 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled" 1) enable QueryAutoTruncate at your sysprefs. 2) perform a search using range, for example: "[1999 TO 2020]", it shouldn't work the way it's supposed to. 3) apply the patch. 4) perform the same search with range, ensure that it works correctly. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Followed patch test plans (and run unit test t/Koha/SearchEngine/Elasticsearch/QueryBuilder.t) which work as described in devbox running Elasticsearch. So have signed off.
(In reply to Peter Vashchuk from comment #58) > I think search query like that should never work as it will always cause ES > syntax error, ElasticSearch is a mini-language after all and has it's own > syntax and it will die on something like that. Yes it makes sense. > Though we do need to notify the user, in cases like that, that the search > query is incorrect and that they need to correct it and search with it > again. I will make a ticket for it a bit later if. Thanks for bug 28736 :)
(In reply to Andrew Nugged from comment #61) > Victor, please can you again give-this-a-go? > i.e. Re-Sign-off this ticket again according to that so we now might have > proper "error messages" for the wrong syntax in ES requests (bug 28736), if > that comforts you? test result: my signoffs still hold on top of bug 28736 :) Last two patches still not tested. - I don't know how QueryRegexEscapeOptions works - comment 41 about the last patch But thanks to Alex we have these covered.
Generally about these complications and some mess with ES search: we don't have at all ES query language parser and transmitting requests directly to ES and getting its "parse_exception" reactions, so in Perl can't explain in which part of the syntax the issue, if Operator wants to have a direct search result, then he/she must escape special symbols for not to be "commands" for ES or use quoted texts to search exact terms. this is a "fuzzy" search though so sometimes wrong queries might lead to not having results even if there are such characters in the book name, a tradeoff for full-text search. We have only basic analysis which by the way was lost and not worked and fixed in Bug 28736, at least now the user will know that it's not "nothing found", but there are problems in special ES query syntax. How to use that syntax the user should read from ES language docs (or it we have some intro in Koha Doc for that?), but if the operator knows that there is ES syntax used and there are special characters and should use those properly: escape or quote. In other cases, it becomes a "weird" mix of words and special characters which leads to "unpredictable behavior" if you don't know how to use ES syntax. And also error message was mangled before, but now in Bug 28736 user knows that it's not "book not found", but the syntax and special language for ES used wrongly, so "please escape or quote". But this my answer actually to forward that 28736 brought some light, as first part covering "most things", which we not covering in this current ticket.
Now regarding the current ticket - why it's appeared: There already was some "prevention from dies (exceptions) in ES language" to make requests "just regular characters" instead of special commands, We already had some FUZY pre-processing before this current bugfix code in Koha/SearchEngine/Elasticsearch/QueryBuilder.pm, "sub _clean_search_term": some extra "precleaning", for example, curly braces "{}" any of them were replaced by '"' doublequote, and "=" replaced with ":", and colons with space before or space after (": " or " :") were just removed so only the space left, but trailing colon, trailing exclamation mark, and square range braces which actually not range (like in "[OHO]" phrase: ES dies because it's not [X TO Y] format) misled ES and anyway made ES die sometimes, So, that was incomplete and this ticket adds some more of such fuzziness but in the proper way (for example, ranges like "{... TO ...]" (i.e. full mix of ],},[,{ now works), But please don't demand from it to be "fully covering ES language" thing, in my opinion, this ticket decreases the number of "crashes" and "syntax errors" but at the same time, fixes range searches and started to recognize even "ES regular expressions" if they are enabled (yes, "QueryRegexEscapeOptions" case!), and fixes behavior with "QueryAutoTruncate" enabled, let me provide some examples below.
First, if you create" crazy request like (exactly this to the search field): The reserved characters are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ / it's impossible to do such fuzziness to guess what's wrong in such message in ES query, so we will just tell the user this: Error: There might be results, but there is ElasticSearch query language syntax error in your search request: remove or escape with backslash ES-language reserved characters (those are: + - = && || > < ! ( ) { } [ ] ^ " ~ * ? : / ) or change it to be the proper ES-language request and try again." (as from Bug 28736) But in the current ticket, we added some more clean fuzziness (let me describe difference between "current master" and what this current bugfix brings further
QueryAutoTruncate case: ========================= This is a feature that BLINDLY adds to alphanumeric-ended words asterisk in the end before tossing that phrase to ES as a search query. So, if QueryAutoTruncate is enabled (default), the Perl part just adds to the end of any alphanumeric-ended word asterisk, disregards ES syntax (ranges too), so if we will put such request phrase, the current master branch (Perl part) will transfer the below examples to ES as a query string (here I provide "what we entered" in the search field as 'IN:' and what transferred to ES as 'OUT:' pairs): (before the patch, phrase 1) IN: The [Book] OUT: The* [Book] (will make ES fail with a yellow error text box) in fact, ES doesn't understand your "special language usage" [Book] confuses ES because not in [... TO ...] form, and with ranges: (before the patch, phrase 2) IN: The "Book" {2000 TO 2002} [1900 TO 1990] OUT: The* "Book" "2000 TO 2002" [1900* TO* 1990] (will make ES fail with a yellow error text box) [1900* TO* 1990] confuses ES because not in [... TO ...] form, Here we see that it converted '{' and '}' to '"', and added '*' because of "QueryAutoTruncate enabled" to all alphanumeric-ended words. Second even more weird example: (before the patch, phrase 3) IN: The "Book" [2000 TO 2002} OUT: The* Book* [2000* TO* 2002* (will make ES fail with a yellow error text box) [2000* confuses ES because [ has no pair and no range there, And here quotes get lost because one "}" brace was converted to doublequote, but because it was only one, so was "unpaired", then the logic of the current code is to remove quotes at all if they are unbalanced (I agree, but it shouldn't convert braces to quotes). All requests above lead to a "yellow" Error text box, which means that ES fired an exception because of the error in query language syntax (and in Koha server error log there will be the error message ES 'Failed to parse query'). Anyway, as I see, this "QueryAutoTruncate" mode is designed for very simple usage behavior, i.e. user expected to do the requests without using any special language at the most, and give as much as possible results to the user assuming that there was "just beginnings of the words" requested if one enters: so bi this will be translated by Perl code to so* bi* and passed to ES so ES will match with for examples books named "Something Big" and "Solved Binary Book" This QueryAutoTruncate as well ruins the syntax of ES range requests (that '[1900 TO 1990]' becomes '[1900* TO* 1990]' in ES-language) so no reason to keep the ranges, and that accounted in the patch: Now with the patch: This patch is not intended to make the "full fix", it's not so easy for this, but at least the patch makes braces in QueryAutoTruncate mode just regular symbols by pre-slashing them. Same as above but WITH this patch will make such internal transformations, let's see by example: (after patch, phrase 1) IN: The "Book" {2000 TO 2002} [1900 TO 1990] OUT: The* \[Book\] ES understands that here we search 'the* book' and actually even will find 6 results in the current dev-test database, instead of "failing to the error" like in the example above without the patch, (after patch, phrase 2) IN: The "Book" {2000 TO 2002} [1900 TO 1990] OUT: The* "Book" \{2000* TO* 2002\} \[1900* TO* 1990\] ES understands that here we search 'The* "Book" 2000* TO* 2002 1900* TO* 1990' but it's a pretty rare phrase to find something, so - zero results, but no "ES exception", (after patch, phrase 3) IN: The "Book" [2000 TO 2002} OUT: The* "Book" \[2000* TO* 2002\} Same here as above, zero results but no "exceptions". I.e. those braces now become not-special-language characters and just passed by to ES to decide. In real life, those braces ignored by ES as non-alphanumeric, so querying: some [word] with QueryAutoTruncate enabled after the patch will be the same as search: some* word and BEFORE the patch it was just the "yellow" "error happened" text box result.
QueryRegexEscapeOptions ========================= Now let's turn "QueryAutoTruncate" off and check with QueryRegexEscapeOptions: It's all about RegEx in ES queries, https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-regexp-query.html Koha admin settings QueryRegexEscapeOptions has three options: - escape, i.e. if the request will have any regular expressions between slashes: /book/ this will be passed in "disabled, not RegEx anymore" to ES: \/book\/ so ES will get this just as regular text to search, and in this example will just search for the word 'book', ignoring other non-alphanum chars. (*)NOTE: but this means that passing RegEx to ES is impossible, at all. and this means any book name like in the search field will work: We are / They are so if we copy-paste this to the search field, ES gets: We are \/ They are and Koha just searches it as it is. I assume this was added as a feature to disable "/" meaning as a special RegEx limiter and allow librarians to search "We are / They are" text directly. - don't escape, so: /go+d/ will be passed as it is to ES, so this will be special "Regular Expressions" in this example, this means "'g' then 'o' one or more times then 'd'" so search in ES for /go+d/ example is similar to search: god OR good ( yet not the same because RegExp 'go+d' is not limited by a few 'o's, so might match for 'gooooood' too) BTW it founds 7 books for me in both ways (/go+d/ and god OR good) in the current master dev-test DB. (*)NOTE: but this means that now any '/' which appears in the text will cause ES to throw an exception if the syntax is not understood, i.e. search for exactly this: We are / They are now won't work so the users need to do escaping themself and ask: We are \/ They are (by backslashing '/' in the request field) to make ES not "die" and search to work. That's why I assume to make 1+2 both working, 3rd one was introduced: - unescape escaped, yeah, that's "The Trick" to make 1st way "We are / They are" requests to work and ES not to fail, but at the same time allowing to bring Regular Expressions by users, just in "pre-slashed form", i.e. what pre-slashed WILL BE RegExs, what isn't -- won't be but won't "throw a syntax error" too, so: We are / They are will be tossed to ES as "We are \/ They are" and will search for "We are They are", bit this (NOW THE TRICK! BEHOooooLD!): We are / They are \/go+\/ will be transferred to ES in such form: We are \/ They are /go+/ and ES will search for 'we', 'they', 'are' words, all to be present, plus one of the any: go, goo, gooo, goooo.. words ( this founds, btw, "Seesaw [sound recording] / Beth Hart, Joe Bonamassa" in current test DB :-), hey! ) This is how it is expected to work. But the reality in current master a little worse, Let's have "unescape escaped", let's invent such request: \/go{1,2}d\/ which EXPECTED by us to be transferred to ES as RegEx: /go{1,2}d/ what means we want ES to find for us: 'g', then one or max two 'o', and 'd' i.e. this is exactly ONLY for finding "good" or "god". But current master branch code breaks these expectations: replaces "{" and "}" with double quotes), so for real this transferred to ES as the request: /go"1,2"d/ and obviously, nothing was found. But with this patch, the request: \/go{1,2}d\/ will be passed to ES as: /go{1,2}d/ and voila, "7 result(s) found for '\/go{1,2}d\/'." in our test DB. Huh! More, because this patch respects {,} in regex but escapes those outside, it will pass those escaped, so if you will search: \/go{1,2}d\/ [go] this will pass to ES request WITHOUT patch: /go"1,2"d/ [go] and bring yellow "Error" text box, and this will be passed to ES request WITH patch: /go{1,2}d/ \[go\] and because "\[" and "\]" ignored by ES, it searches just for "good" or "god" plus the word "go", so the final searched text should contain "good to go" or "go god" combinations (or others like those). And yes, on test DB it founds 3 results! With patch. And gives "error/nothing" without the patch.
Ranges in ES language ========================= let's disable QueryAutoTruncate for not adding those asterisks, and QueryRegexEscapeOptions doesn't matter here unless we will add regular expressions. So: ES allows us to have ranges requests, like: novel {1947 TO 1950] this expects to find word "novel" and one of the words in the range between "1948 to 1950", and 1948 said here because '{' means "not included in range" value, but starting from it (i.e. not '>=', but '>'), but code in current master replaces '{' with '"', and then even more, unbalanced (unpaired) '"' removed as well, so the final request to ES will be: novel 1947 TO 1950] which has 'hanging ]' that ES doesn't understands and throws an exception (welcome to the 'yellow Error box' instead of 'found' result here again). Now, with the patch: novel {1947 TO 1950] transferred correctly as it is, and ES on test DB finds single: The vixens. A novel. Publication details: London, 1948. for obvious reasons. Now to play another example "without and with patch", here: [novel] [1900 TO 1950] without the patch, it transferred "as is" to ES, and gives a yellow error, because ES doesn't understands range as in '[novel]'. With patch it becomes transformed to such request to ES: \[novel\] [1900 TO 1950] and because backslashed non-alphanumeric characters are ignored, it works and brings the result - one book, like in the previous example.
Tricks with colons: ====================== colons in Koha expected to separate search field and search value, so "title:good" should search word "good" only in "titles", but if you will use "some : like this" form, when copypasting from usual combined book names we often have, current code removed colons at all from the text, if colon has the space at any side, but there was forgotten trailing ":" removal, i.e. ES failed without patch if to search: Books: also, some fuzziness added, for example, if you rename any book to have "23:59" in the name, then will search with an old code for: title:23:59 it will die ("yellow error box"), but with the patch, it will find, as this transformed by the patch to: title:23\:59 i.e. there every next semicolon after first in the term preslashed.
And the exclamation mark: ====================== this used to be the "negate" command for words in queries, ONLY when '!' sticking to the beginning of the word, so patch fuzziness added: non-sticky and trailing exclamations marks removed, so in the case of the search: Dalek! without the patch, this brought a yellow error box, but with the patch dangling "!" was removed and it just passes "Dalek" to ES. It finds two books in our test DB, btw. And this one: Dalek !War works normally with both, with and without the patch. It searches for "Dalek" but negates "war" word presence. In our test dev-DB by the way, it gives just one book as a result.
(In reply to Victor Grousset/tuxayo from comment #41) > > 4) additionally perform a search using range, Vicor, this seems working now, By the way, it should find normally this with and without patches for you: date-of-publication:[1983 TO 1985] But before the patch, if you will use "not including border values" ranges: date-of-publication:{1983 TO 1985} and then because not-patched code converts it to this: date-of-publication:"1983 TO 1985" it even did not fail to "yellow error box", just said "no results", but there is: with patch now it finds 4 books for me on test DB for {1983 TO 1985}. Besides you for sure can find when ES crashes, :hugs: but without the patch, you will have much more possibilities to get that ES syntax error now. And I covered come in/outs and experiments above
So, the patch adds more order with "QueryRegexEscapeOptions" than before, same with "QueryAutoTruncate", and keeps ranges working as well. Yet it loooo+ng read above :), for a strong coffee and murky cat nearby ^_^
Why this fuzziness and the extra job was done by us? We wanted to limit syntax errors from ES and make it easier for librarians to copy-paste just book names and have results, but NOT TO FORCE THEM backslash or re-edit text "for as long as possible", so for example earlier, they pasted "normal human phrases", like each of these: The Book! The Book [directed by robert b weide] The Book: and that lead earlier for ES to crash and bring "yellow Error box", even worse: before Bug 28736 people were confused that "oh, nothing found!" but for real there not even searched because of syntax error. Now, these above examples working and finding results (except the middle one because no such book in test DB yet :) ).
Created attachment 124372 [details] [review] Bug 28316: add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124373 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124374 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning. To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124375 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124376 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124377 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled" 1) enable QueryAutoTruncate at your sysprefs. 2) perform a search using range, for example: "[1999 TO 2020]", it shouldn't work the way it's supposed to. 3) apply the patch. 4) perform the same search with range, ensure that it works correctly. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124378 [details] [review] Bug 28316: (QA follow-up) Make clean_search_term public With all the work that's gone into improving the internal _clean_search_term method I feel we should expose it publically as it's going to be more widely helpful Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Wow, this is a pretty rigorous treatment.. I'm impressed. I've tested to the extent of my knowledge and am really happy with the results. No regressions found, but I'd love to see it pushed and tested with various configurations in the wild with quick fixes to follow. Passing QA.
*** Bug 28326 has been marked as a duplicate of this bug. ***
+ $term =~ s/(?<!\\)((?:[\\]{2})*)(\{|\[)([^\s\[\]\{\}]+ TO [^\s\[\]\{\}]+(?<!\\)(?:[\\]{2})*)(\}|\])/$1.'~~L'.($2 eq '[' ? 'S':'C').'~~'.$3.'~~R'.($4 eq ']' ? 'S':'C').'~~'/ge; + $term =~ s@( + (?<!\\)(?:[\\]{2})*/ + (?:[^/]+|(?<=\\)(?:[\\]{2})*/)+ + (?<!\\)(?:[\\]{2})*/ + )$lookahead@"~~XI$rgx_i~~"@ex I can push this kind of stuffs if everybody agrees, but I must warn that we are opening a sensible door. If we find regressions that will be a mess to maintain, or even to enhance. Questions and suggestions: 1. Could we use named capture in the regexs? 2. What means XI? 3. Why do we need the 'e' modifier in the second one? 4. Can we split on several lines the regexs to make them more readable? (/xms)
1: yes, naming can be added 2: this is a "placeholder" which: a) first: part of text removed and replaced by placeholders (~~XI1~~ / ~~LC1~~ for example and others, life "LC for LeftCurly" and so on). This is for temporary hiding out current brackets from "next RegExs" b) then text processed expecting that all others curly/square brackets and regex and what must be kept – removed and hidden, so we can clean up and be brave with all other not-replaced-by-placeholders brackets, c) so we cleaning up text, screening, removing or whatever those special characters, d) and now we replacing BACK those placeholders with their saved in step a) values back to keep unscreened and working special characters we wanted to be kept 3: because it has " .. ? .. : .." depending of what bracket was found, but otherwise if you speaking security-wise and don't want to have "e"'s at all this RE probably can be split to few "just with different brackets" each, should Petro split this regex to two without "e"? We discussed this too, I also pointed that this is less secure "if something changes", but in this case, I don't see how this can be hacked (how?) 4: for sure, this is good point. And some more comments in code. Let's do this.
> This is for temporary hiding out current brackets from "next RegExs" I agree that this is not the best solution: with 0.00..1% probability that some books can have in the name "~~LC1~~" and this then will make code work unexpectedly for that book ... but then we need to have ES to allow to index "~~..~~" things, ... :). I also pointed this to Petro but we thought together that then the best solution is to scan text for placeholders and change placeholder (i.e. "generate placeholder" to be unique) but oh – that then will make code so complicated... we will create our own MIME then :)... If think from perfectionism POV this not solved yet, but other way – ... )... eh?
(In reply to Andrew Nugged from comment #92) > 1: yes, naming can be added > > 2: this is a "placeholder" which: Yes, I got that, and L for Left and R for Right. But then XI for... what? :) > 3: because it has " .. ? .. : .." depending of what bracket was found No, the second regex does not have it. > , but > otherwise if you speaking security-wise and don't want to have "e"'s at all > this RE probably can be split to few "just with different brackets" each, > should Petro split this regex to two without "e"? We discussed this too, I > also pointed that this is less secure "if something changes", but in this > case, I don't see how this can be hacked (how?) Looks ok for now. But if it can be avoided I should do it. > 4: for sure, this is good point. And some more comments in code. Let's do > this. Thx!
Created attachment 124560 [details] [review] Bug 28316: add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124561 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124562 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning. To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124563 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124564 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124565 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled" 1) enable QueryAutoTruncate at your sysprefs. 2) perform a search using range, for example: "[1999 TO 2020]", it shouldn't work the way it's supposed to. 3) apply the patch. 4) perform the same search with range, ensure that it works correctly. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124566 [details] [review] Bug 28316: (QA follow-up) Make clean_search_term public With all the work that's gone into improving the internal _clean_search_term method I feel we should expose it publically as it's going to be more widely helpful Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Jonathan Druart from comment #91) > Questions and suggestions: > 1. Could we use named capture in the regexs? > 2. What means XI? > 3. Why do we need the 'e' modifier in the second one? > 4. Can we split on several lines the regexs to make them more readable? > (/xms) I implemented those changes and pushed the changed version of the patches, please take a look :)
For future reference, once a bug has gone through SO and QA please respond to change requests as follow-ups... that way it's easy for a reviewer to see that the requested change has been made and nothing else beyond it.. with the changed squashed in it's much more difficult.. especially if one has cleaned up their previous branches.
Created attachment 124709 [details] [review] Bug 28316: add tests Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124710 [details] [review] Bug 28316: screen unquoted semicolons and all followup colons Currently searches like: "book:", ":book" and "host-item:test:n" cause internal server errors. This patch adds additional regexes that remove the colons at the start and end of the query, and another regex that screens all follow-up colons that go after the first colon to avoid errors when searching for "host-item:test:n". To reproduce: 1) using ES, search for the book with title that contains semicolon at the start or at the end of the line, separated with spaces, this should cause internal server error. 2) try doing the same with something like "host-item:test:n", it should result in error as well. 3) apply the patch. 4) repeat steps 1-2, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124711 [details] [review] Bug 28316: escape brackets in the search query This patch screens square and curly brackets which have no special language meaning. To reproduce: 1) using ES, search for the book with title that contains square and/or curly brackets, like "book [second edition]", which will result in error. 2) apply the patch. 3) search for that book again, ensure that it works now. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124712 [details] [review] Bug 28316: escape exclamation signs in the query Currently having exclamation sign at the end of the query makes ES search fail, and when you try to search for a book that has exclamation sign in the tittle (something like "Words! words") won't show results correctly as it tries to negate everything that is after exclamation sign, making it impossible to search for books that have in in the title This patch escapes exclamation signs if it's at the end of the query or has a space after it, resolving both of the issues listed above. To reproduce: 1) with ES enabled, search for the book with title that contains exclamation sight at the end, like "book!", this search should result in error. 2) do another search, but this time find/prepare beforehand book with a title that has exclamation sign with a space after it, e.g "exclamation! sign", it shouldn't find it as ES treats everything after that exclamation sign as negation. 2) apply the patch. 3) perform searches from the steep one and two again. Search from step one should no longer fail, while search from the step two should find that book. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124713 [details] [review] Bug 28316: avoid messing up regexes in the search queries This patch ensures that the behavior with QueryRegexEscapeOptions set to values other than "Escape" still will works as expected. It does so by storing the contents of regexes before escaping special characters and then restores the contents of regexes back to how it was before, ensuring that searching with regex is possible. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124714 [details] [review] Bug 28316: escape ES ranges if QueryAutoTruncate is enabled if QueryAutoTruncate enabled we will have any special operators ruined for example: "test [6 TO 7]" will be converted to "test* [6* TO* 7]" so no reason to keep ranges when QueryAutoTruncate set to "enabled" 1) enable QueryAutoTruncate at your sysprefs. 2) perform a search using range, for example: "[1999 TO 2020]", it shouldn't work the way it's supposed to. 3) apply the patch. 4) perform the same search with range, ensure that it works correctly. Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 124715 [details] [review] Bug 28316: (QA follow-up) Make clean_search_term public With all the work that's gone into improving the internal _clean_search_term method I feel we should expose it publically as it's going to be more widely helpful Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Phew.. I had an old branch hanging around I could diff with. Passing QA again, looks like everything has been taken care of.
:huggs: yeah, right, Petro, look, we should next time add follow-up (thanks Martin!)
(In reply to Andrew Nugged from comment #112) > :huggs: > > yeah, right, Petro, look, we should next time add follow-up (thanks Martin!) No worries; if there's a mistake to be made like this I've probably made it at least a few times already myself ;)
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Can we get a backport for this for 20.11 pretty please?
Yep, I was waiting for Bug 29284. Will be in 20.11.12 release.
Any chance of having this for 20.11 now that 29284 is in master and 21.11 and 21.05?
Backported: Pushed to 20.11.x branch for 20.11.14