Opened 4 years ago

Last modified 2 years ago

#24048 new Bug

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

Reported by: Will Earp Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: defer, only, QuerySet, DeferredAttribute
Cc: Triage Stage: Accepted
Has patch: no 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 (5)

comment:1 Changed 4 years ago by Tim Graham

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 Changed 4 years ago by Will Earp

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 4 years ago by Will Earp (previous) (diff)

comment:3 in reply to:  1 Changed 4 years ago by Will Earp

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 Changed 2 years ago by Arnaud Limbourg

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

comment:5 Changed 2 years ago by Tim Graham

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

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