[Django] #22397: Issues removing M2M field with explicit through model
Django
----------------------------+------------------------
Reporter: melinath | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------
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')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: melinath | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
@melinath, this rings a bell, could you test with the master version of
Django in case it's already fixed?
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:1>
Django
Reporter: melinath | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by melinath):
I get the same error on master.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:2>
Django
Reporter: melinath | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* cc: loic@… (added)
* version: 1.7-beta-1 => master
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:3>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewsg):
* owner: nobody => andrewsg
* status: new => assigned
Comment:
Going to take a crack at this at the PyCon 2014 sprints.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:4>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:5>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by melinath):
So to clarify, you'll make sure that the M2M fields get removed first, not
that the migration fails to compute?
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:6>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewsg):
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:7>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewsg):
Fix submitted as https://github.com/django/django/pull/2575
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:8>
Django
Reporter: melinath | Owner: andrewsg
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"00e3b9a2a992ee0b7288eeeb03e7cbd52ebc6dce"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:9>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"0d397e5a5b98c475b664ff12f11729671fe1ad42"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:10>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:11>
Django
Reporter: melinath | Owner: andrewsg
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* status: closed => new
* resolution: fixed =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:12>
Django
Reporter: melinath | Owner: andrewsg
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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 #22459.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:13>
Django
Reporter: melinath | Owner: andrewsg
Component: Migrations | Version: master
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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()
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:14>
Django
Reporter: melinath | Owner: andrewsg
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"956bd644249337b9467c017aac4eec228dde8c5d"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:15>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"bc5d568e1e5c8f109bc83c1cdd943de60b66f763"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:16>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:17>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by melinath):
Seems to be related to my use of a custom AUTH_USER_MODEL. I'll file as a
new bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:18>
Django
Reporter: melinath | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by melinath):
Filed as #22518
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:19>
Django
Reporter: Stephen Burrows | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"8e352876c337332b45a72da8bbccad2830c7b1e0" 8e352876]:
{{{
#!CommitTicketReference repository=""
revision="8e352876c337332b45a72da8bbccad2830c7b1e0"
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:20>
Django
Reporter: Stephen Burrows | Owner: andrewsg
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markus Holtermann <info@…>):
In [changeset:"38e116906703540379fd7e72d5404e7a9b123b1d" 38e1169]:
{{{
#!CommitTicketReference repository=""
revision="38e116906703540379fd7e72d5404e7a9b123b1d"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22397#comment:21>