Refs #26421 -- Refactored Apps.lazy_model_operation() for better checks and tests

django/apps/registry.py

@@ -372,29 +372,35 @@ class Apps(object):
         The function passed to this method must accept exactly n models as
         arguments, where n=len(model_keys).
         """
-        # If this function depends on more than one model, we recursively turn
-        # it into a chain of functions that accept a single model argument and
-        # pass each in turn to lazy_model_operation.
-        model_key, more_models = model_keys[0], model_keys[1:]
-        if more_models:
-            supplied_fn = function
-
-            def function(model):
-                next_function = partial(supplied_fn, model)
-                # Annotate the function with its field for retrieval in
-                # migrations.state.StateApps.
-                if getattr(supplied_fn, 'keywords', None):
-                    next_function.field = supplied_fn.keywords.get('field')
-                self.lazy_model_operation(next_function, *more_models)
-
-        # If the model is already loaded, pass it to the function immediately.
-        # Otherwise, delay execution until the class is prepared.
-        try:
-            model_class = self.get_registered_model(*model_key)
-        except LookupError:
-            self._pending_operations[model_key].append(function)
+        # Base case: no arguments, just execute the function.
+        if not model_keys:
+            function()
+        # Recursive case: take the head of model_keys, wait for the
+        # corresponding model class to be imported and registered, then apply
+        # that argument to the supplied function. Pass the resulting partial
+        # to lazy_model_operation() along with the remaining model args and
+        # repeat until all models are loaded and all arguments are applied.
         else:
-            function(model_class)
+            next_model, more_models = model_keys[0], model_keys[1:]
+
+            # This will be executed after the class corresponding to next_model
+            # has been imported and registered. The `func` attribute provides
+            # duck-type compatibility with partials.
+            def apply_next_model(model):
+                next_function = partial(apply_next_model.func, model)
+                self.lazy_model_operation(next_function, *more_models)
+            apply_next_model.func = function
+
+            # If the model has already been imported and registered, partially
+            # apply it to the function now. If not, add it to the list of
+            # pending operations for the model, where it will be executed with
+            # the model class as its sole argument once the model is ready.
+            try:
+                model_class = self.get_registered_model(*next_model)
+            except LookupError:
+                self._pending_operations[next_model].append(apply_next_model)
+            else:
+                apply_next_model(model_class)
 
     def do_pending_operations(self, model):
         """

django/contrib/contenttypes/fields.py

@@ -318,14 +318,23 @@ class GenericRelation(ForeignObject):
         errors.extend(self._check_generic_foreign_key_existence())
         return errors
 
+    def _is_matching_generic_foreign_key(self, field):
+        """
+        Return True if field is a GenericForeignKey whose content type and
+        object id fields correspond to the equivalent attributes on this
+        GenericRelation.
+        """
+        return (
+            isinstance(field, GenericForeignKey) and
+            field.ct_field == self.content_type_field_name and
+            field.fk_field == self.object_id_field_name
+        )
+
     def _check_generic_foreign_key_existence(self):
         target = self.remote_field.model
         if isinstance(target, ModelBase):
             fields = target._meta.private_fields
-            if any(isinstance(field, GenericForeignKey) and
-                    field.ct_field == self.content_type_field_name and
-                    field.fk_field == self.object_id_field_name
-                    for field in fields):
+            if any(self._is_matching_generic_foreign_key(field) for field in fields):
                 return []
             else:
                 return [
@@ -401,20 +410,16 @@ class GenericRelation(ForeignObject):
         self.model = cls
         setattr(cls, self.name, ReverseGenericManyToOneDescriptor(self.remote_field))
 
-        # Add get_RELATED_order() and set_RELATED_order() methods if the model
-        # on the other end of this relation is ordered with respect to this.
-        def matching_gfk(field):
-            return (
-                isinstance(field, GenericForeignKey) and
-                self.content_type_field_name == field.ct_field and
-                self.object_id_field_name == field.fk_field
-            )
+        # Add get_RELATED_order() and set_RELATED_order() to the model this
+        # field belongs to, if the model on the other end of this relation
+        # is ordered with respect to its corresponding GenericForeignKey.
+        if not cls._meta.abstract:
 
-        def make_generic_foreign_order_accessors(related_model, model):
-            if matching_gfk(model._meta.order_with_respect_to):
-                make_foreign_order_accessors(model, related_model)
+            def make_generic_foreign_order_accessors(related_model, model):
+                if self._is_matching_generic_foreign_key(model._meta.order_with_respect_to):
+                    make_foreign_order_accessors(model, related_model)
 
-        lazy_related_operation(make_generic_foreign_order_accessors, self.model, self.remote_field.model)
+            lazy_related_operation(make_generic_foreign_order_accessors, self.model, self.remote_field.model)
 
     def set_attributes_from_rel(self):
         pass

django/core/checks/model_checks.py

@@ -63,3 +63,99 @@ def check_model_signals(app_configs=None, **kwargs):
                         )
                     )
     return errors
