Deprecated transaction.commit/rollback_unless_managed.

Since "unless managed" now means "if database-level autocommit",
committing or rolling back doesn't have any effect.

Restored transactional integrity in a few places that relied on
automatically-started transactions with a transitory API.

django/contrib/gis/utils/layermapping.py

@@ -555,10 +555,6 @@ class LayerMapping(object):
                     except SystemExit:
                         raise
                     except Exception as msg:
-                        if self.transaction_mode == 'autocommit':
-                            # Rolling back the transaction so that other model saves
-                            # will work.
-                            transaction.rollback_unless_managed()
                         if strict:
                             # Bailing out if the `strict` keyword is set.
                             if not silent:

django/contrib/sessions/backends/db.py

@@ -74,7 +74,6 @@ class SessionStore(SessionBase):
     @classmethod
     def clear_expired(cls):
         Session.objects.filter(expire_date__lt=timezone.now()).delete()
-        transaction.commit_unless_managed()
 
 
 # At bottom to avoid circular import

django/core/cache/backends/db.py

@@ -10,7 +10,7 @@ except ImportError:
 
 from django.conf import settings
 from django.core.cache.backends.base import BaseCache
-from django.db import connections, router, transaction, DatabaseError
+from django.db import connections, router, DatabaseError
 from django.utils import timezone, six
 from django.utils.encoding import force_bytes
 
@@ -70,7 +70,6 @@ class DatabaseCache(BaseDatabaseCache):
             cursor = connections[db].cursor()
             cursor.execute("DELETE FROM %s "
                            "WHERE cache_key = %%s" % table, [key])
-            transaction.commit_unless_managed(using=db)
             return default
         value = connections[db].ops.process_clob(row[1])
         return pickle.loads(base64.b64decode(force_bytes(value)))
@@ -124,10 +123,8 @@ class DatabaseCache(BaseDatabaseCache):
                                [key, b64encoded, connections[db].ops.value_to_db_datetime(exp)])
         except DatabaseError:
             # To be threadsafe, updates/inserts are allowed to fail silently
-            transaction.rollback_unless_managed(using=db)
             return False
         else:
-            transaction.commit_unless_managed(using=db)
             return True
 
     def delete(self, key, version=None):
@@ -139,7 +136,6 @@ class DatabaseCache(BaseDatabaseCache):
         cursor = connections[db].cursor()
 
         cursor.execute("DELETE FROM %s WHERE cache_key = %%s" % table, [key])
-        transaction.commit_unless_managed(using=db)
 
     def has_key(self, key, version=None):
         key = self.make_key(key, version=version)
@@ -184,7 +180,6 @@ class DatabaseCache(BaseDatabaseCache):
         table = connections[db].ops.quote_name(self._table)
         cursor = connections[db].cursor()
         cursor.execute('DELETE FROM %s' % table)
-        transaction.commit_unless_managed(using=db)
 
 # For backwards compatibility
 class CacheClass(DatabaseCache):

django/core/management/commands/createcachetable.py

@@ -53,14 +53,13 @@ class Command(LabelCommand):
         for i, line in enumerate(table_output):
             full_statement.append('    %s%s' % (line, i < len(table_output)-1 and ',' or ''))
         full_statement.append(');')
-        curs = connection.cursor()
-        try:
-            curs.execute("\n".join(full_statement))
-        except DatabaseError as e:
-            transaction.rollback_unless_managed(using=db)
-            raise CommandError(
-                "Cache table '%s' could not be created.\nThe error was: %s." %
-                    (tablename, force_text(e)))
-        for statement in index_output:
-            curs.execute(statement)
-        transaction.commit_unless_managed(using=db)
+        with transaction.commit_on_success_unless_managed():
+            curs = connection.cursor()
+            try:
+                curs.execute("\n".join(full_statement))
+            except DatabaseError as e:
+                raise CommandError(
+                    "Cache table '%s' could not be created.\nThe error was: %s." %
+                        (tablename, force_text(e)))
+            for statement in index_output:
+                curs.execute(statement)

django/core/management/commands/flush.py

@@ -57,18 +57,17 @@ Are you sure you want to do this?
 
         if confirm == 'yes':
             try:
-                cursor = connection.cursor()
-                for sql in sql_list:
-                    cursor.execute(sql)
+                with transaction.commit_on_success_unless_managed():
+                    cursor = connection.cursor()
+                    for sql in sql_list:
+                        cursor.execute(sql)
             except Exception as e:
