Opened 11 years ago

Closed 10 years ago

Last modified 8 years ago

#21719 closed Cleanup/optimization (fixed)

Forbid importing models before their application configuration

Reported by: Aymeric Augustin Owned by: nobody
Component: Core (Other) Version: dev
Severity: Normal Keywords: app-loading really-hard
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 Aymeric Augustin)

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 Aymeric Augustin 11 years ago.
21719-postponing-wip-2.patch (25.4 KB ) - added by Aymeric Augustin 11 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by Simon Charette, 11 years ago

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 by Aymeric Augustin, 11 years ago

Description: modified (diff)

comment:3 by Aymeric Augustin, 11 years ago

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 by Aymeric Augustin, 11 years ago

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

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 3326a412ccde9f72e22a070a0b4d922048ed2286:

Deprecated importing a model before loading its application.

Refs #21719, #21680.

comment:7 by Aymeric Augustin, 11 years ago

Keywords: 1.9 added

This ticket cannot move forward until Django 1.9.

comment:8 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

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 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 76ff266df1d68eb76836f159b799ed3e64979089:

Avoided importing models from django.contrib.admin.

Fixed #21923. Refs #21719.

comment:10 by Carl Meyer, 11 years ago

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 by Carl Meyer, 11 years ago

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 by Aymeric Augustin, 11 years ago

Keywords: 1.9 removed
Severity: NormalRelease blocker
Type: Cleanup/optimizationBug

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

comment:13 by Aymeric Augustin, 11 years ago

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

by Aymeric Augustin, 11 years ago

Attachment: 21719-postponing-wip.patch added

comment:14 by Aymeric Augustin, 11 years ago

Don't forget to revert a718fcf201b04ba254e9073be82f51ae1ae3a853 when fixing this.

comment:15 by Aymeric Augustin, 11 years ago

Postponing most of ModelBase.__new__ isn'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.

Version 0, edited 11 years ago by Aymeric Augustin (next)

by Aymeric Augustin, 11 years ago

comment:16 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

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 by Aymeric Augustin, 11 years ago

Severity: Release blockerNormal
Type: BugCleanup/optimization

comment:18 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

Resolution: fixed
Status: newclosed

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.

comment:19 by Aymeric Augustin, 8 years ago

Keywords: really-hard added

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.

Last edited 8 years ago by Aymeric Augustin (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top