Bug 19559

Summary: Elasticsearch QueryAutoTruncate truncate field names with hyphens if data is quoted
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ElasticsearchAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Julian Maurice <julian.maurice>
Severity: major    
Priority: P5 - low CC: david.bourgault, fridolin.somers, glasklas, jonathan.druart, julian.maurice, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18374    
Bug Blocks: 18131, 19580    
Attachments: Bug 19559 - Unit tests
Bug 19559 - Add '-' to list of characters we don't split search terms on
Bug 19559 - Unit tests
Bug 19559 - Add '-' to list of characters we don't split search terms on
Bug 19559 - Unit tests
Bug 19559 - Add '-' to list of characters we don't split search terms on

Description Nick Clemens 2017-11-01 15:46:47 UTC
With QueryAutoTruncate enabled a search for:
Local-number:"4"
is truncated as:
Local-* number:"4"
Comment 1 Nick Clemens 2017-11-01 15:51:06 UTC
Created attachment 68881 [details] [review]
Bug 19559 - Unit tests
Comment 2 Nick Clemens 2017-11-01 15:51:08 UTC
Created attachment 68882 [details] [review]
Bug 19559 - Add '-' to list of characters we don't split search terms on

Test plan assumes
QueryAutoTruncate = automatically
SearchEngine = Elasticsearch

To test:
0 - Apply Unit test patch only
1 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
2 - Should fail
3 - Apply this patch
4 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
5 - should pass
6 - search for 'Local-number:"4"' (or a vlid biblionumber)
7 - should get expected result
Comment 3 David Gustafsson 2017-11-02 15:01:25 UTC
*** Bug 19565 has been marked as a duplicate of this bug. ***
Comment 4 David Gustafsson 2017-11-02 15:03:26 UTC
Would be nice if "." could as be included as a valid field characters, since it is used for subfields in Elasticsearch. ("Control-number.raw" for example).
Comment 5 David Bourgault 2017-11-03 17:57:47 UTC
Created attachment 68924 [details] [review]
Bug 19559 - Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 6 David Bourgault 2017-11-03 17:57:49 UTC
Created attachment 68925 [details] [review]
Bug 19559 - Add '-' to list of characters we don't split search terms on

Test plan assumes
QueryAutoTruncate = automatically
SearchEngine = Elasticsearch

To test:
0 - Apply Unit test patch only
1 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
2 - Should fail
3 - Apply this patch
4 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
5 - should pass
6 - search for 'Local-number:"4"' (or a vlid biblionumber)
7 - should get expected result

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 7 David Bourgault 2017-11-03 17:58:21 UTC
Works as described in test plan.
Comment 8 Nick Clemens 2017-11-04 01:33:35 UTC
(In reply to David Gustafsson from comment #4)
> Would be nice if "." could as be included as a valid field characters, since
> it is used for subfields in Elasticsearch. ("Control-number.raw" for
> example).

This could be done, opening a new bug 19580

We may need more adjustments to when we do and do not truncate/split
Comment 9 Julian Maurice 2017-11-10 09:42:50 UTC
Created attachment 69075 [details] [review]
Bug 19559 - Unit tests

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Julian Maurice 2017-11-10 09:42:54 UTC
Created attachment 69076 [details] [review]
Bug 19559 - Add '-' to list of characters we don't split search terms on

Test plan assumes
QueryAutoTruncate = automatically
SearchEngine = Elasticsearch

To test:
0 - Apply Unit test patch only
1 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
2 - Should fail
3 - Apply this patch
4 - prove t/db_dependent/Koha_SearchEngine_Elasticsearch_Search.t
5 - should pass
6 - search for 'Local-number:"4"' (or a vlid biblionumber)
7 - should get expected result

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 11 Jonathan Druart 2017-11-26 16:26:33 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 12 Fridolin Somers 2018-01-30 07:55:51 UTC
Pushed to 17.05.x for v17.05.09