Fixed #23938 -- Added migration support for m2m to concrete fields and vice versa

Thanks to Michael D. Hoyle for the report and Tim Graham for the review.

django/db/migrations/autodetector.py

@@ -773,73 +773,71 @@ class MigrationAutodetector(object):
         Fields that have been added
         """
         for app_label, model_name, field_name in sorted(self.new_field_keys - self.old_field_keys):
-            field = self.new_apps.get_model(app_label, model_name)._meta.get_field_by_name(field_name)[0]
+            self._generate_added_field(app_label, model_name, field_name)
-            # Fields that are foreignkeys/m2ms depend on stuff
+
-            dependencies = []
+    def _generate_added_field(self, app_label, model_name, field_name):
-            if field.rel and field.rel.to:
+        field = self.new_apps.get_model(app_label, model_name)._meta.get_field_by_name(field_name)[0]
-                # Account for FKs to swappable models
+        # Fields that are foreignkeys/m2ms depend on stuff
-                swappable_setting = getattr(field, 'swappable_setting', None)
+        dependencies = []
-                if swappable_setting is not None:
+        if field.rel and field.rel.to:
-                    dep_app_label = "__setting__"
+            # Account for FKs to swappable models
-                    dep_object_name = swappable_setting
+            swappable_setting = getattr(field, 'swappable_setting', None)
-                else:
+            if swappable_setting is not None:
-                    dep_app_label = field.rel.to._meta.app_label
+                dep_app_label = "__setting__"
-                    dep_object_name = field.rel.to._meta.object_name
+                dep_object_name = swappable_setting
-                dependencies = [(dep_app_label, dep_object_name, None, True)]
-                if getattr(field.rel, "through", None) and not field.rel.through._meta.auto_created:
-                    dependencies.append((
-                        field.rel.through._meta.app_label,
-                        field.rel.through._meta.object_name,
-                        None,
-                        True
-                    ))
-            # You can't just add NOT NULL fields with no default or fields
-            # which don't allow empty strings as default.
-            if (not field.null and not field.has_default() and
-                    not isinstance(field, models.ManyToManyField) and
-                    not (field.blank and field.empty_strings_allowed)):
-                field = field.clone()
-                field.default = self.questioner.ask_not_null_addition(field_name, model_name)
-                self.add_operation(
-                    app_label,
-                    operations.AddField(
-                        model_name=model_name,
-                        name=field_name,
-                        field=field,
-                        preserve_default=False,
-                    ),
-                    dependencies=dependencies,
-                )
             else:
-                self.add_operation(
+                dep_app_label = field.rel.to._meta.app_label
-                    app_label,
+                dep_object_name = field.rel.to._meta.object_name
-                    operations.AddField(
+            dependencies = [(dep_app_label, dep_object_name, None, True)]
-                        model_name=model_name,
+            if getattr(field.rel, "through", None) and not field.rel.through._meta.auto_created:
-                        name=field_name,
+                dependencies.append((
-                        field=field,
+                    field.rel.through._meta.app_label,
-                    ),
+                    field.rel.through._meta.object_name,
-                    dependencies=dependencies,
+                    None,
-                )
+                    True,
+                ))
+        # You can't just add NOT NULL fields with no default or fields
+        # which don't allow empty strings as default.
+        preserve_default = True
+        if (not field.null and not field.has_default() and
+                not isinstance(field, models.ManyToManyField) and
+                not (field.blank and field.empty_strings_allowed)):
+            field = field.clone()
+            field.default = self.questioner.ask_not_null_addition(field_name, model_name)
+            preserve_default = False
+        self.add_operation(
+            app_label,
+            operations.AddField(
+                model_name=model_name,
+                name=field_name,
+                field=field,
+                preserve_default=preserve_default,
+            ),
+            dependencies=dependencies,
+        )
 
     def generate_removed_fields(self):
         """
         Fields that have been removed.
         """
         for app_label, model_name, field_name in sorted(self.old_field_keys - self.new_field_keys):
-            self.add_operation(
+            self._generate_removed_field(app_label, model_name, field_name)
-                app_label,
+
-                operations.RemoveField(
+    def _generate_removed_field(self, app_label, model_name, field_name):
-                    model_name=model_name,
+        self.add_operation(
-                    name=field_name,
+            app_label,
-                ),
+            operations.RemoveField(
-                # We might need to depend on the removal of an
+                model_name=model_name,
-                # order_with_respect_to or index/unique_together operation;
+                name=field_name,
-                # this is safely ignored if there isn't one
+            ),
-                dependencies=[
+            # We might need to depend on the removal of an
-                    (app_label, model_name, field_name, "order_wrt_unset"),
+            # order_with_respect_to or index/unique_together operation;
-                    (app_label, model_name, field_name, "foo_together_change"),
+            # this is safely ignored if there isn't one
-                ],
+            dependencies=[
-            )
+                (app_label, model_name, field_name, "order_wrt_unset"),
+                (app_label, model_name, field_name, "foo_together_change"),
+            ],
+        )
 
     def generate_altered_fields(self):
         """
