Issues removing M2M field with explicit through model

[Stephen Burrows]

I get the error below when I try to delete a field that has an explicit through model. It comes down to: for some reason, field.rel.through is a string instead of being resolved to a model (maybe in part because the model has been deleted already?)

  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File ".../django/django/core/management/__init__.py", line 427, in execute_from_command_line
    utility.execute()
  File ".../django/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File ".../django/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File ".../django/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File ".../django/django/core/management/commands/migrate.py", line 145, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File ".../django/django/db/migrations/executor.py", line 60, in migrate
    self.apply_migration(migration, fake=fake)
  File ".../django/django/db/migrations/executor.py", line 94, in apply_migration
    migration.apply(project_state, schema_editor)
  File ".../django/django/db/migrations/migration.py", line 97, in apply
    operation.database_forwards(self.app_label, schema_editor, project_state, new_state)
  File ".../django/django/db/migrations/operations/fields.py", line 83, in database_forwards
    schema_editor.remove_field(from_model, from_model._meta.get_field_by_name(self.name)[0])
  File ".../django/django/db/backends/sqlite3/schema.py", line 125, in remove_field
    if isinstance(field, ManyToManyField) and field.rel.through._meta.auto_created:
AttributeError: 'str' object has no attribute '_meta'

If I reverse the order of operations (so that the field is removed *before* the through model is deleted) I get the following error:

  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File ".../django/django/core/management/__init__.py", line 427, in execute_from_command_line
    utility.execute()
  File ".../django/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File ".../django/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File ".../django/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File ".../django/django/core/management/commands/migrate.py", line 145, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File ".../django/django/db/migrations/executor.py", line 60, in migrate
    self.apply_migration(migration, fake=fake)
  File ".../django/django/db/migrations/executor.py", line 94, in apply_migration
    migration.apply(project_state, schema_editor)
  File ".../django/django/db/migrations/migration.py", line 97, in apply
    operation.database_forwards(self.app_label, schema_editor, project_state, new_state)
  File ".../django/django/db/migrations/operations/fields.py", line 83, in database_forwards
    schema_editor.remove_field(from_model, from_model._meta.get_field_by_name(self.name)[0])
  File ".../django/django/db/backends/sqlite3/schema.py", line 128, in remove_field
    self._remake_table(model, delete_fields=[field])
  File ".../django/django/db/backends/sqlite3/schema.py", line 65, in _remake_table
    del body[field.name]
KeyError: 'items'

Model is changing from

class ItemDiscount(models.Model):
    PERCENT = 'percent'
    FLAT = 'flat'

    TYPE_CHOICES = (
        (PERCENT, _('Percent')),
        (FLAT, _('Flat')),
    )
    item = models.ForeignKey(Item)
    discount = models.ForeignKey('Discount')
    discount_type = models.CharField(max_length=7,
                                     choices=TYPE_CHOICES,
                                     default=PERCENT)
    amount = models.DecimalField(max_digits=5, decimal_places=2,
                                 blank=True, null=True,
                                 validators=[MinValueValidator(0)])


class Discount(models.Model):
    name = models.CharField(max_length=40)
    code = models.CharField(max_length=20)
    items = models.ManyToManyField(Item, through=ItemDiscount)
    available_start = models.DateTimeField()
    available_end = models.DateTimeField()
    event = models.ForeignKey(Event)

    class Meta:
        unique_together = ('code', 'event')

to

class Discount(models.Model):
    PERCENT = 'percent'
    FLAT = 'flat'

    TYPE_CHOICES = (
        (PERCENT, _('Percent')),
        (FLAT, _('Flat')),
    )
    name = models.CharField(max_length=40)
    code = models.CharField(max_length=20)
    item_option = models.ForeignKey(ItemOption)
    available_start = models.DateTimeField(blank=True, null=True)
    available_end = models.DateTimeField(blank=True, null=True)
    discount_type = models.CharField(max_length=7,
                                     choices=TYPE_CHOICES,
                                     default=PERCENT)
    amount = models.DecimalField(max_digits=5, decimal_places=2,
                                 blank=True, null=True,
                                 validators=[MinValueValidator(0)])
    event = models.ForeignKey(Event)

    class Meta:
        unique_together = ('code', 'event')

