Bug 24823

Summary: Drop Catmandu dependency
Product: Koha Reporter: David Gustafsson <glasklas>
Component: Searching - ElasticsearchAssignee: David Gustafsson <glasklas>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, ere.maijala, julian.maurice, katrin.fischer, m.de.rooy, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23986
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25342
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 25955, 26309    
Attachments: Bug 24823: Drop Catmandu dependency
Bug 24823: Drop Catmandu dependency
Bug 24823: Fix spelling error and remove last of Catmandu
Bug 24823: Remove unused parameters
Bug 24823: Fix spelling error and remove last of Catmandu
Bug 24823: Remove unused parameters
Bug 24823: Drop Catmandu dependency
Bug 24823: Fix spelling error and remove last of Catmandu
Bug 24823: Remove unused parameters
Bug 24823: Drop Catmandu dependency
Bug 24823: Fix spelling error and remove last of Catmandu
Bug 24823: Remove unused parameters
Bug 24823: Drop Catmandu dependency
Bug 24823: Fix spelling error and remove last of Catmandu
Bug 24823: Remove unused parameters
Bug 24823: (QA follow-up) Fix deletion and POD

Description David Gustafsson 2020-03-06 11:58:57 UTC
This is a follow up of Bug 19893 where most of, but not all, of the dependencies on Catmandu was replaced with our own code. Since there is not much left it would be nice to get rid of the Catmandu dependency altogether.
Comment 1 David Gustafsson 2020-03-06 12:13:47 UTC
Created attachment 100268 [details] [review]
Bug 24823: Drop Catmandu dependency

Replace remaining Catmandu dependant code with the Search::Elasticsearch
equivalent.

