[Django] #17760: connection.features.supports_transactions is None
Django
----------------------------------------------+--------------------
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
Background: To speed up our tests we have a custom nose-based test runner
which *doesn't* drop/create the database. It shouldn't need to when tests
are run in a transaction. This prevents us having to load fixtures etc, as
most of the test data lives permanently in the database.
However after upgrading to django 1.3,
`connection.features.supports_transactions` is always None, causing our
tests to use TRUNCATEs instead of transactions.
This appears to be because connection.features.confirm() is only called
from create_test_db():
https://code.djangoproject.com/browser/django/tags/releases/1.3/django/db/backends/creation.py#L357
I'd argue that's the wrong place to do it. It should probably happen when
the connection is first initialised, or at least in
`TestCase._pre_setup()`
This has caused issues before: #16622 #16885 (possibly others)
(To be fair it shouldn't really be on connection.features at all, since
it's only set during testing and is otherwise always None)
Tested with 1.3.1 and master.
--
Ticket URL: <https://code.djangoproject.com/ticket/17760>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by cdestigter):
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_better_patch: => 0
Comment:
initial fix:
https://github.com/koordinates/django/commit/66de288e80377e645369d56e44fcdb02dbed0276
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Comment (by akaariai):
I think the connection.features.supports_transactions should be a cached
property. It checks the support when first accessed, later on it is just a
normal boolean. That way there would be no need to run features.confirm()
at all, supports transactions is always usable.
I haven't verified the issue, so the above is more a general note...
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* stage: Unreviewed => Accepted
Comment:
I just translated akaariai's comment:2 into a patch. Not sure if something
should be tested here.
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
There is a @cached_property decorator at django/utils/functional.py, it is
meant for situations like this. Maybe it would make the implementation a
bit cleaner.
You could perhaps test that no queries are made after first use of the
property, but if you use the @cached_property decorator, I don't know how
necessary that is. It might be better to write a test for the
@cached_property decorator, and then trust that it does the right thing.
In short, I don't know if there is any need for additional tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by claudep):
Thanks for the hint Anssi, it's indeed nicer with the cached_property
decorator.
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
FWIW I recently committed a patch to south to use the same approach for
[https://bitbucket.org/andrewgodwin/south/changeset/00074f0d28e7 DDL
transaction support detection] and it works quite well.
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
(models, ORM) | Resolution:
Keywords: | checkin
Changes (by claudep):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [aa423575e7b464433fcfc4bf4b8e1d7627b17ce6]:
{{{
#!CommitTicketReference repository=""
revision="aa423575e7b464433fcfc4bf4b8e1d7627b17ce6"
Fixed #17760 -- Implemented callable database features as cached
properties
This does remove the requirement to call features.confirm() method
before checking the properties.
Thanks cdestiger and Ramiro Morales for their work on the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:8>
Django
-------------------------------------+-------------------------------------
Component: Database layer | Version: 1.3
(models, ORM) | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Comment (by Claude Paroz <claude@…>):
In [ad47364dd324508e8332ea853da59772431398aa]:
{{{
#!CommitTicketReference repository=""
revision="ad47364dd324508e8332ea853da59772431398aa"
Reverted 905e33f, now that DatabaseFeatures does not need confirm
Connection.features does not need to be confirmed any more, after
commit aa42357, rendering obsolete the workaround when using
TEST_MIRROR (Refs #16885, #17760).
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17760#comment:9>