[Django] #25034: Remove attempts to access settings at import time
Django
--------------------------------------+--------------------
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
There are currently two places (that I can find) in Django where settings
are read at import time:
1.
[https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/core/cache/__init__.py#L32
django/django/core/cache/__init__.py]
2.
[https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/contrib/staticfiles/urls.py#L17
django/django/contrib/staticfiles/urls.py]
Case 1 is causing some people confusion when attempting to import things
in `wsgi.py`, as the import breaks if it comes before
`DJANGO_SETTINGS_MODULE` is defined. See for example:
[https://github.com/evansd/whitenoise/issues/31#issuecomment-104185649]
This check also seems a bit superfluous as failing to define a default
cache backend will trigger an `InvalidCacheBackendError` when attempting
to access it. It's not clear that triggering `ImproperlyConfigured` on
import adds much here.
Case 2 is bit strange as I can't find anywhere that actually uses this
`urls.py` file. Is it possible that it was just left in place and is now
redundant?
If we could remove these two cases then we have the simple rule that
settings are not to be accessed at import time, and we won't have any
subtle import-order requirements.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by evansd):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> There are currently two places (that I can find) in Django where settings
> are read at import time:
> 1.
> [https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/core/cache/__init__.py#L32
> django/django/core/cache/__init__.py]
> 2.
> [https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/contrib/staticfiles/urls.py#L17
> django/django/contrib/staticfiles/urls.py]
>
> Case 1 is causing some people confusion when attempting to import things
> in `wsgi.py`, as the import breaks if it comes before
> `DJANGO_SETTINGS_MODULE` is defined. See for example:
> [https://github.com/evansd/whitenoise/issues/31#issuecomment-104185649]
>
> This check also seems a bit superfluous as failing to define a default
> cache backend will trigger an `InvalidCacheBackendError` when attempting
> to access it. It's not clear that triggering `ImproperlyConfigured` on
> import adds much here.
>
> Case 2 is bit strange as I can't find anywhere that actually uses this
> `urls.py` file. Is it possible that it was just left in place and is now
> redundant?
>
> If we could remove these two cases then we have the simple rule that
> settings are not to be accessed at import time, and we won't have any
> subtle import-order requirements.
New description:
There are currently two places (that I can find) in Django where settings
are read at import time:
1.
[https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/core/cache/__init__.py#L32
django/django/core/cache/__init__.py]
2.
[https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/contrib/staticfiles/urls.py#L17
django/django/contrib/staticfiles/urls.py]
Case 1 is causing some people confusion when attempting to import things
in `wsgi.py`, as the import breaks if it comes before
`DJANGO_SETTINGS_MODULE` is defined. See for example:
[https://github.com/evansd/whitenoise/issues/31#issuecomment-104185649]
This check on the contents of `settings.CACHE` seems to me to be
superfluous as failing to define a default cache backend will trigger an
`InvalidCacheBackendError` anyway when attempting to access it. I don't
see that triggering `ImproperlyConfigured` on import adds much here.
Case 2 seems to be entirely redundant as it's only used for defining
`urlpatterns` in that file and I can't find anything which uses this or
any mention of it in the docs going back to v1.4. If I remove those lines
the tests continue to pass, so I hoping this can be safely removed.
If we could remove these two cases then we have the simple rule that
settings are not to be accessed at import time, and we won't have any
subtle import-order requirements.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:1>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by evansd:
Old description:
> There are currently two places (that I can find) in Django where settings
> are read at import time:
> 1.
> [https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/core/cache/__init__.py#L32
> django/django/core/cache/__init__.py]
> 2.
> [https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/contrib/staticfiles/urls.py#L17
> django/django/contrib/staticfiles/urls.py]
>
> Case 1 is causing some people confusion when attempting to import things
> in `wsgi.py`, as the import breaks if it comes before
> `DJANGO_SETTINGS_MODULE` is defined. See for example:
> [https://github.com/evansd/whitenoise/issues/31#issuecomment-104185649]
>
> This check on the contents of `settings.CACHE` seems to me to be
> superfluous as failing to define a default cache backend will trigger an
> `InvalidCacheBackendError` anyway when attempting to access it. I don't
> see that triggering `ImproperlyConfigured` on import adds much here.
>
> Case 2 seems to be entirely redundant as it's only used for defining
> `urlpatterns` in that file and I can't find anything which uses this or
> any mention of it in the docs going back to v1.4. If I remove those lines
> the tests continue to pass, so I hoping this can be safely removed.
>
> If we could remove these two cases then we have the simple rule that
> settings are not to be accessed at import time, and we won't have any
> subtle import-order requirements.
New description:
There are currently two places (that I can find) in Django where settings
are read at import time:
1.
[https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/core/cache/__init__.py#L32
django/django/core/cache/__init__.py]
2.
[https://github.com/django/django/blob/ec4f219ecb7a5e43d0353633fac4dac42d0ee492/django/contrib/staticfiles/urls.py#L17
django/django/contrib/staticfiles/urls.py]
Case 1 is causing some people confusion when attempting to import things
in `wsgi.py`, as the import breaks if it comes before
`DJANGO_SETTINGS_MODULE` is defined. See for example:
[https://github.com/evansd/whitenoise/issues/31#issuecomment-104185649]
This check on the contents of `settings.CACHE` seems to me to be
superfluous as failing to define a default cache backend will trigger an
`InvalidCacheBackendError` anyway when attempting to access it. I don't
see that triggering `ImproperlyConfigured` on import adds much here.
Case 2 seems to be entirely redundant as it's only used for defining
`urlpatterns` in that file and I can't find anything which uses this or
any mention of it in the docs going back to v1.4. If I remove those lines
the tests continue to pass, so I hoping this can be safely removed.
The coding style document
[https://docs.djangoproject.com/en/1.8/internals/contributing/writing-code
/coding-style/#use-of-django-conf-settings suggests] that such import-time
evaluation of settings is to be avoided, and it's causing at least some
real-world confusion, so it would be good to clean this up if possible.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:2>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by evansd):
* has_patch: 0 => 1
Comment:
Very small pull request here: https://github.com/django/django/pull/4929
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:3>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* cc: me@… (added)
Comment:
Normal django WSGI-application need to call `django.setup()`, which is
configuring settings.
I believe your application somehow overrides what `django.setup()` in
`wsgi.py` do, when you are applying your class
(`DjangoWhiteNoise(application)`), so you're getting an error.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:4>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by evansd):
I don't think that's true. Here's a minimal case to reproduce:
{{{
$ python -c 'import django.core.cache'
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/home/dave/.virtualenvs/tmp-a5eec9fa521d379a/local/lib/python2.7
/site-packages/django/core/cache/__init__.py", line 34, in <module>
if DEFAULT_CACHE_ALIAS not in settings.CACHES:
File "/home/dave/.virtualenvs/tmp-a5eec9fa521d379a/local/lib/python2.7
/site-packages/django/conf/__init__.py", line 48, in __getattr__
self._setup(name)
File "/home/dave/.virtualenvs/tmp-a5eec9fa521d379a/local/lib/python2.7
/site-packages/django/conf/__init__.py", line 42, in _setup
% (desc, ENVIRONMENT_VARIABLE))
django.core.exceptions.ImproperlyConfigured: Requested setting CACHES, but
settings are not configured. You must either define the environment
variable DJANGO_SETTINGS_MODULE or call settings.configure() before
accessing settings.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:5>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
That is what I'm saying, you're trying to import a module without
configuring settings before.
When django application will be runned, it can safely import this, because
`django.setup()` must be called when configuring wsgi-application.
I don't see use case when you need to import this django stuff outside of
configured django environment.
Also, you can not simply remove thinks you removed in you pull request,
because these imports are needed.
Second case is used:
http://djangoci.com/view/coverage/job/django-coverage/HTML_Coverage_Report
/_mnt_jenkinsdata_workspace_django-
coverage_django_contrib_staticfiles_urls.html#n17
The first one is not covered by tests, but I believe it can not be simply
tested or simply test is missing.
I lean to close this ticket as invalid, but lets wait for core dev
decision.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:6>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by evansd):
I think we're talking past each other here: I am saying that it would be
better (though obviously not essential) if importing a module before
configuring settings did not trigger an error. This is because import-time
side effects are generally speaking to be avoided, and the specific case
of evaluating settings at import time is discouraged by Django's own style
guide (as linked above). Moreover I am claiming that neither of these two
cases do anything particularly useful.
The coverage report proves nothing: that file is loaded as part of the
test run and so that line is of course evaluated but that doesn't show it
is used by anything. The tests that exercise that file continue to pass
and there is no mention in any of the docs for any Django version of
importing `urlpatterns` from that file.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:7>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
I don't see nothing wrong to use these imports.
You can not use most of django features before configuring settings.
If you will try to use some method, which is accessing settings inside
without configuring settings, you will get the same effect.
I'm not familiar with `staticfiles` package to say if this stuff with
`urlpatterns` needed or not, so
some core dev will give an answer to that.
`if DEFAULT_CACHE_ALIAS not in settings.CACHES` '''is needed''' because
code in this module relies on this check and you can not simply remove it
without giving the alternative.
Also, It is better to raise error about wrong configuration at the start
than getting an error when trying to use cache stuff.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:8>
Django
Reporter: evansd | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
It would be nice if any Django module could be imported without requiring
configured settings. That's why settings are lazily loaded in the first
place. So I think it's a worthwhile design goal.
The `urlpatterns` variable in `django.contrib.staticfiles.urls` allows
`include('django.contrib.staticfiles.urls')` in an URLconf. I don't think
that's documented. I couldn't get meaninfgul data on whether it's used in
the wild. In doubt, we could deprecate it instead of removing it.
I support refactoring the cache implementation to defer use of the
`CACHES` setting. If needed, we could call initialization code from
`django.setup()`. This isn't as bad at it may sound at first. Almost
everything in Django is an app and apps have a documented start-up hook,
`AppConfig.ready()`. The ORM isn't an app and its initialization is
performed explicitly during app loading. The cache isn't an app either and
could be initialized at the same point.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:9>
Django
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* stage: Unreviewed => Accepted
Comment:
I agree with Aymeric. Avoiding import time use of settings has been
preferred for quite a while. There used to be many more cases of this but
most have been fixed. These should be too.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:10>
Django
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* component: Uncategorized => Core (Other)
Comment:
For `staticfiles`, I also couldn't find any docs of that attribute. We do
suggest reference `settings.DEBUG` in urlpatterns though, e.g.
{{{
from django.conf import settings
from django.contrib.staticfiles import views
if settings.DEBUG:
urlpatterns += [
url(r'^static/(?P<path>.*)$', views.serve),
]
}}}
so it may be counterintuitive to make the recommendation. I guess the
distinction is Django itself vs. user projects. However, I wonder if we
might just ignore the issue here until we improve static serving and make
it usable in production as suggested [https://groups.google.com/d/topic
/django-developers/N0KbgDeLuUE/discussion on django-developers]. Then we
can simply drop `settings.DEBUG` and don't have to worry about backwards
compatibility.
How about making the `CACHES` exception a system check?
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:11>
Django
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by evansd):
Thanks everyone, this sounds sensible. I'll look into making a system
check for cache configuration and update the PR accordingly.
On the staticfiles thing I'm happy to leave it in place for now if the
whole thing is going to be overhauled anyway. Personally, I think it would
be OK to just remove it (with a mention in the release notes) because:
* it's something that only takes effect in `DEBUG` mode, so nobody's
production system is suddenly going to break as a result;
* it's trivial to replace the behaviour by adding two lines to your own
`urlpatterns`;
* it's an undocumented API so strictly speaking people shouldn't be using
it anyway.
But obviously it's not up to me!
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:12>
Django
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by evansd):
Looking at the available [https://docs.djangoproject.com/en/dev/ref/checks
/#builtin-tags system check tags], a check on cache configuration doesn't
really fit into any of them.
Is it better to squeeze it in to an existing tag, or create an entirely
new one?
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:13>
Django
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I'd create a `cache` tag for the check.
I agree there's not much downside to the staticfiles change, but I don't
see much upside either. It could be a useful shortcut to document once/if
we remove the `DEBUG` restriction.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:14>
Django
--------------------------------------+------------------------------------
Reporter: evansd | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* component: Core (Other) => Core (System checks)
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:15>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* cc: tom@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:16>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* has_patch: 0 => 1
Comment:
Pull request: https://github.com/django/django/pull/5221
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:17>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:18>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* easy: 0 => 1
Comment:
Marking as "Easy pickings" for someone to update
[https://github.com/django/django/pull/5221 Tom's pull request] with tests
and documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:19>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_docs: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:20>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by tomchristie):
Tests & docs addressed.
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:21>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"b02f08e02c34bcf19444c15b54c5f64ddd4c9ef2" b02f08e]:
{{{
#!CommitTicketReference repository=""
revision="b02f08e02c34bcf19444c15b54c5f64ddd4c9ef2"
Fixed #25034 -- Converted caches ImproperlyConfigured error to a system
check.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25034#comment:22>