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

5 files changed, 163 insertions, 83 deletions

diff --git a/django/apps/registry.py b/django/apps/registry.py
index 7ed6e5d468..8a45830ccf 100644
--- a/django/apps/registry.py
+++ b/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
+        # 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:
+            next_model, more_models = model_keys[0], model_keys[1:]
 
-            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')
+            # 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 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)
-        else:
-            function(model_class)
+            # 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):
         """
diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py
index 99633a5c67..d4057fcfc6 100644
--- a/django/contrib/contenttypes/fields.py
+++ b/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
diff --git a/django/core/checks/model_checks.py b/django/core/checks/model_checks.py
index 09f5d6492f..a3b8f1b7f3 100644
--- a/django/core/checks/model_checks.py
+++ b/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)
diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py
index d85a511ce4..9bb618e03e 100644
--- a/django/db/migrations/state.py
+++ b/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):
diff --git a/django/db/models/utils.py b/django/db/models/utils.py
index f696e30ce2..cda96277a6 100644
--- a/django/db/models/utils.py
+++ b/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
+        )