Bug 20334 - Elasticsearch - Option for escaping slashes in search queries
Summary: Elasticsearch - Option for escaping slashes in search queries
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-05 12:49 UTC by David Gustafsson
Modified: 2018-10-30 08:54 UTC (History)
2 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

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. :)