There are a number of reserved characters in the Elasticsearch query syntax (+ - = && || > < ! ( ) { } [ ] ^ " ~ * ? : \ /). Slash (/), which is used for regexps, is quite often used in searches with the intent as it should be interpreted as a literal sting. This causes the search to crash since Elasticsearch treats this as an unclosed regular expression (or a regexp if two slashes are used which can be even more confusing). To address this particular case here is a patch with an option to escape slashes (or require them to be escaped to count as a regular expression delimiter).
Created attachment 72434 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed
Created attachment 72435 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed
David, is this ready for testing? If so, please change status to "Needs signoff"
Ok! Yes, ready to signoff.
Just saw that a swap file was accidentally committed, setting failed until this is fixed (later today).
Created attachment 72471 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed
Created attachment 72830 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>
Created attachment 73222 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Hi David, please remember to assign bugs to yourself when you work on them and provide patches!
Ok, done :)
Hi David, 1) Can you please add an entry for the new system preference in sysprefs.sql? 2) I wonder, is this something where picking a sensible default behaviour could make sense? Honestly I am more than a little confused by the different options, but I can confirm that searching for something containing / won't work right now, which is a problem. For example, it will break when someone copies a MARC record for search. Example: Dutch : an essential grammar / William Z. Shetter and Inge Van der Cruysse-Van Antwerpen 3) The system preference doesn't mention that it applies to Elasticsearch only - I think we need to add this information. 4) My sample search: Dutch / William doesn't work with any of the settings. What am I missing?
(In reply to Katrin Fischer from comment #11) > Hi David, > > 1) Can you please add an entry for the new system preference in sysprefs.sql? I added an entry in the atomicupdate directory, has there been a change so it should be added in sysprefs.sql instead? > 2) I wonder, is this something where picking a sensible default behaviour > could make sense? Honestly I am more than a little confused by the different > options, but I can confirm that searching for something containing / won't > work right now, which is a problem. For example, it will break when someone > copies a MARC record for search. Example: > > Dutch : an essential grammar / William Z. Shetter and Inge Van der > Cruysse-Van Antwerpen I changed the default to "Escape", from "Don't escape" since that's probably what most people want (think very few actually uses regexps in search strings). I also expanded the explanation on the system-preferences page. > 3) The system preference doesn't mention that it applies to Elasticsearch > only - I think we need to add this information. I added this info on the system-preferences page. > 4) My sample search: Dutch / William doesn't work with any of the settings. > What am I missing? I think the reason might be that if you included the ":" it will be interpreted as a field query (this could be the subject for another bug). Try just: an essential grammar / William Z. Shetter, for example.
Created attachment 80978 [details] [review] Bug 20334: Improve syspref description and set more sensible defaults
@Katrin Fischer: And to clarify, right now the default (Escape) is to disable regular expressions in searches (all "/", which is used as regexp delimiter in Elasticsearch, will be escaped). The "Escape unescaped" will allow regexps, but instead of "/test(ing)?/" you have to write "\/test(ing)?\/" which is harder to do by mistake.
Hi David, >I added an entry in the atomicupdate directory, has there been a change so it >should be added in sysprefs.sql instead? No change, you need both: the atomicupdate is for updating existing installations. The sysprefs.sql is used to create the needed entries in new installations. See: https://wiki.koha-community.org/wiki/Database_updates Thx for the fixes and explanations! Please do the sysprefs.sql as another follow-up and switch back to signed-off instead of needs sign-off as the changes have been trivial.
Created attachment 81517 [details] [review] Bug 20334: Add entry in sysprefs.sql
Ok! I must have missed that for my previous patches. I hope the release manager or someone in the QU team has added the entry in those cases.
The file is sorted alphabetically to avoid conflicts ;)
Created attachment 81597 [details] [review] Bug 20334: Move new system preferene into right spot in alphabet
Couldn't resist, being a librarian and all. Moving back to "Signed off" as last 2 follow-ups have been trivial and this was already signed-off before.
Created attachment 81613 [details] [review] Bug 20334: Add entry in sysprefs.sql
Ops, didn't realize this was already fix. Will remove the latest patch. :)
Patch doesn't apply right now, conflict looks small. Could we get a rebase please? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20334 - Option for escaping slashes in search queries Using index info to reconstruct a base tree... M Koha/SearchEngine/Elasticsearch/QueryBuilder.pm M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref Auto-merging Koha/SearchEngine/Elasticsearch/QueryBuilder.pm CONFLICT (content): Merge conflict in Koha/SearchEngine/Elasticsearch/QueryBuilder.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 20334 - Option for escaping slashes in search queries The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20334---Option-for-escaping-slashes-in-search--Co2Ks4.patch Also adding Ere here.
Created attachment 85129 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Created attachment 85130 [details] [review] Bug 20334: Improve syspref description and set more sensible defaults
Created attachment 85131 [details] [review] Bug 20334: Move new system preferene into right spot in alphabet
Rebased against master.
Thx David! Please reset the status back to signed off next time :)
I don't really like the fact that a pref may affect how the search terms are interpreted. I'd rather go without the pref and try to do the right automatically: if the term looks like a regexp (i.e. start and ends with a slash), don't escape it so that regexp can be used. Otherwise escape the slashes. Could also additionally validate the regexp to make sure it works. I think this would eliminate the need to for the pref while keeping the versatility and the regular searches working properly.
Hi Ere, thx for your input. What you suggest makes sense for me. Marking FQA to get feedback from David on the suggestion.
Hi! I see two major problems with this approach. Firstly, I don't think librarians generally expect, for example, the search terms "Directive 95/46/EC" to be interpreted as containing a regular expression. I would guess that the fraction of librarians who would ever use regular expressions in searches can be rounded down to zero :) I would probably never use it myself. The second problem is that it's probably really nasty to implement as a regular expression, perhaps not even possible, that is: escaping of unescaped slashes if the slash is odd and not in a quoted context. Would probably have to write some kind of parser. Since I think that strings within slashes are generally not meant to be regexps by those who generally use the search interface I went for the two options either to disable them completely, or be allowed, but using a syntax that one would probably not enter accidentally.
That wouldn't be considered a regexp search since the search term doesn't start and end with a slash. We already split the query to terms, so adding a per-term check is not that complicated.
Yes, it would be much easier to work on a tokenized search string with regards to quotes etc. But do we really do this? We do it in _truncate_terms, but that's only if auto-truncation is enabled. It would be nice if there was some intermediate query format that could be used instead of the raw search string though, but that seems outside the scope of this patch.
It'd be fine for me to change _truncate_terms to something more general. That'd be a good place to do any other clean up, escaping or query terms transformations.
Yes, that change would perhaps be pretty trivial. Still not completely obvious how to try to "autocorrect" the regexps though. The regexps in lucene are always anchored according to https://stackoverflow.com/questions/34312196/elastic-search-query-string-regex That might be the reason why I got strange results when trying out regexps on the format "foo/bar/", which produced no results, while /foobar/ did. I also assume that the regexp is run on the tokenized values in ElasticSearch, with possible stemming, n-grams etc applied (even though I don't think we do any of these at the moment), so it's not very obvious for the user what values are actually matched against. Regexp against a ".raw" subfield behaves more like you would expect, matching against the exact value of the mapped data. I's because of theses issue I think it might be better to disable regexps altogether, possibly with a an option for enabling a less accessible syntax for expert users who know what they are doing. Slashes embedded in word characters are also interpreted as regexp by Elastic since the string "foo/bar" also results in an error. So I also believe it to be a real problem that librarians will input such strings with matching and/or not matching delimiters and get unexpected results.
I'm ok with disabling regexp altogether for the time being.
Is anything happening with this bug?
Is there something we can do to help get this moving again?
David, are you able to work on this? I'd really like to get this in 19.11, so let my know if you need a hand here.
Created attachment 93478 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed
Hi! I have not made any changes to the patch, but I had a more recent rebase of this branch locally that had not submitted, So patch should at least apply now.
Ere and David, are more changes needed?
Looks like this should do it!
The sandbox you've requested is not ready. Some problems occurred applying patches from bug 20334: <h1>Something went wrong !</h1>Applying: Bug 20334: Improve syspref description and set more sensible defaults fatal: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/bug_20334-add-syspref-QueryRegexEscapeOptions.sql). error: could not build fake ancestor Patch failed at 0001 Bug 20334: Improve syspref description and set more sensible defaults
Realized screwed up when attaching the last patch (using an older version, so mismatch between commits etc). Will restore and rebase again.
Created attachment 93584 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Created attachment 93585 [details] [review] Bug 20334: Improve syspref description and set more sensible defaults
Created attachment 93586 [details] [review] Bug 20334: Move new system preferene into right spot in alphabet
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93664 [details] [review] Bug 20334 - Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93665 [details] [review] Bug 20334: Improve syspref description and set more sensible defaults Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93666 [details] [review] Bug 20334: Move new system preferene into right spot in alphabet Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93697 [details] [review] Bug 20334: Option for escaping slashes in search queries Add "QueryRegexEscapeOption" system preference to provide option to escape Elasticsearch regexp delimiters (/) within queries, or alternativly to unescape escaped slashes (\/) while escaping unescaped slashes, in effect making "\/" the new regexp delimiter. How to test: 1) Run tests in ./t/Koha/SearchEngine/ElasticSearch/QueryBuilder.t 2) All tests should succeed Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr> Signed-off-by: Maksim Sen <maksim.sen@inlibro.com> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 93699 [details] [review] Bug 20334: Improve syspref description and set more sensible defaults Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 93700 [details] [review] Bug 20334: Move new system preference into right spot in alphabet Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
This works well, I don't pretend to fully understand the regex,but it does what it says. Possibly the options could be named to be a little more clear. If I understand correctly: Escape - this essentially disables regex searching since all the slashes get escaped and you cannot escape them yourself Don't Escape - does nothing, regex searching works, but slashes must be escaped for normal searching Unescape escaped - enables regex if you escape the regex slashes So maybe? Disable regex searching Enable regex searching (users must escape slashes) Enable regex searching for advanced users I think it is fine as is but needs release notes/documentation
Nice work! Pushed to master for 19.11.00