Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30796 closed Bug (fixed)

Chaining select_related mutates original QuerySet.

Reported by: Darren Maki Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 2.2
Severity: Normal Keywords: select_related prefetch_related mutate queryset
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Darren Maki)

When creating a new QuerySet from an existing QuerySet that has had 'select_related' applied, if you apply another 'select_related' to the new QuerySet it will mutate the original QuerySet to also have the extra 'select_related'.

models.py

from django.db import models

class ModelA(models.Model):
    pass

class ModelB(models.Model):
    pass

class ModelC(models.Model):
    model_a = models.ForeignKey('foobar.ModelA', on_delete=models.CASCADE)
    model_b = models.ForeignKey('foobar.ModelB', on_delete=models.CASCADE)

test.py

query_1 = ModelC.objects.select_related('model_a')
print('QUERY 1:', str(query_1.query))

query_2 = query_1.select_related('model_b')
print('QUERY 2:', str(query_2.query))

print('QUERY 1:', str(query_1.query))

if str(query_1.query) == str(query_2.query):
    print('\n!!! The two queries are the same !!!\n')

output

QUERY 1: SELECT "foobar_modelc"."id", "foobar_modelc"."model_a_id", "foobar_modelc"."model_b_id", "foobar_modela"."id" FROM "foobar_modelc" INNER JOIN "foobar_modela" ON ("foobar_modelc"."model_a_id" = "foobar_modela"."id")
QUERY 2: SELECT "foobar_modelc"."id", "foobar_modelc"."model_a_id", "foobar_modelc"."model_b_id", "foobar_modela"."id", "foobar_modelb"."id" FROM "foobar_modelc" INNER JOIN "foobar_modela" ON ("foobar_modelc"."model_a_id" = "foobar_modela"."id") INNER JOIN "foobar_modelb" ON ("foobar_modelc"."model_b_id" = "foobar_modelb"."id")
QUERY 1: SELECT "foobar_modelc"."id", "foobar_modelc"."model_a_id", "foobar_modelc"."model_b_id", "foobar_modela"."id", "foobar_modelb"."id" FROM "foobar_modelc" INNER JOIN "foobar_modela" ON ("foobar_modelc"."model_a_id" = "foobar_modela"."id") INNER JOIN "foobar_modelb" ON ("foobar_modelc"."model_b_id" = "foobar_modelb"."id")

!!! The two queries are the same !!!

The expectation is that the original QuerySet is not mutated, and the two queries are different. This behavior also happens with 'prefetch_related'.

Since the QuerySet methods call 'self._clone()', and state that they return 'a new QuerySet instance', this behavior does not seem correct.

Change History (10)

comment:1 by Darren Maki, 5 years ago

Description: modified (diff)

comment:2 by Simon Charette, 5 years ago

Triage Stage: UnreviewedAccepted

This seems to have been happening forever.

sql.Query.select_related is made a dict on .add_select_related but never copied on .clone.

comment:3 by Simon Charette, 5 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:4 by Simon Charette, 5 years ago

Has patch: set

comment:5 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 37f8f293:

Fixed #30796 -- Prevented select_related() from mutating a queryset on chaining.

Thanks Darren Maki for the report.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 6b7bd079:

[3.0.x] Fixed #30796 -- Prevented select_related() from mutating a queryset on chaining.

Thanks Darren Maki for the report.

Backport of 37f8f293775d0b672da8ae369d9a4e17f1db7851 from master

comment:7 by Alexander Klimenko, 5 years ago

Are there any plans of releasing this fix?

comment:8 by Simon Charette, 5 years ago

It's already released in Django 3.0.x and won't be backported to 2.2.x LTS since this bug has been around for a few years before getting identified and thus isn't a recent regression.

Last edited 5 years ago by Simon Charette (previous) (diff)

in reply to:  8 comment:9 by Alexander Klimenko, 5 years ago

Replying to Simon Charette:

It's already released in Django 3.0.x and won't be backported to 2.2.x LTS since this bug has been around for a few years before getting identified and thus isn't a recent regression.

It is not trivial, figuring out this problem took me and my teammate about 2 days.
We don't know how many people were unsuccessful figuring it out before.
This is too high cost for leaving it unfixed IMHO.

comment:10 by Simon Charette, 5 years ago

Django 2.2 contains multiple bugs that have been fixed in 3.0 and the master branch that were never backported due to our policy.

It is not trivial, figuring out this problem took me and my teammate about 2 days. We don't know how many people were unsuccessful figuring it out before.

I'm pretty sure the same can be said about many of the aforementioned non-backported bug fixes. The backport policy is there to ensure dot releases are stable and do not incur too much maintenance burden. Every backport has a chance of causing another regression and as branches diverge it becomes harder and riskier to do so. The data loss and security backport policy that 2.2 LTS is now following ensures its stability and this long standing issue simply doesn't qualify for backporting even if it's trivial and likely benign.

This is too high cost for leaving it unfixed IMHO.

It is fixed in Django 3.0. If it is too high cost for your use case you should consider upgrading.

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