Bug 21761 - Koha::Object supports passing through 'update' which means we can side step 'set' + 'store'
Summary: Koha::Object supports passing through 'update' which means we can side step '...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high critical (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 23185 23187
  Show dependency treegraph
 
Reported: 2018-11-05 11:43 UTC by Martin Renvoize
Modified: 2020-04-28 12:57 UTC (History)
8 users (show)

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


Attachments
Bug 21761: Try to overload Koha::Object->update to handle incorrect values (6.75 KB, patch)
2019-03-04 17:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21761: Call set + store as update (1.13 KB, patch)
2019-06-21 15:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21761: Call set + store as update (1.20 KB, patch)
2019-12-03 15:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21761: Do not call $self->store in ->set_password (976 bytes, patch)
2019-12-10 09:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21761: Call set + store as update (1.32 KB, patch)
2019-12-10 13:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21761: Do not call $self->store in ->set_password (1.05 KB, patch)
2019-12-10 13:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2018-11-05 11:43:45 UTC
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.
Comment 1 Jonathan Druart 2018-11-06 15:44:02 UTC
Indeed, I think we should fix it at Koha::Object level (once again), but on top of bug 21610.
We could move the handle of default values to a specific (private) method and call it with get_dirty_columns on ->store, ->update or ->set.
Comment 2 Marcel de Rooy 2018-11-06 15:47:48 UTC
We should replace the remaining DBIx code fragments scattered here and there..
Comment 3 Tomás Cohen Arazi 2019-02-01 11:34:26 UTC
I agree we should either remove it from AUTOLOAD or write our own as a wrapper on top of the class's ->store.
Comment 4 Jonathan Druart 2019-03-04 17:00:15 UTC
Created attachment 86017 [details] [review]
Bug 21761: Try to overload Koha::Object->update to handle incorrect values

And use default values instead.

This is a terrible patch, but publish it to help others having their
try.
Comment 5 Martin Renvoize 2019-06-21 15:48:48 UTC
Created attachment 90914 [details] [review]
Bug 21761: Call set + store as update
Comment 6 Martin Renvoize 2019-06-21 15:49:58 UTC
Sorry Jonathan, I hadn't spotted you'd already written a patch here.. but aren't you over complicating it?

I've submitted an alternative patch which I think does the same but is considerably simpler...  catch me if I'm incorrect please.
Comment 7 Jonathan Druart 2019-06-22 21:24:34 UTC
Apply the changes in the tests, then run it. You will get a warning with your patch:

