Commit 00e3b9a2 authored by Andrew Gorcester's avatar Andrew Gorcester Committed by Simon Charette
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.
parent 47927eb7

django/db/backends/sqlite3/schema.py

+8 −4
Original line number Diff line number Diff line
@@ -121,10 +121,14 @@ 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.

        else:

            self._remake_table(model, delete_fields=[field])



    def alter_field(self, model, old_field, new_field, strict=False):

django/db/migrations/autodetector.py

+10 −10
Original line number Diff line number Diff line
@@ -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:]):

django/db/migrations/migration.py

+1 −0
Original line number Diff line number Diff line
@@ -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):

django/db/migrations/state.py

+10 −0
Original line number Diff line number Diff line
@@ -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

tests/migrations/test_autodetector.py

+54 −3
Original line number Diff line number Diff line
@@ -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")