Bug 19575 - Use canonical field names and resolve aliased fields
Summary: Use canonical field names and resolve aliased fields
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: David Gustafsson
QA Contact: Ere Maijala
URL:
Keywords:
: 19485 21793 22229 (view as bug list)
Depends on:
Blocks: 20390 22495 22524
  Show dependency treegraph
 
Reported: 2017-11-03 12:05 UTC by David Gustafsson
Modified: 2020-01-06 20:14 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patchset makes some changes to Elasticsearch mappings in the database. The changes alter existing indices and are intended to fix issues with the current mappings, however, if you have done customization of mappings you may want to back them up before upgrading.
Version(s) released in:


Attachments
Bug 19575: Use canonical field names and resolve aliased fields (13.29 KB, patch)
2017-11-03 12:28 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (14.89 KB, patch)
2017-11-14 15:12 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (16.00 KB, patch)
2017-11-15 13:43 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (81.66 KB, patch)
2017-11-23 14:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (81.60 KB, patch)
2017-11-23 16:23 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (79.62 KB, patch)
2017-12-04 12:56 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (79.78 KB, patch)
2017-12-04 13:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (79.78 KB, patch)
2017-12-05 15:06 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (91.10 KB, patch)
2018-01-15 17:29 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (91.10 KB, patch)
2018-01-15 18:01 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (90.33 KB, patch)
2018-01-16 15:36 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (92.19 KB, patch)
2018-01-31 16:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (92.49 KB, patch)
2018-02-01 14:46 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (92.23 KB, patch)
2018-02-19 16:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (92.25 KB, patch)
2018-03-12 11:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (93.41 KB, patch)
2018-05-07 12:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (93.84 KB, patch)
2018-10-26 14:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (93.85 KB, patch)
2018-10-26 14:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (93.90 KB, patch)
2018-10-26 14:55 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (93.99 KB, patch)
2018-10-26 15:01 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (85.17 KB, patch)
2018-11-30 14:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (85.19 KB, patch)
2019-01-08 08:42 UTC, Ere Maijala
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (85.17 KB, patch)
2019-01-08 12:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (84.86 KB, patch)
2019-02-12 21:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19575: Rebase and fix tests (3.76 KB, patch)
2019-02-12 21:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19575: Rebase and fix tests (3.84 KB, patch)
2019-02-13 12:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19575: Fix an search field (900 bytes, patch)
2019-02-14 12:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19575: Keep authority-number as alias and fix query (3.05 KB, patch)
2019-02-14 15:55 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 19575: Use canonical field names and resolve aliased fields (84.83 KB, patch)
2019-02-14 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19575: Rebase and fix tests (3.90 KB, patch)
2019-02-14 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19575: Fix an search field (965 bytes, patch)
2019-02-14 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19575: Keep authority-number as alias and fix query (3.09 KB, patch)
2019-02-14 16:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19575: (RM follow-up) Fix mapping typo (822 bytes, patch)
2019-02-15 14:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19575: Don't lower case fields in db and use update script instead of raw SQL (8.11 KB, patch)
2019-02-18 13:58 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 2017-11-03 12:05:58 UTC
The current elastic search fields seems to be a mix between zebra aliases, field names, some duplicates and new fields. This causes issues with sorting in UI and hard-coded search-links in XSL-templates for example. I have attempted to clean things up a bit, which resolves many of these issues. I might not have the time to do so today, but will attempt to find and mark all current issues resolved by this fix.
Comment 1 David Gustafsson 2017-11-03 12:28:33 UTC
Created attachment 68914 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 2 David Gustafsson 2017-11-07 13:11:14 UTC
The following issue should be resolved with this patch applied:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19485

