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.
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
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>
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
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.
Created attachment 100337 [details] [review] Bug 24823: Fix spelling error and remove last of Catmandu
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.
Julian, do you plan to continue here?
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.
(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?
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
Created attachment 104303 [details] [review] Bug 24823: Fix spelling error and remove last of Catmandu
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.
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>
Created attachment 104306 [details] [review] Bug 24823: Fix spelling error and remove last of Catmandu
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.
@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.
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>
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>
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>
This all works as expected during testing.. Signing off.
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>
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>
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>
Created attachment 104504 [details] [review] Bug 24823: (QA follow-up) Fix deletion and POD Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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!
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x
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.
(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.
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.
(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.
(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