Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CONN_HEALTH_CHECKS and OPTIONS #413

Closed
lorddaedra opened this issue Aug 22, 2022 · 12 comments · Fixed by #418
Closed

Add support for CONN_HEALTH_CHECKS and OPTIONS #413

lorddaedra opened this issue Aug 22, 2022 · 12 comments · Fixed by #418
Assignees
Labels
enhancement New feature or request

Comments

@lorddaedra
Copy link

Hello!

How to convert my config to django-environ? I'm worry about CONN_HEALTH_CHECKS and OPTIONS.

DATABASES = types.MappingProxyType({
    'default': {
        'ENGINE': 'django.db.backends.postgresql',
        'HOST': '127.0.0.1',
        'NAME': 'devdatabase',
        'CONN_MAX_AGE': None,
        'CONN_HEALTH_CHECKS': True,
        'OPTIONS': {'sslmode': 'require'} if IS_PRODUCTION else {},
        'PASSWORD': 'django-insecure-database_password',
        'PORT': '5432',
        'USER': 'devdatabaseuser',
    },
})

Names like CONN_MAX_AGE are hardcoded in https://github.com/joke2k/django-environ/blob/main/environ/environ.py#L132-L137...

@denys-pidlisnyi
Copy link
Contributor

#418

@sergeyklay sergeyklay added the enhancement New feature or request label Sep 20, 2022
@sergeyklay sergeyklay self-assigned this Sep 20, 2022
@sergeyklay sergeyklay linked a pull request Sep 20, 2022 that will close this issue
@sergeyklay
Copy link
Collaborator

Implemented in #418. Thank you for contributing!

@lorddaedra
Copy link
Author

@denys-pidlisnyi @sergeyklay
What's about OPTIONS?
(It's not possible to use sslmode, isolation_level etc. settings without OPTIONS)

https://docs.djangoproject.com/en/4.1/ref/databases/#isolation-level
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PARAMKEYWORDS

@sergeyklay
Copy link
Collaborator

Feel free to make a PR :)

@lorddaedra
Copy link
Author

lorddaedra commented Sep 20, 2022

To be honestly, I use another package, dj-database-url. "Sister issue": jazzband/dj-database-url#180 :-)

this is my current settings.py, it works fine without any package code modifications:

# https://docs.djangoproject.com/en/4.1/ref/settings/#databases

DATABASES = types.MappingProxyType({
    'default': dj_database_url.parse(
        url=os.environ['DATABASE_URL'],
        engine='django.db.backends.postgresql',
        conn_max_age=None,
        ssl_require=not IS_DEVELOPMENT and not bool(os.getenv('CI', default=False)),
    ) | {'CONN_HEALTH_CHECKS': True},
})

I just commented because of you closed this issue (may be by mistake) but only half of issue was fixed... May be it should stay open but it's up to you...

@sergeyklay sergeyklay reopened this Sep 21, 2022
@stat1c-void
Copy link

stat1c-void commented Jan 31, 2023

I wonder - are those changes really needed?

Here's example code for adding non-standard params to DB configuration:

DATABASES = {
    'default': {
        **env.db_url(),
        'CONN_MAX_AGE': None,
        'CONN_HEALTH_CHECKS': True,
        'TEST': {
            'MIGRATE': False
        }
    }
}

OPTIONS can be done in a similar way.

@Bensge
Copy link

Bensge commented Feb 21, 2023

@sergeyklay it would be useful if you could bump the package version and release it so that this change is available on PyPi

@sergeyklay
Copy link
Collaborator

@Bensge I'm on this track.

@pataquets
Copy link

Looks like OPTIONS is already supported and released:

if url.query:
config_options = {}
for k, v in parse_qs(url.query).items():
if k.upper() in cls._DB_BASE_OPTIONS:
config.update({k.upper(): _cast(v[0])})
else:
config_options.update({k: _cast_int(v[0])})
config['OPTIONS'] = config_options

@lorddaedra: fancy to close the issue?

Also, I think @stat1c-void smart trick is worth a "tips" section snippet. I'll get to it as soon as I have some more time (unless someone beats to me at it 😄).

@lorddaedra
Copy link
Author

lorddaedra commented Apr 17, 2024

About trick: yes, it's useful workaround (but I usually prefer modern style https://peps.python.org/pep-0584/ )

About options: may be we should add pool and server_side_binding to supported options too.
https://docs.djangoproject.com/en/dev/ref/databases/#connection-pool

Can we somehow do not hardcode them but load on app start instead?

@pataquets
Copy link

Thanks for the trick, @lorddaedra! I didn't knew about it.
About options: if options is supported (in general), wouldn't it be better to file separate issues for specific options?
(unless I'm missing sthg important about options, not very familiar using it, please bear with my newbiness)

@lorddaedra
Copy link
Author

I agree with you, we can open new issue if needed for pool and server_side_binding.

There is nothing special with them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants