From bc5d568e1e5c8f109bc83c1cdd943de60b66f763 Mon Sep 17 00:00:00 2001 From: Andrew Gorcester Date: Tue, 8 Apr 2014 12:30:25 +0700 Subject: [PATCH] [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 --- django/db/backends/sqlite3/schema.py | 12 ++++-- django/db/migrations/autodetector.py | 20 ++++----- django/db/migrations/migration.py | 2 +- django/db/migrations/state.py | 10 +++++ tests/migrations/test_autodetector.py | 59 +++++++++++++++++++++++++-- tests/migrations/test_operations.py | 56 ++++++++++++++++++++----- tests/migrations/test_state.py | 41 +++++++++++++++++++ 7 files changed, 171 insertions(+), 29 deletions(-) diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py index 216ff0958a..a8417d4f66 100644 --- a/django/db/backends/sqlite3/schema.py +++ b/django/db/backends/sqlite3/schema.py @@ -121,11 +121,15 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor): 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): """ diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index adf3686d59..134dcfed95 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -223,16 +223,6 @@ class MigrationAutodetector(object): 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 @@ class MigrationAutodetector(object): ) 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:]): diff --git a/django/db/migrations/migration.py b/django/db/migrations/migration.py index 0114dba60c..e9af9cd994 100644 --- a/django/db/migrations/migration.py +++ b/django/db/migrations/migration.py @@ -127,7 +127,7 @@ class Migration(object): 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): """ diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py index d146bfd8bc..86693f4398 100644 --- a/django/db/migrations/state.py +++ b/django/db/migrations/state.py @@ -51,6 +51,16 @@ class ProjectState(object): 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: + # 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 diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index a56e31568a..b033ed424f 100644 --- a/tests/migrations/test_autodetector.py +++ b/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,7 +556,32 @@ class AutodetectorTests(TestCase): """ # 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), 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[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? @@ -560,10 +589,32 @@ class AutodetectorTests(TestCase): # Right number of actions? migration = changes['testapp'][0] self.assertEqual(len(migration.operations), 2) - # Right actions? + # Right actions in right order? action = migration.operations[0] - self.assertEqual(action.__class__.__name__, "AddField") + 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, "publisher_name") + self.assertEqual(action.name, "authors") + action = migration.operations[1] + self.assertEqual(action.__class__.__name__, "DeleteModel") + self.assertEqual(action.name, "Attribution") diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index 6472fea42c..ec0d046df8 100644 --- a/tests/migrations/test_operations.py +++ b/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) - - # Set up the database + 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.apply(project_state, editor) + + def unapply_operations(self, app_label, project_state, operations): + migration = Migration('name', app_label) + migration.operations = operations + with connection.schema_editor() as editor: + 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 @@ class OperationTests(MigrationTestBase): 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. diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py index 10f5e7d9ab..1c0224ad52 100644 --- a/tests/migrations/test_state.py +++ b/tests/migrations/test_state.py @@ -291,3 +291,44 @@ class StateTests(TestCase): )) 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()