Fixed #24225, #24264, #24282 -- Recursively rebuilt all related models as soon as one changed

django/db/migrations/state.py

@@ -8,7 +8,9 @@
 from django.conf import settings
 from django.db import models
 from django.db.models.fields.proxy import OrderWrt
-from django.db.models.fields.related import do_pending_lookups
+from django.db.models.fields.related import (
+    RECURSIVE_RELATIONSHIP_CONSTANT, do_pending_lookups,
+)
 from django.db.models.options import DEFAULT_NAMES, normalize_together
 from django.utils import six
 from django.utils.encoding import force_text, smart_text
@@ -21,6 +23,39 @@ class InvalidBasesError(ValueError):
     pass
 
 
+def _get_app_label_and_model_name(model, app_label=''):
+    if isinstance(model, six.string_types):
+        split = model.split('.', 1)
+        return (tuple(split) if len(split) == 2 else (app_label, split[0]))
+    else:
+        return model._meta.app_label, model._meta.model_name
+
+
+def get_related_models_recursive(model):
+    """
+    Returns all models that have a direct or indirect relationship
+    to the given model.
+    """
+    def _related_models(m):
+        return [
+            f.related_model for f in m._meta.get_fields(include_parents=True, include_hidden=True)
+            if f.is_relation and not isinstance(f.related_model, six.string_types)
+        ] + [
+            subclass for subclass in m.__subclasses__()
+            if issubclass(subclass, models.Model)
+        ]
+
+    seen = set()
+    queue = _related_models(model)
+    for rel_mod in queue:
+        rel_app_label, rel_model_name = rel_mod._meta.app_label, rel_mod._meta.model_name
+        if (rel_app_label, rel_model_name) in seen:
+            continue
+        seen.add((rel_app_label, rel_model_name))
+        queue.extend(_related_models(rel_mod))
+    return seen - {(model._meta.app_label, model._meta.model_name)}
+
+
 class ProjectState(object):
     """
     Represents the entire project's overall state.
@@ -46,27 +81,57 @@ def remove_model(self, app_label, model_name):
 
     def reload_model(self, app_label, model_name):
         if 'apps' in self.__dict__:  # hasattr would cache the property
-            # Get relations before reloading the models, as _meta.apps may change
             try:
-                related_old = {
-                    f.related_model for f in
-                    self.apps.get_model(app_label, model_name)._meta.related_objects
-                }
+                old_model = self.apps.get_model(app_label, model_name)
             except LookupError:
-                related_old = set()
-            self._reload_one_model(app_label, model_name)
-            # Reload models if there are relations
-            model = self.apps.get_model(app_label, model_name)
-            related_m2m = {f.related_model for f in model._meta.many_to_many}
-            for rel_model in related_old.union(related_m2m):
-                self._reload_one_model(rel_model._meta.app_label, rel_model._meta.model_name)
-            if related_m2m:
-                # Re-render this model after related models have been reloaded
-                self._reload_one_model(app_label, model_name)
-
-    def _reload_one_model(self, app_label, model_name):
-        self.apps.unregister_model(app_label, model_name)
-        self.models[app_label, model_name].render(self.apps)
+                related_models = set()
+            else:
+                # Get all relations to and from the old model before reloading,
+                # as _meta.apps may change
+                related_models = get_related_models_recursive(old_model)
+
+            # Get all outgoing references from the model to be rendered
+            model_state = self.models[(app_label, model_name)]
+            for name, field in model_state.fields:
+                if field.is_relation:
+                    if field.rel.to == RECURSIVE_RELATIONSHIP_CONSTANT:
+                        continue
+                    rel_app_label, rel_model_name = _get_app_label_and_model_name(field.rel.to, app_label)
+                    related_models.add((rel_app_label, rel_model_name.lower()))
+
+            # Unregister all related models
+            for rel_app_label, rel_model_name in related_models:
+                self.apps.unregister_model(rel_app_label, rel_model_name)
+
+            # Unregister the current model
+            self.apps.unregister_model(app_label, model_name)
+
+            # Gather all models states of those models that will be rerendered.
+            # This includes:
+            # 1. The current model
+            try:
+                model_state = self.models[app_label, model_name]
+            except KeyError:
+                states_to_be_rendered = []
+            else:
+                states_to_be_rendered = [model_state]
+
+            # 2. All related models of unmigrated apps
+            for model_state in self.apps.real_models:
+                if (model_state.app_label, model_state.name_lower) in related_models:
+                    states_to_be_rendered.append(model_state)
+
+            # 3. All related models of migrated apps
+            for rel_app_label, rel_model_name in related_models:
+                try:
+                    model_state = self.models[rel_app_label, rel_model_name]
+                except KeyError:
+                    pass
+                else:
+                    states_to_be_rendered.append(model_state)
+
+            # Render all models
+            self.apps.render_multiple(states_to_be_rendered)
 
     def clone(self):
         "Returns an exact copy of this ProjectState"
@@ -136,21 +201,42 @@ def __init__(self, real_apps, models, ignore_swappable=False):
         # are some variables that refer to the Apps object.
         # FKs/M2Ms from real apps are also not included as they just
         # mess things up with partial states (due to lack of dependencies)
-        real_models = []
+        self.real_models = []
         for app_label in real_apps:
             app = global_apps.get_app_config(app_label)
             for model in app.get_models():
-                real_models.append(ModelState.from_model(model, exclude_rels=True))
+                self.real_models.append(ModelState.from_model(model, exclude_rels=True))
         # Populate the app registry with a stub for each application.
         app_labels = {model_state.app_label for model_state in models.values()}
         app_configs = [AppConfigStub(label) for label in sorted(real_apps + list(app_labels))]
         super(StateApps, self).__init__(app_configs)
 
+        self.render_multiple(list(models.values()) + self.real_models)
+
+        # If there are some lookups left, see if we can first resolve them
+        # ourselves - sometimes fields are added after class_prepared is sent
+        for lookup_model, operations in self._pending_lookups.items():
+            try:
+                model = self.get_model(lookup_model[0], lookup_model[1])
+            except LookupError:
+                app_label = "%s.%s" % (lookup_model[0], lookup_model[1])
+                if app_label == settings.AUTH_USER_MODEL and ignore_swappable:
+                    continue
+                # 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"
+                msg = "Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}"
+                raise ValueError(msg.format(field=operations[0][1], model=lookup_model))
+            else:
+                do_pending_lookups(model)
+
+    def render_multiple(self, model_states):
         # We keep trying to render the models in a loop, ignoring invalid
         # base errors, until the size of the unrendered models doesn't
         # decrease by at least one, meaning there's a base dependency loop/
         # missing base.
-        unrendered_models = list(models.values()) + real_models
+        unrendered_models = model_states
         while unrendered_models:
             new_unrendered_models = []
             for model in unrendered_models:
@@ -167,31 +253,15 @@ def __init__(self, real_apps, models, ignore_swappable=False):
                 )
             unrendered_models = new_unrendered_models
 
-        # If there are some lookups left, see if we can first resolve them
-        # ourselves - sometimes fields are added after class_prepared is sent
-        for lookup_model, operations in self._pending_lookups.items():
-            try:
-                model = self.get_model(lookup_model[0], lookup_model[1])
-            except LookupError:
-                app_label = "%s.%s" % (lookup_model[0], lookup_model[1])
-                if app_label == settings.AUTH_USER_MODEL and ignore_swappable:
-                    continue
-                # 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"
-                msg = "Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}"
-                raise ValueError(msg.format(field=operations[0][1], model=lookup_model))
-            else:
-                do_pending_lookups(model)
-
     def clone(self):
         """
         Return a clone of this registry, mainly used by the migration framework.
         """
         clone = StateApps([], {})
         clone.all_models = copy.deepcopy(self.all_models)
         clone.app_configs = copy.deepcopy(self.app_configs)
+        # No need to actually clone them, they'll never change
+        clone.real_models = self.real_models
         return clone
 
     def register_model(self, app_label, model):

tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0001_initial.py

@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='Author',
+            fields=[
+                ('id', models.AutoField(serialize=False, auto_created=True, primary_key=True)),
+                ('name', models.CharField(max_length=50)),
+            ],
+        ),
+    ]

tests/migrations/migrations_test_apps/alter_fk/author_app/migrations/0002_alter_id.py

@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('author_app', '0001_initial'),
+        ('book_app', '0001_initial'),  # Forces the book table to alter the FK
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='author',
+            name='id',
+            field=models.CharField(max_length=10, primary_key=True),
+        ),
+    ]

tests/migrations/migrations_test_apps/alter_fk/book_app/migrations/0001_initial.py

@@ -0,0 +1,22 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('author_app', '0001_initial'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='Book',
+            fields=[
+                ('id', models.AutoField(serialize=False, auto_created=True, primary_key=True)),
+                ('title', models.CharField(max_length=50)),
+                ('author', models.ForeignKey('author_app.Author')),
+            ],
+        ),
+    ]

tests/migrations/test_executor.py

@@ -376,6 +376,41 @@ def callback(*args):
         ]
         self.assertEqual(call_args_list, expected)
 
+    @override_settings(
+        INSTALLED_APPS=[
+            "migrations.migrations_test_apps.alter_fk.author_app",
+            "migrations.migrations_test_apps.alter_fk.book_app",
+        ]
+    )
+    def test_alter_id_type_with_fk(self):
+        try:
+            executor = MigrationExecutor(connection)
+            self.assertTableNotExists("author_app_author")
+            self.assertTableNotExists("book_app_book")
+            # Apply initial migrations
+            executor.migrate([
+                ("author_app", "0001_initial"),
+                ("book_app", "0001_initial"),
+            ])
+            self.assertTableExists("author_app_author")
+            self.assertTableExists("book_app_book")
+            # Rebuild the graph to reflect the new DB state
+            executor.loader.build_graph()
+
+            # Apply PK type alteration
+            executor.migrate([("author_app", "0002_alter_id")])
+
+            # Rebuild the graph to reflect the new DB state
+            executor.loader.build_graph()
+        finally:
+            # We can't simply unapply the migrations here because there is no
+            # implicit cast from VARCHAR to INT on the database level.
+            with connection.schema_editor() as editor:
+                editor.execute(editor.sql_delete_table % {"table": "book_app_book"})
+                editor.execute(editor.sql_delete_table % {"table": "author_app_author"})
+            self.assertTableNotExists("author_app_author")
+            self.assertTableNotExists("book_app_book")
+
 
 class FakeLoader(object):
     def __init__(self, graph, applied):

tests/migrations/test_operations.py

@@ -1659,6 +1659,60 @@ def inner_method(models, schema_editor):
         self.assertEqual(definition[1], [])
         self.assertEqual(sorted(definition[2]), ["atomic", "code"])
 
+    def test_run_python_related_assignment(self):
+        """
+        #24282 - Tests that model changes to a FK reverse side update the model
+        on the FK side as well.
+        """
+
+        def inner_method(models, schema_editor):
+            Author = models.get_model("test_authors", "Author")
+            Book = models.get_model("test_books", "Book")
+            author = Author.objects.create(name="Hemingway")
+            Book.objects.create(title="Old Man and The Sea", author=author)
+
+        create_author = migrations.CreateModel(
+            "Author",
+            [
+                ("id", models.AutoField(primary_key=True)),
+                ("name", models.CharField(max_length=100)),
+            ],
+            options={},
+        )
+        create_book = migrations.CreateModel(
+            "Book",
+            [
+                ("id", models.AutoField(primary_key=True)),
+                ("title", models.CharField(max_length=100)),
+                ("author", models.ForeignKey("test_authors.Author"))
+            ],
+            options={},
+        )
+        add_hometown = migrations.AddField(
+            "Author",
+            "hometown",
+            models.CharField(max_length=100),
+        )
+        create_old_man = migrations.RunPython(inner_method, inner_method)
+
+        project_state = ProjectState()
+        new_state = project_state.clone()
+        with connection.schema_editor() as editor:
+            create_author.state_forwards("test_authors", new_state)
+            create_author.database_forwards("test_authors", editor, project_state, new_state)
+            project_state = new_state
+            new_state = new_state.clone()
+            create_book.state_forwards("test_books", new_state)
+            create_book.database_forwards("test_books", editor, project_state, new_state)
+            project_state = new_state
+            new_state = new_state.clone()
+            add_hometown.state_forwards("test_authors", new_state)
+            add_hometown.database_forwards("test_authors", editor, project_state, new_state)
+            project_state = new_state
+            new_state = new_state.clone()
+            create_old_man.state_forwards("test_books", new_state)
+            create_old_man.database_forwards("test_books", editor, project_state, new_state)
+
     def test_run_python_noop(self):
         """
         #24098 - Tests no-op RunPython operations.