+++ 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.
Note the 's' in the bug title.. this is not the same as 'Koha::Object'
See further discussion in bug 23070
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.
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.
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 ?
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.
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' });
Is adding direct_update not adding the side step that you wanted to catch ?
(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.
Created attachment 103867 [details] [review] Bug 23185: Add tests
Created attachment 103868 [details] [review] Bug 23185: Make Koha::Objects->update loop on the object set if needed
Does this make sense?
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
I passes for me, in ~30sec
It passes for me, in ~30sec
(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
Ho right, that's the tests from bug 25303. Something weird is going on here.
(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.
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
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 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};
Generally.. I really like this however :)
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>
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>
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.
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>
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>
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>
Works! Signed off. Assuming the test plan was only to run the tests.
(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.
(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 :)
I've been following this and the outcome is really good. Everything looks correct including the QA scripts.
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>
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>
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>
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>
Nice work everyone! Pushed to master for 20.05
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>
Created attachment 104538 [details] [review] Bug 23185: Realistic POD for Koha::Objects->search Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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.
Tomas, I feel like we should explode if Koha::Objects->update is called: test ref($self) at the beginning. What do you think?
(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.
(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?
Yes, pushed to master for 20.05
missing dependencies not backported to 19.11.x