But in addition to title-series, most of the other broken search links should also work.
Comment 3 David Gustafsson 2017-11-10 13:47:11 UTC
Think I have missed a few places (facets for example) where field names are hard coded and needs to be changes. So no point in trying out patch in it's current state until these things are fixed.
Comment 4 David Gustafsson 2017-11-14 15:12:32 UTC
Created attachment 69140 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 5 David Gustafsson 2017-11-15 13:43:52 UTC
Created attachment 69153 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 6 David Gustafsson 2017-11-15 13:45:15 UTC
Now most (hopefully all) issues (primarily with facets) should be sorted out.
Comment 7 David Gustafsson 2017-11-16 15:45:12 UTC
Think I have been looking at the Zebra-mapping configuration in the wrong place (this old config, not biblio-koha-indexdefs.xml). I will go through the mapping changes one more time using this as reference instead.
Comment 8 David Bourgault 2017-11-17 19:25:50 UTC
*** Bug 19485 has been marked as a duplicate of this bug. ***
Comment 9 David Gustafsson 2017-11-23 14:04:19 UTC
Created attachment 69306 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 10 David Gustafsson 2017-11-23 14:08:17 UTC
Wrote a script to extract mappings from biblio-koha-indexdefs.xml and convert to mappings.yaml, left the fields with existing mapping-targets as they are (even though discovered several discrepancies with the Zebra mappings) and mostly just added the missing fields. Will now do more thorough local testing so will probably adjust this patch some more before ready to be reviewed.
Comment 11 David Gustafsson 2017-11-23 16:23:18 UTC
Created attachment 69314 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 12 Jonathan Druart 2017-11-27 17:40:42 UTC
Is this ready for signoff?
Comment 13 David Gustafsson 2017-11-27 20:45:55 UTC
Sorry, should have been more clear about that. Yes, this is ready for signoff. There is still some stuff I left out of this patch, like:

- New mappings for other marc-flavours than marc21 extracted from biblio-koha-indexdefs.xml
- Correction for some existing mappings (I think some offsets are off by one for example)
- New mappings and posssible corrections for authorities