-                transaction.rollback_unless_managed(using=db)
                 raise CommandError("""Database %s couldn't be flushed. Possible reasons:
   * The database isn't running or isn't configured correctly.
   * At least one of the expected database tables doesn't exist.
   * The SQL was invalid.
 Hint: Look at the output of 'django-admin.py sqlflush'. That's the SQL this command wasn't able to run.
 The full error: %s""" % (connection.settings_dict['NAME'], e))
-            transaction.commit_unless_managed(using=db)
 
             # Emit the post sync signal. This allows individual
             # applications to respond as if the database had been

django/core/management/commands/loaddata.py

@@ -73,7 +73,6 @@ class Command(BaseCommand):
         # Start transaction management. All fixtures are installed in a
         # single transaction to ensure that all references are resolved.
         if commit:
-            transaction.commit_unless_managed(using=self.using)
             transaction.enter_transaction_management(using=self.using)
 
         class SingleZipReader(zipfile.ZipFile):

django/core/management/commands/syncdb.py

@@ -83,26 +83,25 @@ class Command(NoArgsCommand):
         # Create the tables for each model
         if verbosity >= 1:
             self.stdout.write("Creating tables ...\n")
-        for app_name, model_list in manifest.items():
-            for model in model_list:
-                # Create the model's database table, if it doesn't already exist.
-                if verbosity >= 3:
-                    self.stdout.write("Processing %s.%s model\n" % (app_name, model._meta.object_name))
-                sql, references = connection.creation.sql_create_model(model, self.style, seen_models)
-                seen_models.add(model)
-                created_models.add(model)
-                for refto, refs in references.items():
-                    pending_references.setdefault(refto, []).extend(refs)
-                    if refto in seen_models:
-                        sql.extend(connection.creation.sql_for_pending_references(refto, self.style, pending_references))
-                sql.extend(connection.creation.sql_for_pending_references(model, self.style, pending_references))
-                if verbosity >= 1 and sql:
-                    self.stdout.write("Creating table %s\n" % model._meta.db_table)
-                for statement in sql:
-                    cursor.execute(statement)
-                tables.append(connection.introspection.table_name_converter(model._meta.db_table))
-
-        transaction.commit_unless_managed(using=db)
+        with transaction.commit_on_success_unless_managed(using=db):
+            for app_name, model_list in manifest.items():
+                for model in model_list:
+                    # Create the model's database table, if it doesn't already exist.
+                    if verbosity >= 3:
+                        self.stdout.write("Processing %s.%s model\n" % (app_name, model._meta.object_name))
+                    sql, references = connection.creation.sql_create_model(model, self.style, seen_models)
+                    seen_models.add(model)
+                    created_models.add(model)
+                    for refto, refs in references.items():
+                        pending_references.setdefault(refto, []).extend(refs)
+                        if refto in seen_models:
+                            sql.extend(connection.creation.sql_for_pending_references(refto, self.style, pending_references))
+                    sql.extend(connection.creation.sql_for_pending_references(model, self.style, pending_references))
+                    if verbosity >= 1 and sql:
+                        self.stdout.write("Creating table %s\n" % model._meta.db_table)
+                    for statement in sql:
+                        cursor.execute(statement)
+                    tables.append(connection.introspection.table_name_converter(model._meta.db_table))
 
         # Send the post_syncdb signal, so individual apps can do whatever they need
         # to do at this point.
@@ -122,17 +121,16 @@ class Command(NoArgsCommand):
                     if custom_sql:
                         if verbosity >= 2:
                             self.stdout.write("Installing custom SQL for %s.%s model\n" % (app_name, model._meta.object_name))
-                        try:
-                            for sql in custom_sql:
-                                cursor.execute(sql)
-                        except Exception as e:
-                            self.stderr.write("Failed to install custom SQL for %s.%s model: %s\n" % \
-                                                (app_name, model._meta.object_name, e))
-                            if show_traceback:
-                                traceback.print_exc()
-                            transaction.rollback_unless_managed(using=db)
-                        else:
-                            transaction.commit_unless_managed(using=db)
+                        with transaction.commit_on_success_unless_managed(using=db):
+                            try:
+                                for sql in custom_sql:
+                                    cursor.execute(sql)
+                            except Exception as e:
+                                self.stderr.write("Failed to install custom SQL for %s.%s model: %s\n" % \
+                                                    (app_name, model._meta.object_name, e))
+                                if show_traceback:
+                                    traceback.print_exc()
+                                raise
                     else:
                         if verbosity >= 3:
                             self.stdout.write("No custom SQL for %s.%s model\n" % (app_name, model._meta.object_name))
