[Django] #18682: Make the deletion of stale content types safer
Django
------------------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
When a model is removed, `syncdb` offers to remove its content type :
{{{
The following content types are stale and need to be deleted:
myapp | mymodel
Any objects related to these content types by a foreign key will also
be deleted. Are you sure you want to delete these content types?
If you're unsure, answer 'no'.
Type 'yes' to continue, or 'no' to cancel:
}}}
While perfectly clear, this text is quite unhelpful, because there's no
easy way to determine if any instance still has a foreign key to the
content type.
To mitigate the risk of accidental cascade deletions -- sometimes you can
be sure and wrong -- Django could tell the user how many related objects
will be deleted, if any. This is similar to the admin's cascade delete UI,
but only one level deep.
It would be quite embarrassing to admit why I'm filing this ticket. Let's
just say the problem that shows up in the real world. Until we do
something about this, I'll add `on_delete=models.PROTECT` to all my
foreign key to `ContentType` in `GenericForeignKey` definitions.
--
Ticket URL: <https://code.djangoproject.com/ticket/18682>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.3
Component: | Resolution:
contrib.contenttypes | Triage Stage:
Severity: Normal | Unreviewed
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by charettes):
* cc: charette.s@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 1.3
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by melinath):
* stage: Unreviewed => Accepted
Comment:
I've run into this on more than one occasion. In fact I've had data loss
because of this... though I suppose that probably doesn't qualify it as a
data loss bug. Human error and all that. Still, would be nice to see it
fixed.
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 1.3
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by amfarrell):
Does this ticket need to be re-triaged to determine if it is still
relevant in the era of native migrations and django1.7 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 1.3
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
I believe it's still relevant as the behavior is the same with migrations.
The only change is that the handler now uses the `post_migrate` signal
rather than `post_syncdb`.
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:4>
Django
Reporter: aaugustin | Owner: speijnik
Type: Cleanup/optimization | Status: assigned
Component: contrib.contenttypes | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by speijnik):
* owner: nobody => speijnik
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:5>
Django
Reporter: aaugustin | Owner: speijnik
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by speijnik):
Working on this at EP14 sprint.
Displaying the object count alone is doable and I actually do have code
doing just that already.
I guess it would be a good idea to actually show the number of objects
grouped by their type/model.
Without doing that this will most likely always show a non-zero count, as
permissions are always represented this way and
count towards that number.
Also, I believe that showing each individual object is a rather bad idea,
think of thousands of objects there...
So right now, the output looks like this:
{{{
The following content types are stale and need to be deleted:
app | a (29 objects remaining)
}}}
What I believe would be useful in this case, would be an output in the
form of
{{{
The following content types are stale and need to be deleted:
app | a
- permission objects to be deleted: 3
- [...] objects to be delted: 26
}}}
This way things are pretty clear. The exact formatting might need to look
different though.
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:6>
Django
Reporter: aaugustin | Owner: speijnik
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by speijnik):
* has_patch: 0 => 1
Comment:
Just attached the patch.
Formatting is obviously up for discussion and probably not optimal.
Example:
{{{
The following content types are stale and need to be deleted:
app | a
- auth | permission objects to be deleted: 3
- app | genericrel objects to be deleted: 26
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:7>
Django
Reporter: aaugustin | Owner: speijnik
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by speijnik):
* cc: speijnik (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:8>
Django
Reporter: aaugustin | Owner: speijnik
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by slurms):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:9>
Django
Reporter: aaugustin | Owner: speijnik
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Comment (by claudep):
#24546 was closed as a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:10>
Django
Reporter: aaugustin | Owner:
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 1.3
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by timgraham):
* status: assigned => new
* owner: speijnik =>
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:11>
Django
Reporter: aaugustin | Owner: erikr
Type: Cleanup/optimization | Status: assigned
Component: contrib.contenttypes | Version: 1.3
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by erikr):
* owner: => erikr
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:12>
Django
Reporter: aaugustin | Owner:
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 1.3
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by erikr):
* status: assigned => new
* needs_better_patch: 1 => 0
* owner: erikr =>
* needs_tests: 1 => 0
* needs_docs: 1 => 0
Comment:
I have submitted a new PR: https://github.com/django/django/pull/6869
This PR uses the Collector to determine which objects would be deleted.
This is the same collector that the admin uses to display which objects
might be deleted, so it should be very thorough.
I have also changed the wording away from "need to be deleted" as I've
been unable to find a basis for that claim of necessity. This was
introduced in the confirmation added in #12339, which built upon #5177 in
which the original (without confirmation) delete of stale types was added.
An example, as produced in the tests, now looks like this. Probably we can
still improve the language:
{{{
Some content types in your database are stale and can be deleted.
Any objects that depend on these content types will then also be deleted.
The content types, and the dependent objects that would be deleted, are:
- Content type for contenttypes_tests.Fake
- 1 object of type contenttypes_tests.post:
- post
This list does not include data that might be in your database
outside of Django's models.
Are you sure you want to delete these content types?
If you're unsure, answer 'no'.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:13>
Django
Reporter: aaugustin | Owner: Florian
Type: | Apolloner <apollo13@…>
Cleanup/optimization | Status: closed
Component: | Version: 1.3
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Florian Apolloner <apollo13@…>):
* status: new => closed
* owner: => Florian Apolloner <apollo13@…>
* resolution: => fixed
Comment:
In [changeset:"8db889eaf7dce0cb715b075be32047c1b1b316da" 8db889ea]:
{{{
#!CommitTicketReference repository=""
revision="8db889eaf7dce0cb715b075be32047c1b1b316da"
Fixed #18682 -- Expanded explanation in stale content type deletion.
(#6869)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:14>
Django
Reporter: aaugustin | Owner: Florian
Type: | Apolloner <apollo13@…>
Cleanup/optimization | Status: closed
Component: | Version: 1.3
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"e2dfa81ff7489d97700604d634adacf1384af184" e2dfa81f]:
{{{
#!CommitTicketReference repository=""
revision="e2dfa81ff7489d97700604d634adacf1384af184"
Refs #18682 -- Edited explanation in stale content type deletion.
Follow up to 8db889eaf7dce0cb715b075be32047c1b1b316da.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18682#comment:15>