Opened 10 years ago

Closed 9 years ago

#23270 closed Bug (fixed)

select_related on fields pointing to subclasses does not work when using defer

Reported by: islavov Owned by: nobody
Component: Database layer (models, ORM) Version: 1.7
Severity: Normal Keywords: select_related defer
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have the following models

from django.db import models

class Base(models.Model):
    text = models.TextField()

class SubClassA(Base):
    name = models.CharField(max_length=32)

In shell

In [3]: from defertest.models import SubClassA, Base

# Select related + defer
In [4]: c=Base.objects.select_related("subclassa").defer("text")[0]
(0.001) SELECT "defertest_base"."id", "defertest_subclassa"."base_ptr_id", "defertest_subclassa"."name" FROM "defertest_base" LEFT OUTER JOIN "defertest_subclassa" ON ("defertest_base"."id" = "defertest_subclassa"."base_ptr_id") LIMIT 1; args=()

# We get an extra query when accessing the subclass
In [5]: c.subclassa
(0.000) SELECT "defertest_base"."id", "defertest_base"."text", "defertest_subclassa"."base_ptr_id", "defertest_subclassa"."name" FROM "defertest_subclassa" INNER JOIN "defertest_base" ON ("defertest_subclassa"."base_ptr_id" = "defertest_base"."id") WHERE "defertest_subclassa"."base_ptr_id" = 1 ; args=(1,)
Out[5]: <SubClassA: SubClassA object>

# If no deferred fields
In [6]: c=Base.objects.select_related("subclassa")[0]
(0.000) SELECT "defertest_base"."id", "defertest_base"."text", "defertest_subclassa"."base_ptr_id", "defertest_subclassa"."name" FROM "defertest_base" LEFT OUTER JOIN "defertest_subclassa" ON ("defertest_base"."id" = "defertest_subclassa"."base_ptr_id") LIMIT 1; args=()

# select related works fine ( no extra query )
In [7]: c.subclassa
Out[7]: <SubClassA: SubClassA object>

Change History (14)

comment:1 by Josh Smeaton, 10 years ago

Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Josh Smeaton, 10 years ago

Verified that there is an extra query. Tried the patch from #23001 thinking it may be related, but same problem there.

comment:3 by Kiril Vladimiroff, 10 years ago

The reason behind this is that subclassa (in the given example) is not technically a field. Which means it won't be returned by Base._meta.get_concrete_fields_with_model. The only solution I could think of is creating get_all_concrete_fields_with_model (which should use get_all_field_names instead of local_fields).

Anyone with better ideas?

comment:4 by Kiril Vladimiroff, 10 years ago

Ok, my idea was completely wrong. I had to dig a little bit deeper than I thought.

https://github.com/django/django/pull/3067

comment:5 by Tim Graham, 10 years ago

Has patch: set
Needs documentation: unset
Needs tests: unset
Triage Stage: AcceptedReady for checkin
Type: UncategorizedBug

Ready for review by ORM experts.

comment:6 by Anssi Kääriäinen, 10 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

The patch doesn't seem to work correctly. The obj.derived.base_ptr_id doesn't have correct value of 1 in the test case.

>>> obj.derived.base_ptr_id
'bar'

comment:7 by Kiril Vladimiroff, 10 years ago

@akaariai, you're right. I was building the field_names wrong. The issue was not only that base_ptr_id was with the wrong value, but *all* of the values was getting mapped to the wrong field.

The only solution that I found was completely rebuilding the whole list from scratch. Was that a good idea?
https://github.com/Vladimiroff/django/commit/cfd87b8157bf38ce145758e0251f5c14dfb5e60f

comment:8 by Tim Graham, 10 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

Ready for another review.

comment:9 by Tim Graham, 10 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

See PR for comments.

comment:10 by Tim Graham, 10 years ago

Patch needs improvement: unset

Patch received another update.

comment:11 by Tim Graham, 10 years ago

Patch needs improvement: set

Marking as "patch needs improvement" per latest comment from Anssi on the PR.

comment:12 by Anssi Kääriäinen, 10 years ago

Pull request https://github.com/django/django/pull/3669 fixes this issue.

comment:13 by Tim Graham, 10 years ago

Version: 1.7-rc-21.7

comment:14 by Tim Graham, 9 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top