[Django] #37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped connection methods

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Type: Bug
Status: new | Component: Testing
| framework
Version: 6.0 | Severity: Normal
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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:
{{{
#!python
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:
{{{
#!python
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:

{{{
#!python
@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
* [https://github.com/django/django/pull/20758 PR #20758] by Michele
Fiori uses a hasattr guard; happy to rebase onto isinstance + tests
or open a fresh PR crediting the original author
--
Ticket URL: <https://code.djangoproject.com/ticket/37064>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------

Reporter: Rio Weber | Owner: (none)

Type: Bug | Status: new
Component: Testing framework | Version: 6.0

Severity: Normal | Resolution:

Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Rio Weber):

* Attachment "remove-databases-failures-isinstance-guard.diff" added.

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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?

Nothing seems to indicate that Django is at fault here.
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:1>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rio Weber):

Replying to [comment:1 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.
--

Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:2>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

**Update:** Oh boy this is a deep one. So yes, the bus 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:3>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

Replying to [comment:1 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:4>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:5>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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 6.0.3
project and defining a new app where the single test is

{{{#!python

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:6>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rio Weber):

Replying to [comment:6 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:7>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

Replying to [comment:6 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`
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:8>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

i'm fairly certain that example isolates it, unless i've been setting up
these DB's wrong haha
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:9>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

(note for posterity: issue found from working on https://github.com/riodw
/django-graphene-filters)
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:10>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

I'll let other triagers chime in but to me [https://github.com/riodw
/django-remove_databases_failures-
demo/blob/3e712b13c558a43458f457ab6b7e110a56390051/demo/repro/tests.py#L16-L22
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

{{{#!python
self.addCleanup(setattr, connection, "cursor", original_cursor)
}}}

you won't run into any problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:11>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:12>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

Replying to [comment:11 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 [https://github.com/riodw
/django-remove_databases_failures-
demo/blob/3e712b13c558a43458f457ab6b7e110a56390051/demo/repro/tests.py#L16-L22
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
>
> {{{#!python
> self.addCleanup(setattr, connection, "cursor", original_cursor)
> }}}
>
> you won't run into any problem.

--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:13>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 coordination and whichever
writes last determines whether teardown crashes
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:14>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

I really don't get th push back , 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:15>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 run manage.py
test` to see if would corrupt the coverage

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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:16>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

lol, dddt 6.3 has explicitly added a workaround for this exact Django bug
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:17>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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"]
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:18>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

Got it https://github.com/django-commons/django-debug-
toolbar/blob/217238bced43f9e3045b3197ffc94d5985967c65/debug_toolbar/panels/sql/tracking.py#L38
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:19>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

Mr Harding explicitly cites the same "fragility" as affecting Sentry's
Django integration and notes that correct teardown is "theoretically…
impossible to do correctly" when multiple parties wrap the same attribute
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:20>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:21>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-------------------------------------+-------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: testcase, | Triage Stage:
databases, multi-db, regression | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Rio Weber):

i beg you please fix this
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:22>

[Django]

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

-----------------------------------+--------------------------------------

Reporter: Rio Weber | Owner: (none)

Type: Bug | Status: closed

Component: Testing framework | Version: 6.0

Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-----------------------------------+--------------------------------------
Changes (by Natalia Bidart):

* keywords: testcase, databases, multi-db, regression =>
* resolution: => invalid
* status: new => closed

Comment:

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:23>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-----------------------------------+--------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: closed
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Rio Weber):

mm, a classic "errors should never pass silently" stance.
Alright then, off to file my PR against `django-graphene`.. wish me luck
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:24>

[Django]

#37064: SimpleTestCase._remove_databases_failures() crashes on un-wrapped
connection methods
-----------------------------------+--------------------------------------
Reporter: Rio Weber | Owner: (none)
Type: Bug | Status: closed
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/37064#comment:25>