Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#16873 closed Bug (fixed)

removal of deprecated settings.DATABASE code causes shell scripts to raise ImproperlyConfigured

Reported by: Preston Holmes Owned by: Preston Holmes
Component: Database layer (models, ORM) Version: dev
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a pattern out there for shell scripts that interact with django that simply call:

from django.conf import settings; settings.configure()
import django modules here

loading some modules cause an ImproperlyConfigured exception to be raise in trunk@16848 where such an exception was not raised in 1.3

This is due to the removal of code related to adapting settings.DATABASES for the case of the deprecated single database setting

This change can be seen in this commit:

https://github.com/django/django/commit/4f306a4c27c8537ca9cd966cea1c0de84aafda9e#django/db/__init__.py

The prior code would create a default set of values for settings.DATABASES[DEFAULT_DB_ALIAS]

This no longer occurs, which triggers the raising of ImproperlyConfigured

There are two ways around this:

one is to fully specify the setting with:

from django.conf import settings; settings.configure(
    DATABASES = {
        'default': {
            'ENGINE': 'django.db.backends.sqlite3',
            'NAME': '/tmp/dev'
        }
    })

The other is to use the setup_environ pattern:

from django.core.management import setup_environ
from dummyproj import settings
setup_environ(settings)

While none of these patterns are documented officially in Django's docs, there is widespread use of something like this in cron jobs shell scripts etc. This perhaps could warrant a note in the release notes alongside the part about what is being deprecated?

Change History (9)

comment:1 by Carl Meyer, 13 years ago

Triage Stage: UnreviewedAccepted

I think this side-effect of the back-compat shim was actually accidental: it seems pretty clear from django/db/__init__.py that the intention of that code was to immediately raise ImproperlyConfigured if there was no default database.

Nevertheless, I just discussed this with Alex and we agree that the accidental 1.3 behavior is actually preferable to the current trunk behavior. Raising ImproperlyConfigured at import time sucks and should be avoided whenever possible, and in this case we can avoid it, by setting up a "default" database for you, using the dummy backend, if you have no databases defined. The dummy backend will still error if you try to use it, but that's much better than an import-time error (especially since many parts of Django import the ORM, its hard to predict which those are, and it could change between releases).

If the user does have databases defined, but no "default" one, that case should be better handled than it is now, but not by creating a dummy default. That issue is tracked by #16752.

comment:2 by Carl Meyer, 13 years ago

Severity: NormalRelease blocker

Regression, bumping to release blocker.

comment:3 by Preston Holmes, 13 years ago

Owner: changed from nobody to Preston Holmes
Status: newassigned

comment:4 by Preston Holmes, 13 years ago

Has patch: set
milestone: 1.4
Triage Stage: AcceptedReady for checkin

Here is the pull request:

https://github.com/django/django/pull/54

per review and discussion on IRC with carljm I've marked this RFC subject to comment tweaks

Also patch file attachment available upon request

comment:5 by Carl Meyer, 13 years ago

Triage Stage: Ready for checkinAccepted

Ok, so sorry I didn't think of this until after your good work on the patch, but - why don't we just put the default dummy-backend database in conf/global_settings.py instead? It's simpler and should have the same effect. In "normal" cases it will have no impact because there will be a DATABASES setting in the settings module that overrides it, but it'll provide the dummy default backend for the settings.configure() case. Any issues with this approach that I'm overlooking?

in reply to:  5 comment:6 by Preston Holmes, 13 years ago

Replying to carljm:

Ok, so sorry I didn't think of this until after your good work on the patch, but - why don't we just put the default dummy-backend database in conf/global_settings.py instead? It's simpler and should have the same effect. In "normal" cases it will have no impact because there will be a DATABASES setting in the settings module that overrides it, but it'll provide the dummy default backend for the settings.configure() case. Any issues with this approach that I'm overlooking?

I can't think of any issue with that - I just didn't know enough about the default conf being in there. Its all good learning experience.

Not sure about testing - seems like there is no "behavior" to test - no code to speak of really.

comment:7 by Preston Holmes, 13 years ago

OK, reset the branch on the pull with the improved implementation - reran test suite - all OK.

conferred with jezdez on IRC and he backs basic approach as suggested by carljm

comment:8 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:2 by Carl Meyer, 13 years ago

Resolution: fixed
Status: assignedclosed

In [16946]:

Fixed #16873 - Added dummy database backend in default settings, so that just importing django.db doesn't trigger ImproperlyConfigured if there is no DATABASES setting.

Note: See TracTickets for help on using tickets.
Back to Top