+
+
+def _check_lazy_references(apps, ignore=None):
+    """
+    Ensure all lazy (i.e. string) model references have been resolved.
+
+    Lazy references are used in various places throughout Django, primarily in
+    related fields and model signals. Identify those common cases and provide
+    more helpful error messages for them.
+
+    The ignore parameter is used by StateApps to exclude swappable models from
+    this check.
+    """
+    pending_models = set(apps._pending_operations) - (ignore or set())
+
+    # Short circuit if there aren't any errors.
+    if not pending_models:
+        return []
+
+    def extract_operation(obj):
+        """
+        Take a callable found in Apps._pending_operations and identify the
+        original callable passed to Apps.lazy_model_operation(). If that
+        callable was a partial, return the inner, non-partial function and
+        any arguments and keyword arguments that were supplied with it.
+
+        obj is a callback defined locally in Apps.lazy_model_operation() and
+        annotated there with a `func` attribute so as to imitate a partial.
+        """
+        operation, args, keywords = obj, [], {}
+        while hasattr(operation, 'func'):
+            # The or clauses are redundant but work around a bug (#25945) in
+            # functools.partial in Python 3 <= 3.5.1 and Python 2 <= 2.7.11.
+            args.extend(getattr(operation, 'args', []) or [])
+            keywords.update(getattr(operation, 'keywords', {}) or {})
+            operation = operation.func
+        return operation, args, keywords
+
+    def app_model_error(model_key):
+        try:
+            apps.get_app_config(model_key[0])
+            model_error = "app '%s' doesn't provide model '%s'" % model_key
+        except LookupError:
+            model_error = "app '%s' isn't installed" % model_key[0]
+        return model_error
+
+    # Here are several functions which return CheckMessage instances for the
+    # most common usages of lazy operations throughout Django. These functions
+    # take the model that was being waited on as an (app_label, modelname)
+    # pair, the original lazy function, and its positional and keyword args as
+    # determined by extract_operation().
+
+    def field_error(model_key, func, args, keywords):
+        error_msg = (
+            "The field %(field)s was declared with a lazy reference "
+            "to '%(model)s', but %(model_error)s."
+        )
+        params = {
+            'model': '.'.join(model_key),
+            'field': keywords['field'],
+            'model_error': app_model_error(model_key),
+        }
+        return Error(error_msg % params, obj=keywords['field'], id='fields.E307')
+
+    def default_error(model_key, func, args, keywords):
+        error_msg = "%(op)s contains a lazy reference to %(model)s, but %(model_error)s."
+        params = {
+            'op': func,
+            'model': '.'.join(model_key),
+            'model_error': app_model_error(model_key),
+        }
+        return Error(error_msg % params, obj=func, id='models.E022')
+
+    # Maps common uses of lazy operations to corresponding error functions
+    # defined above. If a key maps to None, no error will be produced.
+    # default_error() will be used for usages that don't appear in this dict.
+    known_lazy = {
+        ('django.db.models.fields.related', 'resolve_related_class'): field_error,
+        ('django.db.models.fields.related', 'set_managed'): None,
+    }
+
+    def build_error(model_key, func, args, keywords):
+        key = (func.__module__, func.__name__)
+        error_fn = known_lazy.get(key, default_error)
+        return error_fn(model_key, func, args, keywords) if error_fn else None
+
+    return sorted(filter(None, (
+        build_error(model_key, *extract_operation(func))
+        for model_key in pending_models
+        for func in apps._pending_operations[model_key]
+    )), key=lambda error: error.msg)
+
+
+@register(Tags.models)
+def check_lazy_references(app_configs=None, **kwargs):
+    return _check_lazy_references(apps)

django/db/migrations/state.py

@@ -240,44 +240,11 @@ class StateApps(Apps):
         self.render_multiple(list(models.values()) + self.real_models)
 
         # There shouldn't be any operations pending at this point.
