From 60626162f76f26d32a38d18151700cb041201fb3 Mon Sep 17 00:00:00 2001 From: Daniel Hillier Date: Sat, 8 Aug 2020 15:17:36 +1000 Subject: [PATCH] Fixed #31866 -- Fixed locking proxy models in QuerySet.select_for_update(of=()). --- django/db/models/sql/compiler.py | 6 ++++-- docs/releases/2.2.16.txt | 5 ++++- docs/releases/3.0.10.txt | 5 ++++- docs/releases/3.1.1.txt | 5 +++++ tests/select_for_update/models.py | 14 ++++++++++++++ tests/select_for_update/tests.py | 32 ++++++++++++++++++++++++++++++- 6 files changed, 62 insertions(+), 5 deletions(-) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 686e75c979..fc7a7aafae 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -982,7 +982,8 @@ class SQLCompiler: the query. """ def _get_parent_klass_info(klass_info): - for parent_model, parent_link in klass_info['model']._meta.parents.items(): + concrete_model = klass_info['model']._meta.concrete_model + for parent_model, parent_link in concrete_model._meta.parents.items(): parent_list = parent_model._meta.get_parent_list() yield { 'model': parent_model, @@ -1007,8 +1008,9 @@ class SQLCompiler: select_fields is filled recursively, so it also contains fields from the parent models. """ + concrete_model = klass_info['model']._meta.concrete_model for select_index in klass_info['select_fields']: - if self.select[select_index][0].target.model == klass_info['model']: + if self.select[select_index][0].target.model == concrete_model: return self.select[select_index][0] def _get_field_choices(): diff --git a/docs/releases/2.2.16.txt b/docs/releases/2.2.16.txt index 7e80406e9d..daba6f6f52 100644 --- a/docs/releases/2.2.16.txt +++ b/docs/releases/2.2.16.txt @@ -9,4 +9,7 @@ Django 2.2.16 fixes a data loss bug in 2.2.15. Bugfixes ======== -* ... +* Fixed a data loss possibility in the + :meth:`~django.db.models.query.QuerySet.select_for_update()`. When using + related fields pointing to a proxy model in the ``of`` argument, the + corresponding model was not locked (:ticket:`31866`). diff --git a/docs/releases/3.0.10.txt b/docs/releases/3.0.10.txt index 66c49c5dff..2d97851163 100644 --- a/docs/releases/3.0.10.txt +++ b/docs/releases/3.0.10.txt @@ -9,4 +9,7 @@ Django 3.0.10 fixes a data loss bug in 3.0.9. Bugfixes ======== -* ... +* Fixed a data loss possibility in the + :meth:`~django.db.models.query.QuerySet.select_for_update()`. When using + related fields pointing to a proxy model in the ``of`` argument, the + corresponding model was not locked (:ticket:`31866`). diff --git a/docs/releases/3.1.1.txt b/docs/releases/3.1.1.txt index c777b3cc60..6aa091bb82 100644 --- a/docs/releases/3.1.1.txt +++ b/docs/releases/3.1.1.txt @@ -20,3 +20,8 @@ Bugfixes * Adjusted admin's navigation sidebar template to reduce debug logging when rendering (:ticket:`31865`). + +* Fixed a data loss possibility in the + :meth:`~django.db.models.query.QuerySet.select_for_update()`. When using + related fields pointing to a proxy model in the ``of`` argument, the + corresponding model was not locked (:ticket:`31866`). diff --git a/tests/select_for_update/models.py b/tests/select_for_update/models.py index 305e8cac49..0a6396bc70 100644 --- a/tests/select_for_update/models.py +++ b/tests/select_for_update/models.py @@ -23,6 +23,20 @@ class EUCity(models.Model): country = models.ForeignKey(EUCountry, models.CASCADE) +class CountryProxy(Country): + class Meta: + proxy = True + + +class CountryProxyProxy(CountryProxy): + class Meta: + proxy = True + + +class CityCountryProxy(models.Model): + country = models.ForeignKey(CountryProxyProxy, models.CASCADE) + + class Person(models.Model): name = models.CharField(max_length=30) born = models.ForeignKey(City, models.CASCADE, related_name='+') diff --git a/tests/select_for_update/tests.py b/tests/select_for_update/tests.py index 2197596a16..705975b760 100644 --- a/tests/select_for_update/tests.py +++ b/tests/select_for_update/tests.py @@ -15,7 +15,9 @@ from django.test import ( ) from django.test.utils import CaptureQueriesContext -from .models import City, Country, EUCity, EUCountry, Person, PersonProfile +from .models import ( + City, CityCountryProxy, Country, EUCity, EUCountry, Person, PersonProfile, +) class SelectForUpdateTests(TransactionTestCase): @@ -205,6 +207,21 @@ class SelectForUpdateTests(TransactionTestCase): expected = [connection.ops.quote_name(value) for value in expected] self.assertTrue(self.has_for_update_sql(ctx.captured_queries, of=expected)) + @skipUnlessDBFeature('has_select_for_update_of') + def test_for_update_sql_model_proxy_generated_of(self): + with transaction.atomic(), CaptureQueriesContext(connection) as ctx: + list(CityCountryProxy.objects.select_related( + 'country', + ).select_for_update( + of=('country',), + )) + if connection.features.select_for_update_of_column: + expected = ['select_for_update_country"."entity_ptr_id'] + else: + expected = ['select_for_update_country'] + expected = [connection.ops.quote_name(value) for value in expected] + self.assertTrue(self.has_for_update_sql(ctx.captured_queries, of=expected)) + @skipUnlessDBFeature('has_select_for_update_of') def test_for_update_of_followed_by_values(self): with transaction.atomic(): @@ -375,6 +392,19 @@ class SelectForUpdateTests(TransactionTestCase): with transaction.atomic(): EUCountry.objects.select_for_update(of=('name',)).get() + @skipUnlessDBFeature('has_select_for_update', 'has_select_for_update_of') + def test_model_proxy_of_argument_raises_error_proxy_field_in_choices(self): + msg = ( + 'Invalid field name(s) given in select_for_update(of=(...)): ' + 'name. Only relational fields followed in the query are allowed. ' + 'Choices are: self, country, country__entity_ptr.' + ) + with self.assertRaisesMessage(FieldError, msg): + with transaction.atomic(): + CityCountryProxy.objects.select_related( + 'country', + ).select_for_update(of=('name',)).get() + @skipUnlessDBFeature('has_select_for_update', 'has_select_for_update_of') def test_reverse_one_to_one_of_arguments(self): """