Convert caches ImproperlyConfigured error to a system check

[David Evans]

There are currently two places (that I can find) in Django where settings are read at import time:

1. ​django/django/core/cache/__init__.py
2. ​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 ​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.

[David Evans]

Very small pull request here: ​https://github.com/django/django/pull/4929

[Andriy Sokolovskiy]

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.

[David Evans]

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.

[Andriy Sokolovskiy]

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.

[David Evans]

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.

[Andriy Sokolovskiy]

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.

[Aymeric Augustin]

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.

[Carl Meyer]

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.

[Tim Graham]

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?

[David Evans]

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!

[David Evans]

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?

[Tim Graham]

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.

[Tom Christie]

Pull request: ​https://github.com/django/django/pull/5221

[Tim Graham]

Marking as "Easy pickings" for someone to update ​Tom's pull request with tests and documentation.

[Tom Christie]

Tests & docs addressed.

[Tim Graham <timograham@…>]

In b02f08e:

Fixed #25034 -- Converted caches ImproperlyConfigured error to a system check.