Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions
Summary: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::m...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching - Elasticsearch (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Alex Arnaud
URL:
Keywords:
Depends on: 17255
Blocks: 17897
  Show dependency treegraph
 
Reported: 2017-01-11 15:52 UTC by Olli-Antti Kivilahti
Modified: 2019-07-05 05:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions (16.49 KB, patch)
2017-01-11 15:53 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions (3.07 KB, patch)
2017-01-13 10:52 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions (3.07 KB, patch)
2017-01-13 10:52 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 17885 - Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions (3.13 KB, patch)
2017-11-24 19:31 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17885: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions (3.03 KB, patch)
2018-08-31 13:33 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 17885: Koha::SearchEngine::Elasticsearch->reset_elasticsearch_mappings throws DBD::mysql Duplicate entry exceptions (3.10 KB, patch)
2019-07-05 05:23 UTC, Johanna Räisä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2017-01-11 15:52:35 UTC

    
Comment 1 Olli-Antti Kivilahti 2017-01-11 15:53:19 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.
Comment 2 Jonathan Druart 2017-01-12 09:07:28 UTC
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.
Comment 3 Olli-Antti Kivilahti 2017-01-12 14:36:24 UTC
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.
Comment 4 Olli-Antti Kivilahti 2017-01-12 14:39:32 UTC
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.
Comment 5 Jonathan Druart 2017-01-12 14:50:48 UTC
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.
Comment 6 Olli-Antti Kivilahti 2017-01-12 15:20:29 UTC
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.
Comment 7 Olli-Antti Kivilahti 2017-01-12 15:21:31 UTC
I am not sure that separating them to separate bugs is the correct solution here.
Comment 8 Olli-Antti Kivilahti 2017-01-13 10:52:11 UTC
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.
Comment 9 Olli-Antti Kivilahti 2017-01-13 10:52:36 UTC
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.
Comment 10 David Bourgault 2017-11-24 19:31:50 UTC
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>
Comment 11 Alex Arnaud 2018-02-08 14:49:07 UTC
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.
Comment 12 Nick Clemens 2018-03-12 14:48:05 UTC
Should be QAed together with 17897 which contains the tests
Comment 13 Katrin Fischer 2018-03-12 14:51:13 UTC
Thx Nick for taking a look. Moving to BLOCKED for now, move back to Signed off once dependency is resolved please.
Comment 14 Joonas Kylmälä 2018-08-31 13:33:29 UTC
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.
Comment 15 Johanna Räisä 2019-07-05 05:23:50 UTC
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>