Opened 4 months ago
Last modified 2 days ago
#36398 assigned Bug
select_for_update(of=...) ignores "self" when using values_list() when not selecting a column from the model
Reported by: | OutOfFocus4 | Owned by: | JakeWalson |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As of the most recent version of Django, if a queryset uses values_list(...)
and select_for_update(of=("self", ...))
, and the resulting SQL contains a nullable join, the resulting SQL will cause a database error if none of the values in values_list(...)
involve columns from the queryset's model's table.
Attachments (3)
Change History (12)
by , 4 months ago
follow-up: 2 comment:1 by , 4 months ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:2 by , 4 months ago
Replying to Sarah Boyce:
Thank you for the ticket
I needed to addavailable_apps = ["django.contrib.auth"]
to the test case and both tests passed for me on SQLite. Based off the ticket description, I think one of them should have raised a database error? Can you clarify how to replicate the issue?
SQLite doesn't support SELECT ... FOR UPDATE
, so the ORM skips the code that is raising the error. If you run the code with a PostgreSQL database, test_proof_of_concept
will fail; the stacktrace should end with django.db.utils.NotSupportedError: FOR UPDATE cannot be applied to the nullable side of an outer join
.
This error should not occur, because the .select_for_update(of=("self",))
on line 47 should apply the FOR UPDATE
to only the auth_user
table.
If you execute print(User.objects.values_list("groups__name", flat=True).order_by("groups__name").select_for_update(of=("self",)).filter(pk=1).query)
in an atomic
block, it prints this SQL:
SELECT "auth_group"."name" AS "groups__name" FROM "auth_user" LEFT OUTER JOIN "auth_user_groups" ON ("auth_user"."id" = "auth_user_groups"."user_id") LEFT OUTER JOIN "auth_group" ON ("auth_user_groups"."group_id" = "auth_group"."id") WHERE "auth_user"."id" = 1 ORDER BY 1 ASC FOR UPDATE
Executing print(User.objects.values_list("groups__name", 'pk').order_by("groups__name").select_for_update(of=("self",)).filter(pk=1).query)
in an atomic
block prints this SQL:
SELECT "auth_group"."name" AS "groups__name", "auth_user"."id" AS "pk" FROM "auth_user" LEFT OUTER JOIN "auth_user_groups" ON ("auth_user"."id" = "auth_user_groups"."user_id") LEFT OUTER JOIN "auth_group" ON ("auth_user_groups"."group_id" = "auth_group"."id") WHERE "auth_user"."id" = 1 ORDER BY 1 ASC FOR UPDATE OF "auth_user"
Notice how FOR UPDATE OF "auth_user"
is only added when a column from the auth_user
table is SELECT
ed.
follow-up: 5 comment:3 by , 4 months ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
Version: | 5.2 → dev |
Thank you, replicated
That this is a m2m field is important I believe
Possible test:
-
tests/select_for_update/models.py
a b class Person(models.Model): 41 41 name = models.CharField(max_length=30) 42 42 born = models.ForeignKey(City, models.CASCADE, related_name="+") 43 43 died = models.ForeignKey(City, models.CASCADE, related_name="+") 44 lived = models.ManyToManyField(City, related_name="people_set") 44 45 45 46 46 47 class PersonProfile(models.Model): -
tests/select_for_update/tests.py
diff --git a/tests/select_for_update/tests.py b/tests/select_for_update/tests.py index 1bc87113ba..49453240c5 100644
a b class SelectForUpdateTests(TransactionTestCase): 278 278 ) 279 279 self.assertEqual(values, [(self.person.pk,)]) 280 280 281 @skipUnlessDBFeature("has_select_for_update_of") 282 def test_for_update_of_followed_by_values_list_of_m2m_field(self): 283 self.person.lived.add(self.city1) 284 self.person.lived.add(self.city2) 285 286 with transaction.atomic(): 287 values = list( 288 Person.objects.select_for_update(of=("self",)) 289 .values_list("lived__name", flat=True) 290 ) 291 self.assertEqual(values, [self.city1.name, self.city2.name]) 292 281 293 @skipUnlessDBFeature("has_select_for_update_of") 282 294 def test_for_update_of_self_when_self_is_not_selected(self):
comment:4 by , 4 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 4 months ago
Replying to Sarah Boyce:
Thank you, replicated
That this is a m2m field is important I believe
I have replicated the error with reverse foreign keys and nullable forward foreign keys. I have attached the tests. The models.py
is identical to the one currently used for Django tests, except the died
field of the Person
model has null=True
.
by , 4 months ago
by , 4 months ago
Attachment: | tests.2.py added |
---|
follow-up: 9 comment:8 by , 4 weeks ago
This won't be resolved until someone fixes the ticket, so there's no need to confirm with each release that this is still open (that the ticket is open is enough to confirm that this should still be an open issue)
We also wouldn't backport a fix into 5.2 unless this is a bug that was introduced by 5.2. If you believe this was introduced in 5.2, you can do a git bisect to find the commit that introduces this issue. This would increase the priority of the ticket.
comment:9 by , 2 days ago
Replying to Sarah Boyce:
This won't be resolved until someone fixes the ticket, so there's no need to confirm with each release that this is still open (that the ticket is open is enough to confirm that this should still be an open issue)
We also wouldn't backport a fix into 5.2 unless this is a bug that was introduced by 5.2. If you believe this was introduced in 5.2, you can do a git bisect to find the commit that introduces this issue. This would increase the priority of the ticket.
I can replicate this issue with older versions of Django, so I don't believe this was introduced in 5.2.
I believe I may have found (part of) the cause. From https://github.com/django/django/blob/d82f25d3f0f4eb7be721a72d0e79a8d13d394d32/django/db/models/sql/compiler.py#L1432C1-L1443C56:
def _get_first_selected_col_from_model(klass_info): """ Find the first selected column from a model. If it doesn't exist, don't lock a model. 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 == concrete_model: return self.select[select_index][0]
If the docstring is to be believed, only tables whose columns are in the query's SELECT
can be locked. I do not know enough about Django's internals to know if that is what the function actually does.
Thank you for the ticket
I needed to add
available_apps = ["django.contrib.auth"]
to the test case and both tests passed for me on SQLite. Based off the ticket description, I think one of them should have raised a database error? Can you clarify how to replicate the issue?