Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to delete associated is not clear #8348

Closed
3 tasks done
2ndGAB opened this issue Feb 25, 2016 · 11 comments
Closed
3 tasks done

How to delete associated is not clear #8348

2ndGAB opened this issue Feb 25, 2016 · 11 comments

Comments

@2ndGAB
Copy link

2ndGAB commented Feb 25, 2016

  • feature-discussion (RFC)
  • CakePHP Version: 3.x
  • Platform and Target: apache mySQL.

I spent few hours to try to understand why I couldn't delete correctly my entities (Thanks ndm, Jose)
I wanted to delete an entity and its 2 associated levels.
So I declared my hasOne/hasMany with dependent because the doc says:

When deleting entities, associated data can also be deleted. If your HasOne and HasMany associations are configured as dependent, delete operations will ‘cascade’ to those entities as well.

Cool, but that doesn't work.

Thanks to ndm and jose, I understood that cascadeCallbacks is needed to delete subsequent levels. ?!?
About cascadeCallbacks, the doc says:

You can elect to have the ORM load related entities, and delete them individually by setting the cascadeCallbacks option to true.

And:

Setting cascadeCallbacks to true, results in considerably slower deletes when compared to bulk deletes. The cascadeCallbacks option should only be enabled when your application has important work handled by event listeners.

So nothing related to delete associated of 2nd level and so !?!

I think there is a real problem on that point. I don't really understand why cascadeCallbacks is for but dependent should be THE flag to permit to delete all associated levels no?

Regards,
Al

@saeideng
Copy link
Member

do you use delete() ?

@markstory markstory added this to the 3.2.4 milestone Feb 25, 2016
@markstory
Copy link
Member

Enabling dependent does delete associated rows. However, they are deleted with a single DELETE command. This means that row callbacks are not fired. Enabling cascadeCallbacks triggers entity deletion that emits N DELETE commands, one for each associated record being deleted.

I'm not really clear on what the 'problem' here is. Did you find the documentation unclear? Or did you have an issue with records not being deleted?

@2ndGAB
Copy link
Author

2ndGAB commented Feb 25, 2016

As I explain in my post, the doc says:

When deleting entities, associated data can also be deleted. If your HasOne and HasMany associations are configured as dependent, delete operations will ‘cascade’ to those entities as well.

So for me, cascade means that the operation will be done up to the last level.

So we should understand that if I declare:

