Bug 30142

Summary: ElasticSearch MARC mappings should not accept whitespaces
Product: Koha Reporter: Thomas Klausner <domm>
Component: Searching - ElasticsearchAssignee: Thomas Klausner <domm>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: normal    
Priority: P5 - low CC: andrewfh, jonathan.druart+koha, kyle, nick, victor
Version: unspecified   
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:
22.05.00,21.11.05,21.05.14
Attachments: replace whitespace in marc mapping
Bug 30142: Remove spaces from ElasticSearch mapping MARC fields
Bug 30142: Remove spaces from ElasticSearch mapping MARC fields

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 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 🦄
Comment 7 Kyle M Hall 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.