Repository navigation
Evict stale connection wrappers after django_db_modify_db_settings - #1320
Open
hossam1244 wants to merge 4 commits into
Open
hossam1244 wants to merge 4 commits into
hossam1244 wants to merge 4 commits into
Conversation
django_db_modify_db_settings may replace settings.DATABASES (e.g. to point at a Testcontainer). When anything materialized a DatabaseWrapper before that (import-time ORM access, AppConfig.ready), the ConnectionHandler kept serving the stale wrapper and its cached settings, so setup_databases connected to the pre-override host. Close connections, drop the materialized wrappers and clear the handler's cached settings so connections are re-created from the possibly modified settings. Fixes pytest-dev#1317
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
django_db_modify_db_settingsreplaces an entry insettings.DATABASES(the typical Testcontainers pattern), the override can silently fail if anything materialized aDatabaseWrapperearlier — import-time ORM access, an app'sAppConfig.ready, or Django itself. Django'sConnectionHandlerthen keeps serving the stale wrapper, and because itssettingscached_propertyreferences the old nested dict,setup_databases()connects to the pre-override host.connections.close_all()does not evict wrappers, andconnections.databaseson modern Django is only a compat property.Fixes #1317 (full root-cause analysis and workaround in the issue — this PR turns that workaround into the fixture's own behavior).
Solution
In
django_db_setup, afterdjango_db_modify_db_settingsresolves and beforesetup_databases():In-place mutation of
settings.DATABASES[alias][...]was never affected (the cached settings dict is the same object); the eviction matters for replacement — which is what the docs example pattern and every Testcontainers setup do.Testing
tests/test_modify_db_settings.py— regression test viadjango_pytester: the inner module materializesconnections["default"]at import time, then the session fixture replaces the alias dict with a differentHOST; the test asserts the connection created bydjango_db_setupuses the modified host. Fails onmain, passes with this change (verified red/green locally).Notes for reviewers
connections.__dict__.pop("settings", None)touches where acached_propertystores its value; there is no public API for this eviction. Happy to switch to something more official if one exists / if you'd rather fix it in Django.