t/db_dependent/Koha/Object.t .. 7/11 Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018.
Comment 8 Jonathan Druart 2019-06-22 21:33:19 UTC
Also, I think, my version will be faster: the fix is done only the attributes we are updating, not all the attributes of the object.
Comment 9 Martin Renvoize 2019-06-24 07:31:49 UTC
(In reply to Jonathan Druart from comment #7)
> Apply the changes in the tests, then run it. You will get a warning with
> your patch:
> 
> t/db_dependent/Koha/Object.t .. 7/11 Argument "" isn't numeric in numeric eq
> (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018.

Surely that highlights a bug in the current 'store' routine then rather than my patch ;)
Comment 10 Martin Renvoize 2019-06-24 07:34:10 UTC
(In reply to Jonathan Druart from comment #8)
> Also, I think, my version will be faster: the fix is done only the
> attributes we are updating, not all the attributes of the object.

Again, I would suggest this is a bug in our store routine as it stands.. update really should just be shortcut to set + store else we're introducing bugs by having two distinct ways of doing the same thing.

Also, in my patch because you're calling the parent store method you're not only dealing with the over-rides withs Koha::Object but also any class specific overrides in, for example, Koha::Patron are you not?
Comment 11 Jonathan Druart 2019-06-25 22:52:42 UTC
(In reply to Martin Renvoize from comment #10)
> (In reply to Jonathan Druart from comment #8)
> > Also, I think, my version will be faster: the fix is done only the
> > attributes we are updating, not all the attributes of the object.
> 
> Again, I would suggest this is a bug in our store routine as it stands..
> update really should just be shortcut to set + store else we're introducing
> bugs by having two distinct ways of doing the same thing.

Ok! :)

> Also, in my patch because you're calling the parent store method you're not
> only dealing with the over-rides withs Koha::Object but also any class
> specific overrides in, for example, Koha::Patron are you not?

I do not get what you mean.


I think we should investigate the warning and provide tests.
Comment 12 Jonathan Druart 2019-06-25 22:54:00 UTC
To investigate:

+    $itemtype = $builder->build_object( { class => 'Koha::ItemTypes' });
+    $itemtype->update({ notforloan => "" });

These two lines will warn "Argument "" isn't numeric in numeric eq (==)"
Comment 13 Marcel de Rooy 2019-11-22 08:15:57 UTC
I was referred here again from report 23260 where I used an update on a Koha object without store. (Table items_last_borrower just 4 fields.)
Can we get this "discussion" further?

As stated in comment1 I think we need some hook subroutine on store, update etc.
If there is no 'hook', could we still do some faster updates than looping with store and next?
Comment 14 Jonathan Druart 2019-12-03 15:03:35 UTC
Created attachment 95947 [details] [review]
Bug 21761: Call set + store as update

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2019-12-03 15:06:50 UTC
(In reply to Jonathan Druart from comment #12)
> To investigate:
> 
> +    $itemtype = $builder->build_object( { class => 'Koha::ItemTypes' });
> +    $itemtype->update({ notforloan => "" });
> 
> These two lines will warn "Argument "" isn't numeric in numeric eq (==)"

I did not manage to remove this warning. However it highlights a misuse: setting a string to an integer field. As we are going to deal in ->store I think that's fine.
Comment 16 Marcel de Rooy 2019-12-06 07:21:28 UTC
Looking here
Comment 17 Marcel de Rooy 2019-12-06 09:12:20 UTC
Patrons.t fails on set_password

set_password contains an update (which was formerly DBIx, and now no longer), so it is now routed back to sub store which starts another txn_do
store contains a get_from_storage for the password, and here we end up in some transaction problem with wrong data
Comment 18 Jonathan Druart 2019-12-10 09:51:37 UTC
Created attachment 96136 [details] [review]
Bug 21761: Do not call $self->store in ->set_password
Comment 19 Joonas Kylmälä 2019-12-10 09:57:29 UTC
The latest patch is not signed-off yet so marking this as "Needs Signoff".
Comment 20 Jonathan Druart 2019-12-10 10:31:58 UTC
(In reply to Joonas Kylmälä from comment #19)
> The latest patch is not signed-off yet so marking this as "Needs Signoff".

When the first patch has been already signed off we do not necessarily ask for a second signoff.
In this case I am expecting a ping pong with Marcel and so do not need to involve another tester. However another eyeball/thoughts from QA is more than welcome :)
Comment 21 Marcel de Rooy 2019-12-10 13:20:20 UTC
(In reply to Joonas Kylmälä from comment #19)
> The latest patch is not signed-off yet so marking this as "Needs Signoff".

Hi Joonas: Please check the assigned QA contact and if there was recent activity as was here. In that case you can pick another?
Comment 22 Joonas Kylmälä 2019-12-10 13:27:33 UTC
(In reply to Marcel de Rooy from comment #21)
> Hi Joonas: Please check the assigned QA contact and if there was recent
> activity as was here. In that case you can pick another?

Thanks, I didn't realize I could check that.
Comment 23 Marcel de Rooy 2019-12-10 13:33:37 UTC
Created attachment 96154 [details] [review]
Bug 21761: Call set + store as update

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Removed a double comma.
Comment 24 Marcel de Rooy 2019-12-10 13:33:42 UTC
Created attachment 96155 [details] [review]
Bug 21761: Do not call $self->store in ->set_password

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Martin Renvoize 2019-12-10 16:10:22 UTC
Really glad to see this one move, thanks to all involved... The code is looking polished and good now, Pushing.
Comment 26 Martin Renvoize 2019-12-10 16:18:15 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Joy Nelson 2019-12-20 15:51:18 UTC
Not backported to 19.11.x