[Django] #24048: potential bug with defer() and only()
Django
-------------------------------------+-------------------------------------
Reporter: wearp | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) | Keywords: defer, only, QuerySet,
Severity: Normal | DeferredAttribute
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
The docs give the following example for the `QuerySet` API's `only`:
{{{
# Final result loads headline and body immediately (only() replaces any
# existing set of fields).
Entry.objects.defer("body").only("headline", "body")
}}}
https://docs.djangoproject.com/en/dev/ref/models/querysets/#django.db.models.query.QuerySet.only
I understand this to mean that "body" will be loaded i.e. not a deferred
field. When testing this, however, "body" is still considered a
`DeferredAttribute`. Is this a possible bug or does the documentation
need changing?
--
Ticket URL: <https://code.djangoproject.com/ticket/24048>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: wearp | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
I checked back to 29050ef999e1931efb6c62471c7e07d2ea5e96ea where `defer()`
was added and this assertion in `tests/defer` (or its equivalent in the
original doc test) fails,
`self.assert_delayed(qs.defer("name").only("value", "name")[0], 1)` as
"name" is indeed deferred despite its inclusion in `only()`. It seems
expected to me that `only()` would override `defer()` so we should see
about making that change, but if we can't due to backwards compatibility
or implementation, we should definitely correct the docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:1>
Django
Reporter: wearp | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by wearp):
It looks as if the `only()`'s "problem" is caused by
`add_immediate_loading()`. Taking the example above,
`field_names.difference(existing)` in the method evaluates to
`set(["value", "name"]).difference(set["name"])`.
Cut a story short, `self.deferred_loading` is `set(["value"]), False` when
it should be `set(["name", "value"]), False`.
If the problem is to be fixed, is it best addressed in
`add_immediate_loading()` or `only()`?
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:2>
Django
Reporter: wearp | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by wearp):
Replying to [comment:1 timgraham]:
> I checked back to 29050ef999e1931efb6c62471c7e07d2ea5e96ea where
`defer()` was added and this assertion in `tests/defer` (or its equivalent
in the original doc test) fails,
`self.assert_delayed(qs.defer("name").only("value", "name")[0], 1)` as
"name" is indeed deferred despite its inclusion in `only()`. It seems
expected to me that `only()` would override `defer()` so we should see
about making that change, but if we can't due to backwards compatibility
or implementation, we should definitely correct the docs.
Looking at the comment for `only()` it says,
`Only the fields passed into this method and that are not already
specified as deferred are loaded immediately when the queryset is
evaluated.`
This would mean that the method behaves as intended and the documentation
is incorrect. `add_immediate_loading()` (which is called by `only()`) also
behaves in the same way, not overriding deferred fields passed to it.
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:3>
Django
Reporter: wearp | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by arnaudlimbourg):
The documentation was updated in the meantime, it seems this ticket can be
closed.
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:4>
Django
Reporter: wearp | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
What commit updated the documentation? The incorrect example quoted in the
ticket description is still there as far as I see.
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:5>
Django
-------------------------------------+-------------------------------------
| Cheley
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Ryan Cheley
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Will Earp | Owner: Ryan
| Cheley
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Cheley):
In ticket:24048#comment:4 refers to the doc string for the docstring for
the `only()` method being updated, not the documentation.
The documentation referenced above is still in the docs. I'll be reviewing
that shortly
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Will Earp | Owner: Ryan
| Cheley
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: defer, only, | Triage Stage: Accepted
QuerySet, DeferredAttribute |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Cheley):
It does appear that there is a contradiction in the `only()` method doc
string when compared to the
[https://docs.djangoproject.com/en/dev/ref/models/querysets/#django.db.models.query.QuerySet.only|
documentation ]
Since the doc string for the method is correct AND making changes to the
way that the `defer()` and `only()` function **now** may break things for
code bases that are already in production, I believe that the
documentation should be updated to clearly state what is happening, i.e.
that the use of `defer()` with `only()` will result in `defer()`
overriding `only()` for fields that are listed in both.
I'm still working out how to express that though. Once done, I'll submit a
PR to update the docs and reference this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Will Earp | Owner: Ryan
| Cheley
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
QuerySet, DeferredAttribute | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/16338 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Will Earp | Owner: Ryan
| Cheley
Component: Database layer | Version: dev
(models, ORM) |
Keywords: defer, only, | Triage Stage: Ready for
QuerySet, DeferredAttribute | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"68bd8f4cb4d14dccfb016bb15177506234f567fb" 68bd8f4]:
{{{
#!CommitTicketReference repository=""
revision="68bd8f4cb4d14dccfb016bb15177506234f567fb"
Fixed #24048 -- Corrected QuerySet.only() docs about interaction with
defer().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Will Earp | Owner: Ryan
| Cheley
Type: Bug | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: defer, only, | Triage Stage: Ready for
QuerySet, DeferredAttribute | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"24170562d44df9db86788eaf96e32b7aaf73df1e" 2417056]:
{{{
#!CommitTicketReference repository=""
revision="24170562d44df9db86788eaf96e32b7aaf73df1e"
[4.1.x] Fixed #24048 -- Corrected QuerySet.only() docs about interaction
with defer().
Backport of 68bd8f4cb4d14dccfb016bb15177506234f567fb from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24048#comment:11>