@@ -147,15 +145,14 @@ class Command(NoArgsCommand):
                     if index_sql:
                         if verbosity >= 2:
                             self.stdout.write("Installing index for %s.%s model\n" % (app_name, model._meta.object_name))
-                        try:
-                            for sql in index_sql:
-                                cursor.execute(sql)
-                        except Exception as e:
-                            self.stderr.write("Failed to install index for %s.%s model: %s\n" % \
-                                                (app_name, model._meta.object_name, e))
-                            transaction.rollback_unless_managed(using=db)
-                        else:
-                            transaction.commit_unless_managed(using=db)
+                        with transaction.commit_on_success_unless_managed(using=db):
+                            try:
+                                for sql in index_sql:
+                                    cursor.execute(sql)
+                            except Exception as e:
+                                self.stderr.write("Failed to install index for %s.%s model: %s\n" % \
+                                                    (app_name, model._meta.object_name, e))
+                                raise
 
         # Load initial_data fixtures (unless that has been disabled)
         if load_initial_data:

django/db/__init__.py

@@ -77,14 +77,3 @@ def close_old_connections(**kwargs):
         conn.close_if_unusable_or_obsolete()
 signals.request_started.connect(close_old_connections)
 signals.request_finished.connect(close_old_connections)
-
-# Register an event that rolls back the connections
-# when a Django request has an exception.
-def _rollback_on_exception(**kwargs):
-    from django.db import transaction
-    for conn in connections:
-        try:
-            transaction.rollback_unless_managed(using=conn)
-        except DatabaseError:
-            pass
-signals.got_request_exception.connect(_rollback_on_exception)

django/db/backends/__init__.py

@@ -339,27 +339,6 @@ class BaseDatabaseWrapper(object):
             return self.transaction_state[-1]
         return settings.TRANSACTIONS_MANAGED
 
-    def commit_unless_managed(self):
-        """
-        Commits changes if the system is not in managed transaction mode.
-        """
-        self.validate_thread_sharing()
-        if not self.is_managed():
-            self.commit()
-            self.clean_savepoints()
-        else:
-            self.set_dirty()
-
-    def rollback_unless_managed(self):
-        """
-        Rolls back changes if the system is not in managed transaction mode.
-        """
-        self.validate_thread_sharing()
-        if not self.is_managed():
-            self.rollback()
-        else:
-            self.set_dirty()
-
     ##### Foreign key constraints checks handling #####
 
     @contextmanager

django/db/backends/dummy/base.py

@@ -58,8 +58,6 @@ class DatabaseWrapper(BaseDatabaseWrapper):
     _set_autocommit = complain
     set_dirty = complain
     set_clean = complain
-    commit_unless_managed = complain
-    rollback_unless_managed = ignore
 
     def __init__(self, *args, **kwargs):
         super(DatabaseWrapper, self).__init__(*args, **kwargs)

django/db/models/base.py

@@ -609,48 +609,48 @@ class Model(six.with_metaclass(ModelBase)):
             if update_fields:
                 non_pks = [f for f in non_pks if f.name in update_fields or f.attname in update_fields]
 