class SitemessagesTable extends Table {
    public function initialize(array $config) {

    $this->hasMany('Sitemessagetitles', [
                'dependent' => true,
        ]);
    }

And then:

class SitemessagetitlesTable extends Table {
    public function initialize(array $config) {

    $this->hasOne('Sitemessagetexts', [
        'dependent' => true,
    ]);
    }
}

Deleting Sitemessage will delete Sitemessagetitles (and it works) which will delete Sitemessagetexts. this last step doesn't work and it's disturbing.

It's difficult to understand why dependent in SitemessagetitlesTable is not taking in account as the operation is normally 'cascaded'.

And the word cascadedCallbacks makes me think of 'calling all callbacks' such as beforeDelete, or afterDelete, not the 'delete' operation itself.

It's because the doc is not really clear.

And we could wonder if cascadeCallbacks is really necessary because again in my case, if Sitemessagetexts depends on Sitemessagetitleswhich depends on Sitemessages, I don't see why deleteting Sitemessages doesn't mandatory delete all levels.
Not doing this will let orpheans in the DB. What's the goal of this? No choice, everything must be deleted.

@markstory
Copy link
Member

Ah ok. The docs on dependent could totally be clearer around the depth limitations they have. The goal of 'dependent' is to solve for the case where you don't have deep trees of associations that are dependent on each other.

because again in my case, if Sitemessagetexts depends on Sitemessagetitleswhich depends on Sitemessages, I don't see why deleteting Sitemessages doesn't mandatory delete all levels.

Cascading through all associations would require knowing that there was deeper data to be deleted, which requires that data to be loaded. The current behavior of dependent avoids loading related data.

And the word cascadedCallbacks makes me think of 'calling all callbacks' such as beforeDelete, or afterDelete, not the 'delete' operation itself.

While we can't remove the current option, we can add an alias that better describes the behavior. Do you have a suggestion on a better name?

@2ndGAB
Copy link
Author

2ndGAB commented Feb 26, 2016

Cascading through all associations would require knowing that there was deeper data to be deleted, which requires that data to be loaded.

But with dependenton first level, you mandatory load the second level and so, you know that there is a dependent third level or no and so on...

I can understand that you can't remove an existing option but 'dependent' is really understandable and I wonder if its current behavior is not dangerous enough to not consider to change it. We currently don't ensure DB integrity because we let orpheans and I really don't see a situation where this behavior could be expected.
What will be the problem of cascading the delete down to the last level ?
Today, we are in a situation where setting dependent needs almost mandatory to set cascadeCallbacks as well.

@markstory
Copy link
Member

But with dependenton first level, you mandatory load the second level and so, you know that there is a dependent third level or no and so on...

No we don't. The associations that support dependent deletes simply emits a deleteAll.

I can understand that you can't remove an existing option but 'dependent' is really understandable and I wonder if its current behavior is not dangerous enough to not consider to change it. We currently don't ensure DB integrity because we let orpheans and I really don't see a situation where this behavior could be expected.

The issue with cascading down to lower levels, is that it can dramatically change the performance characteristics of an application. In order to cascade down each level, we first need to load all the data, and then delete it.

I agree that the orphan issue is not ideal, which is why we also provide cascadeCallbacks which allows developers to opt into using a slower but more thorough form of cascading deletes. Just using dependent in isolation works when you don't have deep hierarchies, and the shallow delete mode is 'enough'.

If you truly want to avoid orphans, databases also offer cascading deletes in constraint definitions, which is the approach I would take if I wanted to be absolutely sure there would never be orphans.

@2ndGAB
Copy link
Author

2ndGAB commented Feb 26, 2016

Ok, why not, but the developper should guess the job impacted by cascading deletes and should know if it could result in thousands of individual deletes.
In that case, as the job MUST be done, he should mark the main entity as 'toBeDeleted' and do the real job next night.
But ok, just clarify the doc maybe.

@markstory
Copy link
Member

In that case, as the job MUST be done, he should mark the main entity as 'toBeDeleted' and do the real job next night.

I don't think deferred jobs is within the ORM's scope, but I might be misunderstanding you.

@2ndGAB
Copy link
Author

2ndGAB commented Feb 27, 2016

You are perfectly right, it's not what I wanted to say. I just mean that I think that ORM must ensure DB integrity but that should cause long job, particularly in the case we are talking about.

So, in that case, I just rely on the doc about cascadeCallbacks:

Setting cascadeCallbacks to true, results in considerably slower deletes when compared to bulk deletes. The cascadeCallbacks option should only be enabled when your application has important work handled by event listeners.

But basically, I just want to say that specifying dependent on a hasOne/hasMany association which is not took in account (i.e. on 2nd level) is maybe not logical.

@markstory
Copy link
Member

I'm hesitant to change the semantics of existing options as historically that has resulted in a number of upset users and bug reports. I think the documentation updates that have been done are the best option we have outside of adding more options.

@2ndGAB
Copy link
Author

2ndGAB commented May 14, 2016

Hello,
Sorry to reopen it but there is again something I find strange in this delete functionnality.
I created a Gist, but basically I have this structure:

Sitemessages hasMany SitemessageTitles
SitemessageTitles hasOne SitemessageTexts
SitemessageTitles hasOne SitemessageImages

So we discussed already how to delete everything when I delete a Sitemessages and it works according to what's done in SitemessageControllers.php line 109.

But now if want to delete one of the SitemessageTitles as done in SitemessagesController.php line 76, it doesn't delete associated SitemessageTexts and SitemessageImages, despite dependent => true and cascadeCallbacks => true are set.

So I think that cascading delete are problematic as it only work`s from the top level and no from intermediate levels. Why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants