[Django] #12339: ContentType update could cascade deletes to generic foreign keys, causing data loss

[Django]

#12339: ContentType update could cascade deletes to generic foreign keys, causing
data loss
------------------------------------------+---------------------------------
Reporter: kcarnold | Owner: nobody
Status: new | Milestone:
Component: Contrib apps | Version: 1.1
Keywords: contenttype, delete, cascade | Stage: Unreviewed
Has_patch: 0 |
------------------------------------------+---------------------------------
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".

--
Ticket URL: <http://code.djangoproject.com/ticket/12339>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: closed | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: duplicate | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------
Changes (by kmtracey):

* status: new => closed
* needs_better_patch: => 0
* resolution: => duplicate
* needs_tests: => 0
* needs_docs: => 0

Comment:

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:1>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: closed | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: duplicate | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Comment (by kcarnold):

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

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:2>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Changes (by kmtracey):

* status: closed => reopened
* resolution: duplicate =>

Comment:

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.)

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:3>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Comment (by kcarnold):

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].

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:4>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Comment (by danros):

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:5>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Comment (by kcarnold):

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:6>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone:

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Unreviewed | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Comment (by danros):

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:7>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone: 1.2

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Accepted | Has_patch: 0

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Changes (by russellm):

* stage: Unreviewed => Accepted
* milestone: => 1.2

Comment:

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:8>

[Django]

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

-----------------------------------+----------------------------------------
Reporter: kcarnold | Owner: nobody
Status: reopened | Milestone: 1.2

Component: Contrib apps | Version: 1.1

Resolution: | Keywords: contenttype, delete, cascade
Stage: Accepted | Has_patch: 1

Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 0 |
-----------------------------------+----------------------------------------

Changes (by kcarnold):

* has_patch: 0 => 1

Comment:

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.

--
Ticket URL: <http://code.djangoproject.com/ticket/12339#comment:9>