But if this get reviewed and committed I can open new bug-reports for those once I have time to further polish the extraction-script.
Comment 14 Jonathan Druart 2017-11-27 21:58:08 UTC
(In reply to David Gustafsson from comment #13)
> But if this get reviewed and committed I can open new bug-reports for those
> once I have time to further polish the extraction-script.

Can you share the script?
Comment 15 David Gustafsson 2017-11-27 22:47:02 UTC
Yes, I was thinking to do so, though it is only a quick and dirty script in Ruby (should perhaps be re-written in perl if to be included in Koha-repo), and though would clean it up just a little bit more, remove some hard-coded stuff, add some simple command line options etc. But will post a link to git-repo with said script once removed the worst ugliness.
Comment 17 David Bourgault 2017-12-01 19:51:48 UTC
Patch fails to apply; conflict on file mappings.yaml.
Comment 18 David Gustafsson 2017-12-04 12:56:34 UTC
Created attachment 69479 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 19 David Gustafsson 2017-12-04 13:18:06 UTC
Ok! Rebased against master. There was a commit in between setting the "sort" for serveral fields, those changes should now be merged in.
Comment 20 David Gustafsson 2017-12-04 13:22:45 UTC
Reviewing the commit-diff once again, I think I made an error in the merge. Will upload again.
Comment 21 David Gustafsson 2017-12-04 13:52:04 UTC
Created attachment 69480 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 22 David Gustafsson 2017-12-04 13:53:49 UTC
Think git auto-merged some stuff incorrectly, but this should now be sorted out.
Comment 23 David Gustafsson 2017-12-05 15:06:17 UTC
Created attachment 69507 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates, and
sorting of search results in UI.
Comment 24 David Gustafsson 2017-12-05 15:09:10 UTC
Field name 'Date/time-last-modified' does not work with elastic since "/" will create subfields, koha will also not tokanize this correctly, changed this (back) to 'Date-time-last-modified'.
Comment 25 Jonathan Druart 2017-12-19 16:06:21 UTC
(In reply to David Gustafsson from comment #22)
> Think git auto-merged some stuff incorrectly, but this should now be sorted
> out.

So, ready to be signed off?
Comment 26 David Gustafsson 2017-12-19 16:09:39 UTC
Yes should be ready for sign off, have discovered no more issues since the most recent patch.
Comment 27 Jonathan Druart 2017-12-19 16:32:30 UTC
(In reply to David Gustafsson from comment #26)
> Yes should be ready for sign off, have discovered no more issues since the
> most recent patch.

The good way to tell other people that your patch is ready to be tested is to change the status of the bug report from "Patch doesn't apply" to "Needs Signoff"
Comment 28 David Gustafsson 2017-12-19 16:34:14 UTC
Aha, I looked for a status drop down but manage to miss it since it wasn't a drop down. :) Will fix.
Comment 29 David Gustafsson 2017-12-19 16:35:52 UTC
And, now I saw there was a dropdown, but below the comment area.
Comment 30 David Gustafsson 2017-12-29 15:00:18 UTC
Just a caveat, just discovered that some field names hard coded in xsl-templates have names that differer in case from mappings.yaml. (control-number for example). Zebra has case-insensitive field names, but this will cause problems with elasticsearch. So this patch, while correcting some links, might also break some that worked before. I'm planning on writing a fix to make Elasticseach field names non case sensitive, but until that issue is resolved this patch might causee issues.
Comment 31 David Gustafsson 2018-01-15 17:29:34 UTC
Created attachment 70494 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library
Comment 32 David Gustafsson 2018-01-15 17:51:19 UTC
Case insensitive field names in searches is now fixed. A few changes I made could use some extra motivation/explanation:

# sub _convert_index_strings_freeform() (line 685):
changed from:
sub _convert_index_strings_freeform {
    my ( $self, $search ) = @_;
    while ( my ( $zeb, $es ) = each %index_field_convert ) {
        $search =~ s/\b$zeb(?:,[\w\-]*)?:/$es:/g;
    }
    return $search;
}
to:
my $field_name_pattern = '[\w\-]+';
my $multi_field_pattern = "(?:\\.$field_name_pattern)*";
...
sub _convert_index_strings_freeform {
    my ( $self, $search ) = @_;
    $search =~ s/($field_name_pattern)(?:,[\w-]*)?($multi_field_pattern):/\L$1\E$2:/og;
    $search =~ s/($field_name_pattern)($multi_field_pattern):/(exists $index_field_convert{$1} ? $index_field_convert{$1} : $1)."$2:"/oge;
    return $search;
}
(Excluding comments)
With new regexps for lower-casing field names etc there is a lot of duplication for things like field names, possible multiple/subfields etc, so created some variables for components that are often reused to prevent some pitfalls in possible future refactoring. I'm not sure it really matters performance wise, but also replaced the alias replacement code with a more efficient code since the %index_field_convert hash is now much larger than before.

Also, the regexp in sub _truncate_terms() has been changed to also use $field_name_pattern and $multi_field_pattern.
Comment 33 David Gustafsson 2018-01-15 18:01:30 UTC
Created attachment 70495 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library
Comment 34 David Gustafsson 2018-01-15 18:02:14 UTC
Found some more places where hard coded fields needed to be changed to lower-case.
Comment 35 David Gustafsson 2018-01-16 15:21:08 UTC
Here is a github-diff that might be easier on the eyes: https://github.com/ub-digit/Koha/commit/8be850d2cdc93e5d121afa19c5eed1e1cc683f1c
Comment 36 David Gustafsson 2018-01-16 15:36:57 UTC
Created attachment 70558 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library
Comment 37 David Gustafsson 2018-01-16 15:38:19 UTC
Found one (hopefully) last place that needed case-adjustment. New diff: https://github.com/ub-digit/Koha/commit/a6fda51b59c65c2459d460fd374377b56b5f10cc
Comment 38 David Gustafsson 2018-01-31 16:24:00 UTC
Created attachment 71090 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library
Comment 39 David Gustafsson 2018-01-31 16:24:22 UTC
Found one more hard coded field value in authorities search that needed lower-casing.
Comment 40 David Gustafsson 2018-02-01 14:46:00 UTC
Created attachment 71112 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library
Comment 41 David Gustafsson 2018-02-01 14:47:15 UTC
Fixed hard coded field name in Koha/SearchEngine/ElasticSearch/Search.pm ('au' to 'authority-number')
Comment 42 David Gustafsson 2018-02-01 14:47:44 UTC
*('an' to 'authority-number')
Comment 43 David Gustafsson 2018-02-19 16:49:10 UTC
Created attachment 71966 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library
Comment 44 David Gustafsson 2018-02-19 16:51:57 UTC
Removed incorrect alias/field name conversions (ctype => "content-type"), and some aliases with identical source and target values.
Comment 45 Nick Clemens 2018-03-12 11:45:16 UTC
Created attachment 72673 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 46 David Gustafsson 2018-05-07 12:19:11 UTC
Created attachment 75115 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 47 David Gustafsson 2018-05-07 12:19:48 UTC
Rebased against master.
Comment 48 Julian Maurice 2018-06-05 09:37:58 UTC
Sorry David, but the patch does not apply on master. Small conflict on mappings.yaml. Could you rebase it ?
Also, could you provide a test plan ?
Comment 49 David Gustafsson 2018-10-22 12:12:02 UTC
Will fix this. Test plan is a litte bit tricky since this corrects a lot of small stuff. But will try to backtrack to list most of what is fixed.
Comment 50 David Gustafsson 2018-10-26 14:42:39 UTC
Created attachment 81297 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

To test:
1) Go to a biblio detail page
2) Click a link under "Publisher" or "Provider"
3) This link should yield no search results
4) Search for "ISBN:x" where x is a valid isbn.
5) The search should produce no results.
6) Apply patch
7) Go to a biblio detail page and click "Publisher" or "Provider" again
8) The link should no produce results
9) Search for "ISBN:x" again
10) The search should now produce results

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 51 David Gustafsson 2018-10-26 14:44:50 UTC
Created attachment 81299 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

