Bug 23185 - Koha::Objects supports passing through 'update' which means we can side step 'set' + 'store'
Summary: Koha::Objects supports passing through 'update' which means we can side step ...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords: additional_work_needed
Depends on: 21761
Blocks: 23070 25423
  Show dependency treegraph
 
Reported: 2019-06-21 15:27 UTC by Martin Renvoize
Modified: 2020-05-11 20:52 UTC (History)
11 users (show)

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


Attachments
Bug 23185: Add tests (13.60 KB, patch)
2020-04-28 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23185: Make Koha::Objects->update loop on the object set if needed (2.96 KB, patch)
2020-04-28 15:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23185: Add tests (13.73 KB, patch)
2020-04-29 14:09 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 23185: Make Koha::Objects->update loop on the object set if needed (3.03 KB, patch)
2020-04-29 14:09 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 23185: Add tests (13.73 KB, patch)
2020-05-01 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23185: Make Koha::Objects->update loop on the object set if needed (3.09 KB, patch)
2020-05-01 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23185: (QA follow-up) Semantics, split fields and options (2.28 KB, patch)
2020-05-01 13:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23185: Add tests (13.78 KB, patch)
2020-05-03 21:02 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23185: Make Koha::Objects->update loop on the object set if needed (3.15 KB, patch)
2020-05-03 21:02 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23185: (QA follow-up) Semantics, split fields and options (2.34 KB, patch)
2020-05-03 21:02 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 23185: Add tests (13.84 KB, patch)
2020-05-04 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23185: Make Koha::Objects->update loop on the object set if needed (3.20 KB, patch)
2020-05-04 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23185: (QA follow-up) Semantics, split fields and options (2.39 KB, patch)
2020-05-04 12:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23185: (QA follow-up) Add POD for Koha::Objects->update (1.10 KB, patch)
2020-05-04 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23185: ->update can only be called on instantiated classes (2.47 KB, patch)
2020-05-07 21:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23185: Realistic POD for Koha::Objects->search (1.18 KB, patch)
2020-05-07 21:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23185: (follow-up) K:C:Register->make_default should use no_triggers (943 bytes, patch)
2020-05-08 00:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-06-21 15:27:17 UTC
+++ This bug was initially created as a clone of Bug #21761 +++

If we're adding triggers at the code level all over the place by overloading the store method we need to never ever let dbic handle it.. by allowing the 'update' method through we are exposing ourselves to places in the codebase skipping the store triggers.

This would in effect bind us to the 'update_all' style of update call from dbic, which results in a loop over the affected rows so that in code triggers can be called per row rather than allowing any form of 'update all these rows to X' in one query.
Comment 1 Martin Renvoize 2019-06-21 16:02:47 UTC
Note the 's' in the bug title.. this is not the same as 'Koha::Object'
Comment 2 Martin Renvoize 2019-06-21 16:03:50 UTC
See further discussion in bug 23070
Comment 3 Martin Renvoize 2019-06-21 16:05:20 UTC
My personal feeling is that perhaps we should allow this one through but add a QA script warning to check that 'store' for the object in question doesn't have any triggers in place for the fields of the set being updated in any changes or additions to the update call in question.
Comment 4 Martin Renvoize 2019-06-21 16:07:43 UTC
I also think it wouldn't be a bad idea to have an 'update_with_triggers' or something along those lines method which in effect did a dbic update_all call(https://metacpan.org/pod/DBIx::Class::ResultSet#update_all), i.e. calling set + store one each Koha::Object in a loop from the Koha::Objects set.
Comment 5 Marcel de Rooy 2019-12-06 07:36:52 UTC
I almost added similar comments on 21761 until I spotted this one.

Obviously, implementing update on a larger resultset by looping and storing individually will be a performance thing.
Wouldnt it be beneficial to pass to DBIx if there is no specific store method in the corresponding Koha object ?
Comment 6 Martin Renvoize 2020-04-28 13:15:57 UTC
More and more I feel like this needs to be caught at the QA level.. whenever we see the 'store' method being changed/created in a Koha::Thing class we should warn the QA person via the qa scripts to check for a corresponding 'update' method in the Koha::Things which just loops over the resultset calling the store on each result.
Comment 7 Martin Renvoize 2020-04-28 13:45:10 UTC
Agreed with Jonathan that we should default Koha::Objects->update to loop through the resultset and call Koha::Object->store..  However, we should also allow for a direct_update option either by adding a direct_update method to Koha::Objects or by checking for a parameter passed to Koha::Objects->update.

Examples:
Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } })->update({ priority => \'priority + 1' }, { skip_triggers => 1});

OR

