diff --git a/AUTHORS b/AUTHORS index 1e99352d5d..6c90cd09f3 100644 --- a/AUTHORS +++ b/AUTHORS @@ -78,6 +78,7 @@ answer newbie questions, and generally made Django that much better: Esdras Beleza Chris Bennett James Bennett + Shai Berger Julian Bez Arvis Bickovskis Natalia Bidart diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py index 890249a77d..9be6e83ee1 100644 --- a/django/db/backends/__init__.py +++ b/django/db/backends/__init__.py @@ -245,10 +245,11 @@ class BaseDatabaseWrapper(local): self.connection = None def cursor(self): - cursor = self._cursor() if (self.use_debug_cursor or (self.use_debug_cursor is None and settings.DEBUG)): - return self.make_debug_cursor(cursor) + cursor = self.make_debug_cursor(self._cursor()) + else: + cursor = util.CursorWrapper(self._cursor(), self) return cursor def make_debug_cursor(self, cursor): diff --git a/django/db/backends/util.py b/django/db/backends/util.py index 3ed18adb37..6438657b71 100644 --- a/django/db/backends/util.py +++ b/django/db/backends/util.py @@ -5,12 +5,28 @@ from time import time from django.utils.hashcompat import md5_constructor from django.utils.log import getLogger + logger = getLogger('django.db.backends') -class CursorDebugWrapper(object): - def __init__(self, cursor, db): + +class CursorWrapper(object): + def __init__(self, cursor, connection): self.cursor = cursor - self.db = db # Instance of a BaseDatabaseWrapper subclass + self.connection = connection + + def __getattr__(self, attr): + if self.connection.is_managed(): + self.connection.set_dirty() + if attr in self.__dict__: + return self.__dict__[attr] + else: + return getattr(self.cursor, attr) + + def __iter__(self): + return iter(self.cursor) + + +class CursorDebugWrapper(CursorWrapper): def execute(self, sql, params=()): start = time() @@ -19,8 +35,8 @@ class CursorDebugWrapper(object): finally: stop = time() duration = stop - start - sql = self.db.ops.last_executed_query(self.cursor, sql, params) - self.db.queries.append({ + sql = self.connection.ops.last_executed_query(self.cursor, sql, params) + self.connection.queries.append({ 'sql': sql, 'time': "%.3f" % duration, }) @@ -35,7 +51,7 @@ class CursorDebugWrapper(object): finally: stop = time() duration = stop - start - self.db.queries.append({ + self.connection.queries.append({ 'sql': '%s times: %s' % (len(param_list), sql), 'time': "%.3f" % duration, }) @@ -52,6 +68,7 @@ class CursorDebugWrapper(object): def __iter__(self): return iter(self.cursor) + ############################################### # Converters from database (string) to Python # ############################################### diff --git a/docs/releases/1.3.txt b/docs/releases/1.3.txt index ad93f09185..aca227f896 100644 --- a/docs/releases/1.3.txt +++ b/docs/releases/1.3.txt @@ -604,6 +604,42 @@ domain): .. _corresponding deprecated features section: loading_of_translations_from_the_project_directory_ +Transaction management +~~~~~~~~~~~~~~~~~~~~~~ + +When using managed transactions -- that is, anything but the default +autocommit mode -- it is important when a transaction is marked as +"dirty". Dirty transactions are committed by the +:func:`~django.db.transaction.commit_on_success` decorator or the +:class:`~django.middleware.transaction.TransactionMiddleware`, and +:func:`~django.db.transaction.commit_manually` forces them to be +closed explicitly; clean transactions "get a pass", which means they +are usually rolled back at the end of a request when the connection is +closed. + +Until Django 1.3, transactions were only marked dirty when Django was +aware of a modifying operation performed in them; that is, either some +model was saved, some bulk update or delete was performed, or the user +explicitly called ``transaction.set_dirty()``. In Django 1.3, a +transaction is marked dirty when *any* database operation is +performed. + +As a result of this change, you no longer need to set a transaction +dirty explicitly when you execute raw SQL or use a data-modifying +``SELECT``. However, you *do* need to explicitly close any read-only +transactions that are being managed using +:func:`~django.db.transaction.commit_manually`. For example:: + + @transaction.commit_manually + def my_view(request, name): + obj = get_object_or_404(MyObject, name__iexact=name) + return render_to_response('template', {'object':obj}) + +Prior to Django 1.3, this would work without error. However, under +Django 1.3, this will raise a :class:`TransactionManagementError` because +the read operation that retrieves the ``MyObject`` instance leaves the +transaction in a dirty state. + .. _deprecated-features-1.3: Features deprecated in 1.3 diff --git a/docs/topics/db/sql.txt b/docs/topics/db/sql.txt index 2698bb0f04..6be59c5d5a 100644 --- a/docs/topics/db/sql.txt +++ b/docs/topics/db/sql.txt @@ -233,37 +233,17 @@ alias:: Transactions and raw SQL ------------------------ -If you are using transaction decorators (such as ``commit_on_success``) to -wrap your views and provide transaction control, you don't have to make a -manual call to ``transaction.commit_unless_managed()`` -- you can manually -commit if you want to, but you aren't required to, since the decorator will -commit for you. However, if you don't manually commit your changes, you will -need to manually mark the transaction as dirty, using -``transaction.set_dirty()``:: - @commit_on_success - def my_custom_sql_view(request, value): - from django.db import connection, transaction - cursor = connection.cursor() +When you make a raw SQL call, Django will automatically mark the +current transaction as dirty. You must then ensure that the +transaction containing those calls is closed correctly. See :ref:`the +notes on the requirements of Django's transaction handling +` for more details. - # Data modifying operation - cursor.execute("UPDATE bar SET foo = 1 WHERE baz = %s", [value]) +.. versionchanged:: 1.3 - # Since we modified data, mark the transaction as dirty - transaction.set_dirty() - - # Data retrieval operation. This doesn't dirty the transaction, - # so no call to set_dirty() is required. - cursor.execute("SELECT foo FROM bar WHERE baz = %s", [value]) - row = cursor.fetchone() - - return render_to_response('template.html', {'row': row}) - -The call to ``set_dirty()`` is made automatically when you use the Django ORM -to make data modifying database calls. However, when you use raw SQL, Django -has no way of knowing if your SQL modifies data or not. The manual call to -``set_dirty()`` ensures that Django knows that there are modifications that -must be committed. +Prior to Django 1.3, it was necessary to manually mark a transaction +as dirty using ``transaction.set_dirty()`` when using raw SQL calls. Connections and cursors ----------------------- diff --git a/docs/topics/db/transactions.txt b/docs/topics/db/transactions.txt index 80971613e0..6e7288ec1d 100644 --- a/docs/topics/db/transactions.txt +++ b/docs/topics/db/transactions.txt @@ -70,35 +70,35 @@ per-function or per-code-block basis. These functions, described in detail below, can be used in two different ways: * As a decorator_ on a particular function. For example:: - + from django.db import transaction - + @transaction.commit_on_success() def viewfunc(request): # ... # this code executes inside a transaction # ... - + This technique works with all supported version of Python (that is, with Python 2.4 and greater). - + * As a `context manager`_ around a particular block of code:: - + from django.db import transaction - + def viewfunc(request): # ... # this code executes using default transaction management - # ... - + # ... + with transaction.commit_on_success(): # ... # this code executes inside a transaction # ... - + The ``with`` statement is new in Python 2.5, and so this syntax can only be used with Python 2.5 and above. - + .. _decorator: http://docs.python.org/glossary.html#term-decorator .. _context manager: http://docs.python.org/glossary.html#term-context-manager @@ -106,7 +106,7 @@ For maximum compatibility, all of the examples below show transactions using the decorator syntax, but all of the follow functions may be used as context managers, too. -.. note:: +.. note:: Although the examples below use view functions as examples, these decorators and context managers can be used anywhere in your code @@ -187,6 +187,25 @@ managers, too. def viewfunc2(request): .... +.. _topics-db-transactions-requirements: + +Requirements for transaction handling +===================================== + +.. versionadded:: 1.3 + +Django requires that every transaction that is opened is closed before +the completion of a request. If you are using :func:`autocommit` (the +default commit mode) or :func:`commit_on_success`, this will be done +for you automatically. However, if you are manually managing +transactions (using the :func:`commit_manually` decorator), you must +ensure that the transaction is either committed or rolled back before +a request is completed. + +This applies to all database operations, not just write operations. Even +if your transaction only reads from the database, the transaction must +be committed or rolled back before you complete a request. + How to globally deactivate transaction management ================================================= diff --git a/tests/regressiontests/delete_regress/tests.py b/tests/regressiontests/delete_regress/tests.py index 06f3b5c866..b339935013 100644 --- a/tests/regressiontests/delete_regress/tests.py +++ b/tests/regressiontests/delete_regress/tests.py @@ -62,6 +62,7 @@ class DeleteLockingTest(TransactionTestCase): Book.objects.filter(pagecount__lt=250).delete() transaction.commit() self.assertEqual(1, Book.objects.count()) + transaction.commit() class DeleteCascadeTests(TestCase): diff --git a/tests/regressiontests/fixtures_regress/tests.py b/tests/regressiontests/fixtures_regress/tests.py index 348a754d34..e7f24585a0 100644 --- a/tests/regressiontests/fixtures_regress/tests.py +++ b/tests/regressiontests/fixtures_regress/tests.py @@ -610,6 +610,7 @@ class TestTicket11101(TransactionTestCase): self.assertEqual(Thingy.objects.count(), 1) transaction.rollback() self.assertEqual(Thingy.objects.count(), 0) + transaction.commit() @skipUnlessDBFeature('supports_transactions') def test_ticket_11101(self): diff --git a/tests/regressiontests/transactions_regress/__init__.py b/tests/regressiontests/transactions_regress/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/regressiontests/transactions_regress/models.py b/tests/regressiontests/transactions_regress/models.py new file mode 100644 index 0000000000..54e6f4f37b --- /dev/null +++ b/tests/regressiontests/transactions_regress/models.py @@ -0,0 +1,4 @@ +from django.db import models + +class Mod(models.Model): + fld = models.IntegerField() diff --git a/tests/regressiontests/transactions_regress/tests.py b/tests/regressiontests/transactions_regress/tests.py new file mode 100644 index 0000000000..a7ff029ed1 --- /dev/null +++ b/tests/regressiontests/transactions_regress/tests.py @@ -0,0 +1,164 @@ +from django.core.exceptions import ImproperlyConfigured +from django.db import connection, transaction +from django.db.transaction import commit_on_success, commit_manually, TransactionManagementError +from django.test import TransactionTestCase, skipUnlessDBFeature + +from models import Mod + + +class TestTransactionClosing(TransactionTestCase): + """ + Tests to make sure that transactions are properly closed + when they should be, and aren't left pending after operations + have been performed in them. Refs #9964. + """ + def test_raw_committed_on_success(self): + """ + Make sure a transaction consisting of raw SQL execution gets + committed by the commit_on_success decorator. + """ + @commit_on_success + def raw_sql(): + "Write a record using raw sql under a commit_on_success decorator" + cursor = connection.cursor() + cursor.execute("INSERT into transactions_regress_mod (id,fld) values (17,18)") + + raw_sql() + # Rollback so that if the decorator didn't commit, the record is unwritten + transaction.rollback() + try: + # Check that the record is in the DB + obj = Mod.objects.get(pk=17) + self.assertEqual(obj.fld, 18) + except Mod.DoesNotExist: + self.fail("transaction with raw sql not committed") + + def test_commit_manually_enforced(self): + """ + Make sure that under commit_manually, even "read-only" transaction require closure + (commit or rollback), and a transaction left pending is treated as an error. + """ + @commit_manually + def non_comitter(): + "Execute a managed transaction with read-only operations and fail to commit" + _ = Mod.objects.count() + + self.assertRaises(TransactionManagementError, non_comitter) + + def test_commit_manually_commit_ok(self): + """ + Test that under commit_manually, a committed transaction is accepted by the transaction + management mechanisms + """ + @commit_manually + def committer(): + """ + Perform a database query, then commit the transaction + """ + _ = Mod.objects.count() + transaction.commit() + + try: + committer() + except TransactionManagementError: + self.fail("Commit did not clear the transaction state") + + def test_commit_manually_rollback_ok(self): + """ + Test that under commit_manually, a rolled-back transaction is accepted by the transaction + management mechanisms + """ + @commit_manually + def roller_back(): + """ + Perform a database query, then rollback the transaction + """ + _ = Mod.objects.count() + transaction.rollback() + + try: + roller_back() + except TransactionManagementError: + self.fail("Rollback did not clear the transaction state") + + def test_commit_manually_enforced_after_commit(self): + """ + Test that under commit_manually, if a transaction is committed and an operation is + performed later, we still require the new transaction to be closed + """ + @commit_manually + def fake_committer(): + "Query, commit, then query again, leaving with a pending transaction" + _ = Mod.objects.count() + transaction.commit() + _ = Mod.objects.count() + + self.assertRaises(TransactionManagementError, fake_committer) + + @skipUnlessDBFeature('supports_transactions') + def test_reuse_cursor_reference(self): + """ + Make sure transaction closure is enforced even when the queries are performed + through a single cursor reference retrieved in the beginning + (this is to show why it is wrong to set the transaction dirty only when a cursor + is fetched from the connection). + """ + @commit_on_success + def reuse_cursor_ref(): + """ + Fetch a cursor, perform an query, rollback to close the transaction, + then write a record (in a new transaction) using the same cursor object + (reference). All this under commit_on_success, so the second insert should + be committed. + """ + cursor = connection.cursor() + cursor.execute("INSERT into transactions_regress_mod (id,fld) values (1,2)") + transaction.rollback() + cursor.execute("INSERT into transactions_regress_mod (id,fld) values (1,2)") + + reuse_cursor_ref() + # Rollback so that if the decorator didn't commit, the record is unwritten + transaction.rollback() + try: + # Check that the record is in the DB + obj = Mod.objects.get(pk=1) + self.assertEquals(obj.fld, 2) + except Mod.DoesNotExist: + self.fail("After ending a transaction, cursor use no longer sets dirty") + + def test_failing_query_transaction_closed(self): + """ + Make sure that under commit_on_success, a transaction is rolled back even if + the first database-modifying operation fails. + This is prompted by http://code.djangoproject.com/ticket/6669 (and based on sample + code posted there to exemplify the problem): Before Django 1.3, + transactions were only marked "dirty" by the save() function after it successfully + wrote the object to the database. + """ + from django.contrib.auth.models import User + + @transaction.commit_on_success + def create_system_user(): + "Create a user in a transaction" + user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com') + # Redundant, just makes sure the user id was read back from DB + Mod.objects.create(fld=user.id) + + # Create a user + create_system_user() + + try: + # The second call to create_system_user should fail for violating a unique constraint + # (it's trying to re-create the same user) + create_system_user() + except: + pass + else: + raise ImproperlyConfigured('Unique constraint not enforced on django.contrib.auth.models.User') + + try: + # Try to read the database. If the last transaction was indeed closed, + # this should cause no problems + _ = User.objects.all()[0] + except: + self.fail("A transaction consisting of a failed operation was not closed.")