Data loss using DatabaseBackend when db connection unstable.

[a-edakin]

Describe the problem

I noticed that once a month or two some most used constance would just reset to default values.

After some insvestigation I found out that it was caused when db connection was unstable (db was overloaded or restarted for example)

Steps to reproduce

I don't think it would be easy to reproduce because of percise timings but it can be reproduced with some mocks in tests.

In method get of DatabaseBackend https://github.com/jazzband/django-constance/blob/master/constance/backends/database/__init__.py#L69

class DatabaseBackend(Backend):
# ...

    def get(self, key):
        key = self.add_prefix(key)
        if self._cache:
            value = self._cache.get(key)
            if value is None:
                self.autofill()
                value = self._cache.get(key)
        else:
            value = None
        if value is None:
            try:
                value = self._model._default_manager.get(key=key).value
            except (OperationalError, ProgrammingError, self._model.DoesNotExist):
                pass
            else:
                if self._cache:
                    self._cache.add(key, value)
        return value

As you can see when we try to get value from database we just ignore exceptions OperationalError and ProgrammingError. Let's assume db was overloaded and caused OperationalError for example so when we return value it is None.

Now we go to wrapper class Config https://github.com/jazzband/django-constance/blob/master/constance/base.py#L20

class Config:
# ...
    def __getattr__(self, key):
        try:
            if not len(settings.CONFIG[key]) in (2, 3):
                raise AttributeError(key)
            default = settings.CONFIG[key][0]
        except KeyError:
            raise AttributeError(key)
        result = self._backend.get(key)
        if result is None:
            result = default
            setattr(self, key, default)
            return result
        return 

Just like I said before method get returns None and in this case wrapper Config.__getattr__ just sets value too default and at this time all database issues are resolved and that is how we just lost data of a constance.

How I fixed issue right now:

I just inherited DatabaseBackend and removed OperationalError and ProgrammingError from except block

class CustomDatabaseBackend(DatabaseBackend):

    def get(self, key):
        key = self.add_prefix(key)
        if self._cache:
            value = self._cache.get(key)
            if value is None:
                self.autofill()
                value = self._cache.get(key)
        else:
            value = None
        if value is None:
            try:
                value = self._model._default_manager.get(key=key).value
            except self._model.DoesNotExist:
                pass
            else:
                if self._cache:
                    self._cache.add(key, value)
        return value

System configuration

• Django version: 3.2
• Python version: 3.8
• Django-Constance version: Latest

[JacoBezuidenhout]

+1 This also just happened to us where randomly this defaulted the settings... Will try your fix thank you. We also had a "too many connections" issue on our DB just before this happened. This has caused quite a bit of havoc here :)

[thiagoferreiraw]

Hi django-constance team,

I also had ~3 instances of this happening in the past month. We're using a Redis backend and the feature flags got wiped.

At first I suspected our Redis instance got flushed or something like that, but we couldn't find any evidence to support that.

Thanks!

[ro-routable]

Would this PR address the issue? https://github.com/jazzband/django-constance/pull/444/files

This issue also looks related: https://github.com/jazzband/django-constance/issues/348

[camilonova]

Feel free to add a test case so we can merge this possible solution.

[a-edakin]

Would this PR address the issue? https://github.com/jazzband/django-constance/pull/444/files

This issue also looks related: #348

No, it won't solve issue since this check for default is not None will just prevent from resetting config value to None, in other words if your constance default value is not None it will be reset to this value == losing data

Even my solution is not ideal since it will expose database errors to users of this package.

IMHO: Ideal solution is to add new exception like ConfigBackendError that will be risen instead of (OperationalError, ProgrammingError or redis client similar errors) so Config class can handle cases when used config backend is not working for some reason

[rudolfolah]

Would this PR address the issue? https://github.com/jazzband/django-constance/pull/444/files This issue also looks related: #348

No, it won't solve issue since this check for default is not None will just prevent from resetting config value to None, in other words if your constance default value is not None it will be reset to this value == losing data

Re-reading the code and tried out a small unit test and I agree with you, this doesn't quite solve the issue.

Even my solution is not ideal since it will expose database errors to users of this package.

IMHO: Ideal solution is to add new exception like ConfigBackendError that will be risen instead of (OperationalError, ProgrammingError or redis client similar errors) so Config class can handle cases when used config backend is not working for some reason

It makes sense to me that the errors would be surfaced; users of django-constance are also using Django which is what raises the errors. If there are other database/redis errors being raised during a request processing or in a background worker, the error types would point to the same database issue. Raising a new type of error could potentially obfuscate what's happening.

I created a draft PR: https://github.com/jazzband/django-constance/pull/525/files

[a-edakin]

users of django-constance are also using Django which is what raises the errors

Redis backend is not part of the Django

Raising a new type of error could potentially obfuscate what's happening.

It won't because I suggest to wrap backend related error in ConfigBackendError, it's common practice even Django is doing the same thing with it's database backends wrapping all possible driver errors in Django standardised errors like OperationalError

Because if django-constance provides the same API for different config backends it also should provide the same exceptions that could be catched with predefined exceptions.

Example:

from constance import config
from constance.exceptions import ConfigBackendError

try:
    config.FOO_CONSTANCE
except ConfigBackendError:
    # handle case when config is not working

And I as a developer don't need to change my code implementation just because I switched backend from database to Redis

[sebastianmanger]

I am able to reproduce this locally:

• create a config with a default (CONSTANCE_CONFIG = {"FOO": ("my-default", "test")}
• overwrite the default, by admin or magement command (python manage.py constance set FOO 1)
• disconnect/stop/whatever your backend, and run python manage.py constance get FOO
  • result: "my-default"
  • expected: 💥 of any sort

A test for this already exists: https://github.com/jazzband/django-constance/pull/525/files#diff-81b4098763925e2da30d4f500777337c3f06a96f6eefa70802858ba8d7ff3909R45

[chrisclark]

I've taken an approach that has the same effect, but also makes the code easier to understand. Please see #538

This does away with exception handling in that block altogether. The only reason the handling was there was because the code pre-dated the introduction of .first(), which handles the case of no results without throwing a DoesNotExist error.

If the DB is down, the database backend will now behave the same way as every other backend (redis, memory) -- which is to say (as @sebastianmanger put it): 💥

I'll let this comment and PR sit here for a bit for comments, and merge it next week.