Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Forbid queries in broken transactions #1659

Closed
wants to merge 5 commits into from

2 participants

django/db/backends/__init__.py
@@ -638,6 +644,9 @@ class BaseDatabaseFeatures(object):
# when autocommit is disabled? http://bugs.python.org/issue8145#msg109965
autocommits_when_autocommit_is_off = False
+ # Does the backend prevent running SQL queries is broken transactions?
@charettes Collaborator

... in broken transactions?

@aaugustin Owner

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
aaugustin added some commits
@aaugustin aaugustin Prevented running queries in transactions that will be rolled back.
This avoids a counter-intuitive behavior in an edge case on databases
with non-atomic transaction semantics.

It prevents using savepoint_rollback() inside an atomic block without
calling set_rollback(False) first, which is backwards-incompatible in
tests.

Refs #21134.
461ecec
@aaugustin aaugustin Replaced manual savepoints by atomic blocks.
This ensures the rollback flag is handled consistently in internal APIs.
f72fcd9
@aaugustin aaugustin Fixed TransactionManagementError in tests.
Previous commit introduced an additional check to prevent running
queries in transactions that will be rolled back, which triggered a few
failures in the tests. In practice using transaction.atomic instead of
the low-level savepoint APIs was enough to fix the problems.
08359d0
@aaugustin aaugustin Allowed nesting constraint_checks_disabled inside atomic.
Since MySQL handles transactions loosely, this isn't a problem.
13f9828
@aaugustin aaugustin Clarified interactions between atomic and exceptions. 40b69af
@aaugustin
Owner

Squashed and merged in 728548e.

@aaugustin aaugustin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 29, 2013
  1. @aaugustin

    Prevented running queries in transactions that will be rolled back.

    aaugustin authored
    This avoids a counter-intuitive behavior in an edge case on databases
    with non-atomic transaction semantics.
    
    It prevents using savepoint_rollback() inside an atomic block without
    calling set_rollback(False) first, which is backwards-incompatible in
    tests.
    
    Refs #21134.
  2. @aaugustin

    Replaced manual savepoints by atomic blocks.

    aaugustin authored
    This ensures the rollback flag is handled consistently in internal APIs.
  3. @aaugustin

    Fixed TransactionManagementError in tests.

    aaugustin authored
    Previous commit introduced an additional check to prevent running
    queries in transactions that will be rolled back, which triggered a few
    failures in the tests. In practice using transaction.atomic instead of
    the low-level savepoint APIs was enough to fix the problems.
  4. @aaugustin

    Allowed nesting constraint_checks_disabled inside atomic.

    aaugustin authored
    Since MySQL handles transactions loosely, this isn't a problem.
  5. @aaugustin