To test:
1) Go to a biblio detail page
2) Click a link under "Publisher" or "Provider"
3) This link should yield no search results
4) Search for "ISBN:x" where x is a valid isbn.
5) The search should yield no results.
6) Apply patch
7) Go to a biblio detail page and click "Publisher" or "Provider" again
8) The link should now yield search results
9) Search for "ISBN:x" again
10) The search should now yield search results

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 52 David Gustafsson 2018-10-26 14:55:26 UTC
Created attachment 81303 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

To test:
1) Go to a biblio detail page
2) Click a link under "Publisher" or "Provider"
3) This link should yield no search results
4) Search for "ISBN:x" where x is a valid isbn.
5) The search should yield no results.
6) Apply patch
7) Reset mappings
8) Perform a full reindexing
9) Go to a biblio detail page and click "Publisher" or "Provider" again
10) The link should now yield search results
11) Search for "ISBN:x" again
12) The search should now yield search results

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 53 David Gustafsson 2018-10-26 15:01:08 UTC
Created attachment 81304 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

To test:
1) Go to a biblio detail page
2) Click a link under "Publisher" or "Provider"
3) This link should yield no search results
4) Search for "ISBN:x" where x is a valid isbn.
5) The search should yield no results.
6) Apply patch
7) Reset mappings
8) Perform a full reindexing
9) Go to a biblio detail page and click "Publisher" or "Provider" again
10) The link should now yield search results
11) Search for "ISBN:x" again
12) The search should now yield search results
13) Also try searching for "isbn:x" and "Isbn:x"
14) This should also work and yield search results

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Katrin Fischer 2018-10-27 13:15:59 UTC
Patch applies, resetting status.
Comment 55 David Gustafsson 2018-11-30 14:24:15 UTC
Created attachment 82794 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 David Gustafsson 2018-11-30 14:24:31 UTC
Rebase against master.
Comment 57 Martin Renvoize 2019-01-07 18:25:12 UTC
This all looks sane to me.. but I'm going to raise it to our resident ES expert and ask for his SO as a PQA.. calling Ere
Comment 58 Ere Maijala 2019-01-08 08:42:40 UTC
Created attachment 83704 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 59 Ere Maijala 2019-01-08 08:43:36 UTC
Awesome, thanks David. This was actually on my list of things to work on when time permits, and it had been bugging me.
Comment 60 Ere Maijala 2019-01-08 08:46:43 UTC
*** Bug 21793 has been marked as a duplicate of this bug. ***
Comment 61 Martin Renvoize 2019-01-08 12:58:50 UTC
Created attachment 83710 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Martin Renvoize 2019-01-08 12:59:49 UTC
Fixed a single whitespace error that was introduced.

