cascaded delete must delete relationships first

[gbarr]

Attempting to delete a row before deleting rows
in other tables that reference it will fail with
a constraint error

[abraxxa]

I think that order is intentionally to give the database the possibility to delete the related rows itself with a contraint that has cascaded delete enabled.
Which RDBMS are yoiu using when you encounter this error? You should provide failing test case too, please.

[gbarr]

I am using MySQL with InnoDB tables where the foreign keys are not defined with any cascade option

If you are expecting the DB to to cascade, the why attempt manually with cascade deletes ?

If the DB does do automatic cascades, then putting the delete after the manual ones, while less efficient, will still work and efficiency can be restored by setting cascade_delete to false in the relationship definition

[gbarr]

I added a test case to this branch. I was able to reproduce the issue with SQLite by turning on the foreign_keys pragma

[ribasushi]

This behavior has been there from the start by design, and at this point it is unrealistic to change:
https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.08250/lib/DBIx/Class/Relationship/Base.pm#L336

To answer your other (rather valid question) "If you are expecting the DB to to cascade, the why attempt manually with cascade deletes ?". At the time CDBI/DBIC were written (close to a decade ago), there was a lot less access to reliable foreign constraints. As such the ability to "post-cleanup" was seen as a much needed feature, and was therefore enabled by default.

The only thing I can offer you at this point is to aid in an effort of deprecating having any DBIC-side cascades by default. The patch attached to this pullreq can not be accepted though - it breaks years of expected behavior and is therefore a non-negotiable "can't do". I am therefore closing this pullreq.

Cheers

[gbarr]

How about a patch that decides on pre/post delete of the relationships based on the value of cascade_delete

1 does as it does now, ie cascade delete after, but -1 will cause the cascade of that relationship before the delete. Or perhaps a second attribute to control before/after ?

[abraxxa]

I can also think if another config value that doesn't try to cascade delete at all if the RDBMS supports cascade delete and is enabled for a rel.

[gbarr]

@abraxxa that is already supported with cascade_delete=0 in the attributes.

The issue is when cascade_delete is turned on, which is the default, then do you delete the row before or after the related rows. Currently it deletes the row first which fails if the RDBMS is not cascading itself and has foreign key checking enabled (which SQLite does not by default)

[abraxxa]

Wouldn't that also disable the RDBMS feature on deploy?

[gbarr]

@abraxxa you are right, I was forgetting about deploy.

So, how about a new attribute cascade_delete_before

If it does not exist, or is undefined, then things stay as they are. If it defined then the cascade code would be run before the delete of the main row, but only if it is true.

So cascade_delete=1 and cascade_delete_before=0 would depend completely on the RDBMS doing the cascade and cascade_delete=1 and cascade_delete_before=1 would delete the relationships before deleting the row

With this existing code would be completely unaffected and the new attribute will control where and if the code attempts the cascade the delete

[ribasushi]

So cascade_delete=1 and cascade_delete_before=0 would depend completely
on the RDBMS doing the cascade and cascade_delete=1 and cascade_delete_before=1
would delete the relationships before deleting the row

Please step back and consider the horror of such an API addition.

In order to move forward on your original issue - I need to understand your use-case better. Could you please elaborate on that? Alternatively - if you are going to Austin the week after - we can table the discussion and figure out a way forward in vivo

[dbsrgits-sync]

On Thu, May 23, 2013 at 06:29:40AM -0700, Alexander Hartmaier wrote:

I can also think if another config value that doesn't try to cascade
delete at all if the RDBMS supports cascade delete and is enabled for
a rel.

Your statement implies it is a piece of cake to determine whether a
RDBMS supports and has declared some sort of cascading on a particular
"relationship" as DBIC understands them.

I hope on second thought you'll realize this is light years away from
reality.

[abraxxa]

What I imagined is which RDBMS supports cascading could be included in the Storage classes, no autodetection.
If cascading in the DB is declared, the rel config should match this and should be taken into account for deploy.
I don't say it's something I need/want.

What would make up a feature for me is to be able to control if cascade delete is handled by DBIC before as @gbarr suggested to trigger delete method modifiers of related result classes, e.g.
Device->has_many('Interface')->belongs_to('Line')
Interface has a delete around method modifier that checks if it was the last interface of its related line, if so deletes the line after being deleted.
This interface method modifier isn't trigger if one deletes a whole device. I had to add a method modifier to the device delete method to loop through the related interfaces and delete them.

[gbarr]

@ribasushi the use case is that the RDBMS enforces foreign key checking, but the schema is defined with ON DELETE RESTRICT