Koha::Holds->search({ biblionumber => $hold->biblionumber, priority => { '>' => 0 } })->direct_update({ priority => \'priority + 1' });
Comment 8 Marcel de Rooy 2020-04-28 13:47:02 UTC
Is adding direct_update not adding the side step that you wanted to catch ?
Comment 9 Martin Renvoize 2020-04-28 13:57:56 UTC
(In reply to Marcel de Rooy from comment #8)
> Is adding direct_update not adding the sidestep that you wanted to catch ?

It's making the side step very explicit.. there are certainly times when you want to have a direct access call.. as per the example above ;) where we're sure that we are only updating one field which otherwise has no triggers in store.
Comment 10 Jonathan Druart 2020-04-28 15:16:15 UTC
Created attachment 103867 [details] [review]
Bug 23185: Add tests
Comment 11 Jonathan Druart 2020-04-28 15:16:18 UTC
Created attachment 103868 [details] [review]
Bug 23185: Make Koha::Objects->update loop on the object set if needed
Comment 12 Jonathan Druart 2020-04-28 15:20:04 UTC
Does this make sense?
Comment 13 Victor Grousset/tuxayo 2020-04-29 05:37:31 UTC
I suppose the test plan is to run the automated tests right?

It's stuck since 32 min I had to kill the process.

root@e0f73671e924:koha((5724c66959...))$ kshell
kohadev-koha@e0f73671e924:/kohadevbox/koha$ time prove t/db_dependent/Koha/Objects.t
t/db_dependent/Koha/Objects.t .. 18/22 

It's infinite looping on something.

PID USER      PRI  NI  VIRT   RES   SHR S CPU% MEM%   TIME+  Command
2549 kohadev-k  20   0  899M  687M 14736 R 75.4  5.9 23:38.44 /usr/bin/perl t/db_dependent/Koha/Objects.t


I reverted to master and ran the test again and it worked.

kohadev-koha@e0f73671e924:/kohadevbox/koha$ time prove t/db_dependent/Koha/Objects.t
t/db_dependent/Koha/Objects.t .. ok     
All tests successful.
Files=1, Tests=21, 21 wallclock secs ( 0.06 usr  0.01 sys + 15.54 cusr  4.21 csys = 19.82 CPU)
Result: PASS

real	0m21.111s
Comment 14 Jonathan Druart 2020-04-29 08:48:25 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2020-04-29 08:48:34 UTC
It passes for me, in ~30sec
Comment 16 Bernardo Gonzalez Kriegel 2020-04-29 12:32:13 UTC
(In reply to Jonathan Druart from comment #15)
> It passes for me, in ~30sec

Same problem as Victor, need to kill it... :(

$ time prove t/db_dependent/Koha/Objects.t
t/db_dependent/Koha/Objects.t .. 18/22 ^C
real    16m31,000s
user    0m0,208s
sys     0m0,026s
Comment 17 Jonathan Druart 2020-04-29 13:37:33 UTC
Ho right, that's the tests from bug 25303. Something weird is going on here.
Comment 18 Jonathan Druart 2020-04-29 13:49:14 UTC
(In reply to Jonathan Druart from comment #17)
> Ho right, that's the tests from bug 25303. Something weird is going on here.

Fixed with a follow-up on bug 25303.
Comment 19 Bernardo Gonzalez Kriegel 2020-04-29 14:09:05 UTC
Created attachment 103946 [details] [review]
Bug 23185: Add tests

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on top of 25303 w/new follow-up
Test pass in ~45s, no errors
Comment 20 Bernardo Gonzalez Kriegel 2020-04-29 14:09:09 UTC
Created attachment 103947 [details] [review]
Bug 23185: Make Koha::Objects->update loop on the object set if needed

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 21 Martin Renvoize 2020-04-29 15:39:59 UTC
Comment on attachment 103947 [details] [review]
Bug 23185: Make Koha::Objects->update loop on the object set if needed

Review of attachment 103947 [details] [review]:
-----------------------------------------------------------------

I'm open to opinions on the below comment but I feel it's clearer to keep the two things distinct.

::: Koha/Objects.pm
@@ +199,5 @@
> +
> +sub update {
> +    my ($self, $params) = @_;
> +
> +    my $no_triggers = delete $params->{no_triggers};

Personally, I think I'd rather keep method options distinct from the row data.. so I would have prefered

my ($self, $params, $options) = @_;

my $no_triggers = $options->{no_triggers};
Comment 22 Martin Renvoize 2020-04-29 15:45:02 UTC
Generally.. I really like this however :)
Comment 23 Martin Renvoize 2020-05-01 13:14:13 UTC
Created attachment 104087 [details] [review]
Bug 23185: Add tests

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-05-01 13:14:17 UTC
Created attachment 104088 [details] [review]
Bug 23185: Make Koha::Objects->update loop on the object set if needed

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-05-01 13:14:21 UTC
Created attachment 104089 [details] [review]
Bug 23185: (QA follow-up) Semantics, split fields and options

This patch improves the semantics of the update routine to more clearly
separate the 'fields' we're modifying from the 'options' we wish to
apply to the modification.
Comment 26 Victor Grousset/tuxayo 2020-05-03 21:02:20 UTC
Created attachment 104232 [details] [review]
Bug 23185: Add tests

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Victor Grousset/tuxayo 2020-05-03 21:02:25 UTC
Created attachment 104233 [details] [review]
Bug 23185: Make Koha::Objects->update loop on the object set if needed

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2020-05-03 21:02:31 UTC
Created attachment 104234 [details] [review]
Bug 23185: (QA follow-up) Semantics, split fields and options

This patch improves the semantics of the update routine to more clearly
separate the 'fields' we're modifying from the 'options' we wish to
apply to the modification.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Victor Grousset/tuxayo 2020-05-03 21:04:41 UTC
Works! Signed off. Assuming the test plan was only to run the tests.
Comment 30 Jonathan Druart 2020-05-04 09:36:42 UTC
(In reply to Victor Grousset/tuxayo from comment #29)
> Works! Signed off. Assuming the test plan was only to run the tests.

Thanks Victor.

When it's a "structural change", and especially if there is no test plan or test plan is simply "run the tests", then it means it's always better to have a look at the code, understand it, and confirm you agree with it.
Comment 31 Victor Grousset/tuxayo 2020-05-04 12:22:26 UTC
(In reply to Jonathan Druart from comment #30)
> When it's a "structural change", and especially if there is no test plan or
> test plan is simply "run the tests", then it means it's always better to
> have a look at the code, understand it, and confirm you agree with it.

Thanks, noted.
I can't competently agree with much changes in general but experience will change that :)
Comment 32 Tomás Cohen Arazi 2020-05-04 12:47:18 UTC
I've been following this and the outcome is really good.
Everything looks correct including the QA scripts.
Comment 33 Tomás Cohen Arazi 2020-05-04 12:49:15 UTC
Created attachment 104271 [details] [review]
Bug 23185: Add tests

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2020-05-04 12:49:37 UTC
Created attachment 104272 [details] [review]
Bug 23185: Make Koha::Objects->update loop on the object set if needed

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi 2020-05-04 12:49:59 UTC
Created attachment 104273 [details] [review]
Bug 23185: (QA follow-up) Semantics, split fields and options

This patch improves the semantics of the update routine to more clearly
separate the 'fields' we're modifying from the 'options' we wish to
apply to the modification.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Tomás Cohen Arazi 2020-05-04 12:55:02 UTC
Created attachment 104276 [details] [review]
Bug 23185: (QA follow-up) Add POD for Koha::Objects->update

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Martin Renvoize 2020-05-05 10:04:24 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 38 Tomás Cohen Arazi 2020-05-07 21:30:16 UTC
Created attachment 104537 [details] [review]
Bug 23185: ->update can only be called on instantiated classes

Since our use ok Koha::Objects, there's been an implicit instantiation
happening in ->search that has somehow hidden the fact that classes need
instantiation...

This change in ->delete and ->update made it clear because of the
failures/hangs on the tests.

This patch instantiates Koha::Patron::Categories before attempting to
call ->update.

The original patches for this bug are correct and the only issue here is
that we need to document better than for some methods, instantiation is
not implicit and is required first. This is awkward and I would prefer
to force everyone to call ->new first. But I understand it would be
inconvenient.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Template/Plugin/Categories.t
=> FAIL: Notice it hangs, Ctrl+c to abort
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests dong hang, and in fact pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2020-05-07 21:30:23 UTC
Created attachment 104538 [details] [review]
Bug 23185: Realistic POD for Koha::Objects->search

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2020-05-08 00:01:20 UTC
Created attachment 104541 [details] [review]
Bug 23185: (follow-up) K:C:Register->make_default should use no_triggers

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2020-05-08 00:03:21 UTC
I leave the last patch for you to review, Martin and Jonathan. I took the liberty to push the others as they were breaking Jenkins.
Comment 42 Jonathan Druart 2020-05-08 09:43:06 UTC
Tomas, I feel like we should explode if Koha::Objects->update is called: test ref($self) at the beginning. What do you think?
Comment 43 Tomás Cohen Arazi 2020-05-08 10:27:35 UTC
(In reply to Jonathan Druart from comment #42)
> Tomas, I feel like we should explode if Koha::Objects->update is called:
> test ref($self) at the beginning. What do you think?

I agree.
Comment 44 Tomás Cohen Arazi 2020-05-08 10:52:03 UTC
(In reply to Tomás Cohen Arazi from comment #41)
> I leave the last patch for you to review, Martin and Jonathan. I took the
> liberty to push the others as they were breaking Jenkins.

Do we agree on that patch?
Comment 45 Jonathan Druart 2020-05-08 13:43:22 UTC
Yes, pushed to master for 20.05
Comment 46 Joy Nelson 2020-05-11 20:52:51 UTC
missing dependencies  not backported to 19.11.x