Generic relations should be counted as children when deleting to avoid problems with post_delete signal

[Ryan Fugger]

When deleting an object and its related objects, generically-related models aren't counted as children in the delete order, and so may be deleted after the original model is deleted. This causes problems when the post_delete signal from the generic relation tries to update its parent.

For example, if a model Article has a generic relation to a Vote model, and the post_delete signal from Vote recomputes the parent Article's cached vote tally and saves the Article, then deleting the Article will fail if the Votes get deleted after the Article.

The solution is to deleted generically related children first.

A workaround is to disconnect the post delete signal when deleting the parent, and reconnect after.

[Ryan Fugger]

Note that the workaround (disconnecting post_delete, deleting, reconnection post_delete) has to be applied to any model whose deletion may trigger the deletion of generically-related objects.

So extending the Article/Vote example above, if each Article has a Section, and each Section has a Sandbox, then you would have to disconnect the post_delete from Vote that updated Article before deleting Article, Section, or Sandbox, and reconnect after the deleting was done.

[Russell Keith-Magee]

You may be observing this with generic relations in your specific case, but from your description, I don't think it's a behavior that is specific to generic relations. If you've got models with circular links between models (e.g., a model with a ManyToManyField('self')) you chould get similar effects.

However, I think this could be considered "by design". Django's bulk deletion doesn't just walk down the related object tree doing deletions; it makes some optimizations to collapse deletes by grouping them by model. The discussion on #13067 proposes to optimize this process even further.

I can see two other options that your report doesn't propose:

The third option you don't mention is that the post_delete handler must account for the fact that Vote post_delete handler must account for the fact that it may be deleted after the article it is related to.

The fourth option is that signal emission for a delete with cascades could be postponed until after all objects have been deleted. This would ensure that all related objects are deleted before any post_delete processing occurs.

My immediate reaction is that (3) is the right response here; this is a case where signal handlers need to be aware of what could be happening. On that basis, I'm closing wontfix. If you're convinced I'm wrong, feel free to start a discussion on django-dev.

[Ryan Fugger]

I agree that your third option, let the post_delete handler check to make sure the parent hasn't been deleted, is the best workaround, since it doesn't require messing with other models. However, since ticket #5559 deleted objects aren't marked as such until after their children are deleted, so it does require another query to determine whether the instance still exists in the db. My proposed solution is more efficient since it doesn't require any more queries, or really change much or anything except the order in which model groups are deleted.

I've attached a patch showing the one-line change that appears to fix my issue.

I suppose there is a risk that my solution would cause other problems, but I can't think what they would be. I'll start a discussion on django-dev to see if anyone can point out a problem here.

[Ryan Fugger]

Don't treat generic relations as nullable when building model delete cascade tree.

[Ryan Fugger]

Ah, I can pass force_update=True and catch DatabaseError when I save the parent, and I don't need an extra query to make sure that it hasn't already been deleted. I still think my patch would make django just do the right thing here without any extra work by the developer...

[Carl Meyer]

Replying to rfugger:

I still think my patch would make django just do the right thing here without any extra work by the developer...

It seems like "the right thing" in your specific use case, but it causes much more serious problems in other cases. Making the change you propose significantly increases the likelihood of a circular dependency in deletion if the generically-related objects have any other relations. The purpose of the "nullable" arg is to tell the deletion algorithm whether it is _possible_ to delete these objects later, in order to avoid circular dependency. It is, in fact, possible to delete generic relations later; therefore setting nullable True is the right thing to do.

[Ryan Fugger]

Replying to carljm:

Making the change you propose significantly increases the likelihood of a circular dependency in deletion if the generically-related objects have any other relations.

How hard would it be to check that the generically-related objects have no other relations before setting nullable=True?

[Ryan Fugger]

Replying to rfugger:

How hard would it be to check that the generically-related objects have no other relations before setting nullable=True?

Never mind. That would make the behaviour inconsistent for models with different kinds of fields, which isn't good here. Thanks for the input.