[Django] #21719: Forbid importing models before their application configuration
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Core | Keywords: app-loading
(Other) | Has patch: 0
Severity: Normal | Needs tests: 0
Triage Stage: Accepted | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
It's most a sane requirement.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Old description:
> It's most a sane requirement.
New description:
It's most likely a sane requirement, although there are significant
backwards-compatibility concerns.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aaugustin):
I'm going to implement a deprecation path for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by aaugustin):
PR: https://github.com/django/django/pull/2161
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:5>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"3326a412ccde9f72e22a070a0b4d922048ed2286"]:
{{{
#!CommitTicketReference repository=""
revision="3326a412ccde9f72e22a070a0b4d922048ed2286"
Deprecated importing a model before loading its application.
Refs #21719, #21680.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:6>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* keywords: app-loading => app-loading 1.9
Comment:
This ticket cannot move forward until Django 1.9.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:7>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading 1.9 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"f9698c43918c118a29516cbef4e23c197eb2dc25"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:8>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading 1.9 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"76ff266df1d68eb76836f159b799ed3e64979089"]:
{{{
#!CommitTicketReference repository=""
revision="76ff266df1d68eb76836f159b799ed3e64979089"
Avoided importing models from django.contrib.admin.
Fixed #21923. Refs #21719.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:9>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading 1.9 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:10>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: app-loading 1.9 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:11>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* keywords: app-loading 1.9 => app-loading
* type: Cleanup/optimization => Bug
* severity: Normal => Release blocker
Comment:
Changing flags as I just closed #22005 as a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:12>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (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__`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:13>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by aaugustin):
Don't forget to revert a718fcf201b04ba254e9073be82f51ae1ae3a853 when
fixing this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:14>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:15>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Release blocker | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"7339f43c718008394cf5c5119994f956e27bce70"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:16>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* type: Bug => Cleanup/optimization
* severity: Release blocker => Normal
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:17>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1b8af4cfa023161924a45fb572399d2f3071bf5b"]:
{{{
#!CommitTicketReference repository=""
revision="1b8af4cfa023161924a45fb572399d2f3071bf5b"
Disallowed importing concrete models without an application.
Removed fragile algorithm to find which application a model belongs to.
Fixed #21680, #21719. Refs #21794.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: aaugustin | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
hard |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* keywords: app-loading => app-loading really-hard
Comment:
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 this is resolved (I'm not sure it ever will), we'll
avoid creating such APIs. We only had to fix preexisting APIs.
--
Ticket URL: <https://code.djangoproject.com/ticket/21719#comment:19>