Bug 20334 - Elasticsearch - Option for escaping slashes in search queries
Summary: Elasticsearch - Option for escaping slashes in search queries
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: David Gustafsson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-05 12:49 UTC by David Gustafsson
Modified: 2019-02-19 07:10 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20334 - Option for escaping slashes in search queries (12.78 KB, patch)
2018-03-05 13:10 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334 - Option for escaping slashes in search queries (12.78 KB, patch)
2018-03-05 13:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334 - Option for escaping slashes in search queries (10.45 KB, patch)
2018-03-06 14:01 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334 - Option for escaping slashes in search queries (10.48 KB, patch)
2018-03-14 09:02 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 20334 - Option for escaping slashes in search queries (10.53 KB, patch)
2018-03-23 20:01 UTC, Maksim Sen
Details | Diff | Splinter Review
Bug 20334: Improve syspref description and set more sensible defaults (2.78 KB, patch)
2018-10-22 13:51 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334: Add entry in sysprefs.sql (1.34 KB, patch)
2018-10-29 14:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334: Move new system preferene into right spot in alphabet (2.13 KB, patch)
2018-10-29 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20334: Add entry in sysprefs.sql (1.25 KB, patch)
2018-10-30 08:47 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334 - Option for escaping slashes in search queries (10.51 KB, patch)
2019-02-14 16:41 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334: Improve syspref description and set more sensible defaults (2.78 KB, patch)
2019-02-14 16:41 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20334: Move new system preferene into right spot in alphabet (1.28 KB, patch)
2019-02-14 16:41 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2018-03-05 12:49:20 UTC
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).
Comment 1 David Gustafsson 2018-03-05 13:10:38 UTC
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
Comment 2 David Gustafsson 2018-03-05 13:20:52 UTC
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
Comment 3 Josef Moravec 2018-03-05 16:32:25 UTC
David, is this ready for testing? If so, please change status to "Needs signoff"
Comment 4 David Gustafsson 2018-03-06 10:25:34 UTC
Ok! Yes, ready to signoff.
Comment 5 David Gustafsson 2018-03-06 10:29:50 UTC
Just saw that a swap file was accidentally committed, setting failed until this is fixed (later today).
Comment 6 David Gustafsson 2018-03-06 14:01:15 UTC
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
Comment 7 Nicolas Legrand 2018-03-14 09:02:28 UTC
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>
Comment 8 Maksim Sen 2018-03-23 20:01:44 UTC
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>
Comment 9 Katrin Fischer 2018-04-22 10:53:53 UTC
Hi David, please remember to assign bugs to yourself when you work on them and provide patches!
Comment 10 David Gustafsson 2018-04-25 19:15:14 UTC
Ok, done :)
Comment 11 Katrin Fischer 2018-07-08 12:50:36 UTC
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?
Comment 12 David Gustafsson 2018-10-22 13:45:58 UTC
(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.
Comment 13 David Gustafsson 2018-10-22 13:51:43 UTC
Created attachment 80978 [details] [review]
Bug 20334: Improve syspref description and set more sensible defaults
Comment 14 David Gustafsson 2018-10-22 16:47:40 UTC
@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.
Comment 15 Katrin Fischer 2018-10-27 21:54:11 UTC
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.
Comment 16 David Gustafsson 2018-10-29 14:49:39 UTC
Created attachment 81517 [details] [review]
Bug 20334: Add entry in sysprefs.sql
Comment 17 David Gustafsson 2018-10-29 14:52:09 UTC
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.
Comment 18 Katrin Fischer 2018-10-29 21:00:59 UTC
The file is sorted alphabetically to avoid conflicts ;)
Comment 19 Katrin Fischer 2018-10-29 22:18:06 UTC
Created attachment 81597 [details] [review]
Bug 20334: Move new system preferene into right spot in alphabet
Comment 20 Katrin Fischer 2018-10-29 22:19:26 UTC
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.
Comment 21 David Gustafsson 2018-10-30 08:47:17 UTC
Created attachment 81613 [details] [review]
Bug 20334: Add entry in sysprefs.sql
Comment 22 David Gustafsson 2018-10-30 08:52:47 UTC
Ops, didn't realize this was already fix. Will remove the latest patch. :)
Comment 23 Katrin Fischer 2019-02-14 07:31:57 UTC
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.
Comment 24 David Gustafsson 2019-02-14 16:41:38 UTC
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>
Comment 25 David Gustafsson 2019-02-14 16:41:41 UTC
Created attachment 85130 [details] [review]
Bug 20334: Improve syspref description and set more sensible defaults
Comment 26 David Gustafsson 2019-02-14 16:41:49 UTC
Created attachment 85131 [details] [review]
Bug 20334: Move new system preferene into right spot in alphabet
Comment 27 David Gustafsson 2019-02-14 16:42:18 UTC
Rebased against master.
Comment 28 Katrin Fischer 2019-02-14 20:00:18 UTC
Thx David! Please reset the status back to signed off next time :)
Comment 29 Ere Maijala 2019-02-15 07:16:07 UTC
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.
Comment 30 Katrin Fischer 2019-02-15 07:17:30 UTC
Hi Ere, thx for your input. What you suggest makes sense for me. Marking FQA to get feedback from David on the suggestion.
Comment 31 David Gustafsson 2019-02-15 13:42:16 UTC
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.
Comment 32 Ere Maijala 2019-02-15 13:47:41 UTC
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.
Comment 33 David Gustafsson 2019-02-15 15:20:11 UTC
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.
Comment 34 Ere Maijala 2019-02-18 07:58:26 UTC
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.
Comment 35 David Gustafsson 2019-02-18 15:16:57 UTC
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.
Comment 36 Ere Maijala 2019-02-19 07:10:18 UTC
I'm ok with disabling regexp altogether for the time being.