Opened 11 years ago

Closed 11 years ago

Last modified 7 years ago

#22397 closed Bug (fixed)

Issues removing M2M field with explicit through model

Reported by: Stephen Burrows Owned by: andrewsg
Component: Migrations Version: dev
Severity: Release blocker Keywords:
Cc: Stephen Burrows, 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 (21)

comment:1 by loic84, 11 years ago

@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 11 years ago by loic84 (previous) (diff)

comment:2 by Stephen Burrows, 11 years ago

I get the same error on master.

comment:3 by loic84, 11 years ago

Cc: loic@… added
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Version: 1.7-beta-1master

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 by andrewsg, 11 years ago

Owner: changed from nobody to andrewsg
Status: newassigned

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

comment:5 by andrewsg, 11 years ago

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 by Stephen Burrows, 11 years ago

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

in reply to:  6 comment:7 by andrewsg, 11 years ago

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.

in reply to:  6 comment:8 by andrewsg, 11 years ago

comment:9 by Simon Charette <charette.s@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

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 by Simon Charette <charette.s@…>, 11 years ago

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 by loic84, 11 years ago

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 by Simon Charette, 11 years ago

Resolution: fixed
Status: closednew

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 by loic84, 11 years ago

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 11 years ago by loic84 (previous) (diff)

comment:14 by Simon Charette, 11 years ago

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 by Simon Charette <charette.s@…>, 11 years ago

Resolution: fixed
Status: newclosed

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 by Simon Charette <charette.s@…>, 11 years ago

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 by Stephen Burrows, 11 years ago

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 by Stephen Burrows, 11 years ago

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

comment:19 by Stephen Burrows, 11 years ago

Filed as #22518

comment:20 by Markus Holtermann <info@…>, 7 years ago

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

comment:21 by Markus Holtermann <info@…>, 7 years ago

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.

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