Bug 28173 - Add plugin hooks object_store_pre and object_store_post
Summary: Add plugin hooks object_store_pre and object_store_post
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-20 08:26 UTC by Julian Maurice
Modified: 2024-03-26 13:42 UTC (History)
4 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 28173: Add plugin hooks object_store_pre and object_store_post (4.11 KB, patch)
2021-04-20 08:26 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 28173: Add plugin hooks object_store_pre and object_store_post (4.13 KB, patch)
2024-03-20 08:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 28173: [DO NOT PUSH] Benchmark script (1.12 KB, patch)
2024-03-20 09:18 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2021-04-20 08:26:13 UTC

    
Comment 1 Julian Maurice 2021-04-20 08:26:52 UTC
Created attachment 119872 [details] [review]
Bug 28173: Add plugin hooks object_store_pre and object_store_post

These two hooks are called in Koha::Object::store, which means any
object stored in the database will trigger these hooks.

object_store_pre is called just before `update_or_insert`, allowing to
modify the object before sending the INSERT/UPDATE query

object_store_post is called just after `update_or_insert`, so it can be
used for any actions that require to have the final object, like
indexing, logging, ...

Test plan:
1. prove t/db_dependent/Koha/Plugins/Object_hooks.t
Comment 2 Fridolin Somers 2021-08-24 09:28:31 UTC
I like that.

Just for hook name.
In https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks we see existing hooks are calles after_xxx :
  after_biblio_action
  after_circ_action

So i think whe should call this hooks :
  before_object_store
  after_object_store
Comment 3 Fridolin Somers 2021-12-08 21:20:52 UTC
Oh crap.

We found a big performance issue with this hook.

Each store() call is checking in DB the plugin version and if it is enabled.

Particulary when DB is distant this is bad for performance.
Comment 4 Fridolin Somers 2021-12-08 21:26:14 UTC
This hook is used by Biblibre for action on record before saving.

I suggest we change to implement hooks 'before_authority_action' and 'before_biblio_action'.
With record as param.
Comment 5 Julian Maurice 2021-12-09 14:41:30 UTC
(In reply to Fridolin Somers from comment #3)
> Oh crap.
> 
> We found a big performance issue with this hook.
> 
> Each store() call is checking in DB the plugin version and if it is enabled.
> 
> Particulary when DB is distant this is bad for performance.

The problem is not this particular hook. It's Koha::Plugins->call that is too slow. I tried making it faster with bug 29672.
Please test and let me know what you think
Comment 6 Fridolin Somers 2021-12-10 07:38:20 UTC
(In reply to Julian Maurice from comment #5)
> 
> The problem is not this particular hook. It's Koha::Plugins->call that is
> too slow. I tried making it faster with bug 29672.
> Please test and let me know what you think

I agree with you.
Thanks a lot for the perf patch, looks great
Comment 7 David Cook 2021-12-20 03:46:30 UTC
When I saw the hook names, I was worried about performance, and I can see that worry was well founded.

I'm curious if bug 29672 will be enough. These hooks are pretty broad. I wonder how much of a slowdown they would bring overall even with a well designed plugin...
Comment 8 Fridolin Somers 2022-11-30 00:13:14 UTC
Bug 29672 pushed master for 22.11.
Comment 9 David Nind 2023-12-17 23:24:30 UTC
The patch no longer applies (not unexpected!).

Not sure about the comments after the patch 8-)


git bz apply 28173

Bug 28173 - Add plugin hooks object_store_pre and object_store_post

119872 - Bug 28173: Add plugin hooks object_store_pre and object_store_post

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 28173: Add plugin hooks object_store_pre and object_store_post
Using index info to reconstruct a base tree...
M	Koha/Object.pm
A	t/lib/Koha/Plugin/Test.pm
Falling back to patching base and 3-way merge...
Auto-merging t/lib/plugins/Koha/Plugin/Test.pm
CONFLICT (content): Merge conflict in t/lib/plugins/Koha/Plugin/Test.pm
Auto-merging Koha/Object.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 28173: Add plugin hooks object_store_pre and object_store_post
...
Comment 10 David Cook 2024-03-20 04:37:12 UTC
While I've had my reservations about this, having some kind of "pre" and "post" store hooks would be amazing...
Comment 11 Julian Maurice 2024-03-20 08:25:15 UTC
Created attachment 163509 [details] [review]
Bug 28173: Add plugin hooks object_store_pre and object_store_post

These two hooks are called in Koha::Object::store, which means any
object stored in the database will trigger these hooks.

object_store_pre is called just before `update_or_insert`, allowing to
modify the object before sending the INSERT/UPDATE query

object_store_post is called just after `update_or_insert`, so it can be
used for any actions that require to have the final object, like
indexing, logging, ...

Test plan:
1. prove t/db_dependent/Koha/Plugins/Object_hooks.t
Comment 12 Julian Maurice 2024-03-20 08:25:31 UTC
Patch rebased on master
Comment 13 Julian Maurice 2024-03-20 09:18:18 UTC
Created attachment 163516 [details] [review]
Bug 28173: [DO NOT PUSH] Benchmark script
Comment 14 Julian Maurice 2024-03-20 09:35:46 UTC
I ran the attached benchmark script in 3 different scenario:
1) master (c6fa96eeca)
2) with the patch, with 2 plugins, 1 enabled but without the object_store_* hooks, 1 with both hooks but disabled. 
3) same as 2, but with both plugins enabled

The code inside object_store_* hooks was very simple (1 call to $object->_type and 1 call to $object->make_column_dirty)

1) The time per `store` call is between 91µs and 104µs
2) The time per `store` call is between 98µs and 110µs
3) The time per `store` call is between 104µs and 115µs

There is a noticeable slowdown, which is expected.
I think 10µs per call is negligible if we only have a few store calls per http request, but I don't know how many store calls we have on a typical request. Does anyone know ?
Comment 15 Fridolin Somers 2024-03-26 13:42:14 UTC
I linked to Bug 34943 recently pushed master