Bug 31897 - New hook when indexing with Elasticsearch
Summary: New hook when indexing with Elasticsearch
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Stefan Berndtsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 31734
  Show dependency treegraph
 
Reported: 2022-10-20 16:45 UTC by Stefan Berndtsson
Modified: 2024-03-26 22:31 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 31897: New before_index_action hook for Elasticsearch (4.77 KB, patch)
2022-10-20 16:45 UTC, Stefan Berndtsson
Details | Diff | Splinter Review
Bug 31897: New before_index_action hook for Elasticsearch (4.82 KB, patch)
2022-10-20 19:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 31897: New before_index_action hook for Elasticsearch (4.77 KB, patch)
2022-10-21 08:55 UTC, Stefan Berndtsson
Details | Diff | Splinter Review
Bug 31897: New before_index_action hook for Elasticsearch (4.82 KB, patch)
2022-10-21 19:31 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2022-10-20 16:45:18 UTC
This patch adds a new hook "before_index_action" with the action "update". This is run on the array reference to the biblio records that are about to be updated in the Elasticsearch index. 

With this, a plugin can change the record so that it better suits the index without having to change the actual record in the database (something that might be prohibited or discouraged due to centralised cataloging).
Comment 1 Stefan Berndtsson 2022-10-20 16:45:51 UTC
Created attachment 142268 [details] [review]
Bug 31897: New before_index_action hook for Elasticsearch

Hooks added:
before_index_action with new action update

How to test:
Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t

Sponsored by: Gothenburg University Library
Comment 2 David Nind 2022-10-20 19:03:42 UTC
Created attachment 142280 [details] [review]
Bug 31897: New before_index_action hook for Elasticsearch

Hooks added:
before_index_action with new action update

How to test:
Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t

Sponsored by: Gothenburg University Library

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Marcel de Rooy 2022-10-21 06:59:00 UTC
 FAIL   t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t
   OK     critic
   FAIL   file permissions
                File must have the exec flag

Sorry, but it should not take you too much time to get those four back in the queue ;)
Comment 4 Stefan Berndtsson 2022-10-21 08:55:53 UTC
Created attachment 142308 [details] [review]
Bug 31897: New before_index_action hook for Elasticsearch

Hooks added:
before_index_action with new action update

How to test:
Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t

Sponsored by: Gothenburg University Library
Comment 5 Stefan Berndtsson 2022-10-21 08:56:31 UTC
Fixed file permissions and some trailing whitespace in this one as well.
Comment 6 Marcel de Rooy 2022-10-21 08:58:38 UTC
(In reply to Stefan Berndtsson from comment #5)
> Fixed file permissions and some trailing whitespace in this one as well.

If you only do cosmetic fixing, keep the previous signoff line and change status back to Signed off. No need to wait for another signoff imo.
Comment 7 David Nind 2022-10-21 19:31:26 UTC
Created attachment 142441 [details] [review]
Bug 31897: New before_index_action hook for Elasticsearch

Hooks added:
before_index_action with new action update

How to test:
Run tests in t/db_dependent/Koha/Plugins/Elasticsearch_Indexer_hooks.t

Sponsored by: Gothenburg University Library

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Marcel de Rooy 2022-10-25 14:25:19 UTC
$VAR1 = {
          'cxn_pool' => 'Static',
          'index_name' => 'koha_myclone',
          'nodes' => [
                       'localhost:9200'
                     ]
        };

Result of Koha::SearchEngine::Elasticsearch->get_elasticsearch_params;
You test that to determine if Elastic exists.
Wrong test.

Later you crash:
Exception 'Koha::Exceptions::Elasticsearch::BadResponse' thrown 'Bad response received when submitting request to Elasticsearch' with details => No nodes are available: [http://localhost:9200], type => NoNodes
Comment 9 Marcel de Rooy 2022-10-25 14:26:06 UTC
    $schema->storage->txn_rollback;
    Koha::Plugins::Methods->delete;

Really, this is wrong.
Comment 10 Marcel de Rooy 2022-10-28 08:48:11 UTC
This still needs attention.
Comment 11 Stefan Berndtsson 2022-10-29 12:59:31 UTC
(In reply to Marcel de Rooy from comment #8)
> $VAR1 = {
>           'cxn_pool' => 'Static',
>           'index_name' => 'koha_myclone',
>           'nodes' => [
>                        'localhost:9200'
>                      ]
>         };
> 
> Result of Koha::SearchEngine::Elasticsearch->get_elasticsearch_params;
> You test that to determine if Elastic exists.
> Wrong test.
> 
> Later you crash:
> Exception 'Koha::Exceptions::Elasticsearch::BadResponse' thrown 'Bad
> response received when submitting request to Elasticsearch' with details =>
> No nodes are available: [http://localhost:9200], type => NoNodes

I'm really not sure what I'm supposed to do here. This is based on what the test t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t does, with the Plugin parts added to it.

That test does the same result-check of get_elasticsearch_params, and if there is no elasticsearch running (localhost:9200 or equivalent) it also fails with the NoNodes error.

I can't see how I can come up with a solution to a test depending on a running Elasticsearch without having a running Elasticsearch.

The current test does not like it when there is an Elastic running but Zebra is set as the engine, so it'll have to be tweaked (the existing Indexer.t works here). I did some more copy&paste from that one so I can make this test work as well regardless of that syspref, but not without actually having Elastic running.

Suggestions are welcome. :)
Comment 12 Marcel de Rooy 2022-11-04 10:03:42 UTC
(In reply to Stefan Berndtsson from comment #11)
> I'm really not sure what I'm supposed to do here. This is based on what the
> test t/db_dependent/Koha/SearchEngine/Elasticsearch/Indexer.t does, with the
> Plugin parts added to it.
> 
> That test does the same result-check of get_elasticsearch_params, and if
> there is no elasticsearch running (localhost:9200 or equivalent) it also
> fails with the NoNodes error.
> 
> I can't see how I can come up with a solution to a test depending on a
> running Elasticsearch without having a running Elasticsearch.
> 
> The current test does not like it when there is an Elastic running but Zebra
> is set as the engine, so it'll have to be tweaked (the existing Indexer.t
> works here). I did some more copy&paste from that one so I can make this
> test work as well regardless of that syspref, but not without actually
> having Elastic running.

I would fail Indexer.t now too for the same reason indeed. But it is pushed already ;)
I am not an Elastic expert. Please ask on IRC or dev list.
Comment 13 Stefan Berndtsson 2022-11-04 19:55:28 UTC
(In reply to Marcel de Rooy from comment #12)

> I would fail Indexer.t now too for the same reason indeed. But it is pushed
> already ;)
> I am not an Elastic expert. Please ask on IRC or dev list.

I'll have to put this on hold for the time being. It's not within the scope of this patch, nor unfortunately within the scope of my available time and resources to deal with a solution to the generic Elasticsearch testing model. We're at the end of a release-phase now, and the actual code in this works for us. It'll have to be resumed at some later date.
Comment 14 Fridolin Somers 2022-11-28 08:51:14 UTC
This looks great
Comment 15 Fridolin Somers 2022-12-02 02:25:50 UTC
Ah we should give to the plugin the index name 'biblios' or 'authorities'.
Maybe with $self->index.

> a plugin can change the record so that it better suits the index 
In this case we may better hook in Koha::SearchEngine::Elasticsearch::marc_records_to_documents().
There already is feature for ISBN forms and IncludeSeeFromInSearches.
Comment 16 Frédéric Demians 2024-03-26 17:34:11 UTC
See also bug 36433