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 mag123c, 3 weeks ago

I looked into this issue and traced the behavior in django/contrib/contenttypes/management/__init__.py.

When IntegrityError is caught during the rename (lines 29-33), the code silently reverts to the old model name. The comment mentions remove_stale_contenttypes will handle it, but as noted, this is now an optional management command - users have no way of knowing the rename failed.

Possible approaches:

  1. Emit a warning via warnings.warn() when the conflict occurs
  2. Log at WARNING level so it appears in server logs
  3. Raise an exception (though this may be too disruptive)

I'm curious whether the silent behavior was intentional for backwards compatibility, or if surfacing this to users would be welcome.

comment:2 by mag123c, 3 weeks ago

Cc: mag123c added

comment:3 by Moksha Choksi, 3 weeks ago

Owner: set to Moksha Choksi
Status: newassigned

comment:4 by Moksha Choksi, 2 weeks ago

Has patch: set

comment:5 by Natalia Bidart, 8 days ago

Resolution: needsinfo
Status: assignedclosed
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 Jacob Walls, 8 days ago

Sure thing. With a fresh app:

  1. 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")
  1. makemigrations && migrate
  2. In a shell:
    >>> song = Song.objects.create()
    >>> tagged = TaggedSong(tag="hit", content_object=song)
    >>> tagged.save()
    
  3. Delete the melody class, makemigrations, migrate. You now have a stale content type for the model "melody". Before, there used to be a post_migrate handler alerting you to the situation.
  4. Rename Song to Melody, makemigrations, migrate, answer "Renamed?" with Y.
  5. If you instrumented the except IntegrityError mentioned in the ticket body, you hit that silent pass.
  6. 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 Jacob Walls, 8 days ago

Resolution: needsinfo
Status: closednew

comment:8 by Natalia Bidart, 8 days ago

Triage Stage: UnreviewedAccepted

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)
Note: See TracTickets for help on using tickets.
Back to Top