Opened 3 weeks ago
Last modified 8 days ago
#36839 new Bug
Renaming of content types on RenameModel operations silently passes when duplicates are found
| Reported by: | Jacob Walls | Owned by: | Moksha Choksi |
|---|---|---|---|
| Component: | contrib.contenttypes | Version: | 6.0 |
| Severity: | Normal | Keywords: | |
| Cc: | mag123c | Triage Stage: | Accepted |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
When renaming a model, the migrate command will inject operations to rename content types. When a target content type already exists, the operation just silently skips the update:
content_type.model = new_model try: with transaction.atomic(using=db): content_type.save(using=db, update_fields={"model"}) except IntegrityError: # Gracefully fallback if a stale content type causes a # conflict as remove_stale_contenttypes will take care of # asking the user what should be done next. content_type.model = old_model
The rationale for skipping was discussed here:
In this case I think the safe approach is simply to silently handle the integrity error and let the update_contenttypes's post_migrate handler prompt the user about what to do next.
But that post_migrate handler was later removed in 6a2af01452966d10155b720f4f5e7b09c7e3e419 in favor of a management command that can be run on demand, meaning the pass is now silent again, AFAICT.
I don't think the silent pass is desired. I can add more details if this isn't enough context, but I think it's worth an initial investigation.
Change History (8)
comment:1 by , 3 weeks ago
comment:2 by , 3 weeks ago
| Cc: | added |
|---|
comment:3 by , 3 weeks ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
comment:4 by , 2 weeks ago
| Has patch: | set |
|---|
comment:5 by , 8 days ago
| Resolution: | → needsinfo |
|---|---|
| Status: | assigned → closed |
| Version: | → 6.0 |
Hello Jacob, thank you for your ticket! I have spent a while trying to reproduce and somehow I have made a mess of my content types without being able to reproduce exactly this. Could you please provide instructions to reproduce? Thank you!
comment:6 by , 8 days ago
Sure thing. With a fresh app:
- add models:
from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.db import models class Song(models.Model): melodious = models.BooleanField(default=True) class Melody(models.Model): melodious = models.BooleanField(default=True) class TaggedSong(models.Model): tag = models.SlugField() content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) object_id = models.PositiveBigIntegerField() content_object = GenericForeignKey("content_type", "object_id")
makemigrations&&migrate- In a shell:
>>> song = Song.objects.create() >>> tagged = TaggedSong(tag="hit", content_object=song) >>> tagged.save()
- Delete the melody class, makemigrations, migrate. You now have a stale content type for the model "melody". Before, there used to be a
post_migratehandler alerting you to the situation. - Rename
SongtoMelody, makemigrations, migrate, answer "Renamed?" with Y. - If you instrumented the
except IntegrityErrormentioned in the ticket body, you hit that silent pass. - You can't look up your tagged items by their new model name, because they were silently ignored:
>>> TaggedSong.objects.filter(content_type__model="melody") <QuerySet []> >>> TaggedSong.objects.filter(content_type__model="song") <QuerySet [<TaggedSong: TaggedSong object (1)>]>
If I were a little more fluent with content types and generic foreign keys I could probably construct a scenario that would show lookups resulting in ObjectDoesNotExist, the point is that the data didn't migrate. The content type is now fresh:
>>> ContentType.objects.get_for_model(Melody) <ContentType: Myapp | melody>
... but all your data is on the old one (see step 7.)
comment:7 by , 8 days ago
| Resolution: | needsinfo |
|---|---|
| Status: | closed → new |
comment:8 by , 8 days ago
| Triage Stage: | Unreviewed → Accepted |
|---|
Thank you Jacob, with these instructions I was able to reproduce. Not only the data did not migrate, but the old content type was left behind:
django=# SELECT * FROM django_content_type WHERE app_label = 'ticket_36839'; id | app_label | model ----+--------------+------------ 65 | ticket_36839 | melody 66 | ticket_36839 | song 67 | ticket_36839 | taggedsong (3 rows) django=# SELECT * FROM ticket_36839_taggedsong; id | tag | object_id | content_type_id ----+-----+-----------+----------------- 1 | hit | 1 | 66 (1 row)
I looked into this issue and traced the behavior in
django/contrib/contenttypes/management/__init__.py.When
IntegrityErroris caught during the rename (lines 29-33), the code silently reverts to the old model name. The comment mentionsremove_stale_contenttypeswill handle it, but as noted, this is now an optional management command - users have no way of knowing the rename failed.Possible approaches:
warnings.warn()when the conflict occursI'm curious whether the silent behavior was intentional for backwards compatibility, or if surfacing this to users would be welcome.