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)

tests.py (1.9 KB ) - added by OutOfFocus4 4 months ago.
models.py (1.0 KB ) - added by OutOfFocus4 4 months ago.
tests.2.py (1.6 KB ) - added by OutOfFocus4 4 months ago.

Download all attachments as: .zip

Change History (12)

by OutOfFocus4, 4 months ago

Attachment: tests.py added

comment:1 by Sarah Boyce, 4 months ago

Resolution: needsinfo
Status: newclosed

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?

in reply to:  1 comment:2 by OutOfFocus4, 4 months ago

Replying to Sarah Boyce:

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?

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 SELECTed.

comment:3 by Sarah Boyce, 4 months ago

Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 5.2dev

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):  
    4141    name = models.CharField(max_length=30)
    4242    born = models.ForeignKey(City, models.CASCADE, related_name="+")
    4343    died = models.ForeignKey(City, models.CASCADE, related_name="+")
     44    lived = models.ManyToManyField(City, related_name="people_set")
    4445
    4546
    4647class 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):  
    278278            )
    279279        self.assertEqual(values, [(self.person.pk,)])
    280280
     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
    281293    @skipUnlessDBFeature("has_select_for_update_of")
    282294    def test_for_update_of_self_when_self_is_not_selected(self):

comment:4 by JakeWalson, 4 months ago

Owner: set to JakeWalson
Status: newassigned

in reply to:  3 comment:5 by OutOfFocus4, 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 OutOfFocus4, 4 months ago

Attachment: models.py added

by OutOfFocus4, 4 months ago

Attachment: tests.2.py added

comment:6 by OutOfFocus4, 2 months ago

The issue is still present in Django 5.2.4.

comment:7 by OutOfFocus4, 4 weeks ago

The issue is still present in Django 5.2.5.

comment:8 by Sarah Boyce, 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.

in reply to:  8 comment:9 by OutOfFocus4, 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.

Note: See TracTickets for help on using tickets.
Back to Top