diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index e166492896..9351169357 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -11,6 +11,7 @@ from django.db.models.sql.constants import (CURSOR, SINGLE, MULTI, NO_RESULTS, from django.db.models.sql.datastructures import EmptyResultSet from django.db.models.sql.expressions import SQLEvaluator from django.db.models.sql.query import get_order_dir, Query +from django.db.transaction import TransactionManagementError from django.db.utils import DatabaseError from django.utils import six from django.utils.six.moves import zip @@ -157,6 +158,9 @@ class SQLCompiler(object): result.append('OFFSET %d' % self.query.low_mark) if self.query.select_for_update and self.connection.features.has_select_for_update: + if self.connection.get_autocommit(): + raise TransactionManagementError("select_for_update cannot be used outside of a transaction.") + # If we've been asked for a NOWAIT query but the backend does not support it, # raise a DatabaseError otherwise we could get an unexpected deadlock. nowait = self.query.select_for_update_nowait diff --git a/docs/ref/models/querysets.txt b/docs/ref/models/querysets.txt index fa3a45e1eb..18941773ba 100644 --- a/docs/ref/models/querysets.txt +++ b/docs/ref/models/querysets.txt @@ -1365,9 +1365,19 @@ do not support ``nowait``, such as MySQL, will cause a :exc:`~django.db.DatabaseError` to be raised. This is in order to prevent code unexpectedly blocking. +Executing a queryset with ``select_for_update`` in autocommit mode is +an error because the rows are then not locked. If allowed, this would +facilitate data corruption, and could easily be caused by calling, +outside of any transaction, code that expects to be run in one. + Using ``select_for_update`` on backends which do not support ``SELECT ... FOR UPDATE`` (such as SQLite) will have no effect. +.. versionchanged:: 1.6.3 + + It is now an error to execute a query with ``select_for_update()`` in + autocommit mode. With earlier releases in the 1.6 series it was a no-op. + raw ~~~ diff --git a/docs/releases/1.6.3.txt b/docs/releases/1.6.3.txt index ca9069d250..c6981fa96b 100644 --- a/docs/releases/1.6.3.txt +++ b/docs/releases/1.6.3.txt @@ -5,7 +5,33 @@ Django 1.6.3 release notes *Under development* This is Django 1.6.3, a bugfix release for Django 1.6. Django 1.6.3 fixes -several bugs in 1.6.2: +several bugs in 1.6.2 and makes one backwards-incompatible change: + +``select_for_update()`` requires a transaction +============================================== + +Historically, queries that use +:meth:`~django.db.models.query.QuerySet.select_for_update()` could be +executed in autocommit mode, outside of a transaction. Before Django +1.6, Django's automatic transactions mode allowed this to be used to +lock records until the next write operation. Django 1.6 introduced +database-level autocommit; since then, execution in such a context +voids the effect of ``select_for_update()``. It is, therefore, assumed +now to be an error, and raises an exception. + +This change may cause test failures if you use ``select_for_update()`` +in a test class which is a subclass of +:class:`~django.test.TransactionTestCase` rather than +:class:`~django.test.TestCase`. + +This change was made because such errors can be caused by including an +app which expects global transactions (e.g. :setting:`ATOMIC_REQUESTS +` set to True), or Django's old autocommit +behavior, in a project which runs without them; and further, such +errors may manifest as data-corruption bugs. + +Other bugfixes and changes +========================== * Content retrieved from the GeoIP library is now properly decoded from its default ``iso-8859-1`` encoding diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 90208e4c8b..aaba82a5bf 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -1086,6 +1086,31 @@ Note also that the admin login form has been updated to not contain the ``this_is_the_login_form`` field (now unused) and the ``ValidationError`` code has been set to the more regular ``invalid_login`` key. +``select_for_update()`` requires a transaction +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Historically, queries that use +:meth:`~django.db.models.query.QuerySet.select_for_update()` could be +executed in autocommit mode, outside of a transaction. Before Django +1.6, Django's automatic transactions mode allowed this to be used to +lock records until the next write operation. Django 1.6 introduced +database-level autocommit; since then, execution in such a context +voids the effect of ``select_for_update()``. It is, therefore, assumed +now to be an error, and raises an exception. + +This change may cause test failures if you use ``select_for_update()`` +in a test class which is a subclass of +:class:`~django.test.TransactionTestCase` rather than +:class:`~django.test.TestCase`. + +This change was made because such errors can be caused by including an +app which expects global transactions (e.g. :setting:`ATOMIC_REQUESTS +` set to True), or Django's old autocommit +behavior, in a project which runs without them; and further, such +errors may manifest as data-corruption bugs. + +This was also fixed in Django 1.6.3. + Miscellaneous ~~~~~~~~~~~~~ diff --git a/docs/topics/db/transactions.txt b/docs/topics/db/transactions.txt index c4ffa4b35e..0f6c03d834 100644 --- a/docs/topics/db/transactions.txt +++ b/docs/topics/db/transactions.txt @@ -695,7 +695,10 @@ Select for update If you were relying on "automatic transactions" to provide locking between :meth:`~django.db.models.query.QuerySet.select_for_update` and a subsequent write operation — an extremely fragile design, but nonetheless possible — you -must wrap the relevant code in :func:`atomic`. +must wrap the relevant code in :func:`atomic`. Since Django 1.6.3, executing +a query with :meth:`~django.db.models.query.QuerySet.select_for_update` in +autocommit mode will raise a +:exc:`~django.db.transaction.TransactionManagementError`. Using a high isolation level ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/select_for_update/tests.py b/tests/select_for_update/tests.py index a4136d7a16..54ba0d1d27 100644 --- a/tests/select_for_update/tests.py +++ b/tests/select_for_update/tests.py @@ -77,7 +77,8 @@ class SelectForUpdateTests(TransactionTestCase): Test that the backend's FOR UPDATE variant appears in generated SQL when select_for_update is invoked. """ - list(Person.objects.all().select_for_update()) + with transaction.atomic(): + list(Person.objects.all().select_for_update()) self.assertTrue(self.has_for_update_sql(connection)) @skipUnlessDBFeature('has_select_for_update_nowait') @@ -86,7 +87,8 @@ class SelectForUpdateTests(TransactionTestCase): Test that the backend's FOR UPDATE NOWAIT variant appears in generated SQL when select_for_update is invoked. """ - list(Person.objects.all().select_for_update(nowait=True)) + with transaction.atomic(): + list(Person.objects.all().select_for_update(nowait=True)) self.assertTrue(self.has_for_update_sql(connection, nowait=True)) @requires_threading @@ -125,6 +127,26 @@ class SelectForUpdateTests(TransactionTestCase): Person.objects.all().select_for_update(nowait=True) ) + @skipUnlessDBFeature('has_select_for_update') + def test_for_update_requires_transaction(self): + """ + Test that a TransactionManagementError is raised + when a select_for_update query is executed outside of a transaction. + """ + with self.assertRaises(transaction.TransactionManagementError): + list(Person.objects.all().select_for_update()) + + @skipUnlessDBFeature('has_select_for_update') + def test_for_update_requires_transaction_only_in_execution(self): + """ + Test that no TransactionManagementError is raised + when select_for_update is invoked outside of a transaction - + only when the query is executed. + """ + people = Person.objects.all().select_for_update() + with self.assertRaises(transaction.TransactionManagementError): + list(people) + def run_select_for_update(self, status, nowait=False): """ Utility method that runs a SELECT FOR UPDATE against all