Passing QA
Comment 63 Nick Clemens 2019-01-08 15:49:28 UTC
(In reply to Martin Renvoize from comment #62)
> Fixed a single whitespace error that was introduced.
> 
> Passing QA

Tests are failing with these patches:

prove -v t/db_dependent/SearchEnginge/ElasticSearch
Comment 64 Ere Maijala 2019-01-28 13:02:28 UTC
David, will you be able to address the test issues?
Comment 65 David Gustafsson 2019-02-01 13:04:53 UTC
Unfortunately I have have been a little bit too busy lately to have a look at this, but hope will have the time to do so some time next week.
Comment 66 Nick Clemens 2019-02-12 21:54:52 UTC
Created attachment 85029 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 67 Nick Clemens 2019-02-12 21:54:57 UTC Comment hidden (obsolete)
Comment 68 Nick Clemens 2019-02-12 21:55:36 UTC
Rebased and updated, I think in spirit of original set - set to Signed off, would like another QA run and Ere's okay
Comment 69 Nick Clemens 2019-02-12 21:57:16 UTC
Extra note: I found that indexes like 'se' and 'an' were not searchable - no results were returned from faceting or searchign like "se:pengiun" changing to title-series (or even 'ser' in testing) seemed to work
Comment 70 Katrin Fischer 2019-02-12 21:58:43 UTC
(In reply to Nick Clemens from comment #69)
> Extra note: I found that indexes like 'se' and 'an' were not searchable - no
> results were returned from faceting or searchign like "se:pengiun" changing
> to title-series (or even 'ser' in testing) seemed to work

an is used in the XSLT templates, sounds like an issue?
Comment 71 David Gustafsson 2019-02-13 12:33:31 UTC
(In reply to Nick Clemens from comment #67)
> Created attachment 85030 [details] [review] [review]
> Bug 19575: Rebase and fix tests
> 
> This patch lower cases the sort by fields to normalize checking them and
> adjusts
> some existing tests to meet the new expectations.
> 
> The regex for splitting terms has been moved into a subroutine so that
> adjustment was made

Thanks for adjusting the tests. The regexp was put outside of the subroutine on purpose though, to avoid recompilation on each call.
Comment 72 Nick Clemens 2019-02-13 12:53:19 UTC
Created attachment 85062 [details] [review]
Bug 19575: Rebase and fix tests

This patch lower cases the sort by fields to normalize checking them and adjusts
some existing tests to meet the new expectations.

The regex for splitting terms has been moved into a subroutine so that adjustment was made
Comment 73 Nick Clemens 2019-02-13 13:15:51 UTC Comment hidden (obsolete)
Comment 74 Nick Clemens 2019-02-13 13:16:28 UTC
(In reply to David Gustafsson from comment #71)
> (In reply to Nick Clemens from comment #67)
> > Created attachment 85030 [details] [review] [review] [review]
> > Bug 19575: Rebase and fix tests
> > 
> > This patch lower cases the sort by fields to normalize checking them and
> > adjusts
> > some existing tests to meet the new expectations.
> > 
> > The regex for splitting terms has been moved into a subroutine so that
> > adjustment was made
> 
> Thanks for adjusting the tests. The regexp was put outside of the subroutine
> on purpose though, to avoid recompilation on each call.

Moved back outside, thanks
Comment 75 David Gustafsson 2019-02-13 14:31:36 UTC
(In reply to Nick Clemens from comment #69)
> Extra note: I found that indexes like 'se' and 'an' were not searchable - no
> results were returned from faceting or searchign like "se:pengiun" changing
> to title-series (or even 'ser' in testing) seemed to work

About this issue and the "an" alias, this does not work with this patch applied? The alias seems to exist in the %index_field_convert map, and is expanded as far as I can tell. But could look into this if not working. I got a little bit confused since bug 20388 says "'se' is fixed in bug 19575". :)
Comment 76 David Gustafsson 2019-02-13 14:33:14 UTC
Oops, bug 22325 I mean, perhaps you linked to the wrong bug number in the previous comment?
Comment 77 David Gustafsson 2019-02-13 14:40:34 UTC
Could the cause be that the value of an: is unquoted in clause where UseAuthoritiesForTracings is set in the excerpt below (koha-tmpl/intranet-tmpl/prog/en/xslt/NORMARCslim2intranetResults.xsl line 308):

<xsl:choose>
<xsl:when test="marc:datafield[@tag=100] or marc:datafield[@tag=110] or marc:datafield[@tag=111] or marc:datafield[@tag=700] or marc:datafield[@tag=710] or marc:datafield[@tag=711]">
    <p class="author">av
    <xsl:for-each select="marc:datafield[(@tag=100 or @tag=700) and @ind1!='z']">
    <a>
    <xsl:choose>
        <xsl:when test="marc:subfield[@code=9] and $UseAuthoritiesForTracings='1'">
            <xsl:attribute name="href">/cgi-bin/koha/catalogue/search.pl?q=an:<xsl:value-of select="str:encode-uri(marc:subfield[@code=9], true())"/></xsl:attribute>
        </xsl:when>
        <xsl:otherwise>
        <xsl:attribute name="href">/cgi-bin/koha/catalogue/search.pl?q=au:"<xsl:value-of select="str:encode-uri(marc:subfield[@code='a'], true())"/>"</xsl:attribute>
        </xsl:otherwise>
    </xsl:choose>
Comment 78 David Gustafsson 2019-02-13 14:54:45 UTC
(And in many other places). Should perhaps not matter though if "an" never contains whitespace.
Comment 79 Nick Clemens 2019-02-14 12:44:37 UTC
Created attachment 85105 [details] [review]
Bug 19575: Fix an search field
Comment 80 Nick Clemens 2019-02-14 12:45:48 UTC
(In reply to David Gustafsson from comment #75)
> (In reply to Nick Clemens from comment #69)
> > Extra note: I found that indexes like 'se' and 'an' were not searchable - no
> > results were returned from faceting or searchign like "se:pengiun" changing
> > to title-series (or even 'ser' in testing) seemed to work
> 
> About this issue and the "an" alias, this does not work with this patch
> applied? The alias seems to exist in the %index_field_convert map, and is
> expanded as far as I can tell. But could look into this if not working. I
> got a little bit confused since bug 20388 says "'se' is fixed in bug 19575".
> :)

Aha, it turns out an should work, but an was mapped to authority-number and koha-auth-number was mapped to authority-number - but koha-auth-number was the field name :-)  Patch attached
Comment 81 David Gustafsson 2019-02-14 15:55:16 UTC
Created attachment 85124 [details] [review]
Bug 19575: Keep authority-number as alias and fix query

Keep authority-number as alias and change field name
from alias to real field in hard coded Elasticsearch query
Comment 82 David Gustafsson 2019-02-14 15:57:47 UTC
Ok! Since it was some time ago I don't really remember how to parse the Zebra field definitions anymore, but the authority-number entry looked a little bit strange and I might have made some a mistake regarding the format. I re-added authority-number as an alias, probably not needed but doesn't hurt. Also found an instance of authority-number being used in a hard coded Elasticsearch query.
Comment 83 Martin Renvoize 2019-02-14 16:25:58 UTC
Created attachment 85125 [details] [review]
Bug 19575: Use canonical field names and resolve aliased fields

Adjust elastic search mappings to more closely match Zebra equivalents
resolving serveral issues with coded Zebra searches in templates and
sorting of search results in UI. Also make field names in search strings
case insensitive to accept case variations in template links and user input.

Sponsored-by: Gothenburg University Library

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 84 Martin Renvoize 2019-02-14 16:26:03 UTC
Created attachment 85126 [details] [review]
Bug 19575: Rebase and fix tests

This patch lower cases the sort by fields to normalize checking them and adjusts
some existing tests to meet the new expectations.

The regex for splitting terms has been moved into a subroutine so that adjustment was made

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 85 Martin Renvoize 2019-02-14 16:26:07 UTC
Created attachment 85127 [details] [review]
Bug 19575: Fix an search field

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 86 Martin Renvoize 2019-02-14 16:26:11 UTC
Created attachment 85128 [details] [review]
Bug 19575: Keep authority-number as alias and fix query

Keep authority-number as alias and change field name
from alias to real field in hard coded Elasticsearch query

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 87 Nick Clemens 2019-02-14 16:59:05 UTC
*** Bug 22229 has been marked as a duplicate of this bug. ***
Comment 88 Nick Clemens 2019-02-15 14:51:15 UTC
Created attachment 85178 [details] [review]
Bug 19575: (RM follow-up) Fix mapping typo

Found with QA tools, simple fix
Comment 89 Nick Clemens 2019-02-15 18:57:51 UTC
I looked at this today, but the update statement is a little confusing.

Should we update a library's mappings in updates? or simply require that they reset mappings when upgrading?

I am okay with updating, but it also seems some of the lines are unnecessary with the generic statement of lower casing at the top?

Can you review and format the update using skeleton.perl? Or let me know if I missed something?
Comment 90 David Gustafsson 2019-02-18 13:40:47 UTC
Yes, the lowercase update statement is pretty pointless since the mixed case field names will be restored on reset. Field names are lower-cased in _foreach_mapping, so the case of field names in mappings.yaml (or database) does not really matter. If lower casing all field names the corresponding field names in mappings.yaml should also be lower cased. The field name renaming is probably still a good idea, so that a reset is not required.
Comment 91 David Gustafsson 2019-02-18 13:58:14 UTC
Created attachment 85247 [details] [review]
Bug 19575: Don't lower case fields in db and use update script instead of raw SQL
Comment 92 Nick Clemens 2019-02-19 11:21:15 UTC
(In reply to David Gustafsson from comment #91)
> Created attachment 85247 [details] [review] [review]
> Bug 19575: Don't lower case fields in db and use update script instead of
> raw SQL

Hi David,

Thanks for that update, I still see some lines I wonder about, the ones like:
dbh->do( "UPDATE `search_field` SET `name` = 'isbn', `label` = 'isbn' WHERE `name` = 'isbn'" );

I don't see any change here - isbn is being renamed to isbn, there are a fair amount of lines like this in the update.

The other ones like se being renamed title-series do make sense

It seems we could trim this down a fair amount
Comment 93 Nick Clemens 2019-03-04 18:25:37 UTC
(In reply to Nick Clemens from comment #92)
Talked with Ere, he verified these make sense
Comment 94 Nick Clemens 2019-03-04 18:49:44 UTC
Awesome work all!

Pushed to master for 19.05
Comment 95 Martin Renvoize 2019-03-05 14:59:05 UTC
Pushed to 18.11.x for 18.11.04

 I had a brief discussion regarding this one in IRC and we reached the consensus that we are both earlier enough in the cycle and the lifetime of the Elasticsearch use that it was a sensible one to backport even though it has further reaching effects than we would normally permit in a stable branch release.
Comment 96 Josef Moravec 2019-03-08 08:40:31 UTC
Why was su-geo facet field removed by this?
Comment 97 David Gustafsson 2019-03-08 10:27:37 UTC
That might have been a mistake. The mappings where generated from this script from Zebra mapping definitions: https://github.com/ub-digit/koha-indexdefs-to-mappings-yaml/blob/master/mappings.rb but su-geo is in the source mappings as far as I can see and should not have been removed. I also manually rebased mappings.yaml a couple of times after upstream changes, don't know if made some mistake there.

Since I seems to have commented out the alias ('su-geo' => 'subject') in the patch, it seams like I expected the field to exist, so I probably did not remove it intentionally and it should be re-added.
Comment 98 David Gustafsson 2019-03-08 11:17:58 UTC
I had a look at the script again and now remembered it does not migrate zebra facets declared in biblio-koha-indexdefs.xml, that might be the reason it got dropped. The other facet fields are present in mappings.yaml though.
Comment 99 Ere Maijala 2019-03-12 09:14:44 UTC
I filed bug 22495 for the su-geo fix.
Comment 100 Ere Maijala 2019-03-12 09:36:46 UTC
Looks to me like the original confusion might stem from the fact that queryparser.yaml defines su-geo as an alias for subject. I think that might be a mistake in queryparser.yaml. It was added in bug 9828 that improved UNIMARC support. I suspect there wasn't proper su-geo field in UNIMARC mappings, and the alias would make sense, but only for UNIMARC.
Comment 101 Lucas Gass 2019-03-13 16:00:33 UTC
not backporting this to 18.05x
Comment 102 claire.hernandez@biblibre.com 2019-04-26 14:55:15 UTC
I see this line: 
$dbh->do( "UPDATE `search_field` SET `name` = 'date-of-publication', `label` = 'date-of-publication' WHERE `name` = 'pubdate'" );

The name is changed in database, but in YAML file, I always have pubdate, this has not been modified.
Comment 103 Katrin Fischer 2019-04-26 15:24:28 UTC
(In reply to claire.hernandez@biblibre.com from comment #102)
> I see this line: 
> $dbh->do( "UPDATE `search_field` SET `name` = 'date-of-publication', `label`
> = 'date-of-publication' WHERE `name` = 'pubdate'" );
> 
> The name is changed in database, but in YAML file, I always have pubdate,
> this has not been modified.

Hi Claire, 

can you please open a new bug report and link to this one? It's already in a released version, so we need a fix on a new bug.
Comment 104 Ere Maijala 2019-04-29 06:54:40 UTC
Claire, the publication date mapping will be fixed in bug 22524.