Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit 956bd644249337b9467c017aac4eec228dde8c5d 1 parent 26d118c
@andrewsg andrewsg authored charettes committed
View
12 django/db/backends/sqlite3/schema.py
@@ -121,11 +121,15 @@ def remove_field(self, model, field):
Removes a field from a model. Usually involves deleting a column,
but for M2Ms may involve deleting a table.
"""
- # Special-case implicit M2M tables
- if isinstance(field, ManyToManyField) and field.rel.through._meta.auto_created:
- return self.delete_model(field.rel.through)
+ # M2M fields are a special case
+ if isinstance(field, ManyToManyField):
+ # For implicit M2M tables, delete the auto-created table
+ if field.rel.through._meta.auto_created:
+ self.delete_model(field.rel.through)
+ # For explicit "through" M2M fields, do nothing
# For everything else, remake.
- self._remake_table(model, delete_fields=[field])
+ else:
+ self._remake_table(model, delete_fields=[field])
def alter_field(self, model, old_field, new_field, strict=False):
"""
View
20 django/db/migrations/autodetector.py
@@ -223,16 +223,6 @@ def _rel_agnostic_fields_def(fields):
unique_together=unique_together
)
)
- # Removing models
- removed_models = set(old_model_keys) - set(new_model_keys)
- for app_label, model_name in removed_models:
- model_state = self.from_state.models[app_label, model_name]
- self.add_to_migration(
- app_label,
- operations.DeleteModel(
- model_state.name,
- )
- )
# Changes within models
kept_models = set(old_model_keys).intersection(new_model_keys)
old_fields = set()
@@ -348,6 +338,16 @@ def _rel_agnostic_fields_def(fields):
)
for app_label, operation in unique_together_operations:
self.add_to_migration(app_label, operation)
+ # Removing models
+ removed_models = set(old_model_keys) - set(new_model_keys)
+ for app_label, model_name in removed_models:
+ model_state = self.from_state.models[app_label, model_name]
+ self.add_to_migration(
+ app_label,
+ operations.DeleteModel(
+ model_state.name,
+ )
+ )
# Alright, now add internal dependencies
for app_label, migrations in self.migrations.items():
for m1, m2 in zip(migrations, migrations[1:]):
View
2  django/db/migrations/migration.py
@@ -127,7 +127,7 @@ def unapply(self, project_state, schema_editor, collect_sql=False):
to_run.reverse()
for operation, to_state, from_state in to_run:
operation.database_backwards(self.app_label, schema_editor, from_state, to_state)
-
+ return project_state
def swappable_dependency(value):
"""
View
10 django/db/migrations/state.py
@@ -51,6 +51,16 @@ def render(self):
if len(new_unrendered_models) == len(unrendered_models):
raise InvalidBasesError("Cannot resolve bases for %r" % new_unrendered_models)
unrendered_models = new_unrendered_models
+ # make sure apps has no dangling references
+ if self.apps._pending_lookups:
@apollo13 Owner

@charettes @andrewsg This breaks as soon as your app has a FK to an unmigrated app (eg contenttypes or user), one fix seems to be:

diff --git a/django/core/management/commands/makemigrations.py b/django/core/management/commands/makemigrations.py
index bdf0b2f..2d58f46 100644
--- a/django/core/management/commands/makemigrations.py
+++ b/django/core/management/commands/makemigrations.py
@@ -53,7 +53,7 @@ class Command(BaseCommand):
         # (makemigrations doesn't look at the database state).
         # Also make sure the graph is built without unmigrated apps shoehorned in.
         loader = MigrationLoader(connections[DEFAULT_DB_ALIAS], load=False)
-        loader.build_graph(ignore_unmigrated=True)
+        loader.build_graph(ignore_unmigrated=False)

         # Before anything else, see if there's conflicting apps and drop out
         # hard if there are any and they don't want to merge

Can you confirm that this is the right fix and modify your tests to check for this?

@charettes Collaborator

I'm not sure this is the correct fix. The ignore_unmigrated kwarg was added in 2085f53 to fix #21968. I guess first thing to do in order to solve this would be to write a regression test for both cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ # Raise an error with a best-effort helpful message
+ # (only for the first issue). Error message should look like:
+ # "ValueError: Lookup failed for model referenced by
+ # field migrations.Book.author: migrations.Author"
+ dangling_lookup = list(self.apps._pending_lookups.items())[0]
+ raise ValueError("Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}".format(
+ field=dangling_lookup[1][0][1],
+ model=dangling_lookup[0]))
return self.apps
@classmethod
View
57 tests/migrations/test_autodetector.py
@@ -33,11 +33,15 @@ class AutodetectorTests(TestCase):
other_stable = ModelState("otherapp", "Stable", [("id", models.AutoField(primary_key=True))])
third_thing = ModelState("thirdapp", "Thing", [("id", models.AutoField(primary_key=True))])
book = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))])
+ book_with_no_author = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("title", models.CharField(max_length=200))])
book_with_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))])
book_with_field_and_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("writer", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))])
+ book_with_multiple_authors = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("authors", models.ManyToManyField("testapp.Author")), ("title", models.CharField(max_length=200))])
+ book_with_multiple_authors_through_attribution = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("authors", models.ManyToManyField("testapp.Author", through="otherapp.Attribution")), ("title", models.CharField(max_length=200))])
book_unique = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("author", "title")]})
book_unique_2 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "author")]})
book_unique_3 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("newfield", models.IntegerField()), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "newfield")]})
+ attribution = ModelState("otherapp", "Attribution", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("book", models.ForeignKey("otherapp.Book"))])
edition = ModelState("thirdapp", "Edition", [("id", models.AutoField(primary_key=True)), ("book", models.ForeignKey("otherapp.Book"))])
custom_user = ModelState("thirdapp", "CustomUser", [("id", models.AutoField(primary_key=True)), ("username", models.CharField(max_length=255))])
knight = ModelState("eggs", "Knight", [("id", models.AutoField(primary_key=True))])
@@ -552,18 +556,65 @@ def test_replace_string_with_foreignkey(self):
"""
# Make state
before = self.make_project_state([self.author_with_publisher_string])
- after = self.make_project_state([self.author_with_publisher])
+ after = self.make_project_state([self.author_with_publisher, self.publisher])
autodetector = MigrationAutodetector(before, after)
changes = autodetector._detect_changes()
# Right number of migrations?
self.assertEqual(len(changes['testapp']), 1)
# Right number of actions?
migration = changes['testapp'][0]
- self.assertEqual(len(migration.operations), 2)
+ self.assertEqual(len(migration.operations), 3)
# Right actions?
action = migration.operations[0]
+ self.assertEqual(action.__class__.__name__, "CreateModel")
+ self.assertEqual(action.name, "Publisher")
+ action = migration.operations[1]
self.assertEqual(action.__class__.__name__, "AddField")
self.assertEqual(action.name, "publisher")
- action = migration.operations[1]
+ action = migration.operations[2]
self.assertEqual(action.__class__.__name__, "RemoveField")
self.assertEqual(action.name, "publisher_name")
+
+ def test_foreign_key_removed_before_target_model(self):
+ """
+ Removing an FK and the model it targets in the same change must remove
+ the FK field before the model to maintain consistency.
+ """
+ before = self.make_project_state([self.author_with_publisher, self.publisher])
+ after = self.make_project_state([self.author_name]) # removes both the model and FK
+ autodetector = MigrationAutodetector(before, after)
+ changes = autodetector._detect_changes()
+ # Right number of migrations?
+ self.assertEqual(len(changes['testapp']), 1)
+ # Right number of actions?
+ migration = changes['testapp'][0]
+ self.assertEqual(len(migration.operations), 2)
+ # Right actions in right order?
+ action = migration.operations[0]
+ self.assertEqual(action.__class__.__name__, "RemoveField")
+ self.assertEqual(action.name, "publisher")
+ action = migration.operations[1]
+ self.assertEqual(action.__class__.__name__, "DeleteModel")
+ self.assertEqual(action.name, "Publisher")
+
+ def test_many_to_many_removed_before_through_model(self):
+ """
+ Removing a ManyToManyField and the "through" model in the same change must remove
+ the field before the model to maintain consistency.
+ """
+ before = self.make_project_state([self.book_with_multiple_authors_through_attribution, self.author_name, self.attribution])
+ after = self.make_project_state([self.book_with_no_author, self.author_name]) # removes both the through model and ManyToMany
+ autodetector = MigrationAutodetector(before, after)
+ changes = autodetector._detect_changes()
+ # Right number of migrations?
+ self.assertEqual(len(changes['otherapp']), 1)
+ # Right number of actions?
+ migration = changes['otherapp'][0]
+ self.assertEqual(len(migration.operations), 2)
+ # Right actions in right order?
+ action = migration.operations[0]
+ self.assertEqual(action.__class__.__name__, "RemoveField")
+ self.assertEqual(action.name, "authors")
+ action = migration.operations[1]
+ self.assertEqual(action.__class__.__name__, "DeleteModel")
+ self.assertEqual(action.name, "Attribution")
View
54 tests/migrations/test_operations.py
@@ -1,9 +1,12 @@
import unittest
-from django.db import connection, models, migrations, router
+
+from django.db import connection, migrations, models, router
+from django.db.migrations.migration import Migration
+from django.db.migrations.state import ProjectState
from django.db.models.fields import NOT_PROVIDED
from django.db.transaction import atomic
from django.db.utils import IntegrityError
-from django.db.migrations.state import ProjectState
+
from .test_base import MigrationTestBase
@@ -15,15 +18,16 @@ class OperationTests(MigrationTestBase):
"""
def apply_operations(self, app_label, project_state, operations):
- new_state = project_state.clone()
- for operation in operations:
- operation.state_forwards(app_label, new_state)
+ migration = Migration('name', app_label)
+ migration.operations = operations
+ with connection.schema_editor() as editor:
+ return migration.apply(project_state, editor)
- # Set up the database
+ def unapply_operations(self, app_label, project_state, operations):
+ migration = Migration('name', app_label)
+ migration.operations = operations
with connection.schema_editor() as editor:
- for operation in operations:
- operation.database_forwards(app_label, editor, project_state, new_state)
- return new_state
+ return migration.unapply(project_state, editor)
def set_up_test_model(self, app_label, second_model=False, related_model=False, mti_model=False):
"""
@@ -396,6 +400,38 @@ def test_alter_field_m2m(self):
Pony = new_apps.get_model("test_alflmm", "Pony")
self.assertTrue(Pony._meta.get_field('stables').blank)
+ def test_remove_field_m2m(self):
+ project_state = self.set_up_test_model("test_rmflmm", second_model=True)
+
+ project_state = self.apply_operations("test_rmflmm", project_state, operations=[
+ migrations.AddField("Pony", "stables", models.ManyToManyField("Stable", related_name="ponies"))
+ ])
+ self.assertTableExists("test_rmflmm_pony_stables")
+
+ operations = [migrations.RemoveField("Pony", "stables")]
+ self.apply_operations("test_rmflmm", project_state, operations=operations)
+ self.assertTableNotExists("test_rmflmm_pony_stables")
+
+ # And test reversal
+ self.unapply_operations("test_rmflmm", project_state, operations=operations)
+ self.assertTableExists("test_rmflmm_pony_stables")
+
+ def test_remove_field_m2m_with_through(self):
+ project_state = self.set_up_test_model("test_rmflmmwt", second_model=True)
+
+ self.assertTableNotExists("test_rmflmmwt_ponystables")
+ project_state = self.apply_operations("test_rmflmmwt", project_state, operations=[
+ migrations.CreateModel("PonyStables", fields=[
+ ("pony", models.ForeignKey('test_rmflmmwt.Pony')),
+ ("stable", models.ForeignKey('test_rmflmmwt.Stable')),
+ ]),
+ migrations.AddField("Pony", "stables", models.ManyToManyField("Stable", related_name="ponies", through='test_rmflmmwt.PonyStables'))
+ ])
+ self.assertTableExists("test_rmflmmwt_ponystables")
+
+ operations = [migrations.RemoveField("Pony", "stables")]
+ self.apply_operations("test_rmflmmwt", project_state, operations=operations)
+
def test_remove_field(self):
"""
Tests the RemoveField operation.
View
41 tests/migrations/test_state.py
@@ -291,3 +291,44 @@ def test_equality(self):
))
self.assertNotEqual(project_state, other_state)
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()
+ project_state.add_model_state(ModelState.from_model(Author))
+ project_state.add_model_state(ModelState.from_model(Book))
+ project_state.add_model_state(ModelState.from_model(Magazine))
+ rendered_state = project_state.render()
+ self.assertEqual(len(rendered_state.get_models()), 3)
+
+ # now make an invalid one with a ForeignKey
+ project_state = ProjectState()
+ project_state.add_model_state(ModelState.from_model(Book))
+ with self.assertRaises(ValueError):
+ rendered_state = project_state.render()
+
+ # and another with ManyToManyField
+ project_state = ProjectState()
+ project_state.add_model_state(ModelState.from_model(Magazine))
+ with self.assertRaises(ValueError):
+ rendered_state = project_state.render()
Please sign in to comment.
Something went wrong with that request. Please try again.