-        pending_models = set(self._pending_operations)
-        if ignore_swappable:
-            pending_models -= {make_model_tuple(settings.AUTH_USER_MODEL)}
-        if pending_models:
-            raise ValueError(self._pending_models_error(pending_models))
-
-    def _pending_models_error(self, pending_models):
-        """
-        Almost all internal uses of lazy operations are to resolve string model
-        references in related fields. We can extract the fields from those
-        operations and use them to provide a nicer error message.
-
-        This will work for any function passed to lazy_related_operation() that
-        has a keyword argument called 'field'.
-        """
-        def extract_field(operation):
-            # operation is annotated with the field in
-            # apps.registry.Apps.lazy_model_operation().
-            return getattr(operation, 'field', None)
-
-        def extract_field_names(operations):
-            return (str(field) for field in map(extract_field, operations) if field)
-
-        get_ops = self._pending_operations.__getitem__
-        # Ordered list of pairs of the form
-        # ((app_label, model_name), [field_name_1, field_name_2, ...])
-        models_fields = sorted(
-            (model_key, sorted(extract_field_names(get_ops(model_key))))
-            for model_key in pending_models
-        )
-
-        def model_text(model_key, fields):
-            field_list = ", ".join(fields)
-            field_text = " (referred to by fields: %s)" % field_list if fields else ""
-            return ("%s.%s" % model_key) + field_text
-
-        msg = "Unhandled pending operations for models:"
-        return "\n  ".join([msg] + [model_text(*i) for i in models_fields])
+        from django.core.checks.model_checks import _check_lazy_references
+        ignore = {make_model_tuple(settings.AUTH_USER_MODEL)} if ignore_swappable else set()
+        errors = _check_lazy_references(self, ignore=ignore)
+        if errors:
+            raise ValueError("\n".join(error.msg for error in errors))
 
     @contextmanager
     def bulk_update(self):

django/db/models/utils.py

@@ -7,12 +7,18 @@ def make_model_tuple(model):
     corresponding ("app_label", "modelname") tuple. If a tuple is passed in,
     it's assumed to be a valid model tuple already and returned unchanged.
     """
-    if isinstance(model, tuple):
-        model_tuple = model
-    elif isinstance(model, six.string_types):
-        app_label, model_name = model.split(".")
-        model_tuple = app_label, model_name.lower()
-    else:
-        model_tuple = model._meta.app_label, model._meta.model_name
-    assert len(model_tuple) == 2, "Invalid model representation: %s" % model
-    return model_tuple
+    try:
+        if isinstance(model, tuple):
+            model_tuple = model
+        elif isinstance(model, six.string_types):
+            app_label, model_name = model.split(".")
+            model_tuple = app_label, model_name.lower()
+        else:
+            model_tuple = model._meta.app_label, model._meta.model_name
+        assert len(model_tuple) == 2
+        return model_tuple
+    except (ValueError, AssertionError):
+        raise ValueError(
+            "Invalid model reference '%s'. String model references "
+            "must be of the form 'app_label.ModelName'." % model
+        )

docs/ref/checks.txt

@@ -138,6 +138,9 @@ Models
 * **models.E020**: The ``<model>.check()`` class method is currently overridden.
 * **models.E021**: ``ordering`` and ``order_with_respect_to`` cannot be used
   together.
+* **models.E022**: ``<function>`` contains a lazy reference to
+  ``<app label>>.<model>``, but app ``<app label>`` isn't installed or
+  doesn't provide model ``<model>``.
 
 Fields
 ~~~~~~
@@ -200,6 +203,9 @@ Related Fields
   for ``<field name>``.
 * **fields.E306**: Related name must be a valid Python identifier or end with
   a ``'+'``.
+* **fields.E307**: The field ``<app label>.<model>.<field name>`` was declared
+  with a lazy reference to ``<app label>.<model>``, but app ``<app label>``
+  isn't installed or doesn't provide model ``<model>``.
 * **fields.E310**: No subset of the fields ``<field1>``, ``<field2>``, ... on
   model ``<model>`` is unique. Add ``unique=True`` on any of those fields or
   add at least a subset of them to a unique_together constraint.

tests/migrations/test_commands.py

@@ -449,6 +449,12 @@ class MigrateTests(MigrationTestBase):
         """
         call_command("migrate", "migrated_unapplied_app", stdout=six.StringIO())
 
