Opened 12 months ago

Last modified 7 months ago

#23270 new Bug

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 (13)

comment:1 Changed 12 months ago by jarshwah

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 12 months ago by jarshwah

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

comment:3 Changed 12 months ago by Vladimiroff

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 Changed 12 months ago by Vladimiroff

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 Changed 12 months ago by timgraham

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin
  • Type changed from Uncategorized to Bug

Ready for review by ORM experts.

comment:6 Changed 12 months ago by akaariai

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 11 months ago by Vladimiroff

@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 Changed 11 months ago by timgraham

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

Ready for another review.

comment:9 Changed 11 months ago by timgraham

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

See PR for comments.

comment:10 Changed 10 months ago by timgraham

  • Patch needs improvement unset

Patch received another update.

comment:11 Changed 8 months ago by timgraham

  • Patch needs improvement set

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

comment:12 Changed 7 months ago by akaariai

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

comment:13 Changed 7 months ago by timgraham

  • Version changed from 1.7-rc-2 to 1.7
Note: See TracTickets for help on using tickets.
Back to Top