Made atomic usable when autocommit is off.

Thanks Anssi for haggling until I implemented this.

This change alleviates the need for atomic_if_autocommit. When
autocommit is disabled for a database, atomic will simply create and
release savepoints, and not commit anything. This honors the contract of
not doing any transaction management.

This change also makes the hack to allow using atomic within the legacy
transaction management redundant.

None of the above will work with SQLite, because of a flaw in the design
of the sqlite3 library. This is a known limitation that cannot be lifted
without unacceptable side effects eg. triggering arbitrary commits.

django/core/cache/backends/db.py

@@ -109,7 +109,7 @@ class DatabaseCache(BaseDatabaseCache):
         if six.PY3:
             b64encoded = b64encoded.decode('latin1')
         try:
-            with transaction.atomic_if_autocommit(using=db):
+            with transaction.atomic(using=db):
                 cursor.execute("SELECT cache_key, expires FROM %s "
                                "WHERE cache_key = %%s" % table, [key])
                 result = cursor.fetchone()

django/db/backends/__init__.py

@@ -50,15 +50,16 @@ class BaseDatabaseWrapper(object):
         # set somewhat aggressively, as the DBAPI doesn't make it easy to
         # deduce if the connection is in transaction or not.
         self._dirty = False
-        # Tracks if the connection is in a transaction managed by 'atomic'
+        # Tracks if the connection is in a transaction managed by 'atomic'.
         self.in_atomic_block = False
+        # Tracks if the outermost 'atomic' block should commit on exit,
+        # ie. if autocommit was active on entry.
+        self.commit_on_exit = True
         # Tracks if the transaction should be rolled back to the next
         # available savepoint because of an exception in an inner block.
         self.needs_rollback = False
         # List of savepoints created by 'atomic'
         self.savepoint_ids = []
-        # Hack to provide compatibility with legacy transaction management
-        self._atomic_forced_unmanaged = False
 
         # Connection termination related attributes
         self.close_at = None

django/db/transaction.py

@@ -206,18 +206,6 @@ class Atomic(object):
         self.using = using
         self.savepoint = savepoint
 
-    def _legacy_enter_transaction_management(self, connection):
-        if not connection.in_atomic_block:
-            if connection.transaction_state and connection.transaction_state[-1]:
-                connection._atomic_forced_unmanaged = True
-                connection.enter_transaction_management(managed=False)
-            else:
-                connection._atomic_forced_unmanaged = False
-
-    def _legacy_leave_transaction_management(self, connection):
-        if not connection.in_atomic_block and connection._atomic_forced_unmanaged:
-            connection.leave_transaction_management()
-
     def __enter__(self):
         connection = get_connection(self.using)
 
@@ -225,12 +213,31 @@ class Atomic(object):
         # autocommit status.
         connection.ensure_connection()
 
-        # Remove this when the legacy transaction management goes away.
-        self._legacy_enter_transaction_management(connection)
-
-        if not connection.in_atomic_block and not connection.autocommit:
-            raise TransactionManagementError(
-                "'atomic' cannot be used when autocommit is disabled.")
+        if not connection.in_atomic_block:
+            # Reset state when entering an outermost atomic block.
+            connection.commit_on_exit = True
+            connection.needs_rollback = False
+            if not connection.autocommit:
+                # Some database adapters (namely sqlite3) don't handle
+                # transactions and savepoints properly when autocommit is off.
+                # Turning autocommit back on isn't an option; it would trigger
+                # a premature commit. Give up if that happens.
+                if connection.features.autocommits_when_autocommit_is_off:
+                    raise TransactionManagementError(
+                        "Your database backend doesn't behave properly when "
+                        "autocommit is off. Turn it on before using 'atomic'.")
+                # When entering an atomic block with autocommit turned off,
+                # Django should only use savepoints and shouldn't commit.
+                # This requires at least a savepoint for the outermost block.
+                if not self.savepoint:
+                    raise TransactionManagementError(
+                        "The outermost 'atomic' block cannot use "
+                        "savepoint = False when autocommit is off.")
+                # Pretend we're already in an atomic block to bypass the code
+                # that disables autocommit to enter a transaction, and make a
+                # note to deal with this case in __exit__.
+                connection.in_atomic_block = True
+                connection.commit_on_exit = False
 
         if connection.in_atomic_block:
             # We're already in a transaction; create a savepoint, unless we
@@ -255,63 +262,58 @@ class Atomic(object):
             else:
                 connection.set_autocommit(False)
             connection.in_atomic_block = True
-            connection.needs_rollback = False
 
     def __exit__(self, exc_type, exc_value, traceback):
         connection = get_connection(self.using)