This page is out of date. Refresh to see the latest.
View
5 django/contrib/sessions/backends/db.py
@@ -58,12 +58,11 @@ def save(self, must_create=False):
expire_date=self.get_expiry_date()
)
using = router.db_for_write(Session, instance=obj)
- sid = transaction.savepoint(using=using)
try:
- obj.save(force_insert=must_create, using=using)
+ with transaction.atomic(using=using):
+ obj.save(force_insert=must_create, using=using)
except IntegrityError:
if must_create:
- transaction.savepoint_rollback(sid, using=using)
raise CreateError
raise
View
9 django/db/backends/__init__.py
@@ -361,6 +361,12 @@ def validate_no_atomic_block(self):
raise TransactionManagementError(
"This is forbidden when an 'atomic' block is active.")
+ def validate_no_broken_transaction(self):
+ if self.needs_rollback:
+ raise TransactionManagementError(
+ "An error occurred in the current transaction. You can't "
+ "execute queries until the end of the 'atomic' block.")
+
def abort(self):
"""
Roll back any ongoing transaction and clean the transaction state
@@ -638,6 +644,9 @@ class BaseDatabaseFeatures(object):
# when autocommit is disabled? http://bugs.python.org/issue8145#msg109965
autocommits_when_autocommit_is_off = False
+ # Does the backend prevent running SQL queries in broken transactions?
+ atomic_transactions = True
+
# Can we roll back DDL in a transaction?
can_rollback_ddl = False
View
9 django/db/backends/mysql/base.py
@@ -172,6 +172,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
requires_explicit_null_ordering_when_grouping = True
allows_primary_key_0 = False
uses_savepoints = True
+ atomic_transactions = False
supports_check_constraints = False
def __init__(self, connection):
@@ -484,7 +485,13 @@ def enable_constraint_checking(self):
"""
Re-enable foreign key checks after they have been disabled.
"""
- self.cursor().execute('SET foreign_key_checks=1')
+ # Override needs_rollback in case constraint_checks_disabled is
+ # nested inside transaction.atomic.
+ self.needs_rollback, needs_rollback = False, self.needs_rollback
+ try:
+ self.cursor().execute('SET foreign_key_checks=1')
+ finally:
+ self.needs_rollback = needs_rollback
def check_constraints(self, table_names=None):
"""
View
1  django/db/backends/oracle/base.py
@@ -96,6 +96,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
has_bulk_insert = True
supports_tablespaces = True
supports_sequence_reset = False
+ atomic_transactions = False
supports_combined_alters = False
max_index_name_length = 30
nulls_order_largest = True
View
1  django/db/backends/sqlite3/base.py
@@ -105,6 +105,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
supports_foreign_keys = False
supports_check_constraints = False
autocommits_when_autocommit_is_off = True
+ atomic_transactions = False
supports_paramstyle_pyformat = False
supports_sequence_reset = False
View
47 django/db/backends/utils.py
@@ -19,14 +19,9 @@ def __init__(self, cursor, db):
self.cursor = cursor
self.db = db
- SET_DIRTY_ATTRS = frozenset(['execute', 'executemany', 'callproc'])
- WRAP_ERROR_ATTRS = frozenset([
- 'callproc', 'close', 'execute', 'executemany',
- 'fetchone', 'fetchmany', 'fetchall', 'nextset'])
+ WRAP_ERROR_ATTRS = frozenset(['fetchone', 'fetchmany', 'fetchall', 'nextset'])
def __getattr__(self, attr):
- if attr in CursorWrapper.SET_DIRTY_ATTRS:
- self.db.set_dirty()
cursor_attr = getattr(self.cursor, attr)
if attr in CursorWrapper.WRAP_ERROR_ATTRS:
return self.db.wrap_database_errors(cursor_attr)
@@ -44,18 +39,42 @@ def __exit__(self, type, value, traceback):
# specific behavior.
self.close()
+ # The following methods cannot be implemented in __getattr__, because the
+ # code must run when the method is invoked, not just when it is accessed.
-class CursorDebugWrapper(CursorWrapper):
+ def callproc(self, procname, params=None):
+ self.db.validate_no_broken_transaction()
+ self.db.set_dirty()
+ with self.db.wrap_database_errors:
+ if params is None:
+ return self.cursor.callproc(procname)
+ else:
+ return self.cursor.callproc(procname, params)
def execute(self, sql, params=None):
+ self.db.validate_no_broken_transaction()
self.db.set_dirty()
+ with self.db.wrap_database_errors:
+ if params is None:
+ return self.cursor.execute(sql)
+ else:
+ return self.cursor.execute(sql, params)
+
+ def executemany(self, sql, param_list):
+ self.db.validate_no_broken_transaction()
+ self.db.set_dirty()
+ with self.db.wrap_database_errors:
+ return self.cursor.executemany(sql, param_list)
+
+
+class CursorDebugWrapper(CursorWrapper):
+
+ # XXX callproc isn't instrumented at this time.
+
+ def execute(self, sql, params=None):
start = time()
try:
- with self.db.wrap_database_errors:
- if params is None:
- # params default might be backend specific
- return self.cursor.execute(sql)
- return self.cursor.execute(sql, params)
+ return super(CursorDebugWrapper, self).execute(sql, params)
finally:
stop = time()
duration = stop - start
@@ -69,11 +88,9 @@ def execute(self, sql, params=None):
)
def executemany(self, sql, param_list):
- self.db.set_dirty()
start = time()
try:
- with self.db.wrap_database_errors:
- return self.cursor.executemany(sql, param_list)
+ return super(CursorDebugWrapper, self).executemany(sql, param_list)
finally:
stop = time()
duration = stop - start
View
26 django/db/models/query.py
@@ -435,14 +435,9 @@ def update_or_create(self, defaults=None, **kwargs):
for k, v in six.iteritems(defaults):
setattr(obj, k, v)
- sid = transaction.savepoint(using=self.db)
- try:
+ with transaction.atomic(using=self.db):
obj.save(using=self.db)
- transaction.savepoint_commit(sid, using=self.db)
- return obj, False
- except DatabaseError:
- transaction.savepoint_rollback(sid, using=self.db)
- six.reraise(*sys.exc_info())
+ return obj, False
def _create_object_from_params(self, lookup, params):
"""
@@ -450,19 +445,16 @@ def _create_object_from_params(self, lookup, params):
Used by get_or_create and update_or_create
"""
obj = self.model(**params)
- sid = transaction.savepoint(using=self.db)
try:
- obj.save(force_insert=True, using=self.db)
- transaction.savepoint_commit(sid, using=self.db)
+ with transaction.atomic(using=self.db):
+ obj.save(force_insert=True, using=self.db)
return obj, True
- except DatabaseError as e:
- transaction.savepoint_rollback(sid, using=self.db)
+ except IntegrityError:
exc_info = sys.exc_info()
- if isinstance(e, IntegrityError):
- try:
- return self.get(**lookup), False
- except self.model.DoesNotExist:
- pass
+ try:
+ return self.get(**lookup), False
+ except self.model.DoesNotExist:
+ pass
six.reraise(*exc_info)
def _extract_model_params(self, defaults, **kwargs):
View
9 django/db/transaction.py
@@ -16,14 +16,15 @@
from functools import wraps
-from django.db import connections, DatabaseError, DEFAULT_DB_ALIAS
+from django.db import (
+ connections, DEFAULT_DB_ALIAS,
+ DatabaseError, ProgrammingError)
from django.utils.decorators import available_attrs
-class TransactionManagementError(Exception):
+class TransactionManagementError(ProgrammingError):
"""
- This exception is thrown when something bad happens with transaction
- management.
+ This exception is thrown when transaction management is used improperly.
"""
pass
View
29 docs/topics/db/transactions.txt
@@ -163,20 +163,31 @@ Django provides a single API to control database transactions.
called, so the exception handler can also operate on the database if
necessary.
- .. admonition:: Don't catch database exceptions inside ``atomic``!
-
- If you catch :exc:`~django.db.DatabaseError` or a subclass such as
- :exc:`~django.db.IntegrityError` inside an ``atomic`` block, you will
- hide from Django the fact that an error has occurred and that the
- transaction is broken. At this point, Django's behavior is unspecified
- and database-dependent. It will usually result in a rollback, which
- may break your expectations, since you caught the exception.
+ .. admonition:: Avoid catching exceptions inside ``atomic``!
+
+ When exiting an ``atomic`` block, Django looks at whether it's exited
+ normally or with an exception to determine whether to commit or roll
+ back. If you catch and handle exceptions inside an ``atomic`` block,
+ you may hide from Django the fact that a problem has happened. This
+ can result in unexpected behavior.
+
+ This is mostly a concern for :exc:`~django.db.DatabaseError` and its
+ subclasses such as :exc:`~django.db.IntegrityError`. After such an
+ error, the transaction is broken and Django will perform a rollback at
+ the end of the ``atomic`` block. If you attempt to run database
+ queries before the rollback happens, Django will raise a
+ :class:`~django.db.transaction.TransactionManagementError`. You may
+ also encounter this behavior when an ORM-related signal handler raises
+ an exception.
The correct way to catch database errors is around an ``atomic`` block
as shown above. If necessary, add an extra ``atomic`` block for this
- purpose -- it's cheap! This pattern is useful to delimit explicitly
+ purpose. This pattern has another advantage: it delimits explicitly
which operations will be rolled back if an exception occurs.
+ If you catch exceptions raised by raw SQL queries, Django's behavior
+ is unspecified and database-dependent.
+
In order to guarantee atomicity, ``atomic`` disables some APIs. Attempting
to commit, roll back, or change the autocommit state of the database
connection within an ``atomic`` block will raise an exception.
View
16 tests/custom_pk/tests.py
@@ -149,11 +149,9 @@ def test_unique_pk(self):
Employee.objects.create(
employee_code=123, first_name="Frank", last_name="Jones"
)
- sid = transaction.savepoint()
- self.assertRaises(IntegrityError,
- Employee.objects.create, employee_code=123, first_name="Fred", last_name="Jones"
- )
- transaction.savepoint_rollback(sid)
+ with self.assertRaises(IntegrityError):
+ with transaction.atomic():
+ Employee.objects.create(employee_code=123, first_name="Fred", last_name="Jones")
def test_custom_field_pk(self):
# Regression for #10785 -- Custom fields can be used for primary keys.
@@ -175,8 +173,6 @@ def test_custom_field_pk(self):
def test_required_pk(self):
# The primary key must be specified, so an error is raised if you
# try to create an object without it.
- sid = transaction.savepoint()
- self.assertRaises(IntegrityError,
- Employee.objects.create, first_name="Tom", last_name="Smith"
- )
- transaction.savepoint_rollback(sid)
+ with self.assertRaises(IntegrityError):
+ with transaction.atomic():
+ Employee.objects.create(first_name="Tom", last_name="Smith")
View
11 tests/expressions/tests.py
@@ -2,6 +2,7 @@
from django.core.exceptions import FieldError
from django.db.models import F
+from django.db import transaction
from django.test import TestCase
from django.utils import six
@@ -185,11 +186,11 @@ def test_filter(self):
"foo",
)
- self.assertRaises(FieldError,
- lambda: Company.objects.exclude(
- ceo__firstname=F('point_of_contact__firstname')
- ).update(name=F('point_of_contact__lastname'))
- )
+ with transaction.atomic():
+ with self.assertRaises(FieldError):
+ Company.objects.exclude(
+ ceo__firstname=F('point_of_contact__firstname')
+ ).update(name=F('point_of_contact__lastname'))
# F expressions can be used to update attributes on single objects
test_gmbh = Company.objects.get(name="Test GmbH")
View
17 tests/force_insert_update/tests.py
@@ -21,24 +21,29 @@ def test_force_update(self):
# Won't work because force_update and force_insert are mutually
# exclusive
c.value = 4
- self.assertRaises(ValueError, c.save, force_insert=True, force_update=True)
+ with self.assertRaises(ValueError):
+ c.save(force_insert=True, force_update=True)
# Try to update something that doesn't have a primary key in the first
# place.
c1 = Counter(name="two", value=2)
- self.assertRaises(ValueError, c1.save, force_update=True)
+ with self.assertRaises(ValueError):
+ with transaction.atomic():
+ c1.save(force_update=True)
c1.save(force_insert=True)
# Won't work because we can't insert a pk of the same value.
- sid = transaction.savepoint()
c.value = 5
- self.assertRaises(IntegrityError, c.save, force_insert=True)
- transaction.savepoint_rollback(sid)
+ with self.assertRaises(IntegrityError):
+ with transaction.atomic():
+ c.save(force_insert=True)
# Trying to update should still fail, even with manual primary keys, if
# the data isn't in the database already.
obj = WithCustomPK(name=1, value=1)
- self.assertRaises(DatabaseError, obj.save, force_update=True)
+ with self.assertRaises(DatabaseError):
+ with transaction.atomic():
+ obj.save(force_update=True)
class InheritanceTests(TestCase):
View
6 tests/one_to_one/tests.py
@@ -118,7 +118,7 @@ def test_multiple_o2o(self):
self.assertEqual(repr(o1.multimodel), '<MultiModel: Multimodel x1>')
# This will fail because each one-to-one field must be unique (and
# link2=o1 was used for x1, above).
- sid = transaction.savepoint()
mm = MultiModel(link1=self.p2, link2=o1, name="x1")
- self.assertRaises(IntegrityError, mm.save)
- transaction.savepoint_rollback(sid)
+ with self.assertRaises(IntegrityError):
+ with transaction.atomic():
+ mm.save()
View
71 tests/transactions/tests.py
@@ -4,7 +4,7 @@
from unittest import skipIf, skipUnless
from django.db import connection, transaction, DatabaseError, IntegrityError
-from django.test import TransactionTestCase, skipUnlessDBFeature
+from django.test import TransactionTestCase, skipIfDBFeature, skipUnlessDBFeature
from django.test.utils import IgnoreDeprecationWarningsMixin
from django.utils import six
@@ -204,10 +204,10 @@ def test_prevent_rollback(self):
with transaction.atomic(savepoint=False):
connection.cursor().execute(
"SELECT no_such_col FROM transactions_reporter")
- transaction.savepoint_rollback(sid)
- # atomic block should rollback, but prevent it, as we just did it.
+ # prevent atomic from rolling back since we're recovering manually
self.assertTrue(transaction.get_rollback())
transaction.set_rollback(False)
+ transaction.savepoint_rollback(sid)
self.assertQuerysetEqual(Reporter.objects.all(), ['<Reporter: Tintin>'])
@@ -267,11 +267,19 @@ def test_merged_outer_rollback(self):
with transaction.atomic(savepoint=False):
Reporter.objects.create(first_name="Calculus")
raise Exception("Oops, that's his last name")
- # It wasn't possible to roll back
+ # The third insert couldn't be roll back. Temporarily mark the
+ # connection as not needing rollback to check it.
+ self.assertTrue(transaction.get_rollback())
+ transaction.set_rollback(False)
self.assertEqual(Reporter.objects.count(), 3)
- # It wasn't possible to roll back
+ transaction.set_rollback(True)
+ # The second insert couldn't be roll back. Temporarily mark the
+ # connection as not needing rollback to check it.
+ self.assertTrue(transaction.get_rollback())
+ transaction.set_rollback(False)
self.assertEqual(Reporter.objects.count(), 3)
- # The outer block must roll back
+ transaction.set_rollback(True)
+ # The first block has a savepoint and must roll back.
self.assertQuerysetEqual(Reporter.objects.all(), [])
def test_merged_inner_savepoint_rollback(self):
@@ -283,36 +291,22 @@ def test_merged_inner_savepoint_rollback(self):
with transaction.atomic(savepoint=False):
Reporter.objects.create(first_name="Calculus")
raise Exception("Oops, that's his last name")
- # It wasn't possible to roll back
+ # The third insert couldn't be roll back. Temporarily mark the
+ # connection as not needing rollback to check it.
+ self.assertTrue(transaction.get_rollback())
+ transaction.set_rollback(False)
self.assertEqual(Reporter.objects.count(), 3)
- # The first block with a savepoint must roll back
+ transaction.set_rollback(True)
+ # The second block has a savepoint and must roll back.
self.assertEqual(Reporter.objects.count(), 1)
self.assertQuerysetEqual(Reporter.objects.all(), ['<Reporter: Tintin>'])
- def test_merged_outer_rollback_after_inner_failure_and_inner_success(self):
- with transaction.atomic():
- Reporter.objects.create(first_name="Tintin")
- # Inner block without a savepoint fails
- with six.assertRaisesRegex(self, Exception, "Oops"):
- with transaction.atomic(savepoint=False):
- Reporter.objects.create(first_name="Haddock")
- raise Exception("Oops, that's his last name")
- # It wasn't possible to roll back
- self.assertEqual(Reporter.objects.count(), 2)
- # Inner block with a savepoint succeeds
- with transaction.atomic(savepoint=False):
- Reporter.objects.create(first_name="Archibald", last_name="Haddock")
- # It still wasn't possible to roll back
- self.assertEqual(Reporter.objects.count(), 3)
- # The outer block must rollback
- self.assertQuerysetEqual(Reporter.objects.all(), [])
-
@skipUnless(connection.features.uses_savepoints,
"'atomic' requires transactions and savepoints.")
class AtomicErrorsTests(TransactionTestCase):
- available_apps = []
+ available_apps = ['transactions']
def test_atomic_prevents_setting_autocommit(self):
autocommit = transaction.get_autocommit()
@@ -336,6 +330,29 @@ def test_atomic_prevents_calling_transaction_management_methods(self):
with self.assertRaises(transaction.TransactionManagementError):
transaction.leave_transaction_management()
+ def test_atomic_prevents_queries_in_broken_transaction(self):
+ r1 = Reporter.objects.create(first_name="Archibald", last_name="Haddock")
+ with transaction.atomic():
+ r2 = Reporter(first_name="Cuthbert", last_name="Calculus", id=r1.id)
+ with self.assertRaises(IntegrityError):
+ r2.save(force_insert=True)
+ # The transaction is marked as needing rollback.
+ with self.assertRaises(transaction.TransactionManagementError):
+ r2.save(force_update=True)
+ self.assertEqual(Reporter.objects.get(pk=r1.pk).last_name, "Haddock")
+
+ @skipIfDBFeature('atomic_transactions')
+ def test_atomic_allows_queries_after_fixing_transaction(self):
+ r1 = Reporter.objects.create(first_name="Archibald", last_name="Haddock")
+ with transaction.atomic():
+ r2 = Reporter(first_name="Cuthbert", last_name="Calculus", id=r1.id)
+ with self.assertRaises(IntegrityError):
+ r2.save(force_insert=True)
+ # Mark the transaction as no longer needing rollback.
+ transaction.set_rollback(False)
+ r2.save(force_update=True)
+ self.assertEqual(Reporter.objects.get(pk=r1.pk).last_name, "Calculus")
+
class AtomicMiscTests(TransactionTestCase):
Something went wrong with that request. Please try again.