Opened 16 months ago

Closed 16 months ago

Last modified 15 months ago

#22397 closed Bug (fixed)

Issues removing M2M field with explicit through model

Reported by: melinath Owned by: andrewsg
Component: Migrations Version: master
Severity: Release blocker Keywords:
Cc: melinath, loic@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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')

Change History (19)

comment:1 Changed 16 months ago by loic84

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

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

Edit: You may need to delete the migration and recreate it.

Last edited 16 months ago by loic84 (previous) (diff)

comment:2 Changed 16 months ago by melinath

I get the same error on master.

comment:3 Changed 16 months ago by loic84

  • Cc loic@… added
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.7-beta-1 to master

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.

comment:4 Changed 16 months ago by andrewsg

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

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

comment:5 Changed 16 months ago by 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.

comment:6 follow-ups: Changed 16 months ago by melinath

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

comment:7 in reply to: ↑ 6 Changed 16 months ago by 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.

comment:8 in reply to: ↑ 6 Changed 16 months ago by andrewsg

comment:9 Changed 16 months ago by Simon Charette <charette.s@…>

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

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.

comment:10 Changed 16 months ago by 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.

comment:11 Changed 16 months ago by 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.

comment:12 Changed 16 months ago by charettes

  • Resolution fixed deleted
  • Status changed from closed to new

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.

comment:13 Changed 16 months ago by 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.

Last edited 16 months ago by loic84 (previous) (diff)

comment:14 Changed 16 months ago by charettes

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()

comment:15 Changed 16 months ago by Simon Charette <charette.s@…>

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

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.

comment:16 Changed 16 months ago by 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

comment:17 Changed 15 months ago by melinath

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

comment:18 Changed 15 months ago by melinath

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

comment:19 Changed 15 months ago by melinath

Filed as #22518

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