Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#17760 closed Bug (fixed)

connection.features.supports_transactions is None

Reported by: cdestigter 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 claudep 4 years ago.
Replace confirm() by cached properties
17760-2.diff (7.0 KB) - added by claudep 4 years ago.
Using the cached_property decorator
17760-3.diff (7.2 KB) - added by cdestigter 4 years ago.
minor bug fix for 17760-2.diff
17760-4.diff (7.6 KB) - added by ramiro 3 years ago.
Fixed problem with detction of STDDEV on sqlite3
17760-5.diff (8.9 KB) - added by claudep 3 years ago.
Updated to current trunk

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by cdestigter

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago 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...

Changed 4 years ago by claudep

Replace confirm() by cached properties

comment:3 Changed 4 years ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

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

comment:4 Changed 4 years ago 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.

Changed 4 years ago by claudep

Using the cached_property decorator

comment:5 Changed 4 years ago by claudep

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

Changed 4 years ago by cdestigter

minor bug fix for 17760-2.diff

comment:6 Changed 4 years ago by charettes

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

Changed 3 years ago by ramiro

Fixed problem with detction of STDDEV on sqlite3

Changed 3 years ago by claudep

Updated to current trunk

comment:7 Changed 3 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 3 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 3 years ago by Claude Paroz <claude@…>

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