SimpleTestCase._remove_databases_failures() crashes on un-wrapped connection methods

[Rio Weber]

Re-filing #36942 - got closed for lack of a repro, I have one

TLDR: _remove_databases_failures blindly does method.wrapped on every
connection method it visits. If the method isn't a _DatabaseFailure at
that point (e.g. SQLite recycled the connection between setUpClass and
tearDownClass, or the class's databases set changed after setup),
teardown dies with AttributeError: 'function' object has no attribute
'wrapped'.

Repro, traceback, one-line fix (isinstance guard), and two regression
tests below. Verified on 6.0.3 and on main @ 526b548c (that commit
touches auth/backends.py, not test/testcases.py).

Note: between 6.0.3 and main, the list of disallowed connection methods
moved from a SimpleTestCase class attribute to
connection.features.disallowed_simple_test_case_connection_methods.
Orthogonal to the bug - both branches still blindly access
method.wrapped - but the patch surface differs slightly per branch.

Minimal repro

settings.py:

DATABASES = {
    "default": {"ENGINE": "django.db.backends.sqlite3", "NAME": BASE_DIR / "db.sqlite3"},
    "shard_b": {"ENGINE": "django.db.backends.sqlite3", "NAME": BASE_DIR / "db_shard_b.sqlite3"},
}

tests/test_repro.py:

from django.test import TransactionTestCase

class ReproTests(TransactionTestCase):
    # Implicit databases = {"default"} - shard_b is NOT declared
    def test_anything(self):
        assert True

Run under pytest-django; the test body doesn't need to touch the DB:

pytest tests/test_repro.py

Traceback (abridged)

.../django/test/testcases.py:280
    in _remove_databases_failures
        setattr(connection, name, method.wrapped)
                                  ^^^^^^^^^^^^^^
AttributeError: 'function' object has no attribute 'wrapped'

Root cause

_add_databases_failures installs _DatabaseFailure wrappers;
_remove_databases_failures assumes every method it iterates is still
one. That symmetry breaks most often because the SQLite backend
replaces connection objects between setUpClass and tearDownClass the
new connection has fresh, un-wrapped methods. Also reproducible if a
subclass/fixture mutates cls.databases after setup or if a class
skipped super().setUpClass() but still inherits the addClassCleanup

Proposed fix

Add an isinstance guard so teardown only touches what setup installed.
On main:

@classmethod
def _remove_databases_failures(cls):
    for alias in connections:
        if alias in cls.databases:
            continue
        connection = connections[alias]
        disallowed_methods = (
            connection.features.disallowed_simple_test_case_connection_methods
        )
        for name, _ in disallowed_methods:
            method = getattr(connection, name)
            if isinstance(method, _DatabaseFailure):
                setattr(connection, name, method.wrapped)

Backport for stable/6.0.x is identical except the iteration source
stays cls._disallowed_connection_methods

isinstance is preferable to the hasattr("wrapped") approach in #20758
because it only unwraps Django's own wrapper class and won't chase
third-party objects that happen to expose .wrapped

Regression tests

Two tests both scoped to teardown:

• test_teardown_noops_when_method_is_not_wrapped - replaces a connection method with a plain function and asserts _remove_databases_failures does not raise.
• test_teardown_still_unwraps_database_failures - installs a _DatabaseFailure manually and asserts it is restored

Full source attached as a patch

Related

• #36942 (closed needsinfo) - same bug, no reprodce
• ​PR #20758 by Michele Fiori uses a hasattr guard; happy to rebase onto isinstance + tests or open a fresh PR crediting the original author

[Simon Charette]

Thanks for spending time reproducing the issue with Claude Rio.

Given this can only be reproduced with pytest-django though I do wonder if the issue doesn't lie over there instead. What does pytest-django do that causes SQLite to recycle the connection between setUpClass and tearDownClass that Django doesn't do?

The fact pytest-django swaps connections entries makes me worried that they have flawed logic on their side that defeats the purpose of the database query prevention mechanisms that Django puts in place.

Nothing seems to indicate that Django is at fault here.

[Rio Weber]

Replying to Simon Charette:

Humm, good call. I thought it was Django by default because I found the #36942 ticket.

I'll investigate and check back

Thanks for spending time reproducing the issue with Claude Rio.

Given this can only be reproduced with pytest-django though I do wonder if the issue doesn't lie over there instead. What does pytest-django do that causes SQLite to recycle the connection between setUpClass and tearDownClass that Django doesn't do?

The fact pytest-django swaps connections entries makes me worried that they have flawed logic on their side that defeats the purpose of the database query prevention mechanisms that Django puts in place.

Nothing seems to indicate that Django is at fault here.

[Rio Weber]

Update: Oh boy this is a deep one. So yes, the bug is in Django, but there's also another bug in ​https://github.com/graphql-python/graphene-django that'll I'll have to file another PR for

I was able to reproduce it under Django's stock manage.py test runner, and I fully removed pytest-django. Same error, same line (testcases.py:280.method.wrapped).

Will comment back with full report

[Rio Weber]

Replying to Simon Charette:

i'm going to wait to keep making PR's,can't update after close.

anyway, I was able to confirm this is a Django bug....
_remove_databases_failures() accesses method.wrapped on every "disallowed" connection-method it walks, so the attribute is still the _DatabaseFailure instance that setup installed. So any mutation between setUpClass and tearDownClass makes teardown crash with:

AttributeError: 'function' object has no attribute 'wrapped'

....…which is unrecoverable, seems like the whole class teardown fails

Notes: Reproduced under stock python manage.py test on Django 6.0.3 (main [@526b548cfb])

Fix is one line of code:
add isinstance(method, _DatabaseFailure) before unwrapping, so teardown becomes same regardless of intermediate attribute state.

(Happy to share the full trace if useful)

[Rio Weber]

I'm going to leave this comment here too for SEO until I get around to opening a PR over there too...

this is the error from graphene-django DjangoDebugMiddleware:
AttributeError: 'function' object has no attribute 'wrapped' at tearDownClass on a TransactionTestCase whose databases excludes a registered alias.

Fix here should resolves it without requiring any change in graphene-django, but i'll open one there too incase this PR takes a whjile to merge

[Simon Charette]

I cannot reproduce against Django 6.0.4 with manage.py test or with pytest with pytest-django installed by creating a new Django project and defining a new app where the single test is

from django.test import TransactionTestCase

class ReproTests(TransactionTestCase):
    # Implicit databases = {"default"} - shard_b is NOT declared
    def test_anything(self):
        assert True

The fact you mention graphene-django and django-debug-toolbar makes me believe there is something else at play here.

Please ensure you are able to reproduce against a minimal project and are not misguided into thinking Django is necessarily at fault by your LLM please.

[Rio Weber]

Replying to Simon Charette:

yeah that's a fare criticism, i was just about to work on that, i'll strip my project down the bare bones for demo

[Rio Weber]

Replying to Simon Charette:

you were right, it's actual.........

hahah jk 😋

I'm sure you get a ton of these, promise i'm not trying to waste your time, 12 years working with django

Here ya go: ​https://github.com/riodw/django-remove_databases_failures-demo

git clone https://github.com/riodw/django-remove_databases_failures-demo.git

[Rio Weber]

i'm fairly certain that example isolates it, unless i've been setting up these DB's wrong haha

[Rio Weber]

(note for posterity: issue found from working on ​https://github.com/riodw/django-graphene-filters) rare going to run into a bug like this unless youre doing crazy stuff like im there

[Simon Charette]

I'll let other triagers chime in but to me ​this doesn't constitute a valid reproduction case.

If you mock with Django internals in your setUp it's your responsibility to restore things to how they were prior to tearDown exit. If you properly do so with

self.addCleanup(setattr, connection, "cursor", original_cursor)

you won't run into any problem.

[Rio Weber]

mmmm, alright.
don't like my barest of bones example, I'll come up with a real world one then.
Mark my words I'm going to get this patched 😅

I hate that i have to add a conftest.py > TransactionTestCase.databases = "__all__" to my django-graphene-filters repo to get the test to not spaz out because i'm switching DB's to make sure it's compatible with sharding, drivng me nuts

[Rio Weber]

Replying to Simon Charette:

So my friend, do you want an example using django-debug-toolbar or django-silk or graphene-django ?

Like it or not I'm going to get you a real word example of a connection.cursor replacement 🤪 it's how every major Django SQL observability tool work

I'll let other triagers chime in but to me ​this doesn't constitute a valid reproduction case.

If you mock with Django internals in your setUp it's your responsibility to restore things to how they were prior to tearDown exit. If you properly do so with

self.addCleanup(setattr, connection, "cursor", original_cursor)

you won't run into any problem.

[Rio Weber]

you know what, how about i give you all threee hahaha

but basically its effectively the sequential execution of like a race condition is the best way i can descrivbe it... django, and graphene-django, both mutate connection.cursor with no cordination and whichever writes last determines whether teardown crashs

[Rio Weber]

I really don't get th push back here , the simple fix i'm asking for is an one line of isinstance to make teardown idempotent, and the impact of this is the entire third-party ecosystem that wraps cursors right now, which is like a lot

[Rio Weber]

so i looked a closer at it, obviously this isn't a security hole or anything but i thought maybe you could leverage this make the pipleines fail but report a pass, nope.

The teardown crash currently errors as hard non-silent failure but.... the defensive isinstance guard turns it into a clean teardow so still worth it.

i built a TransactionTestCase that swaps connection.cursor in setUp call a one function from a 2 method module and ran coverage to see if would corrupt the coverage

anyway, The only way to use this bug to ship bad code would be to have a ci that ignores exit codes and ignores errors > 0 in junit xml

still worth fixing though

[Rio Weber]

lol, dddt 6.3 has explicitly added a workaround for this exact Django bug

[Rio Weber]

hahaha 😂
django-debug-toolbar's own maintainer Daniel Hardign added a check for this

isinstance(connection.cursor, django.test.testcases._DatabaseFailure): return to their wrap_cursor
[a6b65a7c, "Don't try to undo cache method monkey patching"]

[Rio Weber]

Got it ​https://github.com/django-commons/django-debug-toolbar/blob/217238bced43f9e3045b3197ffc94d5985967c65/debug_toolbar/panels/sql/tracking.py#L38

[Rio Weber]

Mr Harding explicitly cites the same "fragility" as affecting Sentrys Django integration and notes that correct teardown is "theoretically… impossible to do correctly" when multiple parties wrap the same attribute

[Rio Weber]

my defensive isinstance guard in the pr there moves that workaround into Django itself, where it's as part of djangos "contract" instead of every cursor-wrapping library

[Rio Weber]

🤞🙏

[Natalia Bidart]

The premise of this ticket is that _remove_databases_failures should be defensive about state it itself established. That framing inverts the actual contract.

_add_databases_failures and _remove_databases_failures are a symmetric setup/teardown pair. The setup installs a _DatabaseFailure wrapper on each disallowed connection method, the teardown removes it. The teardown's assumption that .wrapped exists is not a fragility: it is a direct consequence of the setup having run. If that attribute is missing, something replaced Django's wrapper without restoring the original, which is a bug in that code, not in Django.

The submitted PR confirm this. The test validates the case where a method has been swapped out from under Django without a proper restore, therefore proving the bug exists in the code doing the swapping, then asks Django to silently accommodate it. The proposed isinstance guard also makes teardown silently incomplete: if the invariant is broken, the original method is never restored. The connection is left in whatever unknown state the third-party code deposited. That failure mode is harder to debug than the current AttributeError, which at least points clearly at the code that broke the invariant.

Closing as invalid. If a reproduction can be demonstrated with no third-party connection wrapping involved, and not connection mutation without proper restore, please reopen with a self-contained test case against a clean Django install.

[Rio Weber]

mm, a classic "errors should never pass silently" stance.
Alright then, off to file my PR against graphene-django.. wish me luck

[Rio Weber]

to be clear for anyone else who stumbles here.... SOMEONE has to have isinstance guard when you have more than one connection wrapping. Django has decided to kick the can to other parties, so be aware if the "other party" you're using doesn't do this (since django won't) you will run into problems