Opened 3 years ago

Last modified 4 months ago

#18682 assigned Cleanup/optimization

Make the deletion of stale content types safer

Reported by: aaugustin Owned by: speijnik
Component: contrib.contenttypes Version: 1.3
Severity: Normal Keywords:
Cc: charette.s@…, speijnik Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

django_fix_18682.patch (1.8 KB) - added by speijnik 12 months ago.
Initial version of the patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by charettes

  • Cc charette.s@… added

comment:2 Changed 3 years ago by melinath

  • Triage Stage changed from Unreviewed to Accepted

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.

comment:3 Changed 12 months ago 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 ?

comment:4 Changed 12 months ago 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.

comment:5 Changed 12 months ago by speijnik

  • Owner changed from nobody to speijnik
  • Status changed from new to assigned

comment:6 Changed 12 months ago 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.

Changed 12 months ago by speijnik

Initial version of the patch

comment:7 Changed 12 months ago by speijnik

  • Has patch set

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

comment:8 Changed 12 months ago by speijnik

  • Cc speijnik added

comment:9 Changed 12 months ago by slurms

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

comment:10 Changed 4 months ago by claudep

#24546 was closed as a duplicate.

Note: See TracTickets for help on using tickets.
Back to Top