[Django] #28549: Can't defer() fields from super- and sub-class at the same time
Django
-----------------------------------------+------------------------
Reporter: Jeremy Kerr | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Using the models:
{{{#!python
from django.db import models
class Base(models.Model):
f1 = models.CharField(max_length=10)
class Sub(Base):
f2 = models.CharField(max_length=10)
}}}
it seems that I can't `defer()` both `f1` and `f2` in a single query:
{{{
>>> Sub.objects.defer('f1', 'f2').query.sql_with_params()[0]
u'SELECT "defer_base"."id", "defer_base"."f1", "defer_sub"."base_ptr_id"
FROM "defer_sub" INNER JOIN "defer_base" ON ("defer_sub"."base_ptr_id" =
"defer_base"."id")'
}}}
(we're seeing `f1` in the SELECT value list).
I seem to be able to defer `f1` or `f2` separately though.
I'm no django hacker, but: it seems as though
`django.db.models.sql.query.Query.deferred_to_data()` is iterating both
models in the loop:
{{{#!python
#640:
for model, values in six.iteritems(seen):
for field in model._meta.fields:
if field in values:
continue
}}}
^^- and so is discovering `f1` twice: once as `Base.f1` and again as
`Sub.f1`. Since the `field in values` test only skips `Base.f1`, we're
still left with `Sub.f1` in the `workset` dict.
--
Ticket URL: <https://code.djangoproject.com/ticket/28549>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jeremy Kerr | Owner: nobody
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jeremy Kerr):
* cc: Jeremy Kerr (added)
* component: Uncategorized => Database layer (models, ORM)
* type: Uncategorized => Bug
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:1>
Django
Reporter: Jeremy Kerr | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* version: 1.11 => master
* stage: Unreviewed => Accepted
Comment:
I think your intuition is right and that the bug lies in
`deferred_to_data()`. Could you try replacing `for field in
model._meta.fields` by `for field in model._meta.local_fields` and see if
it helps?
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:2>
Django
Reporter: Jeremy Kerr | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jeremy Kerr):
Replying to [comment:2 Simon Charette]:
> I think your intuition is right and that the bug lies in
`deferred_to_data()`. Could you try replacing `for field in
model._meta.fields` by `for field in model._meta.local_fields` and see if
it helps?
Yep, that seems to fix the issue:
{{{
>>> Sub.objects.defer('f1', 'f2').query.sql_with_params()[0]
u'SELECT "defer_base"."id", "defer_sub"."base_ptr_id" FROM "defer_sub"
INNER JOIN "defer_base" ON ("defer_sub"."base_ptr_id" =
"defer_base"."id")'
}}}
I was concerned that using `local_fields` would omit inherited fields from
the query, but looks like that's not the case; inherited (but not
deferred) fields work fine with that change:
{{{#!python
class Base(models.Model):
f1 = models.CharField(max_length=10)
f3 = models.CharField(max_length=10)
class Sub(Base):
f2 = models.CharField(max_length=10)
}}}
{{{
>>> Sub.objects.defer('f1', 'f2').query.sql_with_params()[0]
u'SELECT "defer_base"."id", "defer_base"."f3", "defer_sub"."base_ptr_id"
FROM "defer_sub" INNER JOIN "defer_base" ON ("defer_sub"."base_ptr_id" =
"defer_base"."id")'
}}}
^^- correctly includes `f3` in the query. Behaviour also look to remain
correct when only deferring fields from one class too.
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:3>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jeremy Kerr):
* status: new => assigned
* owner: nobody => Jeremy Kerr
Comment:
Patch coming.
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:4>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jeremy Kerr):
* has_patch: 0 => 1
Comment:
Pull request sent: [https://github.com/django/django/pull/8994 PR]
Since this is a trivial patch (and my first), I've not added to `AUTHORS`
or submitted a CLA. Let me know if I need to do either of those.
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:5>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
Comment:
Awesome, thank you for submitting a PR and adding a test!
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:6>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"84b7cb7df00192b2f804e2c6fd98b78b5bfd1ffa" 84b7cb7d]:
{{{
#!CommitTicketReference repository=""
revision="84b7cb7df00192b2f804e2c6fd98b78b5bfd1ffa"
Fixed #28549 -- Fixed QuerySet.defer() with super and subclass fields.
Previously, deferring fields in different classes didn't omit the
superclass' deferred field.
Thanks Simon Charette for the suggested fix.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:7>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Lamb):
Curious why this didn't reach into the 1.11.6 bugfix release.? We are
carrying this patch in Debian after a user request:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=876816
(It was the cause of performance regressions in his application so seemed
sufficiently important to carry)
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:8>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Chris, probably because it was never reported as a regression in the first
place. Can you confirm this was working in a previous version of Django?
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:9>
Django
Reporter: Jeremy Kerr | Owner: Jeremy
| Kerr
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Lamb):
Replying to [comment:9 Simon Charette]:
> Chris, probably because it was never reported as a regression in the
first place. Can you confirm this was working in a previous version of
Django?
Yep. If I understand your question correctly, from:
https://bugs.debian.org/876816#5:
> This bug caused significant performance degradation when we upgraded a
> Django [1.x] application to a new version that relied on model
inheritance.
--
Ticket URL: <https://code.djangoproject.com/ticket/28549#comment:10>