[Bug 24823] New: Drop Catmandu dependency
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Bug ID: 24823 Summary: Drop Catmandu dependency Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Searching - Elasticsearch Assignee: koha-bugs@lists.koha-community.org Reporter: glasklas@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #1 from David Gustafsson <glasklas@gmail.com> --- Created attachment 100268 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100268&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |glasklas@gmail.com |ity.org | Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100268|0 |1 is obsolete| | --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 100308 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100308&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #3 from Julian Maurice <julian.maurice@biblibre.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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #4 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #5 from David Gustafsson <glasklas@gmail.com> --- Created attachment 100337 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100337&action=edit Bug 24823: Fix spelling error and remove last of Catmandu -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #6 from David Gustafsson <glasklas@gmail.com> --- Created attachment 100617 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100617&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact| |julian.maurice@biblibre.com CC| |ere.maijala@helsinki.fi, | |katrin.fischer@bsz-bw.de, | |nick@bywatersolutions.com --- Comment #7 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Julian, do you plan to continue here? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #8 from Julian Maurice <julian.maurice@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|julian.maurice@biblibre.com | --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Patch doesn't apply CC| |m.de.rooy@rijksmuseum.nl --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100337|0 |1 is obsolete| | --- Comment #11 from David Gustafsson <glasklas@gmail.com> --- Created attachment 104303 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104303&action=edit Bug 24823: Fix spelling error and remove last of Catmandu -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100617|0 |1 is obsolete| | --- Comment #12 from David Gustafsson <glasklas@gmail.com> --- Created attachment 104304 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104304&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100308|0 |1 is obsolete| | --- Comment #13 from David Gustafsson <glasklas@gmail.com> --- Created attachment 104305 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104305&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104303|0 |1 is obsolete| | --- Comment #14 from David Gustafsson <glasklas@gmail.com> --- Created attachment 104306 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104306&action=edit Bug 24823: Fix spelling error and remove last of Catmandu -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104304|0 |1 is obsolete| | --- Comment #15 from David Gustafsson <glasklas@gmail.com> --- Created attachment 104307 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104307&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #16 from David Gustafsson <glasklas@gmail.com> --- @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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=23986 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 David Gustafsson <glasklas@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=25342 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104305|0 |1 is obsolete| | --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104371 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104371&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104306|0 |1 is obsolete| | --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104372 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104372&action=edit Bug 24823: Fix spelling error and remove last of Catmandu Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104307|0 |1 is obsolete| | --- Comment #19 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 104373 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104373&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Needs Signoff |Signed Off --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This all works as expected during testing.. Signing off. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_20_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104371|0 |1 is obsolete| | --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 104501 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104501&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104372|0 |1 is obsolete| | --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 104502 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104502&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #104373|0 |1 is obsolete| | --- Comment #23 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 104503 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104503&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 104504 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=104504&action=edit Bug 24823: (QA follow-up) Fix deletion and POD Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #25 from 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |joy@bywatersolutions.com --- Comment #27 from Joy Nelson <joy@bywatersolutions.com> --- enhancement not backported to 19.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_20_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #28 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #29 from Ere Maijala <ere.maijala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #30 from David Gustafsson <glasklas@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #31 from Ere Maijala <ere.maijala@helsinki.fi> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 --- Comment #32 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |25955 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25955 [Bug 25955] compare_es_to_db.pl broken by drop of Catmandu dependency -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |26309 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26309 [Bug 26309] Elasticsearch cxn_pool must be configurable (again) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24823 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to master |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org