[loic84]

@melinath, this rings a bell, could you test with the master version of Django in case it's already fixed?

[Stephen Burrows]

I get the same error on master.

[loic84]

Didn't have time to work on a fix, but I could reproduce.

Wrote some test cases for removal of M2M fields: ​https://github.com/django/django/pull/2527.

test_remove_field_m2m_with_through1 and test_remove_field_m2m_with_through2 reproduce the two issues from the ticket description.

[andrewsg]

Going to take a crack at this at the PyCon 2014 sprints.

[andrewsg]

The problem with the AttributeError seems to actually be a problem with allowing the removal of a model that has M2M fields pointing to it at all, so I'm going to focus on putting checks in to make sure that doesn't happen, and also to make sure makemigrations does the right thing in that case.

The problem with the KeyError appears to only affect sqlite and has a straightforward fix.

[Stephen Burrows]

So to clarify, you'll make sure that the M2M fields get removed first, not that the migration fails to compute?

[andrewsg]

Replying to melinath:

So to clarify, you'll make sure that the M2M fields get removed first, not that the migration fails to compute?

I talked to Andrew Godwin about this today and we settled on a solution that boils down to that, yes. A migration that removes the model before the field should fail -- it should fail for a better and clearer reason than KeyError or AttributeError but it should fail. (This also applies to removing at able pointed to by a foreign key). makemigrations should be aware of this and do the right thing.

The functional changes are doneish; I'm working on regression tests now.

[andrewsg]

Fix submitted as ​https://github.com/django/django/pull/2575

[Simon Charette <charette.s@…>]

In 00e3b9a2a992ee0b7288eeeb03e7cbd52ebc6dce:

Fixed #22397 -- Issues removing M2M field with explicit through model.

Changed the migration autodetector to remove models last so that FK
and M2M fields will not be left as dangling references. Added a check
in the migration state renderer to error out in the presence of
dangling references instead of leaving them as strings. Fixed a bug
in the sqlite backend to handle the deletion of M2M fields with
"through" models properly (i.e., do nothing successfully).

Thanks to melinath for report, loic for tests and andrewgodwin and
charettes for assistance with architecture.

[Simon Charette <charette.s@…>]

In 0d397e5a5b98c475b664ff12f11729671fe1ad42:

Revert "Fixed #22397 -- Issues removing M2M field with explicit through model."

This reverts commit 00e3b9a2a992ee0b7288eeeb03e7cbd52ebc6dce.

It's causing a regression when tested with the proxy_model_inheritance tests.

[loic84]

Dunno if you noticed my mentions on IRC yesterday but I suspected it was because of a clash with the models in test_dangling_references_throw_error .

I can reproduce the error by running runtests.py proxy_model_inheritance migrations, but if I rename the 3 models Author > XYZAuthor, Book > XYZBook, Magazine > XYZMagazine, it seems to work.

[Simon Charette]

Sorry I missed your mention on IRC, I identified proxy_model_inheritance as the source of the conflict (thanks to bisect) but I stopped there.

I reverted the commit until we figure out the correct fix for this in order to avoid blocking CI for everyone.

[loic84]

No problem, actually I think it's a good opportunity to investigate, I suspect we have a few issues with testing at the intersection of app-loading and migrations.

I'm pretty sure I had this issue in passing ("table already exist") when figuring out #22462.

[Simon Charette]

Looks like the following is solving it:

diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py
index 045ac6b..1c0224a 100644
--- a/tests/migrations/test_state.py
+++ b/tests/migrations/test_state.py
@@ -293,14 +293,25 @@ class StateTests(TestCase):
         self.assertEqual(project_state == other_state, False)
 
     def test_dangling_references_throw_error(self):
+        new_apps = Apps()
+
         class Author(models.Model):
             name = models.TextField()