-            # First, try an UPDATE. If that doesn't update anything, do an INSERT.
-            pk_val = self._get_pk_val(meta)
-            pk_set = pk_val is not None
-            record_exists = True
-            manager = cls._base_manager
-            if pk_set:
-                # Determine if we should do an update (pk already exists, forced update,
-                # no force_insert)
-                if ((force_update or update_fields) or (not force_insert and
-                        manager.using(using).filter(pk=pk_val).exists())):
-                    if force_update or non_pks:
-                        values = [(f, None, (raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks]
-                        if values:
-                            rows = manager.using(using).filter(pk=pk_val)._update(values)
-                            if force_update and not rows:
-                                raise DatabaseError("Forced update did not affect any rows.")
-                            if update_fields and not rows:
-                                raise DatabaseError("Save with update_fields did not affect any rows.")
-                else:
+            with transaction.commit_on_success_unless_managed(using=using):
+                # First, try an UPDATE. If that doesn't update anything, do an INSERT.
+                pk_val = self._get_pk_val(meta)
+                pk_set = pk_val is not None
+                record_exists = True
+                manager = cls._base_manager
+                if pk_set:
+                    # Determine if we should do an update (pk already exists, forced update,
+                    # no force_insert)
+                    if ((force_update or update_fields) or (not force_insert and
+                            manager.using(using).filter(pk=pk_val).exists())):
+                        if force_update or non_pks:
+                            values = [(f, None, (raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks]
+                            if values:
+                                rows = manager.using(using).filter(pk=pk_val)._update(values)
+                                if force_update and not rows:
+                                    raise DatabaseError("Forced update did not affect any rows.")
+                                if update_fields and not rows:
+                                    raise DatabaseError("Save with update_fields did not affect any rows.")
+                    else:
+                        record_exists = False
+                if not pk_set or not record_exists:
+                    if meta.order_with_respect_to:
+                        # If this is a model with an order_with_respect_to
+                        # autopopulate the _order field
+                        field = meta.order_with_respect_to
+                        order_value = manager.using(using).filter(**{field.name: getattr(self, field.attname)}).count()
+                        self._order = order_value
+
+                    fields = meta.local_fields
+                    if not pk_set:
+                        if force_update or update_fields:
+                            raise ValueError("Cannot force an update in save() with no primary key.")
+                        fields = [f for f in fields if not isinstance(f, AutoField)]
+
                     record_exists = False
-            if not pk_set or not record_exists:
-                if meta.order_with_respect_to:
-                    # If this is a model with an order_with_respect_to
-                    # autopopulate the _order field
-                    field = meta.order_with_respect_to
-                    order_value = manager.using(using).filter(**{field.name: getattr(self, field.attname)}).count()
-                    self._order = order_value
 
-                fields = meta.local_fields
-                if not pk_set:
-                    if force_update or update_fields:
-                        raise ValueError("Cannot force an update in save() with no primary key.")
-                    fields = [f for f in fields if not isinstance(f, AutoField)]
+                    update_pk = bool(meta.has_auto_field and not pk_set)
+                    result = manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw)
 
-                record_exists = False
-
-                update_pk = bool(meta.has_auto_field and not pk_set)
-                result = manager._insert([self], fields=fields, return_id=update_pk, using=using, raw=raw)
-
-                if update_pk:
-                    setattr(self, meta.pk.attname, result)
-            transaction.commit_unless_managed(using=using)
+                    if update_pk:
+                        setattr(self, meta.pk.attname, result)
 
         # Store the database on which the object was saved
         self._state.db = using
@@ -963,9 +963,9 @@ def method_set_order(ordered_obj, self, id_list, using=None):
     order_name = ordered_obj._meta.order_with_respect_to.name
     # FIXME: It would be nice if there was an "update many" version of update
     # for situations like this.
-    for i, j in enumerate(id_list):
-        ordered_obj.objects.filter(**{'pk': j, order_name: rel_val}).update(_order=i)
-    transaction.commit_unless_managed(using=using)
+    with transaction.commit_on_success_unless_managed(using=using):
+        for i, j in enumerate(id_list):
+            ordered_obj.objects.filter(**{'pk': j, order_name: rel_val}).update(_order=i)
 
 
 def method_get_order(ordered_obj, self):

django/db/models/deletion.py

@@ -62,8 +62,6 @@ def force_managed(func):
             func(self, *args, **kwargs)
             if forced_managed:
                 transaction.commit(using=self.using)
-            else:
-                transaction.commit_unless_managed(using=self.using)
         finally:
             if forced_managed:
                 transaction.leave_transaction_management(using=self.using)

django/db/models/query.py

@@ -460,8 +460,6 @@ class QuerySet(object):
                     self._batched_insert(objs_without_pk, fields, batch_size)
             if forced_managed:
                 transaction.commit(using=self.db)
-            else:
-                transaction.commit_unless_managed(using=self.db)
         finally:
             if forced_managed:
                 transaction.leave_transaction_management(using=self.db)
@@ -590,8 +588,6 @@ class QuerySet(object):
             rows = query.get_compiler(self.db).execute_sql(None)
             if forced_managed:
                 transaction.commit(using=self.db)
-            else:
-                transaction.commit_unless_managed(using=self.db)
         finally:
             if forced_managed:
                 transaction.leave_transaction_management(using=self.db)

django/db/transaction.py

@@ -123,16 +123,12 @@ def managed(flag=True, using=None):
         PendingDeprecationWarning, stacklevel=2)
 
 def commit_unless_managed(using=None):
-    """
-    Commits changes if the system is not in managed transaction mode.
-    """
-    get_connection(using).commit_unless_managed()
+    warnings.warn("'commit_unless_managed' is now a no-op.",
+        PendingDeprecationWarning, stacklevel=2)
 
 def rollback_unless_managed(using=None):
-    """
-    Rolls back changes if the system is not in managed transaction mode.
-    """
-    get_connection(using).rollback_unless_managed()
+    warnings.warn("'rollback_unless_managed' is now a no-op.",
+        PendingDeprecationWarning, stacklevel=2)
 
 ###############
 # Public APIs #
@@ -280,3 +276,18 @@ def commit_manually(using=None):
         leave_transaction_management(using=using)
 
     return _transaction_func(entering, exiting, using)
+
+def commit_on_success_unless_managed(using=None):
+    """
+    Transitory API to preserve backwards-compatibility while refactoring.
+    """
+    if is_managed(using):
+        def entering(using):
+            pass
+
+        def exiting(exc_value, using):
+            set_dirty(using=using)
+
+        return _transaction_func(entering, exiting, using)
+    else:
+        return commit_on_success(using)

django/test/testcases.py

@@ -157,14 +157,6 @@ class DocTestRunner(doctest.DocTestRunner):
         doctest.DocTestRunner.__init__(self, *args, **kwargs)
         self.optionflags = doctest.ELLIPSIS
 
-    def report_unexpected_exception(self, out, test, example, exc_info):
-        doctest.DocTestRunner.report_unexpected_exception(self, out, test,
-                                                          example, exc_info)
-        # Rollback, in case of database errors. Otherwise they'd have
-        # side effects on other tests.
-        for conn in connections:
-            transaction.rollback_unless_managed(using=conn)
-
 
 class _AssertNumQueriesContext(CaptureQueriesContext):
     def __init__(self, test_case, num, connection):
@@ -490,14 +482,10 @@ class TransactionTestCase(SimpleTestCase):
                 conn.ops.sequence_reset_by_name_sql(no_style(),
                                                     conn.introspection.sequence_list())
             if sql_list:
-                try:
+                with transaction.commit_on_success_unless_managed(using=db_name):
                     cursor = conn.cursor()
                     for sql in sql_list:
                         cursor.execute(sql)
-                except Exception:
-                    transaction.rollback_unless_managed(using=db_name)
-                    raise
-                transaction.commit_unless_managed(using=db_name)
 
     def _fixture_setup(self):
         for db_name in self._databases_names(include_mirrors=False):
@@ -537,11 +525,6 @@ class TransactionTestCase(SimpleTestCase):
             conn.close()
 
     def _fixture_teardown(self):
-        # Roll back any pending transactions in order to avoid a deadlock
-        # during flush when TEST_MIRROR is used (#18984).
-        for conn in connections.all():
-            conn.rollback_unless_managed()
-
         for db in self._databases_names(include_mirrors=False):
             call_command('flush', verbosity=0, interactive=False, database=db,
                          skip_validation=True, reset_sequences=False)

docs/internals/deprecation.txt

@@ -352,6 +352,8 @@ these changes.
   - ``django.db.close_connection()``
   - ``django.db.backends.creation.BaseDatabaseCreation.set_autocommit()``
   - ``django.db.transaction.managed()``
+  - ``django.db.transaction.commit_unless_managed()``
+  - ``django.db.transaction.rollback_unless_managed()``
 
 2.0
 ---

tests/transactions_regress/tests.py

@@ -208,38 +208,6 @@ class TestNewConnection(TransactionTestCase):
         connection.leave_transaction_management()
         self.assertEqual(orig_dirty, connection._dirty)
 
-    # TODO: update this test to account for database-level autocommit.
-    @expectedFailure
-    def test_commit_unless_managed(self):
-        cursor = connection.cursor()
-        cursor.execute("INSERT into transactions_regress_mod (fld) values (2)")
-        connection.commit_unless_managed()
-        self.assertFalse(connection.is_dirty())
-        self.assertEqual(len(Mod.objects.all()), 1)
-        self.assertTrue(connection.is_dirty())
-        connection.commit_unless_managed()
-        self.assertFalse(connection.is_dirty())
-
-    # TODO: update this test to account for database-level autocommit.
-    @expectedFailure
-    def test_commit_unless_managed_in_managed(self):
-        cursor = connection.cursor()
-        connection.enter_transaction_management()
-        cursor.execute("INSERT into transactions_regress_mod (fld) values (2)")
-        connection.commit_unless_managed()
-        self.assertTrue(connection.is_dirty())
-        connection.rollback()
-        self.assertFalse(connection.is_dirty())
-        self.assertEqual(len(Mod.objects.all()), 0)
-        connection.commit()
-        connection.leave_transaction_management()
-        self.assertFalse(connection.is_dirty())
-        self.assertEqual(len(Mod.objects.all()), 0)
-        self.assertTrue(connection.is_dirty())
-        connection.commit_unless_managed()
-        self.assertFalse(connection.is_dirty())
-        self.assertEqual(len(Mod.objects.all()), 0)
-
 
 @skipUnless(connection.vendor == 'postgresql',
             "This test only valid for PostgreSQL")