#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:3 by , 11 years ago
Cc: | added |
---|---|
Severity: | Normal → Release blocker |
Triage Stage: | Unreviewed → Accepted |
Version: | 1.7-beta-1 → 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 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Going to take a crack at this at the PyCon 2014 sprints.
comment:5 by , 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.
follow-ups: 7 8 comment:6 by , 11 years ago
So to clarify, you'll make sure that the M2M fields get removed first, not that the migration fails to compute?
comment:7 by , 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.
comment:9 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:11 by , 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 , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 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.
comment:14 by , 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 , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:17 by , 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 , 11 years ago
Seems to be related to my use of a custom AUTH_USER_MODEL. I'll file as a new bug.
@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.