Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16873 closed Bug (fixed)

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

Reported by: ptone Owned by: ptone
Component: Database layer (models, ORM) Version: master
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 Changed 3 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by carljm

  • Severity changed from Normal to Release blocker

Regression, bumping to release blocker.

comment:3 Changed 3 years ago by ptone

  • Owner changed from nobody to ptone
  • Status changed from new to assigned

comment:4 Changed 3 years ago by ptone

  • Has patch set
  • milestone set to 1.4
  • Triage Stage changed from Accepted to Ready 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 follow-up: Changed 3 years ago by carljm

  • Triage Stage changed from Ready for checkin to Accepted

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?

comment:6 in reply to: ↑ 5 Changed 3 years ago by ptone

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 Changed 3 years ago by ptone

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 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:2 Changed 3 years ago by carljm

  • Resolution set to fixed
  • Status changed from assigned to closed

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