Ticket #9964: 9964-r15054-bugfix.patch

django/db/backends/util.py

@@ -52 +52 @@
     def __iter__(self):
         return iter(self.cursor)
 
+class CursorUseNotifyWrapper(object):
+    def __init__(self, cursor, on_use):
+        self.cursor = cursor
+        self.on_use = on_use
+    def __getattr__(self, attr):
+        self.on_use()
+        return getattr(self.cursor, attr)
+    def __iter__(self):
+        return iter(self.cursor)
+
 ###############################################
 # Converters from database (string) to Python #
 ###############################################

django/db/backends/__init__.py

@@ -22 +22 @@
         self.settings_dict = settings_dict
         self.alias = alias
         self.use_debug_cursor = None
+        self.on_used = None
 
     def __eq__(self, other):
         return self.alias == other.alias
@@ -75 +76 @@
     def cursor(self):
         from django.conf import settings
         cursor = self._cursor()
+        # Used by transaction mechanism to be notified that db was used
+        if self.on_used:
+            cursor = util.CursorUseNotifyWrapper(cursor, self.on_used)
         if (self.use_debug_cursor or
             (self.use_debug_cursor is None and settings.DEBUG)):
             return self.make_debug_cursor(cursor)

django/db/transaction.py

@@ -67 +67 @@
     if thread_ident not in dirty or using not in dirty[thread_ident]:
         dirty.setdefault(thread_ident, {})
         dirty[thread_ident][using] = False
+    def set_dirty_if_managed():
+        if is_managed(using): set_dirty(using)
+    connection.on_used = set_dirty_if_managed
     connection._enter_transaction_management(managed)
 
 def leave_transaction_management(using=None):

tests/regressiontests/transaction_regress/tests.py

@@ +1 @@
+from django.test import TransactionTestCase
+from django.db import connection, transaction
+from django.db.transaction import \
+    commit_on_success, commit_manually, \
+    TransactionManagementError
+from django.core.exceptions import ImproperlyConfigured
+from models import Mod
+
+class Test9964(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.
+
+    Prompted by http://code.djangoproject.com/ticket/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 transaction_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)
+
+    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 transaction_regress_mod (id,fld) values (1,2)")
+            transaction.rollback()
+            cursor.execute("INSERT into transaction_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.")

tests/regressiontests/transaction_regress/models.py

@@ +1 @@
+from django.db import models
+
+class Mod(models.Model):
+    fld = models.IntegerField()

tests/regressiontests/delete_regress/tests.py

@@ -61 +61 @@
         Book.objects.filter(pagecount__lt=250).delete()
         transaction.commit()
         self.assertEqual(1, Book.objects.count())
+        transaction.commit()
 
 class DeleteCascadeTests(TestCase):
     def test_generic_relation_cascade(self):

tests/regressiontests/fixtures_regress/tests.py

@@ -610 +610 @@
         self.assertEqual(Thingy.objects.count(), 1)
         transaction.rollback()
         self.assertEqual(Thingy.objects.count(), 0)
+        transaction.commit()
 
     def test_ticket_11101(self):
         """Test that fixtures can be rolled back (ticket #11101)."""

docs/topics/db/sql.txt

@@ -231 +231 @@
 
 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()
+.. versionchanged:: 1.3
 
-        # Data modifying operation
-        cursor.execute("UPDATE bar SET foo = 1 WHERE baz = %s", [value])
+If you write functions that modify the database via raw SQL, you need to
+consider the transaction management mode they run under. If this may be
+Django's default mode (by default or using the ``auto_commit``
+decorator), you need to make manual calls to
+``transaction.commit_unless_managed()`` to make sure the changes are
+committed. 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 such calls.
 
-        # 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.
-
 Connections and cursors
 -----------------------
 

docs/releases/1.3.txt

@@ -378 +378 @@
 
 
 
+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 ``commit_on_success``
+decorator or the ``TransactionMiddleware``, and ``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;
+this means you no longer need to set a transaction dirty explicitly
+when you execute raw SQL or use a data-modifying ``select``. On the
+other hand, you do need to explicitly close read-only transactions
+under ``commit_manually``. For example, take a look at this code::
+
+      @transaction.commit_manually
+      def my_view(request, name):
+          obj = get_object_or_404(MyObject, name__iexact=name)
+          return render_to_response('template', {'object':obj})
+
+Until Django 1.3, this works fine. With Django 1.3, this raises a
+``TransactionManagementError``; you need to end the transaction
+explicitly, one way or another.
+
 .. _deprecated-features-1.3:
 
 Features deprecated in 1.3