Opened 9 years ago

Closed 16 months ago

Last modified 16 months ago

#24048 closed Bug (fixed)

only() doesn't override fields included in defer() as documented

Reported by: Will Earp Owned by: Ryan Cheley
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: defer, only, QuerySet, DeferredAttribute
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (11)

comment:1 by Tim Graham, 9 years ago

Summary: potential bug with defer() and only()only() doesn't override fields included in defer() as documented
Triage Stage: UnreviewedAccepted

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.

comment:2 by Will Earp, 9 years ago

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.

only()'s documentations suggests it will override defer(). However, add_immediate_loading() is to "respect existing referrals", according to its comments.

If the problem is to be fixed, is it best addressed in add_immediate_loading() or only()? I'm guessing it will be to change only() as it contradicts the docs?

Last edited 9 years ago by Will Earp (previous) (diff)

in reply to:  1 comment:3 by Will Earp, 9 years ago

Replying to 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.

comment:4 by Arnaud Limbourg, 8 years ago

The documentation was updated in the meantime, it seems this ticket can be closed.

comment:5 by Tim Graham, 8 years ago

What commit updated the documentation? The incorrect example quoted in the ticket description is still there as far as I see.

comment:6 by Ryan Cheley, 17 months ago

Owner: changed from nobody to Ryan Cheley
Status: newassigned

comment:7 by Ryan Cheley, 17 months ago

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

comment:8 by Ryan Cheley, 17 months ago

It does appear that there is a contradiction in the only() method doc string when compared to the 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.

comment:9 by Mariusz Felisiak, 16 months ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 16 months ago

Resolution: fixed
Status: assignedclosed

In 68bd8f4:

Fixed #24048 -- Corrected QuerySet.only() docs about interaction with defer().

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 16 months ago

In 2417056:

[4.1.x] Fixed #24048 -- Corrected QuerySet.only() docs about interaction with defer().

Backport of 68bd8f4cb4d14dccfb016bb15177506234f567fb from main

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