diff options
| -rw-r--r-- | django/db/backends/sqlite3/schema.py | 12 | ||||
| -rw-r--r-- | django/db/migrations/autodetector.py | 20 | ||||
| -rw-r--r-- | django/db/migrations/migration.py | 1 | ||||
| -rw-r--r-- | django/db/migrations/state.py | 10 | ||||
| -rw-r--r-- | tests/migrations/test_autodetector.py | 57 | ||||
| -rw-r--r-- | tests/migrations/test_operations.py | 54 | ||||
| -rw-r--r-- | tests/migrations/test_state.py | 30 |
7 files changed, 158 insertions, 26 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..4ebaa431ba 100644 --- a/django/db/migrations/migration.py +++ b/django/db/migrations/migration.py @@ -127,6 +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..2b8e940c4e 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,18 +556,65 @@ 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), 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") 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) + 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 @@ 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..045ac6be10 100644 --- a/tests/migrations/test_state.py +++ b/tests/migrations/test_state.py @@ -291,3 +291,33 @@ class StateTests(TestCase): )) self.assertNotEqual(project_state, other_state) self.assertEqual(project_state == other_state, False) + + def test_dangling_references_throw_error(self): + class Author(models.Model): + name = models.TextField() + + class Book(models.Model): + author = models.ForeignKey(Author) + + class Magazine(models.Model): + authors = models.ManyToManyField(Author) + + # 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() |