+            class Meta:
+                app_label = "migrations"
+                apps = new_apps
 
         class Book(models.Model):
             author = models.ForeignKey(Author)
+            class Meta:
+                app_label = "migrations"
+                apps = new_apps
 
         class Magazine(models.Model):
             authors = models.ManyToManyField(Author)
+            class Meta:
+                app_label = "migrations"
+                apps = new_apps
 
         # Make a valid ProjectState and render it
         project_state = ProjectState()

[Simon Charette <charette.s@…>]

In 956bd644249337b9467c017aac4eec228dde8c5d:

Fixed #22397 -- Issues removing M2M field with explicit through model

Changed the migration autodetector to remove models last so that FK
and M2M fields will not be left as dangling references. Added a check
in the migration state renderer to error out in the presence of
dangling references instead of leaving them as strings. Fixed a bug
in the sqlite backend to handle the deletion of M2M fields with
"through" models properly (i.e., do nothing successfully).

Thanks to melinath for report, loic for tests and andrewgodwin and
charettes for assistance with architecture.

[Simon Charette <charette.s@…>]

In bc5d568e1e5c8f109bc83c1cdd943de60b66f763:

[1.7.x] Fixed #22397 -- Issues removing M2M field with explicit through model

Changed the migration autodetector to remove models last so that FK
and M2M fields will not be left as dangling references. Added a check
in the migration state renderer to error out in the presence of
dangling references instead of leaving them as strings. Fixed a bug
in the sqlite backend to handle the deletion of M2M fields with
"through" models properly (i.e., do nothing successfully).

Thanks to melinath for report, loic for tests and andrewgodwin and
charettes for assistance with architecture.

Backport of 956bd64424 from master

[Stephen Burrows]

I'm sorry to say that this commit seems to have broken migrations for me altogether. (Checked with git bisect.)

$ ./manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: floppyforms, sessions, admin, auth, zenaida, contenttypes
  Apply all migrations: brambling, auth
Synchronizing apps without migrations:
  Creating tables...
    Creating table django_admin_log
    Creating table auth_permission
    Creating table auth_group_permissions
    Creating table auth_group
    Creating table django_content_type
    Creating table django_session
  Installing custom SQL...
  Installing indexes...
Running migrations:
  Applying brambling.0001_initial... OK
  Applying brambling.0002_eventhousing_eventperson_home_housingslot_itemoption_payment_person_persondiscount...Traceback (most recent call last):
  File "./manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File ".../django/django/core/management/__init__.py", line 427, in execute_from_command_line
    utility.execute()
  File ".../django/django/core/management/__init__.py", line 419, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File ".../django/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File ".../django/django/core/management/base.py", line 337, in execute
    output = self.handle(*args, **options)
  File ".../django/django/core/management/commands/migrate.py", line 145, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File ".../django/django/db/migrations/executor.py", line 60, in migrate
    self.apply_migration(migration, fake=fake)
  File ".../django/django/db/migrations/executor.py", line 88, in apply_migration
    if self.detect_soft_applied(migration):
  File ".../django/django/db/migrations/executor.py", line 132, in detect_soft_applied
    apps = project_state.render()
  File ".../django/django/db/migrations/state.py", line 63, in render
    model=dangling_lookup[0]))
ValueError: Lookup failed for model referenced by field auth.Permission.content_type: contenttypes.ContentType

[Stephen Burrows]

Seems to be related to my use of a custom AUTH_USER_MODEL. I'll file as a new bug.

[Stephen Burrows]

Filed as #22518

[Markus Holtermann <info@…>]

In 8e352876:

Refs #22397 -- Removed model in test cleanup

The test was failing when using --keepdb due to a pre-existing
PonyStables model.

Thanks Florian Apolloner for the report

[Markus Holtermann <info@…>]

In 38e1169:

[1.11.x] Refs #22397 -- Removed model in test cleanup

The test was failing when using --keepdb due to a pre-existing
PonyStables model.

Thanks Florian Apolloner for the report

Backport of 8e352876c337332b45a72da8bbccad2830c7b1e0 from master.