@@ -863,25 +861,30 @@ class MigrationAutodetector(object):
             old_field_dec = self.deep_deconstruct(old_field)
             new_field_dec = self.deep_deconstruct(new_field)
             if old_field_dec != new_field_dec:
-                preserve_default = True
+                if (not isinstance(old_field, models.ManyToManyField) and
-                if (old_field.null and not new_field.null and not new_field.has_default() and
                         not isinstance(new_field, models.ManyToManyField)):
-                    field = new_field.clone()
+                    preserve_default = True
-                    new_default = self.questioner.ask_not_null_alteration(field_name, model_name)
+                    if (old_field.null and not new_field.null and not new_field.has_default() and
-                    if new_default is not models.NOT_PROVIDED:
+                            not isinstance(new_field, models.ManyToManyField)):
-                        field.default = new_default
+                        field = new_field.clone()
-                        preserve_default = False
+                        new_default = self.questioner.ask_not_null_alteration(field_name, model_name)
-                else:
+                        if new_default is not models.NOT_PROVIDED:
-                    field = new_field
+                            field.default = new_default
-                self.add_operation(
+                            preserve_default = False
-                    app_label,
+                    else:
-                    operations.AlterField(
+                        field = new_field
-                        model_name=model_name,
+                    self.add_operation(
-                        name=field_name,
+                        app_label,
-                        field=field,
+                        operations.AlterField(
-                        preserve_default=preserve_default,
+                            model_name=model_name,
+                            name=field_name,
+                            field=field,
+                            preserve_default=preserve_default,
+                        )
                     )
-                )
+                else:
+                    self._generate_removed_field(app_label, model_name, field_name)
+                    self._generate_added_field(app_label, model_name, field_name)
 
     def _generate_altered_foo_together(self, operation):
         option_name = operation.option_name

docs/releases/1.7.2.txt

@@ -44,7 +44,7 @@ Bugfixes
 * Fixed a migration crash that prevented changing a nullable field with a
   default to non-nullable with the same default (:ticket:`23738`).
 
-* Fixed a migrations crash when adding ``GeometryField``\s with ``blank=True``
+* Fixed a migration crash when adding ``GeometryField``\s with ``blank=True``
   on PostGIS (:ticket:`23731`).
 
 * Allowed usage of ``DateTimeField()`` as ``Transform.output_field``
@@ -63,7 +63,7 @@ Bugfixes
 * Fixed a migration crash when a field is renamed that is part of an
   ``index_together`` (:ticket:`23859`).
 
-* Fixed :djadmin:`squashmigrations` to respect the  ``--no-optimize`` parameter
+* Fixed :djadmin:`squashmigrations` to respect the ``--no-optimize`` parameter
   (:ticket:`23799`).
 
 * Made :class:`~django.db.migrations.operations.RenameModel` reversible
@@ -144,7 +144,7 @@ Bugfixes
 * ``makemigrations`` no longer prompts for a default value when adding
   ``TextField()`` or ``CharField()`` without a ``default`` (:ticket:`23405`).
 
-* Fixed migration crash when adding ``order_with_respect_to`` to a table
+* Fixed a migration crash when adding ``order_with_respect_to`` to a table
   with existing rows (:ticket:`23983`).
 
 * Restored the ``pre_migrate`` signal if all apps have migrations
@@ -181,3 +181,6 @@ Bugfixes
 
 * Supported strings escaped by third-party libraries with the ``__html__``
   convention in the template engine (:ticket:`23831`).
+
+* Fixed a migration crash when changing a ``ManyToManyField`` into a concrete
+  field and vice versa (:ticket:`23938`).

tests/migrations/test_autodetector.py

@@ -126,6 +126,10 @@ class AutodetectorTests(TestCase):
         ("id", models.AutoField(primary_key=True)),
         ("publishers", models.ManyToManyField("testapp.Publisher", through="testapp.Contract")),
     ])
