Bug 25303 - Koha::Objects->delete should not skip overridden object class ->delete
Summary: Koha::Objects->delete should not skip overridden object class ->delete
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-04-28 12:53 UTC by Jonathan Druart
Modified: 2020-05-08 22:42 UTC (History)
3 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 25303: Make Koha::Objects->delete loop on the object set (2.36 KB, patch)
2020-04-28 12:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25303: Make Koha::Objects->delete loop on the object set (2.43 KB, patch)
2020-04-28 13:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25303: Unit tests (1.63 KB, patch)
2020-04-28 14:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25303: Make Koha::Objects->delete loop on the object set (2.48 KB, patch)
2020-04-28 14:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25303: Effectively delete the city to avoid endless loop (724 bytes, patch)
2020-04-29 13:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25303: (QA follow-up) Fix test construction (3.66 KB, patch)
2020-04-29 21:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25303: Call ->reset before iterate on the set (923 bytes, patch)
2020-04-30 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25303: Add a test for ->reset (953 bytes, patch)
2020-04-30 12:16 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-04-28 12:53:17 UTC
If we call Koha::Libraries->delete but Koha::Library->delete exists (ie. Koha::Object->delete is overriden), then we Koha::Objects->delete will be called and the overriden will not be executed.
Comment 1 Jonathan Druart 2020-04-28 12:56:58 UTC
Created attachment 103855 [details] [review]
Bug 25303: Make Koha::Objects->delete loop on the object set

If we call Koha::Libraries->delete but Koha::Library->delete exists
(ie. Koha::Object->delete is overridden), then we Koha::Objects->delete
will be called and the overridden will not be executed.

This patch suggests to test if the method is overridden (using
Class::Inspector->function_exists). If so we loop on the different
objects of the set in a transaction and call the overridden ->delete method.

