Forbid importing models before their application configuration

[Aymeric Augustin]

It's most likely a sane requirement, although there are significant backwards-compatibility concerns.

[Simon Charette]

We'll have to document the usage AppConfig.get_model as a best practice for model retrieval in AppConfig.ready in this case since people will naively import them in their apps module when they want to connect signals.

[Aymeric Augustin]

In #21680 I've suggested to required that models should:

• be defined in an installed (ie. already imported) application — which means the application configuration can't import the model — or
• have an explicit app_label

This provides an escape hatch for users who have a good reason to import models in their application packages or application configurations, while creating enough friction to nudge them towards a more responsible design.

[Aymeric Augustin]

I'm going to implement a deprecation path for this.

[Aymeric Augustin]

PR: ​https://github.com/django/django/pull/2161

[Aymeric Augustin <aymeric.augustin@…>]

In 3326a412ccde9f72e22a070a0b4d922048ed2286:

Deprecated importing a model before loading its application.

Refs #21719, #21680.

[Aymeric Augustin]

This ticket cannot move forward until Django 1.9.

[Aymeric Augustin <aymeric.augustin@…>]

In f9698c43918c118a29516cbef4e23c197eb2dc25:

Suppressed the if Site._meta.installed pattern.

The purpose of this construct is to test if the django.contrib.sites
application is installed. But in Django 1.9 it will be forbidden to
import the Site model when the django.contrib.sites application isn't
installed.

No model besides Site used this pattern.

Refs #21719, #21923.

[Aymeric Augustin <aymeric.augustin@…>]

In 76ff266df1d68eb76836f159b799ed3e64979089:

Avoided importing models from django.contrib.admin.

Fixed #21923. Refs #21719.

[Carl Meyer]

My feeling is that this is not the best approach and will likely result in a continuing game of imports whack-a-mole indefinitely into the future. Attempting to control import ordering in Python is, in my experience, doomed to frustration; it's better to allow things to import whenever they are imported, and take actions lazily as needed (which the new well-defined setup() moment should in theory allow).

I understand the implementation-complexity reasons for this choice, and unfortunately don't expect to have time to play with patches for alternatives between now and the 1.7 release date, so that probably means that this approach is locked in for 1.7. But I just wanted to note that I intend to at least experiment with patches in the 1.8 time frame to remove this restriction before we reach the end of the deprecation process at 1.9.

[Carl Meyer]

Note from IRC discussion: if I do attempt a patch to reintroduce support for importing models before their app is configured, one of the tricky issues will be avoiding the bugs we had previously where related managers from not-installed models would still be added to their related models. One possible approach to avoid these problems is to avoid setting up any relations until setup() time, right before calling ready(), when we have a fully-populated app cache in hand.

[Aymeric Augustin]

Changing flags as I just closed #22005 as a duplicate.

[Aymeric Augustin]

I tried postponing the resolution of the application a model belongs to. It's going to be complicated.

I'm attaching a fairly simple patch that stores "orphan models" in the registry, that is, models whose application isn't known at the time they're imported. Unfortunately, app_label is required in ModelBase.__new__:

  File "/Users/myk/Documents/dev/django/django/__init__.py", line 21, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/Users/myk/Documents/dev/django/django/apps/registry.py", line 88, in populate
    app_config = AppConfig.create(entry)
  File "/Users/myk/Documents/dev/django/django/apps/config.py", line 87, in create
    module = import_module(entry)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/Users/myk/Documents/dev/django/django/contrib/comments/__init__.py", line 8, in <module>
    from django.contrib.comments.models import Comment
  File "/Users/myk/Documents/dev/django/django/contrib/comments/models.py", line 45, in <module>
    class Comment(BaseCommentAbstractModel):
  File "/Users/myk/Documents/dev/django/django/db/models/base.py", line 140, in __new__
    new_class.add_to_class(obj_name, obj)
  File "/Users/myk/Documents/dev/django/django/db/models/base.py", line 271, in add_to_class
    value.contribute_to_class(cls, name)
  File "/Users/myk/Documents/dev/django/django/db/models/fields/related.py", line 1558, in contribute_to_class
    super(ForeignObject, self).contribute_to_class(cls, name, virtual_only=virtual_only)
  File "/Users/myk/Documents/dev/django/django/db/models/fields/related.py", line 262, in contribute_to_class
    'app_label': cls._meta.app_label.lower()
AttributeError: 'NoneType' object has no attribute 'lower'

It order to make this work, we'll have to postpone most of ModelBase.__new__.

[Aymeric Augustin]

Don't forget to revert a718fcf201b04ba254e9073be82f51ae1ae3a853 when fixing this.

[Aymeric Augustin]

Postponing most of ModelBase.__new__ doesn't work because class decorators are executed as soon as the class is defined and they may depend on arbitrary bits of the class definition.

  File "/Users/myk/Documents/dev/django/django/__init__.py", line 21, in setup
    apps.populate(settings.INSTALLED_APPS)
  File "/Users/myk/Documents/dev/django/django/apps/registry.py", line 88, in populate
    app_config = AppConfig.create(entry)
  File "/Users/myk/Documents/dev/django/django/apps/config.py", line 87, in create
    module = import_module(entry)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/Users/myk/Documents/dev/django/django/contrib/comments/__init__.py", line 8, in <module>
    from django.contrib.comments.models import Comment
  File "/Users/myk/Documents/dev/django/django/contrib/comments/models.py", line 45, in <module>
    class Comment(BaseCommentAbstractModel):
  File "/Users/myk/Documents/dev/django/django/utils/encoding.py", line 36, in python_2_unicode_compatible
    klass.__name__)
ValueError: @python_2_unicode_compatible cannot be applied to Comment because it doesn't define __str__().

[Aymeric Augustin <aymeric.augustin@…>]

In 7339f43c718008394cf5c5119994f956e27bce70:

Prevented admin from importing auth.User.

Since we don't enforce order between apps, root packages of contrib apps
cannot import models from unrelated apps.

Fix #22005, refs #21719.

[Aymeric Augustin <aymeric.augustin@…>]

In 1b8af4cfa023161924a45fb572399d2f3071bf5b:

Disallowed importing concrete models without an application.

Removed fragile algorithm to find which application a model belongs to.

Fixed #21680, #21719. Refs #21794.

[Aymeric Augustin]

Thankfully, a few years later, we can conclude that the whack-a-mole game hasn't gone too far.

The commits referenced in this ticket were necessary because some public APIs were available in the root package of contrib apps and these APIs depend on models. The current design of the app-loading process doesn't support this use case.

In the future, unless the possibility to import models before their app is added (I'm not sure it ever will), we'll avoid creating such APIs. We only had to fix preexisting APIs.