django-pgconnection icon indicating copy to clipboard operation
django-pgconnection copied to clipboard

Overriding parts of `DATABASES` in place silently breaks

Open ralokt opened this issue 3 years ago • 5 comments

If DATABASES is configured using pgconnection.configure, in-place alterations in derived settings will only apply in that thread.

For example:

# base_settings.py
DATABASES = pgconnection.configure({
    (...)
})
# testing_settings.py
from base_settings import *

# this will only be applied in the current thread
DATABASES["default"]["TEST_NAME"] = "test_{}".format(generate_unique_name())

I am aware that this is by design, and changing it is at odds with what pgconnection needs to do - but I'm wondering if there are ways to make it less likely that developers stumble over this.

For example, might it be feasible to disallow setting keys (perhaps before django is ready)? Or make pgconnection a django app that patches django.conf.settings if installed? Or at the very least, adding a warning to the docs so that it is easier to identify the cause of this?

With a complex preexisting setup, it can be hard to identify this - especially since everything works fine as long as only one thread is involved. In my case, the only thing that didn't work were tavern tests that started a live server thread to run against, and I spent hours thinking that the cause was probably some corner case that happens when using pgconnection together with tavern and pytest-django's version of django.test.testcases.LiveServerTestCase...

Anyway, if all else fails, I hope to save someone else from my fate by hereby making the issue googleable.

ralokt avatar Oct 19 '21 13:10 ralokt

After changing the settings didn't resolve my issue, I did some more digging, and:

https://github.com/django/django/blob/main/django/db/backends/base/creation.py#L60

So basically, whenever a test needs to run more than one thread, only one of the threads will see the testing DB, the rest will see the normal default DB.

In particular, this breaks any kind of test that runs the live server and uses the DB.

ralokt avatar Oct 20 '21 08:10 ralokt

For those who are here because they googled the issue, I found a workaround: install a signal handler for connection_created in a global session-scoped autouse fixture that makes sure that the test database is used.

The fact that it's a signal handler means that it can run in every thread, and using connection_created in particular means that it happens before anything can use the wrong connection.

For those who use django's testing framework, the server_thread_class attribute of LiveServerTestCase could probably be used fairly easily to do this - simply create a subclass of LiveServerThread and do it in run. This doesn't take care of any other threads that might be started by tests, however.

Re this issue - perhaps it would be a good idea to include a utility signal handler that does this, with a setting to enable it? That would still leave the problem of detecting whether or not tests are running - perhaps it would be possible to detect/remember if the test database was patched in in the main thread (this happens upon test db creation)?

ralokt avatar Oct 25 '21 13:10 ralokt

Hi,

I'm experiencing similar issues to this in my project. I don't fully understand how to implement this workaround. Any chance you can reply with a snip of code that does what you're describing?

Thanks!

dfrank-a avatar Dec 07 '21 19:12 dfrank-a

@dfrank-a I did something like this (code untested):

global conftest.py


import pytest

from django.conf import settings
from django.db.backends.signals import connection_created


@pytest.fixture(autouse=True)
def patch_connection_setings(django_db, request):
    def connection_created_handler(sender, connection):
        # modify settings.DATABASES and the connection config based on the testing DB name in the settings and the fixture request
        # you might need the fixture request in case you customize the test db name somehow, fE if you use xdist to run tests in parallel, in which case you can extract the suffix of the database for the current thread from it
        # also, you probably only wanna run this code once per thread => check if settings.DATABASES is already modified
        # you also will probably need to perform a reconnect on the connection object after modifying it

    connection_created.connect(connection_created_handler, weak=False)

Unfortunately, I can't just copy/paste the actual code for legal reasons... I hope this is still somewhat helpful. And also, feel free to ask for more help if you get stuck!

ralokt avatar Dec 08 '21 05:12 ralokt

@ralokt many thanks for the initial report. Your investigation was invaluable for me trying to resolve this issue for our implementation.

I wasn't able to get the signal solution you described working, but I did manage to get it working with the solution below which can similarly be placed in a global conftest.py (if using pytest-django):

@pytest.fixture(scope="session")
def django_db_modify_db_settings(
    django_db_modify_db_settings_parallel_suffix: None,
) -> None:
    from django.conf import settings

    settings.DATABASES._initial_databases = dict(settings.DATABASES)

This hack definitely relies on the existence of _Databases._initial_databases so it's a bit fragile. It also seems possible that folks with a more complex testing setup (e.g. multiple databases or manipulation of database settings in tests or fixtures) could still experience issues with this solution.

We are running pytest-django, pytest-xdist, and Tox (among others) and our suite appears to be working correctly now that I applied this fix.

Hope that helps others!

chrisrink10 avatar Jan 06 '22 18:01 chrisrink10