Bug 21684 - Koha::Object[s]->delete methods must behave identically as the corresponding DBIx::Class ones
Summary: Koha::Object[s]->delete methods must behave identically as the corresponding ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 17669 24150
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-26 13:56 UTC by Jonathan Druart
Modified: 2020-01-18 15:09 UTC (History)
6 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 21684: (QA follow-up) On the road to a perfect delete (4.39 KB, patch)
2018-10-26 16:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Add auto_savepoint to DBIC connection (1018 bytes, patch)
2018-10-26 16:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Add tests to show some of the discrepancies (4.13 KB, patch)
2018-10-26 16:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Add tests to show some of the discrepancies (3.97 KB, patch)
2019-06-22 23:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Fix delete methods and add more tests (19.83 KB, patch)
2019-12-12 13:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Adjust some tests (5.47 KB, patch)
2019-12-13 15:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Add tests to show some of the discrepancies (4.03 KB, patch)
2019-12-13 16:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21684: Fix delete methods and add more tests (19.88 KB, patch)
2019-12-13 16:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21684: Adjust some tests (5.53 KB, patch)
2019-12-13 16:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21684: Add tests to show some of the discrepancies (4.08 KB, patch)
2019-12-18 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21684: Fix delete methods and add more tests (19.94 KB, patch)
2019-12-18 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21684: Adjust some tests (5.59 KB, patch)
2019-12-18 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21684: Fix UploadedFile[s]->delete (4.39 KB, patch)
2020-01-07 11:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Koha::Object->delete must return a Koha::Object (3.43 KB, patch)
2020-01-07 12:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: (follow-up) Fix UploadedFile[s]->delete (2.19 KB, patch)
2020-01-07 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21684: Fix Hold.t (1.51 KB, patch)
2020-01-07 15:07 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 2018-10-26 13:56:32 UTC
for instance:

  Koha::Cities->find(1)->delete
and
  $schema->resultset('City')->delete
must returns the same value, as well as throw an exception if needed.

Same for Koha::Objects->delete

This is the result of a discussion started on bug 21337.
Comment 1 Jonathan Druart 2018-10-26 16:57:15 UTC
Created attachment 81317 [details] [review]
Bug 21684: (QA follow-up) On the road to a perfect delete

Adjusting Koha::Patrons->delete:
We should not throw an exception if we get a -1 from Patron->delete.
A zero should trigger an exception. And an undef too since it is probably
a DBIx exception..

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Jonathan Druart 2018-10-26 16:57:19 UTC
Created attachment 81318 [details] [review]
Bug 21684: Add auto_savepoint to DBIC connection

https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/Cookbook.pod#Nested-transactions-and-auto-savepoints

Otherwise:
DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274

Possible side-effects? Slowness?
Comment 3 Jonathan Druart 2018-10-26 16:57:23 UTC
Created attachment 81319 [details] [review]
Bug 21684: Add tests to show some of the discrepancies
Comment 4 Jonathan Druart 2018-10-26 16:59:50 UTC
I think we should remove our specific code from Koha::Object->delete, but that will imply we will need to adjust callers code.

I am in favour of such move but I would like people to give their opinion.

