Bug 19575

Summary: Use canonical field names and resolve aliased fields
Product: Koha Reporter: David Gustafsson <glasklas>
Component: Searching - ElasticsearchAssignee: David Gustafsson <glasklas>
Status: Patch doesn't apply --- QA Contact:
Severity: major    
Priority: P5 - low CC: david.bourgault, jonathan.druart, julian.maurice, m.de.rooy, nick
Version: master   
Hardware: All   
OS: All   
Whiteboard:
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 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields
Bug 19575: Use canonical field names and resolve aliased fields

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 ?