Opened 20 months ago

Closed 7 months ago

#21719 closed Cleanup/optimization (fixed)

Forbid importing models before their application configuration

Reported by: aaugustin Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords: app-loading
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by aaugustin)

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

Attachments (2)

21719-postponing-wip.patch (8.4 KB) - added by aaugustin 18 months ago.
21719-postponing-wip-2.patch (25.4 KB) - added by aaugustin 18 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 20 months ago by charettes

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.

comment:2 Changed 20 months ago by aaugustin

  • Description modified (diff)

comment:3 Changed 20 months ago by aaugustin

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.

comment:4 Changed 20 months ago by aaugustin

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

comment:6 Changed 20 months ago by Aymeric Augustin <aymeric.augustin@…>

In 3326a412ccde9f72e22a070a0b4d922048ed2286:

Deprecated importing a model before loading its application.

Refs #21719, #21680.

comment:7 Changed 20 months ago by aaugustin

  • Keywords 1.9 added

This ticket cannot move forward until Django 1.9.

comment:8 Changed 19 months ago by 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.

comment:9 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

In 76ff266df1d68eb76836f159b799ed3e64979089:

Avoided importing models from django.contrib.admin.

Fixed #21923. Refs #21719.

comment:10 Changed 19 months ago by carljm

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.

comment:11 Changed 19 months ago by carljm

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.

comment:12 Changed 18 months ago by aaugustin

  • Keywords 1.9 removed
  • Severity changed from Normal to Release blocker
  • Type changed from Cleanup/optimization to Bug

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

comment:13 Changed 18 months ago by aaugustin

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__.

Changed 18 months ago by aaugustin

comment:14 Changed 18 months ago by aaugustin

Don't forget to revert a718fcf201b04ba254e9073be82f51ae1ae3a853 when fixing this.

comment:15 Changed 18 months ago by aaugustin

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__().
Last edited 18 months ago by aaugustin (previous) (diff)

Changed 18 months ago by aaugustin

comment:16 Changed 18 months ago by 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.

comment:17 Changed 18 months ago by aaugustin

  • Severity changed from Release blocker to Normal
  • Type changed from Bug to Cleanup/optimization

comment:18 Changed 7 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.
Back to Top