Fixed #22970: Incorrect dependencies for existing migrated apps

django/db/migrations/autodetector.py

@@ -47,7 +47,7 @@ class MigrationAutodetector(object):
         Takes a graph to base names on and an optional set of apps
         to try and restrict to (restriction is not guaranteed)
         """
-        changes = self._detect_changes(convert_apps)
+        changes = self._detect_changes(convert_apps, graph)
         changes = self.arrange_for_graph(changes, graph)
         if trim_to_apps:
             changes = self._trim_to_apps(changes, trim_to_apps)
@@ -90,7 +90,7 @@ class MigrationAutodetector(object):
             fields_def.append(deconstruction)
         return fields_def
 
-    def _detect_changes(self, convert_apps=None):
+    def _detect_changes(self, convert_apps=None, graph=None):
         """
         Returns a dict of migration plans which will achieve the
         change from from_state to to_state. The dict has app labels
@@ -98,6 +98,12 @@ class MigrationAutodetector(object):
 
         The resulting migrations aren't specially named, but the names
         do matter for dependencies inside the set.
+
+        convert_apps is the list of apps to convert to use migrations
+        (i.e. to make initial migrations for, in the usual case)
+
+        graph is an optional argument that, if provided, can help improve
+        dependency generation and avoid potential circular dependencies.
         """
 
         # The first phase is generating all the operations for each app
@@ -245,10 +251,16 @@ class MigrationAutodetector(object):
                                 if self.migrations.get(dep[0], None):
                                     operation_dependencies.add((dep[0], self.migrations[dep[0]][-1].name))
                                 else:
-                                    # If we can't find the other app, we add a __first__ dependency,
+                                    # If we can't find the other app, we add a first/last dependency,
                                     # but only if we've already been through once and checked everything
                                     if chop_mode:
-                                        operation_dependencies.add((dep[0], "__first__"))
+                                        # If the app already exists, we add __last__, as we don't know which
+                                        # migration contains the target field.
+                                        # If it's not yet migrated or has no migrations, we use __first__
+                                        if graph and not graph.root_nodes(dep[0]):
+                                            operation_dependencies.add((dep[0], "__first__"))
+                                        else:
+                                            operation_dependencies.add((dep[0], "__last__"))
                                     else:
                                         deps_satisfied = False
                     if deps_satisfied:

tests/migrations/models.py

@@ -42,3 +42,11 @@ class UnserializableModel(models.Model):
     class Meta:
         # Disable auto loading of this model as we load it on our own
         apps = Apps()
+
+
+class UnmigratedModel(models.Model):
+    """
+    A model that is in a migration-less app (which this app is
+    if its migrations directory has not been repointed)
+    """
+    pass

tests/migrations/test_autodetector.py

@@ -4,7 +4,8 @@ from django.db.migrations.autodetector import MigrationAutodetector
 from django.db.migrations.questioner import MigrationQuestioner
 from django.db.migrations.state import ProjectState, ModelState
 from django.db.migrations.graph import MigrationGraph
-from django.db import models
+from django.db.migrations.loader import MigrationLoader
+from django.db import models, connection
 from django.contrib.auth.models import AbstractBaseUser
 
 
@@ -58,6 +59,7 @@ class AutodetectorTests(TestCase):
     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_proxy_fk = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("thirdapp.AuthorProxy")), ("title", models.CharField(max_length=200))])
+    book_migrations_fk = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("migrations.UnmigratedModel")), ("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))])
@@ -1017,3 +1019,42 @@ class AutodetectorTests(TestCase):
         self.assertOperationTypes(changes, 'testapp', 0, ["CreateModel", "CreateModel"])
         self.assertOperationAttributes(changes, 'testapp', 0, 0, name="Author")
         self.assertOperationAttributes(changes, 'testapp', 0, 1, name="Aardvark")
+
+    def test_first_dependency(self):
+        """
+        Tests that a dependency to an app with no migrations uses __first__.
+        """
+        # Load graph
+        loader = MigrationLoader(connection)
+        # Make state
+        before = self.make_project_state([])
+        after = self.make_project_state([self.book_migrations_fk])
+        after.real_apps = ["migrations"]
+        autodetector = MigrationAutodetector(before, after)
+        changes = autodetector._detect_changes(graph=loader.graph)
+        # Right number of migrations?
+        self.assertNumberMigrations(changes, 'otherapp', 1)
+        self.assertOperationTypes(changes, 'otherapp', 0, ["CreateModel"])
+        self.assertOperationAttributes(changes, 'otherapp', 0, 0, name="Book")
+        # Right dependencies?
+        self.assertEqual(changes['otherapp'][0].dependencies, [("migrations", "__first__")])
+
+    @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations"})
+    def test_last_dependency(self):
+        """
+        Tests that a dependency to an app with existing migrations uses __first__.
+        """
+        # Load graph
+        loader = MigrationLoader(connection)
+        # Make state
+        before = self.make_project_state([])
+        after = self.make_project_state([self.book_migrations_fk])
+        after.real_apps = ["migrations"]
+        autodetector = MigrationAutodetector(before, after)
+        changes = autodetector._detect_changes(graph=loader.graph)
+        # Right number of migrations?
+        self.assertNumberMigrations(changes, 'otherapp', 1)
+        self.assertOperationTypes(changes, 'otherapp', 0, ["CreateModel"])
+        self.assertOperationAttributes(changes, 'otherapp', 0, 0, name="Book")
+        # Right dependencies?
+        self.assertEqual(changes['otherapp'][0].dependencies, [("migrations", "__last__")])

tests/migrations/test_migrations_no_changes/0003_third.py

@@ -16,8 +16,15 @@ class Migration(migrations.Migration):
             fields=[
                 ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
             ],
-            options={
-            },
+            options={},
+            bases=(models.Model,),
+        ),
+        migrations.CreateModel(
+            name='UnmigratedModel',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+            ],
+            options={},
             bases=(models.Model,),
         ),
         migrations.DeleteModel(