From ba5138b1c0253fcf390b7509ad7b954117b3be88 Mon Sep 17 00:00:00 2001 From: Aymeric Augustin Date: Mon, 4 Mar 2013 13:12:59 +0100 Subject: [PATCH] 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 | 4 - django/contrib/sessions/backends/db.py | 1 - django/core/cache/backends/db.py | 7 +- .../management/commands/createcachetable.py | 21 +++-- django/core/management/commands/flush.py | 9 +- django/core/management/commands/loaddata.py | 1 - django/core/management/commands/syncdb.py | 77 ++++++++--------- django/db/__init__.py | 11 --- django/db/backends/__init__.py | 21 ----- django/db/backends/dummy/base.py | 2 - django/db/models/base.py | 84 +++++++++---------- django/db/models/deletion.py | 2 - django/db/models/query.py | 4 - django/db/transaction.py | 27 ++++-- django/test/testcases.py | 19 +---- docs/internals/deprecation.txt | 2 + tests/transactions_regress/tests.py | 32 ------- 17 files changed, 116 insertions(+), 208 deletions(-) diff --git a/django/contrib/gis/utils/layermapping.py b/django/contrib/gis/utils/layermapping.py index e4ea44d0d2..51f70f5350 100644 --- a/django/contrib/gis/utils/layermapping.py +++ b/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: diff --git a/django/contrib/sessions/backends/db.py b/django/contrib/sessions/backends/db.py index 47e89b66e5..30da0b7a10 100644 --- a/django/contrib/sessions/backends/db.py +++ b/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 diff --git a/django/core/cache/backends/db.py b/django/core/cache/backends/db.py index bb91d8cb05..53d7f4d22a 100644 --- a/django/core/cache/backends/db.py +++ b/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): diff --git a/django/core/management/commands/createcachetable.py b/django/core/management/commands/createcachetable.py index 411042ee76..94b6b09400 100644 --- a/django/core/management/commands/createcachetable.py +++ b/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) diff --git a/django/core/management/commands/flush.py b/django/core/management/commands/flush.py index 3bf1e9c672..9bd65e735c 100644 --- a/django/core/management/commands/flush.py +++ b/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 diff --git a/django/core/management/commands/loaddata.py b/django/core/management/commands/loaddata.py index 77b9a44a43..674e6be7b0 100644 --- a/django/core/management/commands/loaddata.py +++ b/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): diff --git a/django/core/management/commands/syncdb.py b/django/core/management/commands/syncdb.py index 4ce2910fb5..e7e11a8c90 100644 --- a/django/core/management/commands/syncdb.py +++ b/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: diff --git a/django/db/__init__.py b/django/db/__init__.py index 60fe8f6ce2..13ba68ba7e 100644 --- a/django/db/__init__.py +++ b/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) diff --git a/django/db/backends/__init__.py b/django/db/backends/__init__.py index 4031e8f668..848f6df2d6 100644 --- a/django/db/backends/__init__.py +++ b/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 diff --git a/django/db/backends/dummy/base.py b/django/db/backends/dummy/base.py index 02f0b6462d..9a220ffd8b 100644 --- a/django/db/backends/dummy/base.py +++ b/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) diff --git a/django/db/models/base.py b/django/db/models/base.py index 543cdfc165..ab0e42d461 100644 --- a/django/db/models/base.py +++ b/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): diff --git a/django/db/models/deletion.py b/django/db/models/deletion.py index 93ef0006cb..26f63391d5 100644 --- a/django/db/models/deletion.py +++ b/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) diff --git a/django/db/models/query.py b/django/db/models/query.py index b41007ee4f..22f71c6aee 100644 --- a/django/db/models/query.py +++ b/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) diff --git a/django/db/transaction.py b/django/db/transaction.py index dd48e14bf4..a8e80c6c02 100644 --- a/django/db/transaction.py +++ b/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) diff --git a/django/test/testcases.py b/django/test/testcases.py index 4b9116e3bc..55673dca25 100644 --- a/django/test/testcases.py +++ b/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) diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 74fbb563f0..a81b16278f 100644 --- a/docs/internals/deprecation.txt +++ b/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 --- diff --git a/tests/transactions_regress/tests.py b/tests/transactions_regress/tests.py index e6750cddcf..3d571fba2f 100644 --- a/tests/transactions_regress/tests.py +++ b/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")