Opened 9 years ago
Closed 9 years ago
#25279 closed New feature (fixed)
Make prefetch_related_objects a public API
Reported by: | Adam Johnson | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.8 |
Severity: | Normal | Keywords: | |
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 (last modified by )
prefetch_related
is great, but it would be nice to be able to reuse its capabilities with lists of model instances already retrieved by other code (or from cache, or newly constructed, etc.). It seems this is possible by using django.db.models.query.prefetch_related_objects
which is the function implementing all the prefetch capability - I've just seen it used fine, however it is not part of Django's public API.
As far as I can tell, just doing an import
in django.db.models.__init__
and adding documentation should be enough to make it public, since prefetch_related_objects
contains all the functionality that prefetch_related
exposes.
Change History (10)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Has patch: | set |
---|
Oh #24975 is nice. It doesn't seem that hard to do - on Model would it not just require...
def prefetch_related(self, related_lookups): prefetch_related_objects([self], related_lookups)
Basic PR at https://github.com/django/django/pull/5138, with a documentation sketch. Agree that more tests would be required.
comment:3 by , 9 years ago
Needs tests: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 9 years ago
I've added some tests for the basics (one-to-one/many-to-many traversal plus Prefetch
objects) but don't want to go too far basically duplicating the tests that already exist for prefetch_related
. The aim is just to check it accepts the same arguments and thought is put in to changing it in the future.
I've also changed the function signature to prefetch_related_objects(list_of_objects, *lookups)
for the simpler invocation prefetch_related_objects(objs, 'lookup1', 'lookup2__and3')
, as opposed to prefetch_related_objects(objs, ['lookup1', 'lookup2__and3'])
. This is to defend against the possible bug from prefetch_related_objects(objs, 'lookup1')
which would iterate the characters in lookup1
leading to errors about attribute l
etc. not existing.
I'm guessing that although some existing projects may already be using prefetch_related_objects
, it's not Django's policy to worry about changing the signature of a (previously) private function? Or should I add type-checking like if len(args) == 1 and isinstance(args[0], (tuple, list))
to allow the old style?
comment:5 by , 9 years ago
Needs tests: | unset |
---|
comment:6 by , 9 years ago
Description: | modified (diff) |
---|
comment:7 by , 9 years ago
Keywords: | 1.9 added |
---|
Loic had some doubts about making prefetch_related_objects()
a public API. So, I'm awaiting his feedback here or on the mailing list before proceeding with this ticket.
comment:8 by , 9 years ago
Keywords: | 1.9 removed |
---|
Loic's concerns are, "I have nothing against exposing that functionality itself, I just wanted to review if it works as-is as an elegant and reliable public API, right now it's a utility function it wasn't designed with those concerns in mind."
Since it's mostly a documentation patch at this point, it doesn't seem critical for 1.9.
Seems related to #24975 which proposes to add the ability to call
prefetch_related()
on a model instance. I wonder if we could find a common solution to address both of these tickets.If we make
prefetch_related_objects()
public, I think some tests would also be needed to ensure that it isn't refactored so that it's signature changes.