Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#17760 closed Bug (fixed)

connection.features.supports_transactions is None

Reported by: Craig de Stigter Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (5)

17760.diff (6.0 KB ) - added by Claude Paroz 13 years ago.
Replace confirm() by cached properties
17760-2.diff (7.0 KB ) - added by Claude Paroz 13 years ago.
Using the cached_property decorator
17760-3.diff (7.2 KB ) - added by Craig de Stigter 13 years ago.
minor bug fix for 17760-2.diff
17760-4.diff (7.6 KB ) - added by Ramiro Morales 13 years ago.
Fixed problem with detction of STDDEV on sqlite3
17760-5.diff (8.9 KB ) - added by Claude Paroz 12 years ago.
Updated to current trunk

Download all attachments as: .zip

Change History (14)

comment:2 by Anssi Kääriäinen, 13 years ago

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...

by Claude Paroz, 13 years ago

Attachment: 17760.diff added

Replace confirm() by cached properties

comment:3 by Claude Paroz, 13 years ago

Triage Stage: UnreviewedAccepted

I just translated akaariai's comment:2 into a patch. Not sure if something should be tested here.

comment:4 by Anssi Kääriäinen, 13 years ago

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.

by Claude Paroz, 13 years ago

Attachment: 17760-2.diff added

Using the cached_property decorator

comment:5 by Claude Paroz, 13 years ago

Thanks for the hint Anssi, it's indeed nicer with the cached_property decorator.

by Craig de Stigter, 13 years ago

Attachment: 17760-3.diff added

minor bug fix for 17760-2.diff

comment:6 by Simon Charette, 13 years ago

FWIW I recently committed a patch to south to use the same approach for DDL transaction support detection and it works quite well.

by Ramiro Morales, 13 years ago

Attachment: 17760-4.diff added

Fixed problem with detction of STDDEV on sqlite3

by Claude Paroz, 12 years ago

Attachment: 17760-5.diff added

Updated to current trunk

comment:7 by Claude Paroz, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Claude Paroz <claude@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [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.

comment:9 by Claude Paroz <claude@…>, 12 years ago

In [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).

Note: See TracTickets for help on using tickets.
Back to Top