Opened 3 years ago

Closed 15 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 Simon Charette, 3 years ago

If I understand correctly you'd like to be able to have the prefetch querysets issued when doing Node.objects.prefetch_related('content_object') for ItemA and ItemB 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 following

class Node(models.Model):
    content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE)
    object_id = models.PositiveIntegerField()
    content_object = PrefetchedManagerGenericForeignKey("name_only", "content_type", "object_id")

class AbstractNodeItem(models.Model):
    name = models.CharField(max_length=100)

class NameOnlyManager(models.Manager):
    def get_queryset(self):
        return super().only("name")

class ItemA(AbstractNodeItem):
    a_content = models.TextField()
    name_only = NameOnlyManager()

class ItemB(AbstractNodeItem):
    b_content = models.TextField()
    name_only = NameOnlyManager()

If 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 along

Node.objects.prefetch_related(
    GenericPrefetch(
        "content_object", querysets={
            ItemA: ItemA.objects.only("a"),
            ItemB: ItemB.objects.only("a"),
        }
    )
)

Accepting on the basis that I think this would be a valuable feature.

comment:2 by Simon Charette, 3 years ago

Component: Database layer (models, ORM)contrib.contenttypes
Triage Stage: UnreviewedAccepted

comment:3 by Gullapalli Saisurya Revanth, 3 years ago

Owner: changed from nobody to Gullapalli Saisurya Revanth
Status: newassigned

comment:4 by Gullapalli Saisurya Revanth, 3 years ago

Has patch: set

PR -> #15636

Version 0, edited 3 years ago by Gullapalli Saisurya Revanth (next)

comment:5 by Mariusz Felisiak, 3 years ago

Needs documentation: set

comment:6 by Gullapalli Saisurya Revanth, 3 years ago

Needs documentation: unset

comment:7 by Mariusz Felisiak, 3 years ago

Needs documentation: set

comment:8 by Gullapalli Saisurya Revanth, 3 years ago

Needs documentation: unset

comment:9 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:10 by David Wobrock, 2 years ago

Cc: David Wobrock added

comment:11 by Gullapalli Saisurya Revanth, 2 years ago

Patch needs improvement: unset

comment:12 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:13 by Gullapalli Saisurya Revanth, 2 years ago

Patch needs improvement: unset

comment:14 by Simon Charette, 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 Todor Velichkov, 2 years ago

Cc: Todor Velichkov added

comment:16 by Gullapalli Saisurya Revanth, 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.

Last edited 2 years ago by Gullapalli Saisurya Revanth (previous) (diff)

comment:17 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

Per Simon's comment.

comment:18 by Rohan Nahata, 2 years ago

Cc: Rohan Nahata added

comment:19 by Todor Velichkov, 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')),
)

in reply to:  19 comment:20 by revanthgss, 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 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:21 by joeli, 20 months ago

Cc: joeli added

comment:22 by Clément Escolano, 17 months ago

Cc: Clément Escolano added

comment:23 by Clément Escolano, 17 months ago

Owner: changed from Gullapalli Saisurya Revanth to Clément Escolano

comment:24 by David Smith, 16 months ago

Patch needs improvement: unset

comment:25 by Mariusz Felisiak, 16 months ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:26 by Clément Escolano, 15 months ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:27 by Mariusz Felisiak, 15 months ago

Patch needs improvement: set

comment:28 by Mariusz Felisiak, 15 months ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:29 by Mariusz Felisiak <felisiak.mariusz@…>, 15 months ago

Resolution: fixed
Status: assignedclosed

In cac94dd8:

Fixed #33651 -- Added support for prefetching GenericForeignKey.

Co-authored-by: revanthgss <revanthgss@…>
Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>

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