-        if exc_value is None and not connection.needs_rollback:
-            if connection.savepoint_ids:
-                # Release savepoint if there is one
-                sid = connection.savepoint_ids.pop()
-                if sid is not None:
+
+        if connection.savepoint_ids:
+            sid = connection.savepoint_ids.pop()
+        else:
+            # Prematurely unset this flag to allow using commit or rollback.
+            connection.in_atomic_block = False
+
+        try:
+            if exc_value is None and not connection.needs_rollback:
+                if connection.in_atomic_block:
+                    # Release savepoint if there is one
+                    if sid is not None:
+                        try:
+                            connection.savepoint_commit(sid)
+                        except DatabaseError:
+                            connection.savepoint_rollback(sid)
+                            raise
+                else:
+                    # Commit transaction
                     try:
-                        connection.savepoint_commit(sid)
+                        connection.commit()
                     except DatabaseError:
-                        connection.savepoint_rollback(sid)
-                        # Remove this when the legacy transaction management goes away.
-                        self._legacy_leave_transaction_management(connection)
+                        connection.rollback()
                         raise
             else:
-                # Commit transaction
-                connection.in_atomic_block = False
-                try:
-                    connection.commit()
-                except DatabaseError:
-                    connection.rollback()
-                    # Remove this when the legacy transaction management goes away.
-                    self._legacy_leave_transaction_management(connection)
-                    raise
-                finally:
-                    if connection.features.autocommits_when_autocommit_is_off:
-                        connection.autocommit = True
+                # This flag will be set to True again if there isn't a savepoint
+                # allowing to perform the rollback at this level.
+                connection.needs_rollback = False
+                if connection.in_atomic_block:
+                    # Roll back to savepoint if there is one, mark for rollback
+                    # otherwise.
+                    if sid is None:
+                        connection.needs_rollback = True
                     else:
-                        connection.set_autocommit(True)
-        else:
-            # This flag will be set to True again if there isn't a savepoint
-            # allowing to perform the rollback at this level.
-            connection.needs_rollback = False
-            if connection.savepoint_ids:
-                # Roll back to savepoint if there is one, mark for rollback
-                # otherwise.
-                sid = connection.savepoint_ids.pop()
-                if sid is None:
-                    connection.needs_rollback = True
+                        connection.savepoint_rollback(sid)
                 else:
-                    connection.savepoint_rollback(sid)
-            else:
-                # Roll back transaction
-                connection.in_atomic_block = False
-                try:
+                    # Roll back transaction
                     connection.rollback()
-                finally:
-                    if connection.features.autocommits_when_autocommit_is_off:
-                        connection.autocommit = True
-                    else:
-                        connection.set_autocommit(True)
-
-        # Remove this when the legacy transaction management goes away.
-        self._legacy_leave_transaction_management(connection)
 
+        finally:
+            # Outermost block exit when autocommit was enabled.
+            if not connection.in_atomic_block:
+                if connection.features.autocommits_when_autocommit_is_off:
+                    connection.autocommit = True
+                else:
+                    connection.set_autocommit(True)
+            # Outermost block exit when autocommit was disabled.
+            elif not connection.savepoint_ids and not connection.commit_on_exit:
+                connection.in_atomic_block = False
 
     def __call__(self, func):
         @wraps(func, assigned=available_attrs(func))
@@ -331,24 +333,6 @@ def atomic(using=None, savepoint=True):
         return Atomic(using, savepoint)
 
 
-def atomic_if_autocommit(using=None, savepoint=True):
-    # This variant only exists to support the ability to disable transaction
-    # management entirely in the DATABASES setting. It doesn't care about the
-    # autocommit state at run time.
-    db = DEFAULT_DB_ALIAS if callable(using) else using
-    autocommit = get_connection(db).settings_dict['AUTOCOMMIT']
-
-    if autocommit:
-        return atomic(using, savepoint)
-    else:
-        # Bare decorator: @atomic_if_autocommit
-        if callable(using):
-            return using
-        # Decorator: @atomic_if_autocommit(...)
-        else:
-            return lambda func: func
-
-
 ############################################
 # Deprecated decorators / context managers #
 ############################################
@@ -472,16 +456,15 @@ def commit_on_success_unless_managed(using=None, savepoint=False):
     Transitory API to preserve backwards-compatibility while refactoring.
 
     Once the legacy transaction management is fully deprecated, this should
