Opened 3 years ago
Closed 14 months ago
#33651 closed New feature (fixed)
Support prefetch GenericForeignKey with custom queryset.
Reported by: | elonzh | Owned by: | Clément Escolano |
---|---|---|---|
Component: | contrib.contenttypes | Version: | 4.0 |
Severity: | Normal | Keywords: | GenericForeignKey |
Cc: | David Wobrock, Todor Velichkov, Rohan Nahata, joeli, Clément Escolano | 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
For example:
class Node(models.Model): content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) object_id = models.PositiveIntegerField() content_object = GenericForeignKey("content_type", "object_id") class AbstractNodeItem(models.Model): name = models.CharField(max_length=100) class ItemA(AbstractNodeItem): a_content = models.TextField() class ItemB(AbstractNodeItem): b_content = models.TextField()
Currently we can't list all node with only content_object.name
prefetched.
Change History (29)
comment:1 by , 3 years ago
comment:2 by , 3 years ago
Component: | Database layer (models, ORM) → contrib.contenttypes |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 3 years ago
Needs documentation: | set |
---|
comment:6 by , 3 years ago
Needs documentation: | unset |
---|
comment:7 by , 3 years ago
Needs documentation: | set |
---|
comment:8 by , 3 years ago
Needs documentation: | unset |
---|
comment:9 by , 2 years ago
Patch needs improvement: | set |
---|
comment:10 by , 2 years ago
Cc: | added |
---|
comment:11 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:12 by , 2 years ago
Patch needs improvement: | set |
---|
comment:13 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:14 by , 2 years ago
#24272 was a duplicate. One thing I noticed while reviewing it is that the signature of GenericPrefetch
would likely be ergonomic as GenericPrefetch(relation: str, querysets: list[QuerySet])
instead of querysets: dict[Model, QuerySet]
since the model can be inferred from queryset.model
anyway.
comment:15 by , 2 years ago
Cc: | added |
---|
comment:16 by , 2 years ago
Yes Simon. The change of querysets from dictionary to list is also suggested by Mariusz Felisiak. I have recently updated the PR with this change.
Thanks.
comment:18 by , 2 years ago
Cc: | added |
---|
follow-up: 20 comment:19 by , 2 years ago
Hi everyone, I took a brief look at the conversation in the pull request #15636 and I have a question about the issue being discussed there (passing list of querysets into an queryset argument).
Is there anything specific about this, that is enforcing the use of a list of querysets? Is it possible to send as many GenericPrefetch
objects as the number of querysets being required ? For example:
Node.objects.prefetch_related( GenericPrefetch("content_object", queryset=ItemA.objects.all()), GenericPrefetch("content_object", queryset=ItemB.objects.all()), )
this is in the spirit of the current Prefetch
class, which actually allows you to prefetch multiple levels of objects, splitted into different prefetch calls. i.e:
Book.objects.prefetch_related( Prefetch("author", queryset=Author.objects.all()), Prefetch("author__house", queryset=House.objects.all()), )
And now I'm a little bit repeating myself with ticket 24272, but this could probably open the door for reusing related_query_name
when there is a defined GenericRelation
which could give us the following interface w/o using the generic content_object
argument:
TaggedItem.objects.all().prefetch_related( GenericPrefetch('books', queryset=Book.objects.all().select_related('author')), GenericPrefetch('movies', queryset=Movie.objects.all().select_related('director')), )
comment:20 by , 2 years ago
Hi,
I think there is no way to add two lookups on same foreign key on same model as currently we populate the prefetch cache for this foreign key. Anyway, for a generic foreign key currently giving a queryset is not supported. To add this support I think the best way is to have a class that extends the prefetch class which handles all the custom logic of prefetching which is what we are working on in this issue.
To implement what you said i.e adding two Prefetch of content object in prefetch_related function. Some of the logic will need to be handled in the prefetch_related function which is not good.
Replying to Todor Velichkov:
Hi everyone, I took a brief look at the conversation in the pull request #15636 and I have a question about the issue being discussed there (passing list of querysets into an queryset argument).
Is there anything specific about this, that is enforcing the use of a list of querysets? Is it possible to send as many
GenericPrefetch
objects as the number of querysets being required ? For example:
Node.objects.prefetch_related( GenericPrefetch("content_object", queryset=ItemA.objects.all()), GenericPrefetch("content_object", queryset=ItemB.objects.all()), )this is in the spirit of the current
Prefetch
class, which actually allows you to prefetch multiple levels of objects, splitted into different prefetch calls. i.e:
Book.objects.prefetch_related( Prefetch("author", queryset=Author.objects.all()), Prefetch("author__house", queryset=House.objects.all()), )And now I'm a little bit repeating myself with ticket 24272, but this could probably open the door for reusing
related_query_name
when there is a definedGenericRelation
which could give us the following interface w/o using the genericcontent_object
argument:
TaggedItem.objects.all().prefetch_related( GenericPrefetch('books', queryset=Book.objects.all().select_related('author')), GenericPrefetch('movies', queryset=Movie.objects.all().select_related('director')), )
comment:21 by , 19 months ago
Cc: | added |
---|
comment:22 by , 16 months ago
Cc: | added |
---|
comment:24 by , 15 months ago
Patch needs improvement: | unset |
---|
comment:25 by , 15 months ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
comment:26 by , 15 months ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:27 by , 14 months ago
Patch needs improvement: | set |
---|
comment:28 by , 14 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
If I understand correctly you'd like to be able to have the prefetch querysets issued when doing
Node.objects.prefetch_related('content_object')
forItemA
andItemB
be.objects.only('name')
.I've had to implement such a feature in the past and ended up with writing my own
GenericForeignKeySubclass
that would allow for the followingIf we were to do a proper implementation here though I think that a
GenericPrefetch(Prefetch)
subclass would make for a better API as that's the current way custom querysets are provided to the prefetching logic. Something alongAccepting on the basis that I think this would be a valuable feature.