To test:
1) Apply patch
2) Run tests in t/Koha/SearchEngine/Elasticsearch.t, t/Koha/SearchEngine/ElasticSearch/*
and t/db_dependent/Koha/SearchEngine/*
3) All tests should pass
Comment 2 David Nind 2020-03-07 09:43:56 UTC
Created attachment 100308 [details] [review]
Bug 24823: Drop Catmandu dependency

Replace remaining Catmandu dependant code with the Search::Elasticsearch
equivalent.

To test:
1) Apply patch
2) Run tests in t/Koha/SearchEngine/Elasticsearch.t, t/Koha/SearchEngine/ElasticSearch/*
and t/db_dependent/Koha/SearchEngine/*
3) All tests should pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Julian Maurice 2020-03-08 11:24:25 UTC
Catmandu::Store::ElasticSearch is still used in Koha/SearchEngine/Elasticsearch/Browse.pm. So either this code should be replaced too, or it can be done in another bug but the commit message should be updated to clarify that.

In any case, please update the cpanfile accordingly.

Also, QA script returns 1 spelling error in Koha/SearchEngine/Elasticsearch/Indexer.pm: 
occured  ==> occurred
Comment 4 David Gustafsson 2020-03-09 10:47:34 UTC
Thanks for pointing this out, I was working with a slightly outdated master not containing the cpanfile and Browse.pm changes. Have now rebased against latest master and fixed the spelling error.
Comment 5 David Gustafsson 2020-03-09 10:49:22 UTC
Created attachment 100337 [details] [review]
Bug 24823: Fix spelling error and remove last of Catmandu
Comment 6 David Gustafsson 2020-03-12 10:57:12 UTC
Created attachment 100617 [details] [review]
Bug 24823: Remove unused parameters

Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'.
Refactor so that get_elasticsearch_params only returns parameters used
by Search::Elasticsearch, add class accessor for index_name.
Comment 7 Katrin Fischer 2020-03-22 13:10:07 UTC
Julian, do you plan to continue here?
Comment 8 Julian Maurice 2020-03-22 18:10:29 UTC
Katrin, I'm not very familiar with the ES code yet, I just pointed out the obvious problems. It's probably better if someone else review the patches.
Comment 9 Katrin Fischer 2020-03-22 18:28:14 UTC
(In reply to Julian Maurice from comment #8)
> Katrin, I'm not very familiar with the ES code yet, I just pointed out the
> obvious problems. It's probably better if someone else review the patches.

Thx Julian for the feedback. Ere or Nick, could one of you have a look?
Comment 10 Marcel de Rooy 2020-05-01 07:26:20 UTC
error: sha1 information is lacking or useless (Koha/SearchEngine/Elasticsearch.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 24823: Remove unused parameters
Comment 11 David Gustafsson 2020-05-04 15:24:21 UTC
Created attachment 104303 [details] [review]
Bug 24823: Fix spelling error and remove last of Catmandu
Comment 12 David Gustafsson 2020-05-04 15:24:27 UTC
Created attachment 104304 [details] [review]
Bug 24823: Remove unused parameters

Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'.
Refactor so that get_elasticsearch_params only returns parameters used
by Search::Elasticsearch, add class accessor for index_name.
Comment 13 David Gustafsson 2020-05-04 15:25:35 UTC
Created attachment 104305 [details] [review]
Bug 24823: Drop Catmandu dependency

Replace remaining Catmandu dependant code with the Search::Elasticsearch
equivalent.

To test:
1) Apply patch
2) Run tests in t/Koha/SearchEngine/Elasticsearch.t, t/Koha/SearchEngine/ElasticSearch/*
and t/db_dependent/Koha/SearchEngine/*
3) All tests should pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Gustafsson 2020-05-04 15:25:41 UTC
Created attachment 104306 [details] [review]
Bug 24823: Fix spelling error and remove last of Catmandu
Comment 15 David Gustafsson 2020-05-04 15:25:48 UTC
Created attachment 104307 [details] [review]
Bug 24823: Remove unused parameters

Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'.
Refactor so that get_elasticsearch_params only returns parameters used
by Search::Elasticsearch, add class accessor for index_name.
Comment 16 David Gustafsson 2020-05-04 15:27:02 UTC
@Marcel de Rooy

Did not get the same error, are you sure you where in master when applying the patch? It did need rebasing though, so have now rebased against latest master.
Comment 17 Martin Renvoize 2020-05-05 14:25:28 UTC
Created attachment 104371 [details] [review]
Bug 24823: Drop Catmandu dependency

Replace remaining Catmandu dependant code with the Search::Elasticsearch
equivalent.

To test:
1) Apply patch
2) Run tests in t/Koha/SearchEngine/Elasticsearch.t, t/Koha/SearchEngine/ElasticSearch/*
and t/db_dependent/Koha/SearchEngine/*
3) All tests should pass

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2020-05-05 14:25:32 UTC
Created attachment 104372 [details] [review]
Bug 24823: Fix spelling error and remove last of Catmandu

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2020-05-05 14:25:36 UTC
Created attachment 104373 [details] [review]
Bug 24823: Remove unused parameters

Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'.
Refactor so that get_elasticsearch_params only returns parameters used
by Search::Elasticsearch, add class accessor for index_name.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2020-05-05 14:25:58 UTC
This all works as expected during testing.. Signing off.
Comment 21 Nick Clemens 2020-05-07 12:13:04 UTC
Created attachment 104501 [details] [review]
Bug 24823: Drop Catmandu dependency

Replace remaining Catmandu dependant code with the Search::Elasticsearch
equivalent.

To test:
1) Apply patch
2) Run tests in t/Koha/SearchEngine/Elasticsearch.t, t/Koha/SearchEngine/ElasticSearch/*
and t/db_dependent/Koha/SearchEngine/*
3) All tests should pass

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens 2020-05-07 12:13:08 UTC
Created attachment 104502 [details] [review]
Bug 24823: Fix spelling error and remove last of Catmandu

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2020-05-07 12:13:12 UTC
Created attachment 104503 [details] [review]
Bug 24823: Remove unused parameters

Remove unused Elasticsearch parameters 'key_prefix' and 'request_timeout'.
Refactor so that get_elasticsearch_params only returns parameters used
by Search::Elasticsearch, add class accessor for index_name.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2020-05-07 12:13:15 UTC
Created attachment 104504 [details] [review]
Bug 24823: (QA follow-up) Fix deletion and POD

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2020-05-07 12:16:45 UTC
Tested with ES5 and ES6, everything seems to work as before.

This really exposes how little we used Catmandu for at this point, using the Elasticsearch perl modules directly makes things a little simpler and more direct.

I did add back in the quotes around ids passed to delete - I saw the same problem as before we had them, the passed value was treated as a number 16.0 and records were not deleted

It did seem to fix issue from bug 25342, however, I would suggest pushing that one forward anyway as being explicit shouldn't hurt us

Excellent work!
Comment 26 Martin Renvoize 2020-05-12 10:47:44 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Joy Nelson 2020-05-13 19:09:46 UTC
enhancement not backported to 19.11.x
Comment 28 David Gustafsson 2020-05-18 12:12:20 UTC
Regarding the last fix (quoting ids for delete), I don't think this i necessary any more as of this patch (thought the code that caused the issues was in Catmandu). When I get the time will test to see if this is the case, and create a new issue where the quoting of ids is removed.
Comment 29 Ere Maijala 2020-05-18 12:24:20 UTC
(In reply to David Gustafsson from comment #28)
> Regarding the last fix (quoting ids for delete), I don't think this i
> necessary any more as of this patch (thought the code that caused the issues
> was in Catmandu). When I get the time will test to see if this is the case,
> and create a new issue where the quoting of ids is removed.

I think I saw some Koha code passing the id as numeric. I'd be vary of removing the quoting.
Comment 30 David Gustafsson 2020-05-18 12:49:54 UTC
To my knowledge Search::Elasticsearch does not care about the type of id values, so think can be removed, but would of course have to check and verify first that that is the case.
Comment 31 Ere Maijala 2020-05-18 12:50:40 UTC
(In reply to David Gustafsson from comment #30)
> To my knowledge Search::Elasticsearch does not care about the type of id
> values, so think can be removed, but would of course have to check and
> verify first that that is the case.

If that's the case, I stand corrected.
Comment 32 Nick Clemens 2020-05-18 15:09:47 UTC
(In reply to David Gustafsson from comment #30)
> To my knowledge Search::Elasticsearch does not care about the type of id
> values, so think can be removed, but would of course have to check and
> verify first that that is the case.

I did test this and saw issues when the values were unquoted - it doesn't seem to happen under all circumstances but it does happen. Make sure to test in ES6 as well