Existing tests widely cover this change.
t/db_dependent/Koha/Objects.t
    subtest 'Return same values as DBIx::Class' => sub {
Comment 2 Martin Renvoize 2020-04-28 13:12:19 UTC
This is more similar to bug 23185 than bug 21761.

For performance reasons I'm not so sure about always forcing a loop rather than allowing a fast path.. I think I would rather see checks for the corresponding 'delete' method in the Koha::Things class whenever we detect that a Koha::Thing has had a delete method added or updated.

That way, we only require the loop to take place for Koha::Objects where an in-code delete trigger has indeed been added.
Comment 3 Tomás Cohen Arazi 2020-04-28 13:20:06 UTC
It all comes down to the DB triggers discussion...
I agree we could enforce people overriding Koha::Thing->delete to take care of implementing and overriden Koha::Things->delete that is conformant to the expected behaviour.
Comment 4 Martin Renvoize 2020-04-28 13:38:01 UTC
Created attachment 103859 [details] [review]
Bug 25303: Make Koha::Objects->delete loop on the object set

If we call Koha::Libraries->delete but Koha::Library->delete exists
(ie. Koha::Object->delete is overridden), then we Koha::Objects->delete
will be called and the overridden will not be executed.

This patch suggests to test if the method is overridden (using
Class::Inspector->function_exists). If so we loop on the different
objects of the set in a transaction and call the overridden ->delete method.

Existing tests widely cover this change.
t/db_dependent/Koha/Objects.t
    subtest 'Return same values as DBIx::Class' => sub {

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2020-04-28 13:38:59 UTC
Having discussed this I see Tomas and I both missed the Class::Inspector use.. happy to signoff now I've confirmed this works as I wanted.
Comment 6 Tomás Cohen Arazi 2020-04-28 14:46:32 UTC
Created attachment 103864 [details] [review]
Bug 25303: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2020-04-28 14:46:37 UTC
Created attachment 103865 [details] [review]
Bug 25303: Make Koha::Objects->delete loop on the object set

If we call Koha::Libraries->delete but Koha::Library->delete exists
(ie. Koha::Object->delete is overridden), then we Koha::Objects->delete
will be called and the overridden will not be executed.

This patch suggests to test if the method is overridden (using
Class::Inspector->function_exists). If so we loop on the different
objects of the set in a transaction and call the overridden ->delete method.

Existing tests widely cover this change.
t/db_dependent/Koha/Objects.t
    subtest 'Return same values as DBIx::Class' => sub {

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 8 Jonathan Druart 2020-04-29 13:43:02 UTC
Don't push this, the tests never finish (infinite loop).
Comment 9 Jonathan Druart 2020-04-29 13:47:48 UTC
Created attachment 103941 [details] [review]
Bug 25303: Effectively delete the city to avoid endless loop
Comment 10 Tomás Cohen Arazi 2020-04-29 14:03:12 UTC
(In reply to Jonathan Druart from comment #8)
> Don't push this, the tests never finish (infinite loop).

Good catch, I wonder why it passed for me.
Comment 11 Martin Renvoize 2020-04-29 18:06:33 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Tomás Cohen Arazi 2020-04-29 21:49:02 UTC
Created attachment 103998 [details] [review]
Bug 25303: (QA follow-up) Fix test construction

The test called ->delete on the resultset directly, which is not the
same when we already called ->next and the Koha::Hold->delete won't be
called as there's no ->next hold.

I took the oportunity to wrap the subtest in its own transaction and
build its own data to avoid interference between tests...

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2020-04-29 21:51:15 UTC
@RM, the follow-up fixes the failing tests in master. It might be relevant to document the behaviour change, whlst I think it was just a bad construction of the test in this case.
Comment 14 Jonathan Druart 2020-04-30 08:23:12 UTC
This follow-up is doing this:

-    $holds->delete;
+    $biblio->holds->delete;

I was not expected such behaviour changes, it's not as easy as fixing the tests. Did you check the occurrences in the code?

Maybe we should call ->reset before the loop instead?

@ Koha/Objects.pm:185 @ sub delete {
     if ( Class::Inspector->function_exists( $self->object_class, 'delete' ) ) {
         my $objects_deleted;
         $self->_resultset->result_source->schema->txn_do( sub {
+            $self->reset;
             while ( my $o = $self->next ) {
                 $o->delete;
                 $objects_deleted++;
Comment 15 Tomás Cohen Arazi 2020-04-30 10:58:03 UTC
(In reply to Jonathan Druart from comment #14)
> This follow-up is doing this:
> 
> -    $holds->delete;
> +    $biblio->holds->delete;
> 
> I was not expected such behaviour changes, it's not as easy as fixing the
> tests. Did you check the occurrences in the code?

I haven't. But that's why I highlighted the behavior change. I'm sure there wouldn't be placed in which we do the same we did on those tests.

> Maybe we should call ->reset before the loop instead?

I think we should. We should be consistent with DBIC behaviour and the code change you propose looks correct. The good thing is that you have a good schema for the regression tests that are needed.

> 
> @ Koha/Objects.pm:185 @ sub delete {
>      if ( Class::Inspector->function_exists( $self->object_class, 'delete' )
> ) {
>          my $objects_deleted;
>          $self->_resultset->result_source->schema->txn_do( sub {
> +            $self->reset;
>              while ( my $o = $self->next ) {
>                  $o->delete;
>                  $objects_deleted++;
Comment 16 Jonathan Druart 2020-04-30 11:26:07 UTC
Created attachment 104018 [details] [review]
Bug 25303: Call ->reset before iterate on the set

In case we already started to iterate.

This fixes a failing tests in t/db_dependent/Koha/Biblios.t
Comment 17 Tomás Cohen Arazi 2020-04-30 11:27:13 UTC
(In reply to Jonathan Druart from comment #16)
> Created attachment 104018 [details] [review] [review]
> Bug 25303: Call ->reset before iterate on the set
> 
> In case we already started to iterate.
> 
> This fixes a failing tests in t/db_dependent/Koha/Biblios.t

+10
Comment 18 Tomás Cohen Arazi 2020-04-30 11:29:12 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> (In reply to Jonathan Druart from comment #16)
> > Created attachment 104018 [details] [review] [review] [review]
> > Bug 25303: Call ->reset before iterate on the set
> > 
> > In case we already started to iterate.
> > 
> > This fixes a failing tests in t/db_dependent/Koha/Biblios.t
> 
> +10

Please add tests on Objects.t
Comment 19 Jonathan Druart 2020-04-30 12:16:05 UTC
Created attachment 104027 [details] [review]
Bug 25303: Add a test for ->reset
Comment 20 Martin Renvoize 2020-05-01 07:08:36 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-05-08 22:42:45 UTC
not backported to 19.11