Ticket #9964: 9964-make-managed-transactions-dirty-v3-full.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.alias = alias
         self.vendor = 'unknown'
         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

@@ -12 +12 @@
 or implicit commits or rollbacks.
 """
 import sys
+from warnings import warn
 
 try:
     import thread
@@ -32 +33 @@
     """
     pass
 
+class ReadonlyTransactionWarning(PendingDeprecationWarning):
+    """
+    This is a warning issued when there is a difference between current behavior
+    and future behavior due to read-only transactions being changed from clean
+    to dirty.
+    """
+    pass
+
 # The states are dictionaries of dictionaries of lists. The key to the outer
 # dict is the current thread, and the key to the inner dictionary is the
 # connection alias and the list is handled as a stack of values.
 state = {}
 savepoint_state = {}
 
-# The dirty flag is set by *_unless_managed functions to denote that the
-# code under transaction management has changed things to require a
+# The dirty flag is set whenever a database cursor is touched to denote that the
+# code under transaction management has done things to require a
 # database commit.
 # This is a dictionary mapping thread to a dictionary mapping connection
 # alias to a boolean.
 dirty = {}
+# The deprecated_dirty flag is set by *_unless_managed functions. It is the old,
+# slightly incorrect sense of dirty whereby read-only transactions may be left pending.
+# This has the same structure as the dirty flag.
+deprecated_dirty = {}
 
 def enter_transaction_management(managed=True, using=None):
     """
@@ -67 +80 @@
     if thread_ident not in dirty or using not in dirty[thread_ident]:
         dirty.setdefault(thread_ident, {})
         dirty[thread_ident][using] = False
+    if thread_ident not in deprecated_dirty or using not in deprecated_dirty[thread_ident]:
+        deprecated_dirty.setdefault(thread_ident, {})
+        deprecated_dirty[thread_ident][using] = False
+    def set_dirty_if_managed():
+        if is_managed(using): _set_dirty(dirty, using)
+    connection.on_used = set_dirty_if_managed
     connection._enter_transaction_management(managed)
 
 def leave_transaction_management(using=None):
@@ -84 +103 @@
         del state[thread_ident][using][-1]
     else:
         raise TransactionManagementError("This code isn't under transaction management")
-    if dirty.get(thread_ident, {}).get(using, False):
+    if deprecated_dirty.get(thread_ident, {}).get(using, False):
+        # The transaction is dirty in the strict sense. This has always been an error
         rollback(using=using)
         raise TransactionManagementError("Transaction managed block ended with pending COMMIT/ROLLBACK")
+    elif dirty.get(thread_ident, {}).get(using, False):
+        #rollback(using=using)
+        warn("Transaction managed block ended with pending COMMIT/ROLLBACK", ReadonlyTransactionWarning)
     dirty[thread_ident][using] = False
 
+def _is_dirty(dirty_dict, using=None):
+    """
+    Returns True if the current transaction requires a commit to close.
+    """
+    # TODO: Once deprecated_dirty is eliminated, fold this back into is_dirty
+    if using is None:
+        using = DEFAULT_DB_ALIAS
+    return dirty_dict.get(thread.get_ident(), {}).get(using, False)
+
 def is_dirty(using=None):
     """
     Returns True if the current transaction requires a commit for changes to
-    happen.
+    happen. Warns if no known changes happened, but a commit/rollback is
+    still required.
     """
     if using is None:
         using = DEFAULT_DB_ALIAS
-    return dirty.get(thread.get_ident(), {}).get(using, False)
+    flag = _is_dirty(deprecated_dirty, using)
+    if flag != _is_dirty(dirty, using):
+        warn("Read-only transactions will be considered dirty in the future",
+             ReadonlyTransactionWarning)
+    return flag
 
-def set_dirty(using=None):
+def _set_dirty(dirty_dict, using=None):
     """
     Sets a dirty flag for the current thread and code streak. This can be used
     to decide in a managed block of code to decide whether there are open
     changes waiting for commit.
     """
+    # TODO: Once deprecated_dirty is eliminated, fold this back into set_dirty
     if using is None:
         using = DEFAULT_DB_ALIAS
     thread_ident = thread.get_ident()
-    if thread_ident in dirty and using in dirty[thread_ident]:
-        dirty[thread_ident][using] = True
+    if thread_ident in dirty_dict and using in dirty_dict[thread_ident]:
+        dirty_dict[thread_ident][using] = True
     else:
         raise TransactionManagementError("This code isn't under transaction management")
 
-def set_clean(using=None):
+def set_dirty(using=None):
     """
