diff --git a/django/apps/registry.py b/django/apps/registry.py index 7ed6e5d468b..8a45830ccf4 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 - - 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): """ diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 99633a5c676..d4057fcfc6d 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 09f5d6492f4..a3b8f1b7f32 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 d85a511ce43..9bb618e03e1 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 f696e30ce2b..cda96277a60 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 + ) diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index f852c777ad4..e68c1a5a574 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -138,6 +138,9 @@ Models * **models.E020**: The ``.check()`` class method is currently overridden. * **models.E021**: ``ordering`` and ``order_with_respect_to`` cannot be used together. +* **models.E022**: ```` contains a lazy reference to + ``>.``, but app ```` isn't installed or + doesn't provide model ````. Fields ~~~~~~ @@ -200,6 +203,9 @@ Related Fields for ````. * **fields.E306**: Related name must be a valid Python identifier or end with a ``'+'``. +* **fields.E307**: The field ``..`` was declared + with a lazy reference to ``.``, but app ```` + isn't installed or doesn't provide model ````. * **fields.E310**: No subset of the fields ````, ````, ... on model ```` is unique. Add ``unique=True`` on any of those fields or add at least a subset of them to a unique_together constraint. diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py index cc655f112f8..c9efa096677 100644 --- a/tests/migrations/test_commands.py +++ b/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): """ diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py index f2e91267e90..2e53e4f8257 100644 --- a/tests/migrations/test_state.py +++ b/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 diff --git a/tests/model_validation/tests.py b/tests/model_validation/tests.py index 3d079d5befe..b98fd2ddac9 100644 --- a/tests/model_validation/tests.py +++ b/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)