Bug 30142 - ElasticSearch MARC mappings should not accept whitespaces
Summary: ElasticSearch MARC mappings should not accept whitespaces
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Thomas Klausner
QA Contact: Nick Clemens (kidclamp)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-02-21 11:21 UTC by Thomas Klausner
Modified: 2022-12-12 21:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.05,21.05.14
Circulation function:


Attachments
replace whitespace in marc mapping (1.95 KB, patch)
2022-02-21 11:37 UTC, Thomas Klausner
Details | Diff | Splinter Review
Bug 30142: Remove spaces from ElasticSearch mapping MARC fields (1.99 KB, patch)
2022-02-21 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30142: Remove spaces from ElasticSearch mapping MARC fields (2.05 KB, patch)
2022-03-29 13:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Klausner 2022-02-21 11:21:25 UTC
We had a "funny" bug today, because I updated some ElasticSearch Biblio MARC mappings, and entered "245bn " instead of "245bn" (notice the trailing space?)

The Form accepted the invalid value, but after any edit to a biblio we'd get a 500 Error, with this (not very helpful) error message in plack-error.log 
"Invalid MARC field expression: 245bn "

After spotting this, it was easy to fix the typo, but I think that the form should either be stricter when validating input or at least remove whitespace from the MARC input (AFAIK whitesapce is not allowed here).

There is a TODO marker in admin/searchengine/elasticsearch/mappings.pl#161 which is probably related :-)

I will submit a patch in a minute that will at least remove all whitespaces. Actually checking the format should not be too hard with some regex, but I'm not sure if the format is specified anywhere, and in the DB I see quite different values ("440a", "111(abcd)", "008_/14", "leader_/0-4", ...)
Comment 1 Thomas Klausner 2022-02-21 11:37:17 UTC
Created attachment 130948 [details] [review]
replace whitespace in marc mapping

A simple fix to prevent whitespaces in marc field mappings
Comment 2 Katrin Fischer 2022-02-21 22:17:14 UTC
Don't forget to set to "Needs Sign-off" so your patches don't get lost!
Comment 3 Katrin Fischer 2022-02-21 22:18:21 UTC
Created attachment 130997 [details] [review]
Bug 30142: Remove spaces from ElasticSearch mapping MARC fields

A first step to "validate" the MARC mappings: Remove all whitespace, so
if a user enters "245a " (with a trailing whitespace, which can easily
happen when copy/pasting) we only store "245a" in the DB. This is
neccessary, because the ES indexer will throw an exception in an invalid
MARC mapping.

Test Plan:
* Go to /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl
* Go to the Bibliographic Records Tab
* Enter "100 a b c " (notice the whitespaces!) in the first "mapping"
  field
* Scroll down and save
* Go back to the Bibliographic Records Tab
* The spaces are still there

Now apply the patch

* Repeat the above steps
* After saving you should see "100abc" without any spaces in the
  "mapping" field

Sponsored-by: Steiermärkische Landesbibliothek

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Nick Clemens (kidclamp) 2022-03-29 13:22:15 UTC
Created attachment 132411 [details] [review]
Bug 30142: Remove spaces from ElasticSearch mapping MARC fields

A first step to "validate" the MARC mappings: Remove all whitespace, so
if a user enters "245a " (with a trailing whitespace, which can easily
happen when copy/pasting) we only store "245a" in the DB. This is
neccessary, because the ES indexer will throw an exception in an invalid
MARC mapping.

Test Plan:
* Go to /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl
* Go to the Bibliographic Records Tab
* Enter "100 a b c " (notice the whitespaces!) in the first "mapping"
  field
* Scroll down and save
* Go back to the Bibliographic Records Tab
* The spaces are still there

Now apply the patch

* Repeat the above steps
* After saving you should see "100abc" without any spaces in the
  "mapping" field

Sponsored-by: Steiermärkische Landesbibliothek

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 5 Katrin Fischer 2022-03-29 21:20:33 UTC
Thanks Nick!
Comment 6 Fridolin Somers 2022-04-13 13:57:36 UTC
Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑
Comment 7 Kyle M Hall (khall) 2022-04-15 11:40:43 UTC
Pushed to 21.11.x for 21.11.05
Comment 8 Andrew Fuerste-Henry 2022-04-25 20:42:33 UTC
Pushed to 21.05.x for 21.05.14
Comment 9 Victor Grousset/tuxayo 2022-04-25 20:55:21 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.