This means that any cascading delete MUST be done in the application and the related rows MUST be deleted first or the foreign key checking will raise an exception

[SineSwiper]

MySQL has commands for disabling FK checks. Not sure if we should go that route or not, but I'm going to have to side with @gbarr on this being a critical support issue, previous behavior be damned. We can't just say that we don't support cascade deletes when we have a cascade_delete flag in the relationship.

Sure, the exception is documented, and yes, it's been in there forever. But causing an exception for a fairly common use case is still a bug, especially when there isn't any way around it. Let's provide some answers instead of constantly shooting down his suggestions.

I'm in favor of creating a cascade_delete => -1 behavior. It doesn't create a dual property complexity problem, passes TRUE/FALSE tests (on the edge case of relationship introspection), and it's mnemonic. (After = 1, Before = -1)

[ribasushi]

On Sat, May 25, 2013 at 09:03:22PM -0700, Brendan Byrd wrote:

... previous behavior be damned ...

In case it is not exceedingly clear - this sentence is a good
approximation of the current development philosophy of the DBIC project:
http://perl5.git.perl.org/perl.git/blob/d2534c:/pod/perlpolicy.pod#l128

Your approach to problem-resolution is not applicable on this project
under the current project management. Neither currently, nor in the near
future. Please refrain from suggesting things in this tone - it achieves
nothing more than evoke the desire to bitterly cling to guns and/or
religion among all participants.

I will reply on the technical part of your email separately.

[dbsrgits-sync]

On Sat, May 25, 2013 at 09:03:22PM -0700, Brendan Byrd wrote:

MySQL has commands for disabling FK checks

... and is supported by DBIC's with_deferred_fk_checks()[1](which is a
tad unmaintained[2] but works).

... We can't just say that we don't support cascade deletes when we
have a cascade_delete flag in the relationship.

We can if cascade_delete is declared a kludge that was a good idea at
the time but definitely isn't these days. I know it is my fault this
wasn't done yet in a public manner (i.e. deprecation etc). The scope of
the problem with deprecating this bunch of suck is a bit larger, I am
currently working out through a way to do it safely. urns out this isn't
relevant to the discussion though, see below.

Sure, the exception is documented, and yes, it's been in there
forever. But causing an exception for a fairly common use case is
still a bug

It is if you look at it this way. Resd below.

especially when there isn't any way around it

Um... are you saying there is no way to write a project-wide cascading
visitor that cals the deletes before DBIC does what it intended to
do...? It is not asy to do it, but it sure isn't impossible

Let's provide some answers instead of constantly shooting down his
suggestions.

I only shot down a particularly gnarly addition to the API, not the
entire idea. In fact @joseds and @vanstyn are working on a critical part
of infrastructure which will make this stuff much easier.

But now to the actual problem at hand: We currently have DBIC-side
delete cascades which are designed to emulate RDBMS-side cascades. It
is not a mechanism to work around RDBMS-side cascade restrictions.
This ticket is proposing to make it one (either replace the old behavior
or augment it).

Fusing these two concerns together "emulate RDBMS cascading delete
triggers" and "avoid RDBMS cascading delete triggers" is dangerous from
any API standpoint. Providing such mechanism in the core library (and
thus hinting it's a good idea) is just irresponsible.

As such I am declaring "let's extend DBIC with a way to declare a
relationship to delete the children first without asking" an absolute
no-go, let's not waste time discussing it further.

The bigger picture however yields the problem which is worth solving:
"Let's provide a canonical way to write a relationship visitor so it is
easy to write any sort of pre/post trigger behavior in 20 lines and
throw it on CPAN". This is something that is already being worked on,
and which will become available very soon, possibly with 0.08251.

I believe this addresses the original problem elegantly without
compromising further the coherency of the DBIC core. Note - I am not
saying the current core state is especially coherent as it is, but
that's no excuse to keep mudding down the API.

Cheers

[dbsrgits-sync]

On Sun, May 26, 2013 at 03:03:21PM +1000, Peter Rabbitson wrote:

On Sat, May 25, 2013 at 09:03:22PM -0700, Brendan Byrd wrote:

MySQL has commands for disabling FK checks

... and is supported by DBIC's with_deferred_fk_checks()[1](which is a
tad unmaintained[2] but works).

Bah - forgot the links:

[1] https://metacpan.org/module/DBIx::Class::Storage::DBI#with_deferred_fk_checks
[2] http://lists.scsys.co.uk/pipermail/dbix-class-devel/2013-January/000249.html

[frioux]

For what it's worth if I were to fix this problem I'd make a helper. That way you can have it very configurable and not need to upgrade DBIC itself, which some people are reticent to do due to far reaching effects.