Description
Olli-Antti Kivilahti
2017-01-11 15:52:35 UTC
Created attachment 58825 [details] [review] Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions When executing Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings() against populated search engine mappings tables, one gets this exception: DBD::mysql::st execute failed: Duplicate entry '388-73' for key 'PRIMARY' [for Statement \"INSERT INTO `search_marc_to_field` ( `facet`, `search_field_id`, `search_marc_map_id`, `sort`, `suggestible`) VALUES ( ?, ?, ?, ?, ? )\" with ParamValues: 0='', 1='73', 2='388', 3=undef, 4=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.\nDBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '388-73' for key 'PRIMARY' at /home/koha/Koha/Koha/SearchField.pm line 38" This patch fixes this, by upserting those entries instead of adding them. Also adds a Koha::SearchMappingManager to manage the complexities of CRUD:ing these search mappings Also adds nice tests to make sure the Manager and the reset_elasticsearch_mappings() keep working as expected. What are the steps to recreate? select count(*) from search_marc_to_field; 476 Hit /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 => no error. Hi. You can recreate the problem with this: perl -e 'use Koha::SearchEngine::Elasticsearch; Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings' Your solution looks very simple :) Unfortunately there is a bunch of business logic clumped together in the /admin/searchengine/elasticsearch/mappings.pl And I don't think business logic belongs to a controller module. Simply because that functionality is needed when writing tests. When writing tests it is better to call that one subroutine reset_elasticsearch_mappings instead of the bunch of code in mappings.pl If a subroutine says reset_elasticsearch_mappings(), I expect it to reset the elasticsearch mappings without errors. A simple solution would be to move: Koha::SearchMarcMaps->search->delete; inside Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings; which might be a better one than the DBIC-monster I created. My modification preserves existing changes. The way it works in the mappings.pl, is to drop all changes and add the defaults. I guess the original one is the desired behaviour. But I strongly feel that the reset_elasticsearch_mappings()-subroutine needs to perform on its own. Ok got it, so I guess it would be better to simply move the delete inside the reset_elasticsearch_mappings. And move the refactoring to another bug report. Any suggestions for the title of the new refactorings bug? It is all closely related to this if you take a loot at the tests. Refactorings have been done so I can test this change. I am not sure that separating them to separate bugs is the correct solution here. Created attachment 58940 [details] [review] Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions When executing Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings() against populated search engine mappings tables, one gets this exception: DBD::mysql::st execute failed: Duplicate entry '388-73' for key 'PRIMARY' [for Statement \"INSERT INTO `search_marc_to_field` ( `facet`, `search_field_id`, `search_marc_map_id`, `sort`, `suggestible`) VALUES ( ?, ?, ?, ?, ? )\" with ParamValues: 0='', 1='73', 2='388', 3=undef, 4=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.\nDBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '388-73' for key 'PRIMARY' at /home/koha/Koha/Koha/SearchField.pm line 38" This patch fixes this, by first deleting all entries, then adding the default ones. If a subroutine says reset_elasticsearch_mappings(), I expect it to reset the elasticsearch mappings without errors. When writing tests it is better to call that one subroutine reset_elasticsearch_mappings instead of the bunch of code in admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 Also this promotes code reuse. TEST PLAN: a. perl -e 'use Koha::SearchEngine::Elasticsearch; Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings' b. Run misc/devel/populate_db.pl on an already populated DB and you eventually face this problem. After this patch, step a. works. Created attachment 58941 [details] [review] Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions When executing Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings() against populated search engine mappings tables, one gets this exception: DBD::mysql::st execute failed: Duplicate entry '388-73' for key 'PRIMARY' [for Statement \"INSERT INTO `search_marc_to_field` ( `facet`, `search_field_id`, `search_marc_map_id`, `sort`, `suggestible`) VALUES ( ?, ?, ?, ?, ? )\" with ParamValues: 0='', 1='73', 2='388', 3=undef, 4=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.\nDBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '388-73' for key 'PRIMARY' at /home/koha/Koha/Koha/SearchField.pm line 38" This patch fixes this, by first deleting all entries, then adding the default ones. If a subroutine says reset_elasticsearch_mappings(), I expect it to reset the elasticsearch mappings without errors. When writing tests it is better to call that one subroutine reset_elasticsearch_mappings instead of the bunch of code in admin/searchengine/elasticsearch/mappings.pl?op=reset&i_know_what_i_am_doing=1 Also this promotes code reuse. TEST PLAN: a. perl -e 'use Koha::SearchEngine::Elasticsearch; Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings' b. Run misc/devel/populate_db.pl on an already populated DB and you eventually face this problem. After this patch, step a. works. Created attachment 69338 [details] [review] Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions TEST PLAN: a. perl -e 'use Koha::SearchEngine::Elasticsearch; Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings' b. Run misc/devel/populate_db.pl on an already populated DB and you eventually face this problem. After this patch, step a. works. Signed-off-by: David Bourgault <david.bourgault@inlibro.com> Hello Oli, Could you please add unit tests for sub reset_elasticsearch_mappings? At least to check that it deletes mappings as it is what you change in the behavior. But more tests are welcome. Should be QAed together with 17897 which contains the tests Thx Nick for taking a look. Moving to BLOCKED for now, move back to Signed off once dependency is resolved please. Created attachment 78327 [details] [review] Bug 17885: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions I modified the patch to work with the recent Elasticsearch changes. Please sign-off if you think it is fine. I will check up next the status of bug #17897. Created attachment 91308 [details] [review] Bug 17885: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions When executing Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings() against populated search engine mappings tables, one gets this exception: DBD::mysql::st execute failed: Duplicate entry '388-73' for key 'PRIMARY' [for Statement \"INSERT INTO `search_marc_to_field` ( `facet`, `search_field_id`, `search_marc_map_id`, `sort`, `suggestible`) VALUES ( ?, ?, ?, ?, ? )\" with ParamValues: 0='', 1='73', 2='388', 3=undef, 4=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.\nDBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '388-73' for key 'PRIMARY' at /home/koha/Koha/Koha/SearchField.pm line 38" This patch fixes this, by first deleting all entries, then adding the default ones. If a subroutine says reset_elasticsearch_mappings(), I expect it to reset the elasticsearch mappings without errors. When writing tests it is better to call that one subroutine reset_elasticsearch_mappings instead of the bunch of code in admin/searchengine/elasticsearch/mappings.pl?op=reset_confirmed Also this promotes code reuse. TEST PLAN: a. perl -e 'use Koha::SearchEngine::Elasticsearch; Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings' b. Run misc/devel/populate_db.pl on an already populated DB and you eventually face this problem. After this patch, step a. works. This patch is adapted from the original patch made by Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Created attachment 97198 [details] [review] Bug 17885: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions When executing Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings() against populated search engine mappings tables, one gets this exception: DBD::mysql::st execute failed: Duplicate entry '388-73' for key 'PRIMARY' [for Statement \"INSERT INTO `search_marc_to_field` ( `facet`, `search_field_id`, `search_marc_map_id`, `sort`, `suggestible`) VALUES ( ?, ?, ?, ?, ? )\" with ParamValues: 0='', 1='73', 2='388', 3=undef, 4=''] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.\nDBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '388-73' for key 'PRIMARY' at /home/koha/Koha/Koha/SearchField.pm line 38" This patch fixes this, by first deleting all entries, then adding the default ones. If a subroutine says reset_elasticsearch_mappings(), I expect it to reset the elasticsearch mappings without errors. When writing tests it is better to call that one subroutine reset_elasticsearch_mappings instead of the bunch of code in admin/searchengine/elasticsearch/mappings.pl?op=reset_confirmed Also this promotes code reuse. TEST PLAN: a. perl -e 'use Koha::SearchEngine::Elasticsearch; Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings' b. Run misc/devel/populate_db.pl on an already populated DB and you eventually face this problem. After this patch, step a. works. This patch is adapted from the original patch made by Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Test plan is wrong, but changes make sense Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x branch for 19.11.03 backported to 19.05.x for 19.05.08 |