+        # unmigrated_app.SillyModel has a foreign key to 'migrations.Tribble',
+        # but that model is only defined in a migration, so the global app
+        # registry never sees it and the reference is left dangling. Remove it
+        # to avoid problems in subsequent tests.
+        del apps._pending_operations[('migrations', 'tribble')]
+
     @override_settings(MIGRATION_MODULES={"migrations": "migrations.test_migrations_squashed"})
     def test_migrate_record_replaced(self):
         """

tests/migrations/test_state.py

@@ -661,9 +661,10 @@ class StateTests(SimpleTestCase):
         project_state = ProjectState()
         project_state.add_model(ModelState.from_model(Book))
         msg = (
-            "Unhandled pending operations for models:\n"
-            "  migrations.author (referred to by fields: migrations.Book.author)\n"
-            "  migrations.publisher (referred to by fields: migrations.Book.publisher)"
+            "The field migrations.Book.author was declared with a lazy reference "
+            "to 'migrations.author', but app 'migrations' doesn't provide model 'author'.\n"
+            "The field migrations.Book.publisher was declared with a lazy reference "
+            "to 'migrations.publisher', but app 'migrations' doesn't provide model 'publisher'."
         )
         with self.assertRaisesMessage(ValueError, msg):
             project_state.apps
@@ -672,9 +673,10 @@ class StateTests(SimpleTestCase):
         project_state = ProjectState()
         project_state.add_model(ModelState.from_model(Magazine))
         msg = (
-            "Unhandled pending operations for models:\n"
-            "  migrations.author (referred to by fields: "
-            "migrations.Magazine.authors, migrations.Magazine_authors.author)"
+            "The field migrations.Magazine.authors was declared with a lazy reference "
+            "to 'migrations.author\', but app 'migrations' doesn't provide model 'author'.\n"
+            "The field migrations.Magazine_authors.author was declared with a lazy reference "
+            "to \'migrations.author\', but app 'migrations' doesn't provide model 'author'."
         )
         with self.assertRaisesMessage(ValueError, msg):
             project_state.apps
@@ -682,10 +684,14 @@ class StateTests(SimpleTestCase):
         # And now with multiple models and multiple fields.
         project_state.add_model(ModelState.from_model(Book))
         msg = (
-            "Unhandled pending operations for models:\n"
-            "  migrations.author (referred to by fields: migrations.Book.author, "
-            "migrations.Magazine.authors, migrations.Magazine_authors.author)\n"
-            "  migrations.publisher (referred to by fields: migrations.Book.publisher)"
+            "The field migrations.Book.author was declared with a lazy reference "
+            "to 'migrations.author', but app 'migrations' doesn't provide model 'author'.\n"
+            "The field migrations.Book.publisher was declared with a lazy reference "
+            "to 'migrations.publisher', but app 'migrations' doesn't provide model 'publisher'.\n"
+            "The field migrations.Magazine.authors was declared with a lazy reference "
+            "to 'migrations.author', but app 'migrations' doesn't provide model 'author'.\n"
+            "The field migrations.Magazine_authors.author was declared with a lazy reference "
+            "to 'migrations.author', but app 'migrations' doesn't provide model 'author'."
         )
         with self.assertRaisesMessage(ValueError, msg):
             project_state.apps

tests/model_validation/tests.py

@@ -1,8 +1,10 @@
 from django.core import management
 from django.core.checks import Error, run_checks
+from django.core.checks.model_checks import _check_lazy_references
+from django.db import models
 from django.db.models.signals import post_init
 from django.test import SimpleTestCase
-from django.test.utils import override_settings
+from django.test.utils import isolate_apps, override_settings
 from django.utils import six
 
 
@@ -15,6 +17,10 @@ def on_post_init(**kwargs):
     pass
 
 
+def dummy_function(model):
+    pass
+
+
 @override_settings(
     INSTALLED_APPS=['django.contrib.auth', 'django.contrib.contenttypes'],
     SILENCED_SYSTEM_CHECKS=['fields.W342'],  # ForeignKey(unique=True)
@@ -54,3 +60,42 @@ class ModelValidationTest(SimpleTestCase):
         self.assertEqual(errors, expected)
 
         post_init.unresolved_references = unresolved_references
+
+    @isolate_apps('django.contrib.auth', kwarg_name='apps')
+    def test_lazy_reference_checks(self, apps):
+
+        class DummyModel(models.Model):
+            author = models.ForeignKey('Author', models.CASCADE)
+
+            class Meta:
+                app_label = "model_validation"
+
+        apps.lazy_model_operation(dummy_function, ('auth', 'imaginarymodel'))
+        apps.lazy_model_operation(dummy_function, ('fanciful_app', 'imaginarymodel'))
+
+        errors = _check_lazy_references(apps)
+
+        expected = [
+            Error(
+                "%r contains a lazy reference to auth.imaginarymodel, "
+                "but app 'auth' doesn't provide model 'imaginarymodel'." % dummy_function,
+                obj=dummy_function,
+                id='models.E022',
+            ),
+            Error(
+                "%r contains a lazy reference to fanciful_app.imaginarymodel, "
+                "but app 'fanciful_app' isn't installed." % dummy_function,
+                obj=dummy_function,
+                id='models.E022',
+            ),
+            Error(
+                "The field model_validation.DummyModel.author was declared "
+                "with a lazy reference to 'model_validation.author', but app "
+                "'model_validation' isn't installed.",
+                hint=None,
+                obj=DummyModel.author.field,
+                id='fields.E307',
+            ),
+        ]
+
+        self.assertEqual(errors, expected)