-    simply be replaced by atomic_if_autocommit. Until then, it's necessary to
-    avoid making a commit where Django didn't use to, since entering atomic in
-    managed mode triggers a commmit.
+    simply be replaced by atomic. Until then, it's necessary to guarantee that
+    a commit occurs on exit, which atomic doesn't do when it's nested.
 
     Unlike atomic, savepoint defaults to False because that's closer to the
     legacy behavior.
     """
     connection = get_connection(using)
     if connection.autocommit or connection.in_atomic_block:
-        return atomic_if_autocommit(using, savepoint)
+        return atomic(using, savepoint)
     else:
         def entering(using):
             pass

docs/topics/db/transactions.txt

@@ -153,9 +153,6 @@ Django provides a single API to control database transactions.
     to commit, roll back, or change the autocommit state of the database
     connection within an ``atomic`` block will raise an exception.
 
-    ``atomic`` can only be used in autocommit mode. It will raise an exception
-    if autocommit is turned off.
-
     Under the hood, Django's transaction management code:
 
     - opens a transaction when entering the outermost ``atomic`` block;
@@ -171,6 +168,10 @@ Django provides a single API to control database transactions.
     the overhead of savepoints is noticeable. It has the drawback of breaking
     the error handling described above.
 
+    You may use ``atomic`` when autocommit is turned off. It will only use
+    savepoints, even for the outermost block, and it will raise an exception
+    if the outermost block is declared with ``savepoint=False``.
+
 .. admonition:: Performance considerations
 
     Open transactions have a performance cost for your database server. To
@@ -271,9 +272,8 @@ another. Review the documentation of the adapter you're using carefully.
 You must ensure that no transaction is active, usually by issuing a
 :func:`commit` or a :func:`rollback`, before turning autocommit back on.
 
-:func:`atomic` requires autocommit to be turned on; it will raise an exception
-if autocommit is off. Django will also refuse to turn autocommit off when an
-:func:`atomic` block is active, because that would break atomicity.
+Django will refuse to turn autocommit off when an :func:`atomic` block is
+active, because that would break atomicity.
 
 Transactions
 ------------
@@ -392,8 +392,11 @@ When autocommit is enabled, savepoints don't make sense. When it's disabled,
 commits before any statement other than ``SELECT``, ``INSERT``, ``UPDATE``,
 ``DELETE`` and ``REPLACE``.)
 
-As a consequence, savepoints are only usable inside a transaction ie. inside
-an :func:`atomic` block.
+This has two consequences:
+
+- The low level APIs for savepoints are only usable inside a transaction ie.
+  inside an :func:`atomic` block.
+- It's impossible to use :func:`atomic` when autocommit is turned off.
 
 Transactions in MySQL
 ---------------------
@@ -584,9 +587,6 @@ During the deprecation period, it's possible to use :func:`atomic` within
 However, the reverse is forbidden, because nesting the old decorators /
 context managers breaks atomicity.
 
-If you enter :func:`atomic` while you're in managed mode, it will trigger a
-commit to start from a clean slate.
-
 Managing autocommit
 ~~~~~~~~~~~~~~~~~~~
 
@@ -623,8 +623,8 @@ Disabling transaction management
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
 Instead of setting ``TRANSACTIONS_MANAGED = True``, set the ``AUTOCOMMIT`` key
-to ``False`` in the configuration of each database, as explained in :ref
-:`deactivate-transaction-management`.
+to ``False`` in the configuration of each database, as explained in
+:ref:`deactivate-transaction-management`.
 
 Backwards incompatibilities
 ---------------------------

tests/transactions/tests.py

@@ -6,7 +6,7 @@ import warnings
 from django.db import connection, transaction, IntegrityError
 from django.test import TransactionTestCase, skipUnlessDBFeature
 from django.utils import six
-from django.utils.unittest import skipUnless
+from django.utils.unittest import skipIf, skipUnless
 
 from .models import Reporter
 
@@ -197,6 +197,23 @@ class AtomicInsideTransactionTests(AtomicTests):
         self.atomic.__exit__(*sys.exc_info())
 
 
+@skipIf(connection.features.autocommits_when_autocommit_is_off,
+        "This test requires a non-autocommit mode that doesn't autocommit.")
+class AtomicWithoutAutocommitTests(AtomicTests):
+    """All basic tests for atomic should also pass when autocommit is turned off."""
+
+    def setUp(self):
+        transaction.set_autocommit(False)
+
+    def tearDown(self):
+        # The tests access the database after exercising 'atomic', initiating
+        # a transaction ; a rollback is required before restoring autocommit.
+        transaction.rollback()
+        transaction.set_autocommit(True)
+
+
+@skipIf(connection.features.autocommits_when_autocommit_is_off,
+        "This test requires a non-autocommit mode that doesn't autocommit.")
 class AtomicInsideLegacyTransactionManagementTests(AtomicTests):
 
     def setUp(self):
@@ -268,16 +285,7 @@ class AtomicMergeTests(TransactionTestCase):
         "'atomic' requires transactions and savepoints.")
 class AtomicErrorsTests(TransactionTestCase):
 
-    def test_atomic_requires_autocommit(self):
-        transaction.set_autocommit(False)
-        try:
-            with self.assertRaises(transaction.TransactionManagementError):
-                with transaction.atomic():
-                    pass
-        finally:
-            transaction.set_autocommit(True)
-
-    def test_atomic_prevents_disabling_autocommit(self):
+    def test_atomic_prevents_setting_autocommit(self):
         autocommit = transaction.get_autocommit()
         with transaction.atomic():
             with self.assertRaises(transaction.TransactionManagementError):