Should we raise a KeyError if the env var is missing and default is None?

[dschenck]

Shouldn't this function raise a KeyError if s is None?

def config(env=DEFAULT_ENV, default=None, engine=None, conn_max_age=0, ssl_require=False):
    """Returns configured DATABASE dictionary from DATABASE_URL."""

    config = {}

    s = os.environ.get(env, default)

    if s:
        config = parse(s, engine, conn_max_age, ssl_require)

    return config

If the user has not properly configured his environment settings (most likely), and not provided a default fallback value either (also likely), the config will return an empty dictionary, implicitly silencing the issue... (speaking of experience here...)
https://devcenter.heroku.com/articles/heroku-postgresql#connecting-in-python

[jacobian]

Hm, I'm not sure - there might be reasons that failing silently is ok. Maybe a warning, or something using the checks framework?

What do other folks think?

[mattseymour]

@jacobian I agree that maybe a warning should be logged in an instance where parse is not called due to environment variables not being read. I do not think that a KeyError should be raised mainly because this function should fall back onto the default argument value provided.

An option could be to add a raises_exception kwarg (defaulted to False) into the function which can be set if the users want an explicit exception to be raised. This option would keep backwards compatibility whilst giving users a more explicit error option.

[jacobian]

I think a warning when default is given and used will become annoying, and we should avoid it. A pretty common pattern I use for local dev is:

DATABASES = dj_database_url.config(default="postgres://local-db-name")

This uses a hardcoded fallback locally, but allows DATABASE_URL (from production, or a .env) to override the default. If I got a warning every time I ran my local server I'd get super-annoyed.

I think what we're talking about is this situation, when env[DATABASE_URL] is not set:

DATABASES = dj_database_url.config()

i.e. no default, and no environ. In that case, settings.DATABASES just ends up silently being {}, which seems wrong to me.

I'm leaning towards a warning in this specific situation -- I think something like "warning: env[DATABASE_URL] is blank, and so is default, so you have no databases. you might want to fix this" would be helpful. I do like your idea of a flag to turn this warning into an actual exception.

So, if env[DATABASE_URL] isn't set, then:

DATABASES = dj_database_url.config()

--> warning - something about setting environ or default

DATABASES = dj_database_url.config(requires_db=True)

---> exception - something about environ being required

Thoughts?

[mattseymour]

DATABASES = dj_database_url.config(default="postgres://local-db-name")
This uses a hardcoded fallback locally, but allows DATABASE_URL (from production, or a .env) to override the default. If I got a warning every time I ran my local server I'd get super-annoyed.

Completely agree with this. We would only want to raise a warning if the code is called with only default arguments:

DATABASES = dj_database_url.config()

or if a key miss occurs with no default option set.

DATABASES = dj_database_url.config(env='DATABASE_STRING')

[palfrey]

#199 implements this

[rtpg]

Reading this, it does feel "obvious" that if you're calling the config function you really probably do not want an empty dictionary. I would say enough so that raising an exception seems right! A major-version bump change for sure but major versions functionally grow on trees.

So the logic is simply dj_database_url.config(...) raises if it ends up with an empty dictionary (with the text hinting at DATABASE_URL not being set, and default being None?).

I ... I really can't think of a scenario where you want an empty dictionary in the end by default.

I do of course don't think we should be warning when using a specified default.

[ddelange]

you might want to generate docs or run CI or something, which involves importing settings.py, from an environment which does not contain runtime environment variables:

in our case we build a docker image with RUN ./manage.py collectstatic (imports settings.py), and we don't mount/export a DATABASE_URL variable into the docker context/build because it's not needed at buildtime.

[palfrey]

I think we're converging to where I got to on #199 (comment) so to try and double-check:

• warning log message if the database resolves to None, because most of the time, not setting it is bad, and we should tell people for that majority case
• OTOH, no exception for that, as there are scenarios where it's fine (because you're not using the DB right now)
• Don't do the extra arg thing currently in Throw warning if DATABASE_URL isn't set #199 as that's not ideal either i.e. keep the API as-is.

Thoughts folks?

[ddelange]

SGTM, a logging.warning won't hurt (no breaking change) and will make debugging trivial in case this slips through the cracks!

[palfrey]

I've updated #199 in line with that.