+    author_with_former_m2m = ModelState("testapp", "Author", [
+        ("id", models.AutoField(primary_key=True)),
+        ("publishers", models.CharField(max_length=100)),
+    ])
     author_with_options = ModelState("testapp", "Author", [
         ("id", models.AutoField(primary_key=True)),
     ], {
@@ -1326,6 +1330,39 @@ class AutodetectorTests(TestCase):
         self.assertOperationAttributes(changes, "testapp", 0, 3, name="Author")
         self.assertOperationAttributes(changes, "testapp", 0, 4, name="Contract")
 
+    def test_concrete_field_changed_to_many_to_many(self):
+        """
+        #23938 - Tests that changing a concrete field into a ManyToManyField
+        first removes the concrete field and then adds the m2m field.
+        """
+        before = self.make_project_state([self.author_with_former_m2m])
+        after = self.make_project_state([self.author_with_m2m, self.publisher])
+        autodetector = MigrationAutodetector(before, after)
+        changes = autodetector._detect_changes()
+        # Right number/type of migrations?
+        self.assertNumberMigrations(changes, "testapp", 1)
+        self.assertOperationTypes(changes, "testapp", 0, ["CreateModel", "RemoveField", "AddField"])
+        self.assertOperationAttributes(changes, 'testapp', 0, 0, name='Publisher')
+        self.assertOperationAttributes(changes, 'testapp', 0, 1, name="publishers", model_name='author')
+        self.assertOperationAttributes(changes, 'testapp', 0, 2, name="publishers", model_name='author')
+
+    def test_many_to_many_changed_to_concrete_field(self):
+        """
+        #23938 - Tests that changing a ManyToManyField into a concrete field
+        first removes the m2m field and then adds the concrete field.
+        """
+        before = self.make_project_state([self.author_with_m2m, self.publisher])
+        after = self.make_project_state([self.author_with_former_m2m])
+        autodetector = MigrationAutodetector(before, after)
+        changes = autodetector._detect_changes()
+        # Right number/type of migrations?
+        self.assertNumberMigrations(changes, "testapp", 1)
+        self.assertOperationTypes(changes, "testapp", 0, ["RemoveField", "AddField", "DeleteModel"])
+        self.assertOperationAttributes(changes, 'testapp', 0, 0, name="publishers", model_name='author')
+        self.assertOperationAttributes(changes, 'testapp', 0, 1, name="publishers", model_name='author')
+        self.assertOperationAttributes(changes, 'testapp', 0, 2, name='Publisher')
+        self.assertOperationFieldAttributes(changes, 'testapp', 0, 1, max_length=100)
+
     def test_non_circular_foreignkey_dependency_removal(self):
         """
         If two models with a ForeignKey from one to the other are removed at the