Opened 8 years ago

Closed 7 years ago

#5177 closed (fixed)

ContentType should purge orphaned models

Reported by: Rob Hudson <treborhudson@…> Owned by: nobody
Component: Contrib apps Version: master
Severity: Keywords:
Cc: treborhudson@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When working with models there are often times when one refactors model class names which leaves orphaned content types in the database. Content types should clean themselves when they notice an app no longer contains models in the database.

Attachments (4)

ctremove.diff (2.1 KB) - added by Rob Hudson <treborhudson@…> 8 years ago.
First attempt at a patch for this. This is a diff from git so this may need tweaking…
ctremove.2.diff (2.5 KB) - added by Rob Hudson <treborhudson@…> 8 years ago.
Fixing points 1 through 3 from Malcolm. Point #4 will need more time to ponder.
ctremove.3.diff (2.5 KB) - added by Rob Hudson <treborhudson@…> 8 years ago.
Making the doc string more correct.
ctremove.4.diff (3.6 KB) - added by Rob Hudson <treborhudson@…> 7 years ago.

Download all attachments as: .zip

Change History (11)

Changed 8 years ago by Rob Hudson <treborhudson@…>

First attempt at a patch for this. This is a diff from git so this may need tweaking...

comment:1 Changed 8 years ago by Rob Hudson <treborhudson@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

See: http://groups.google.com/group/django-developers/browse_thread/thread/c0a99afe82d4eaa8

The first patch works but I haven't tested fully. I had assumed I would also have to figure out how to manage the various permissions tables but it seems to be taking care of itself. I looked to see if the delete() on the ContentType object had some trickery to update the permissions but didn't find anything.

This doesn't remove the orphaned model tables themselves either, which is probably good... the idea for this patch was to avoid having to delete rows in the contenttype and various permissions tables, dropping a table is easy enough.

comment:2 Changed 8 years ago by Rob Hudson <treborhudson@…>

  • Needs documentation set

Just an update on this patch: I figured out the "trickery" in that Permissions has a FK to ContentType and therefore takes care of itself when a content type is deleted. Nice.

This patch accomplishes what I think it should...

  • If a model in an installed app gets orphaned, it removes that content type.
  • If a content type is renamed, it removes the old one and installs the new one.
    • It does not track or understand a renaming happened and migrate permissions or anything else. That's a bit outside the scope of this, I think.

I'll work on adding docs and test cases if I can.

-Rob

comment:3 Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

I like this patch and assuming all the concerns below are not real problems, it's close to reday. A few small things to fix and one large thing to ponder:

  1. create_contenttypes() now really needs a docstring, because it's not just creating new content types. It's also cleaning up old ones. Looks like it's a bit fiddly to move the "remove" processing out from the "create" loop, so it'll have to stay as one method. But at least document what it's doing in a broad way.
  2. We're strong believers in the English language and full sentences. The comment you've got to needs an initial capital and final full stop.
  3. The AUTHORS file is in alphabetical order by surname. Try to find where your name belongs (hint: it's not at the end).
  4. Do you need to get app_label from the models? What happens if a model has Meta.app_label set?

Now here's the big problem: is this a safe thing to do? I haven't followed through all the code, but I would guess it's not impossible for models in different locations to have the same value for their Meta.app_label attribute. I'm not sure if Django works well in that case, or if django.core.management sorts that out and groups things properly (I suspect not), or if it's something we want to support. But right now it might be possible. If so, that would mean you could end up calling create_contenttypes() multiple times for the same app_label.

At the moment, I suspect that just means not as much cleaning up gets done, so it fails safely (no still-valid data is deleted), right? I think this bears some thinking about; I don't know the answer yet, because it just occurred to me when I was reviewing the patch.

Not sure it's going to really be possible to test this, since the ContentTypes table is flushed for each new test anyway. If you can come up with something, great. However, I can't think of how to do it offhand.

What docs do you think are necessary? A quick observational note in the contrib apps docs? It's almost doing the natural thing and could pass without comment. By all means, write whatever you think is good, but I wouldn't hold back the patch if you think there's nothing worth saying.

Changed 8 years ago by Rob Hudson <treborhudson@…>

Fixing points 1 through 3 from Malcolm. Point #4 will need more time to ponder.

Changed 8 years ago by Rob Hudson <treborhudson@…>

Making the doc string more correct.

comment:4 Changed 8 years ago by Rob Hudson <treborhudson@…>

  • Triage Stage changed from Accepted to Design decision needed

Discussion and a design decision needed request regarding Malcolm's point 4 above posted to the developers list here:
http://groups.google.com/group/django-developers/browse_thread/thread/60de62f7a1b05f17/

Marking as design decision needed.

Changed 7 years ago by Rob Hudson <treborhudson@…>

comment:5 Changed 7 years ago by Rob Hudson <treborhudson@…>

  • Triage Stage changed from Design decision needed to Ready for checkin

Per Adrian and Jacob's suggestion, changing method name to update since it's now doing more than creating.

comment:6 Changed 7 years ago by Rob Hudson <treborhudson@…>

  • Needs documentation unset
  • Patch needs improvement unset

comment:7 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [6287]) Fixed #5177 -- Changed content type creation to also remove the types for any orphaned entries (so it's now an "update" feature). Thanks, Rob Hudson.

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