GeeWee / django-auto-prefetching

Automatic prefetching for Django
MIT License
231 stars 18 forks source link

Why PrimaryKeyRelatedField is not in IGNORED_FIELD_TYPES? #56

Open FelipeSanchezCalzada opened 1 year ago

FelipeSanchezCalzada commented 1 year ago

I think PrimaryKeyRelatedField also doesn't need to apply prefetchin. The code of to_representation in PrimaryKeyRelatedField is:


    def to_representation(self, value):
        if self.pk_field is not None:
            return self.pk_field.to_representation(value.pk)
        return value.pk

The pk_field field must be a simple type (not related, i think) and it is also accessible directly in the main query without applying a join.

I'm having big overloads because the PrimaryKeyRelatedField add a lot of unnecessary joins.

What do you think about it?? I can send a PR. In case you don't want to add that change, maybe it would be good to make it configurable. By default it would stay as it is to avoid risking other projects that use the library

Thanks!

GeeWee commented 1 year ago

Hm, I thought PrimaryKeyRelatedField is for a relation that is keyed in with a primary key? I feel like representing that (or doing work with it) should trigger a database fetch?

FelipeSanchezCalzada commented 1 year ago

I think it would not be a good idea to add it directly to IGNORED_FIELD_TYPES directly. In 99% of cases it will work, but there are rare cases that it might fail.

I've been looking at it a bit more and I think the best idea is to make IGNORED_FIELD_TYPES configurable and leave it as the default:

IGNORED_FIELD_TYPES = (
    # This is a subclass of RelatedField, but it always generates a URL no matter the depth, so we shouldn't prefetch
    # based on it.
    HyperlinkedRelatedField
)

This is de 99% cases:

image

As you can see, since it is a PKOnlyObject object, no additional queries are needed, but this may not be the case in some more complex cases (1% or 0.1%).

So I'm going to put out a PR that simply supports customizing IGNORED_FIELD_TYPES from django settings.

GeeWee commented 1 year ago

That seems sensible to me.