Opened 9 years ago
Closed 9 years ago
#25034 closed Cleanup/optimization (fixed)
Convert caches ImproperlyConfigured error to a system check
Reported by: | David Evans | Owned by: | nobody |
---|---|---|---|
Component: | Core (System checks) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | me@…, tom@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
There are currently two places (that I can find) in Django where settings are read at import time:
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 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.
Change History (22)
comment:1 by , 9 years ago
Description: | modified (diff) |
---|
comment:2 by , 9 years ago
Description: | modified (diff) |
---|
comment:3 by , 9 years ago
Has patch: | set |
---|
comment:4 by , 9 years ago
Cc: | added |
---|
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.
comment:5 by , 9 years ago
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.
comment:6 by , 9 years ago
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 things you deleted 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.
comment:7 by , 9 years ago
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.
comment:8 by , 9 years ago
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.
comment:9 by , 9 years ago
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.
comment:10 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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.
comment:11 by , 9 years ago
Component: | Uncategorized → Core (Other) |
---|---|
Patch needs improvement: | set |
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 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?
comment:12 by , 9 years ago
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!
comment:13 by , 9 years ago
Looking at the available 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?
comment:14 by , 9 years ago
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.
comment:15 by , 9 years ago
Component: | Core (Other) → Core (System checks) |
---|---|
Has patch: | unset |
Patch needs improvement: | unset |
Summary: | Remove attempts to access settings at import time → Convert caches ImproperlyConfigured error to a system check |
comment:16 by , 9 years ago
Cc: | added |
---|
comment:17 by , 9 years ago
Has patch: | set |
---|
Pull request: https://github.com/django/django/pull/5221
comment:18 by , 9 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
comment:19 by , 9 years ago
Easy pickings: | set |
---|
Marking as "Easy pickings" for someone to update Tom's pull request with tests and documentation.
comment:20 by , 9 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Very small pull request here: https://github.com/django/django/pull/4929