From f095356ba2244f6ed48463bac0b46d3c3e671204 Mon Sep 17 00:00:00 2001 From: Shai Berger Date: Sun, 30 Mar 2014 20:03:35 +0300 Subject: [PATCH] Fixed #22343 -- Disallowed select_for_update in autocommit mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ticket was originally about two failing tests, which are fixed by putting their queries in transactions. Thanks Tim Graham for the report, Aymeric Augustin for the fix, and Simon Charette, Tim Graham & Loïc Bistuer for review. --- django/db/models/sql/compiler.py | 4 ++++ docs/ref/models/querysets.txt | 10 ++++++++++ docs/releases/1.6.3.txt | 28 +++++++++++++++++++++++++++- docs/releases/1.7.txt | 25 +++++++++++++++++++++++++ docs/topics/db/transactions.txt | 5 ++++- tests/select_for_update/tests.py | 26 ++++++++++++++++++++++++-- 6 files changed, 94 insertions(+), 4 deletions(-) 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