It seems to be too big (side-effects) to make it part of 18.11 anyway.
Comment 5 Marcel de Rooy 2018-10-29 09:40:08 UTC
(In reply to Jonathan Druart from comment #2)
> Created attachment 81318 [details] [review] [review]
> Bug 21684: Add auto_savepoint to DBIC connection
> 
> https://metacpan.org/pod/distribution/DBIx-Class/lib/DBIx/Class/Manual/
> Cookbook.pod#Nested-transactions-and-auto-savepoints
> 
> Otherwise:
> DBIx::Class::Storage::txn_rollback(): A txn_rollback in nested transaction
> is ineffective! (depth 1) at t/db_dependent/Koha/Objects.t line 274
> 
> Possible side-effects? Slowness?

Would be nice to have, and has support in InnoDB. We should be sure about performance though.
Comment 6 Jonathan Druart 2019-06-22 23:51:49 UTC
Created attachment 90937 [details] [review]
Bug 21684: Add tests to show some of the discrepancies
Comment 7 Jonathan Druart 2019-06-22 23:54:00 UTC
(In reply to Jonathan Druart from comment #4)
> I am in favour of such move but I would like people to give their opinion.

Is silence acquiescence?
Comment 8 Jonathan Druart 2019-12-12 13:28:36 UTC
Created attachment 96218 [details] [review]
Bug 21684: Fix delete methods and add more tests
Comment 9 Jonathan Druart 2019-12-12 13:29:24 UTC
I think we should follow this path.
If you all agree I will provide a follow-up to fix the different calls.
Comment 10 Marcel de Rooy 2019-12-13 07:04:00 UTC
Distracted by:
warn "Deleted $count patrons\n" if $params->{verbose};
in Koha/Patrons.pm ;)
Comment 11 Marcel de Rooy 2019-12-13 07:10:00 UTC
Not sure if this works:
+                ok( ref($r_us) && ref($r_them),
+                    'Successful delete should return the object ' );

Note that it depends on the storage backend:
"The return value is a pass through of what the underlying storage backend returned, and may vary."
Comment 12 Martin Renvoize 2019-12-13 09:15:28 UTC
I'm on board with this one.. we should be consistent in our handling of object return values.

For consistency with DBIx::Class (which underpins our objects) we should stick to exceptions for failure cases.

If you are able to continue this work Jonathan, that would be great :)
Comment 13 Martin Renvoize 2019-12-13 09:17:34 UTC
(In reply to Marcel de Rooy from comment #11)
> Not sure if this works:
> +                ok( ref($r_us) && ref($r_them),
> +                    'Successful delete should return the object ' );
> 
> Note that it depends on the storage backend:
> "The return value is a pass through of what the underlying storage backend
> returned, and may vary."

Hmm, well at the present time we only support MySQL and MariaDB as our backends and I believe those are consistent with one another in this regard.
Comment 14 Jonathan Druart 2019-12-13 15:02:25 UTC
Created attachment 96254 [details] [review]
Bug 21684: Adjust some tests
Comment 15 Jonathan Druart 2019-12-13 15:03:02 UTC
Actually it seems that the only occurrence is:members/deletemem.pl
but actually it is an improvement, so could be done on a separate bug report

I did not find any others delete statement where the return value where useful, but I may be wrong here!
Comment 16 Nick Clemens 2019-12-13 16:53:06 UTC
Created attachment 96270 [details] [review]
Bug 21684: Add tests to show some of the discrepancies

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2019-12-13 16:53:09 UTC
Created attachment 96271 [details] [review]
Bug 21684: Fix delete methods and add more tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2019-12-13 16:53:11 UTC
Created attachment 96272 [details] [review]
Bug 21684: Adjust some tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Martin Renvoize 2019-12-18 16:22:28 UTC
Created attachment 96429 [details] [review]
Bug 21684: Add tests to show some of the discrepancies

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2019-12-18 16:22:31 UTC
Created attachment 96430 [details] [review]
Bug 21684: Fix delete methods and add more tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2019-12-18 16:22:34 UTC
Created attachment 96431 [details] [review]
Bug 21684: Adjust some tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-12-18 16:23:00 UTC
Thanks Jonathan, Passing QA
Comment 23 Martin Renvoize 2020-01-03 11:01:51 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 24 Martin Renvoize 2020-01-03 12:56:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 25 Jonathan Druart 2020-01-07 10:51:38 UTC
#   Failed test 'Test delete via UploadedFile as well as UploadedFiles'
#   at t/db_dependent/Upload.t line 193.
DBIx::Class::Row::delete(): Not in database at /kohadevbox/koha/Koha/Object.pm line 219

Failing test, the previous test was expecting 0E0 if the delete failed, now it raises an exception. I think we can simply adjust the test accordingly.
Comment 26 Jonathan Druart 2020-01-07 11:12:37 UTC
Created attachment 96872 [details] [review]
Bug 21684: Fix UploadedFile[s]->delete

Tests were failing with:
 #   Failed test 'Test delete via UploadedFile as well as UploadedFiles'
 #   at t/db_dependent/Upload.t line 193.
 DBIx::Class::Row::delete(): Not in database at /kohadevbox/koha/Koha/Object.pm line 219

I am not sure this patch is perfect, a set of uploaded files should be
deleted in a transaction, which would be rollback if something is wrong.
But it will be tricky to restore the files after they have been deleted.
It seems that we should deal with that with a more complicated process
and should be part of a separate bug.
Comment 27 Jonathan Druart 2020-01-07 11:13:02 UTC
Marcel, could I get your opinion on this follow-up?
Comment 28 Jonathan Druart 2020-01-07 12:31:40 UTC
Created attachment 96888 [details] [review]
Bug 21684: Koha::Object->delete must return a Koha::Object

Not a DBIx::Class object.

Tests have been added, add Bookseller.t fixed
Comment 29 Marcel de Rooy 2020-01-07 13:28:31 UTC
(In reply to Jonathan Druart from comment #25)
> #   Failed test 'Test delete via UploadedFile as well as UploadedFiles'
> #   at t/db_dependent/Upload.t line 193.
> DBIx::Class::Row::delete(): Not in database at
> /kohadevbox/koha/Koha/Object.pm line 219
> 
> Failing test, the previous test was expecting 0E0 if the delete failed, now
> it raises an exception. I think we can simply adjust the test accordingly.

Yes, we should bring that in line with the changed delete subs. The subroutines in UploadedFile[s] also need attention..
Comment 30 Marcel de Rooy 2020-01-07 13:29:34 UTC
(In reply to Jonathan Druart from comment #27)
> Marcel, could I get your opinion on this follow-up?

Looking now
Comment 31 Marcel de Rooy 2020-01-07 13:42:15 UTC
How do we view this line now in both modules?
    return $rv==0 ? "0E0" : $rv;
Also in delete_missing.
The Upload.t test also refers to -1 values. Are they still possible? At first glance the test should still be adjusted a bit more, although it passes now.

Second patch
+    my $deleted = $self->_result()->delete;
+    if ( ref $deleted ) {
+        my $object_class  = Koha::Object::_get_object_class( $self->_result->result_class );
+        $deleted = $object_class->_new_from_dbic($deleted);
+    }
+    return $deleted;
Isnt this a bit expensive to do since we will generally not even look at what we get back?
Most often $obj->delete is not even tested.
Parameterize the return??
Comment 32 Jonathan Druart 2020-01-07 14:24:44 UTC
Created attachment 96895 [details] [review]
Bug 21684: (follow-up) Fix UploadedFile[s]->delete

This can be squashed
Comment 33 Jonathan Druart 2020-01-07 14:27:40 UTC
(In reply to Marcel de Rooy from comment #31)
> How do we view this line now in both modules?
>     return $rv==0 ? "0E0" : $rv;
> Also in delete_missing.

I do not understand exactly what you mean.

> The Upload.t test also refers to -1 values. Are they still possible? At
> first glance the test should still be adjusted a bit more, although it
> passes now.

Indeed, done.

> Second patch
> +    my $deleted = $self->_result()->delete;
> +    if ( ref $deleted ) {
> +        my $object_class  = Koha::Object::_get_object_class(
> $self->_result->result_class );
> +        $deleted = $object_class->_new_from_dbic($deleted);
> +    }
> +    return $deleted;
> Isnt this a bit expensive to do since we will generally not even look at
> what we get back?

I do not think it's expensive, the _new_from_dbic will do nothing if the object is not reused after ->delete. So only _get_object_class will be effective.

> Most often $obj->delete is not even tested.

It is widely tested in Objects.t

> Parameterize the return??

Not a good idea IMO. We must behave like DBIx::Class, it's the whole point of this patchset.
Comment 34 Marcel de Rooy 2020-01-07 14:39:21 UTC
(In reply to Jonathan Druart from comment #33)
> I do not understand exactly what you mean.
Short version: Get rid of 0E0 ?

> I do not think it's expensive, the _new_from_dbic will do nothing if the
> object is not reused after ->delete. So only _get_object_class will be
> effective.
Sounds good to me.
Comment 35 Jonathan Druart 2020-01-07 14:57:45 UTC
(In reply to Marcel de Rooy from comment #34)
> (In reply to Jonathan Druart from comment #33)
> > I do not understand exactly what you mean.
> Short version: Get rid of 0E0 ?

How? It's the expected behavior (DBI return value).
Comment 36 Jonathan Druart 2020-01-07 14:58:56 UTC
As I wrote in the commit message, IMO the whole thing needs a rewrite, to deal with deleting a set in a transaction.
Comment 37 Marcel de Rooy 2020-01-07 15:02:56 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Marcel de Rooy from comment #34)
> > (In reply to Jonathan Druart from comment #33)
> > > I do not understand exactly what you mean.
> > Short version: Get rid of 0E0 ?
> 
> How? It's the expected behavior (DBI return value).

Theoretically it might not be DBI. And DBIx depends on the backend. But we can leave it for now; much more of this kind.
Comment 38 Jonathan Druart 2020-01-07 15:07:30 UTC
Created attachment 96898 [details] [review]
Bug 21684: Fix Hold.t
Comment 39 Martin Renvoize 2020-01-07 16:34:41 UTC
4 followups pushed.. code all makes sense to me and resolves the failures.
Comment 40 Martin Renvoize 2020-01-07 17:03:30 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 41 Joy Nelson 2020-01-18 15:09:42 UTC
Not backported to 19.11.x because 24150 cannot be backported.