[Django] #27074: connection.is_usable() can raise AttributeError in natural uses
Django
----------------------------------------------+--------------------
Reporter: cjerdonek | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
I know a [https://code.djangoproject.com/ticket/26282 similar issue] was
closed relatively recently as won't fix, but I wanted to report a more
obvious example of the same issue just to be sure.
The following test case--
{{{#!python
from django.db import connection
from django.test import TransactionTestCase
class MyTest(TransactionTestCase):
def test_connection_close(self):
self.assertTrue(connection.is_usable())
connection.close()
self.assertFalse(connection.is_usable())
}}}
results in the following error (though if `TransactionTestCase` is
replaced by `TestCase`, it would behave as expected)--
{{{#!python
Traceback (most recent call last):
...
File ".../test_database.py", line 23, in test_connection_close
self.assertTrue(connection.is_usable())
File ".../site-packages/django/db/backends/postgresql/base.py", line
229, in is_usable
self.connection.cursor().execute("SELECT 1")
AttributeError: 'NoneType' object has no attribute 'cursor'
}}}
Would there be any harm in handling the case of `self.connection` being
`None` (e.g. using EAFP)? It seems like this could even be done so the
logic is only in the base class.
--
Ticket URL: <https://code.djangoproject.com/ticket/27074>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: cjerdonek | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by boblefrag):
* cc: boblefrag@… (added)
Comment:
This is no longer the case on django master branch:
{{{
def is_usable(self):
try:
# Use a psycopg cursor directly, bypassing Django's utilities.
self.connection.cursor().execute("SELECT 1")
except Database.Error:
return False
else:
return True
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Unless I missed something, this code matches the trackback reported above,
so the issue likely still exists.
The fix might be as simple as returning False `if self.connection is None`
(assuming that doesn't break tests).
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by boblefrag):
My bad, I was testing with sqlite3. I can reproduce with postgresql
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
I believe the trickiest part of the patch will be handling / dealing with
the fact that there are multiple backend implementations of `is_usable()`.
Ideally, I think one would want to put the logic in the base
implementation and call `super()` as appropriate. Alternatively, it looks
like it would be sufficient to include the logic in all three non-trivial
implementations: mysql, oracle, and postgresql. Those each look like the
following:
{{{#!python
def is_usable(self):
try:
self.connection.ping()
except Database.Error:
return False
else:
return True
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by boblefrag):
PR: https://github.com/django/django/pull/7114
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Also, rather than checking for `connection is None` in advance, using
[https://docs.python.org/3/glossary.html#term-eafp EAFP] and checking for
`None` in an `except` block (e.g. handling AttributeError) is probably
more Pythonic.
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Also, for future reference, an argument for putting the logic in the base
implementation is that it's the base implementation that sets
`self.connection` to `None` in the first place (in the finally block of
its `close()` method, when not in an atomic block). See
[https://github.com/django/django/blob/76ab8851181675a59425f9637bdbf3f2de95f6f1/django/db/backends/base/base.py#L266
here] for the code.
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
This is a private API that Django primarily calls inside an `if
self.connection is not None:` block (and secondarily in testing tools
where database connection breakage isn't a concern and will abort tests in
general). For this reason, I don't expect this issue to appear through the
use of public APIs.
I'm -0 on making that change but I understand that we might have to add a
useless check just to stop these bugs reports. The performance impact will
be negligible.
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
Comment:
I will now put together a PR for this, which is what I was originally
planning to do when I first created this ticket (but someone else opened
one before me).
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Chris Jerdonek => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/27074#comment:13>