+    Sets a dirty flag for the current thread and code streak. This can be used
+    to decide in a managed block of code to decide whether there are open
+    changes waiting for commit.
+    """
+    _set_dirty(deprecated_dirty, using)
+    _set_dirty(dirty, using)
+
+def _set_clean(dirty_dict, using=None):
+    """
     Resets a dirty flag for the current thread and code streak. This can be used
     to decide in a managed block of code to decide whether a commit or rollback
     should happen.
     """
+    # TODO: Once deprecated_dirty is eliminated, fold this back into set_clean
     if using is None:
         using = DEFAULT_DB_ALIAS
     thread_ident = thread.get_ident()
-    if thread_ident in dirty and using in dirty[thread_ident]:
-        dirty[thread_ident][using] = False
+    if thread_ident in dirty_dict and using in dirty_dict[thread_ident]:
+        dirty_dict[thread_ident][using] = False
     else:
         raise TransactionManagementError("This code isn't under transaction management")
+
+def set_clean(using=None):
+    _set_clean(deprecated_dirty, using)
+    _set_clean(dirty, using)
     clean_savepoints(using=using)
 
 def clean_savepoints(using=None):
@@ -347 +399 @@
     def exiting(exc_value, using):
         try:
             if exc_value is not None:
-                if is_dirty(using=using):
+                if _is_dirty(dirty, using=using):
                     rollback(using=using)
             else:
-                if is_dirty(using=using):
+                if _is_dirty(dirty, using=using):
                     try:
                         commit(using=using)
                     except:

django/middleware/transaction.py

@@ -1 +1 @@
 from django.db import transaction
 
+def _transaction_is_dirty():
+    """
+    While the sense of a dirty transaction is being changed,
+    this is the way to use the "new sense" (any touching of the
+    db makes the transaction dirty)
+    """
+    # TODO: once transaction.deprecated_dirty is eliminated, delete this
+    #       and use transaction.is_dirty instead
+    return transaction._is_dirty(transaction.dirty)
+
 class TransactionMiddleware(object):
     """
     Transaction middleware. If this is enabled, each view function will be run
@@ -14 +24 @@
 
     def process_exception(self, request, exception):
         """Rolls back the database and leaves transaction management"""
-        if transaction.is_dirty():
+        if _transaction_is_dirty():
             transaction.rollback()
         transaction.leave_transaction_management()
 
     def process_response(self, request, response):
         """Commits and leaves transaction management."""
         if transaction.is_managed():
-            if transaction.is_dirty():
+            if _transaction_is_dirty():
                 transaction.commit()
             transaction.leave_transaction_management()
         return response
+

tests/regressiontests/transaction_regress/tests.py

@@ +1 @@
+from warnings import filterwarnings
+from django.test import TransactionTestCase
+from django.db import connection, transaction
+from django.db.transaction import \
+    commit_on_success, commit_manually, \
+    TransactionManagementError, ReadonlyTransactionWarning
+from models import Mod
+
+class Test9964(TransactionTestCase):
+
+    def setUp(self):
+        filterwarnings("error", ".*", ReadonlyTransactionWarning)
+        
+    def test_raw_committed_on_success(self):
+        
+        @commit_on_success
+        def raw_sql():
+            cursor = connection.cursor()
+            cursor.execute("INSERT into transaction_regress_mod (id,fld) values (17,18)")
+            
+        raw_sql()
+        transaction.rollback()
+        try:
+            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):
+        @commit_manually
+        def non_comitter():
+            Mod.objects.create(fld=55)
+            
+        self.assertRaises(TransactionManagementError, non_comitter)
+
+    def test_commit_manually_enforced_readonly(self):
+        @commit_manually
+        def non_comitter():
+            _ = Mod.objects.count()
+            
+        self.assertRaises(ReadonlyTransactionWarning, non_comitter)
+
+    def test_commit_manually_commit_ok(self):
+        @commit_manually
+        def committer():
+            _ = 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):
+        @commit_manually
+        def roller_back():
+            _ = 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):
+        @commit_manually
+        def fake_committer():
+            _ = Mod.objects.count()
+            transaction.commit()
+            _ = Mod.objects.count()            
+            
+        self.assertRaises(ReadonlyTransactionWarning, fake_committer)
+
+    def test_reuse_cursor_reference(self):
+        @commit_on_success
+        def reuse_cursor_ref():
+            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()
+        transaction.rollback()
+        try:
+            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_6669(self):
+
+        from django.contrib.auth.models import User
+
+        @transaction.commit_on_success
+        def create_system_user():
+            user = User.objects.create_user(username='system', password='iamr00t', email='root@SITENAME.com')
+            Mod.objects.create(fld=user.id)
+
+        try:
+            create_system_user()
+        except:
+            pass
+
+        try:
+            create_system_user()
+        except:
+            pass
+
+        print User.objects.all()

tests/regressiontests/transaction_regress/models.py

@@ +1 @@
+from django.db import models
+
+class Mod(models.Model):
+    fld = models.IntegerField()
+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
+   calling ``set_dirty()`` no longer required in managed transactions
 
-        # 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 managed transactions (e.g. with the
+``commit_on_success`` decorator), 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

@@ -301 +301 @@
     >>> formset = ArticleFormSet()
     >>> formset = ArticleFormSet(data=None)
 
-
-
 .. _deprecated-features-1.3:
 
 Features deprecated in 1.3
@@ -424 +422 @@
 To ensure compatibility with future versions of Django, existing
 templates should be modified to use the new ``future`` libraries and
 syntax.
+
+Changes in 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()``. Django 1.3 
+starts a transistion to stricter semantics, where 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`` in a
+managed transaction.
+
+On the other hand, the new semantics requires you to explicitly close
+read-only transactions under ``commit_manually``, and implies a different
+return value for ``django.db.transaction.is_dirty()`` in some 
+circumstances. These cases (read-only transactions left pending, and
+calls to ``is_dirty()`` where the new semantics is different) issue a
+``ReadonlyTransactionWarning`` in Django 1.3 and 1.4; in Django 1.5,
+``is_dirty()`` will return the new value, and transctions left pending
+will raise a ``TransactionManagementError``.
+