V8-archive: "Cascade" relationship deletions when deleting a parent item

[srjrol]

Bug Report

Steps to Reproduce

1. Create a many-to-many relationship.
2. Relate items from two collections to each other.
3. Remove an item from one collection.

I used the demo website (member -> favorites) relationship to reproduce after first seeing this in my own instance.

Expected Behavior

The junction table should be updated to remove any rows containing items that are deleted. The correct number of valid relationships is displayed in the table column ("# items").

Actual Behavior

The invalid row is left in the junction table and incorrectly added towards the number of items related to.

Other Context & Screenshots

Technical Details

• Device: Desktop
• OS: Windows 10
• Browser: Chrome
• Directus: 190325A
• Install Method: master branch of directus/directus

[benhaynes]

Thank you very much for the descriptive bug report @srjrol !

We'll look into this asap...

[rijkvanzanten]

I'm moving this over to the API side, seeing that the API should handle the deletion of the junction row on deletion of a related item.

[benhaynes]

I was on the fence about this being a bug or enhancement — but will leave it as a bug for now.

Technically, there's no obligation for the platform to do cascading deletes... and in fact it could be seen as overreaching. But I do agree that this is probably the _expected_ behavior. And we don't have any notices that inform the user that they must manually remove relationships.

I will mention that this is not limited to M2M... as a M2O relation would also break if the related item is deleted without first clearing the FK. Again, it seems risky to start nullifying other values based on a delete... but I guess this is the best option.

I assume that we'll use directus_relations to check for any possible relationships with the deleted item's collection — and then search all those fields for offending keys? In large datasets this could add a lot of latency to DELETE calls as it might be performing many large SELECTS too.

A potentially safer enhancement would be to show a modal reviewing all stale relationships when deleting an item... but this would only work with the App GUI.

Any other thoughts on this?

[srjrol]

The only reason I saw this as a bug was when checking the "# items' in the column display for the collection it did not match up with the actual number of items related to. Maybe the simpler solution (for now at least) is to just update that calculation so it checks if the items related to still exist before including them in the total.

[benhaynes]

Yeah, we'll see what @rijkvanzanten thinks... but I'd like to see this fixed instead of patched. It shouldn't be too difficult to check for relationships in directus_relations when deleting an item. But we'd want a param to override this, maybe keeping the default behavior as-is but adding an optional param to cascade delete relationships.

[rijkvanzanten]

I think this should be handled from just the API as well. Maybe the API should throw an error (400) when you're trying to delete an item that has a relationship to another item. We'd then add a force parameter that will delete this single item anyway, and a (?) delete-relational param that will remove the relation itself too.

I'm not too sure how that second one has to work. For m2m (o2m-m2o) it makes sense to remove the junction table record, but I don't think it makes sense for an M2O to remove the related one.

Maybe this is only relevant for o2m

[benhaynes]

• M2M and M2X — Delete item and junction items (that's it)
• O2M — Deleting item deletes relationship (make sure "count" only counts published status)
• M2O — For this we would need to set the parent field to NULL (if allow_null is true). I think this is acceptable, but if the field does NOT allow_null then we either return an error or leave as-is. In the App we could solve this (in the future) with a modal that asks you to choose a _new_ item for the parent field... _then_ deletes the main item. But that's not for now.

[rijkvanzanten]

I think we're getting O2M and M2O confused..

Also, we don't technically have a M2M or M2X or any of that, as it's an O2M to a junction table (JT), followed by a M2O from the JT to the related collection.

• M2O — Doesn't have to do anything, as the 'relation' is saved in a field in the row you're deleting
• O2M — Set the field in the related collection to null / ask for what to replace with*

* The only confusing part is that we should probably _delete_ the row in the junction table, instead of setting one of the fields to null. But you could have custom fields in the junction table that may or may not can be deleted.

Also, I do think we should have an option to _not_ use any of this automatic relationship deletion.

[benhaynes]

Haha, we're saying the same thing but in opposite directions... which makes sense. I was describing when we delete the "foreign" item and you're talking about deleting the "main" (parent) item. Either way we need to do a check.

And yes, we should have an API option to ignore this whole cascading delete.

[rijkvanzanten]

I'm marking this an enhancement seeing that it's technically not something that's broken as is, but something that was never implemented / meant to work in the first place. I do agree that this would be a great enhancement to have though 🙂

[benhaynes]

To achieve better clarity/visibility, we are now tracking feature requests within the Feature Request project board.

This issue being closed does not mean it's not being considered.

[JLueke]

I'm marking this an enhancement seeing that it's technically not something that's broken as is, but something that was never implemented / meant to work in the first place. I do agree that this would be a great enhancement to have though 🙂

I disagree. Some weeks ago I started to use Directus. Like in other CMSs, I expected that the entries in the tables for the M2M junctions would be deleted if one of the referred items was deleted. Now I have a database with plenty of "parentless" translations and other items that admins need to delete manually, after checking that the entry isn't in use anymore.

This behaviour is not intuitive in comparison to other CMSs.

[benhaynes]

We're not saying it's not worth doing, or that it's not an important feature — just that it's not an actual _bug_ in regards to our "stability triaging". This is high on the list to be added, but our focus at the moment is closing all bugs. Also, we're always happy to review any PRs from contributors!

In short...

• This is still an RC, not a prod release... plenty of time to get this added still
• This can still be accomplished using event hooks as a stopgap solution
• Small team + free software, means that things can't all happen at once
• OSS is a community effort, so it is the responsibility of all users to help test, fix, and add features

Also, I am only talking about v9 here... which is the upcoming version of Directus, with v8 being deprecated upon release.

[JLueke]

Also, I am only talking about v9 here... which is the upcoming version of Directus, with v8 being deprecated upon release.

Then I misunderstood this comment. I thought it'll come in version 10.

We're not saying it's not worth doing, or that it's not an important feature

I understood your argumentation for why it's not a bug before.
It is still bug in my definition ( = (common) user expectations don't equal actual behaviour) and it's just frustrating, that I just noticed this bug (my definition) coincidentally a few hours ago.

I didn't mean my comment to be interpreted as hostile. I know first hand that it takes a long time with a small team - even more if most of the work is done by one person.

[benhaynes]

Months ago we were planning on doing a v9+v10... but now that has been combined into one major release.

I understand your point, but we can't treat everything that doesn't match user expectations as a bug, since user expectations are all over the place and we'd have infinite bugs based on what individuals thought should be included. That said, I agree with you that in most use cases, this is expected... we're just very careful with "automatically" deleting user data (eg: cascade deletes).

All I can say is this is coming (as a new feature 😉 ) in Directus 9, and can happen faster if sponsored, or a PR is submitted. All reported v9 bugs should be resolved in the next few days, and then we'll be able to get back to new features soon.