ContentType update could cascade deletes to generic foreign keys, causing data loss

[Kenneth Arnold]

Imagine that you change the name of a model. contrib/contenttypes/management.py has logic to remove unused content types. It does this by calling ct.delete(). The delete will cascade to any other models that reference that ContentType. If you have generic foreign key, this could cause any items that referred to the old table to get deleted. At least you lose the admin log, which could be valuable audit data; you could also lose any data (like votes) that applied to the old object that you haven't yet ported over -- and unless you're running syncdb doubly verbose, you'd never notice.

I hope this bug is invalid and the delete doesn't actually cascade, or something makes sure things are never deleted first. But I noticed this because of a Postgres IntegrityError upon that hook trying to delete a ContentType that was still referenced by the admin log. I don't recall the exact error text, but what I Googled was "django_content_type still referenced".

[Karen Tracey]

Starting with "Imagine that you change the name of a model" we're off in territory that Django does not yet claim to support: schema migration. Given that, what it looks like to Django when you change the name of a model is that the old model has been deleted. Django then correctly deletes the content type for that model, and yes that will cascade to any rows in other tables that referenced that content type. You do not lose the admin log, but you lose any entries in it that refer to the old model. To omit the cascade would be wrong, because that would leave pointers to nowhere hanging around. The current behavior is correct for the case where a model has been deleted. True, it does not account for the case where you are renaming the model, not actually deleting it, but that is not a problem that will be fixed individually. The correct way to fix that problem would be to implement general schema migration, which is covered by #5934. Presumably a good schema migration would handle updating the content type on a rename instead of deleting it, and so avoid the cascade delete (though I don't know the details of how any of the existing external apps that do migration handle this, I've not had any need to use them). At any rate I'm going to close this as a duplicate of the migration ticket, since this won't be "fixed" except as part of the larger effort to incorporate migrations into the base.

[Kenneth Arnold]

I do very much appreciate that the management command does attempt to clean up deleted content types, but until migrations are fully supported there is no way for that command to know that deletion instead of renaming occurred. So the current unconditional delete is already trying to support schema migration. In contrast, someone who assumes that Django doesn't support schema migration will assume that syncdb will only be creating missing tables and otherwise won't touch anything, and will certainly assume that it won't delete data. (I could see an argument that the bug is in the docs, i.e., that a note should be added to the "Syncdb will not alter existing tables" section.)

Until migrations are incorporated into base, the automatic content-type deletion should require explicit confirmation. Then, if the confirmations become annoying, making them automatic would be part of the migrations effort

[Karen Tracey]

Agreed the behavior should be documented. Not so sure about the confirmation, it strikes me the confirmation message would be hard to make non-confusing. Also might need to ensure the confirmation could be suppressed for thigs like running tests (I have no idea if we have tests that would trigger this, but we do have tests that do syncdbs.)

[Kenneth Arnold]

As far as I'm concerned, removing the "delete" line (keeping the verbose note about stale content types) fixes this bug. The rest of this comment is just about the extra next step, which is to restore convenience in the cases where the previous behavior was desirable--though it might be better to just wait until migrations are merged to do that.

If there were a way to tell that a .delete() would cascade before you do it, like the admin does (race conditions aside), a confirmation message might be: "The model named %s (ContentType id %d) was removed, but other objects refer to it. Do you want to delete those objects also? [y/N] " And of course there should be an option to proceed non-interactively, which would just report the stale ContentType ids without deleting them.

Do any tests delete models? If not, a confirmation shouldn't disrupt tests.

btw, the code in question was added in [6287].

[Dan Ros]

I was just bitten by this!

My situation involved moving some models from one app to another. I might have guessed that syncdb would have created a new contenttype for the models, but i didn't expect syncdb to cascade delete all affected items with a foreignkey referring the 'old' contenttype!

IMO this should not be the default behaviour, or if it is, there should be *large* warnings about the possible loss of data.

[Kenneth Arnold]

Pick your patch.

The first one just removes the dangerous delete, always leaving stale content types alone.

The second one keeps the delete, but guards it by a flag that defaults to False. It documents how to call the command with that flag being true. (It also incidentally enables update_all_contenttypes to pass along the new db parameter. I don't know if that's useful, though.) That documentation might arguably belong in the main docs, but I don't think it's worth integrating, given that it merely patches over the real problem (lack of migration support).

I care that this bug is fixed, but I'm indifferent to which approach you prefer to take.

[Dan Ros]

I agree that this should be fixed, and perhaps at a high priority. The docs generally suggest that running syncdb on live data is 'safe' (it will only create new tables). This is definitely not the case due to this.

[Russell Keith-Magee]

I can't say I'm a fan of either current patch. Leaving content types dangling like that doesn't strike me as a particularly friendly behavior, especially given how easy it is to end up with stale content types during initial development. I think a confirmation message is the way to go - Yes, we need to be careful in the wording of the confirmation prompt, but a broad "This is potentially destructive - If you're not sure, say 'NO'" should be enough to prevent accidental damage.
So - I'd like see the contenttypes syncdb signal handler do the following:

• Gather a list of stale content types
• Print that list to the user
• Ask the user to confirm that the content types should be deleted
• on YES, delete the content types
• on NO, print a helpful message on how to manually purge.

[Kenneth Arnold]

I also prefer interactive; the attached patch is an (untested) shot.

It would be nice to only ask for confirmation if the .delete will actually delete something. (Arguably it should ask for confirmation even if it's just a permission entry, because you might forget to transfer permissions to the new model. Agh, this really needs migration support to do correctly!) I looked briefly at the delete confirmation code in admin, but it looks too tightly woven into admin to use. Might make a feature request for a more reusable "what else will this delete?" query, if there isn't one already.

[Russell Keith-Magee]

(In [12782]) Fixed #12339 -- Made content type deletion an interactive process to prevent accidentally cascade deleting content from a production database. Thanks to kcarnold for the report and patch.

[Russell Keith-Magee]

(In [12783]) [1.1.X] Fixed #12339 -- Made content type deletion an interactive process to prevent accidentally cascade deleting content from a production database. Thanks to kcarnold for the report and patch.

Backport of r12782 from trunk.

[Jacob]

Milestone 1.2 deleted