Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically determine select_related and prefetch_related on ModelSerializer. #1964

Closed
cancan101 opened this issue Oct 16, 2014 · 16 comments
Closed

Comments

@cancan101
Copy link
Contributor

I have a serializer that looks the the following:

class ReportSerializer(serializers.ModelSerializer):
    person = serializers.SlugRelatedField(slug_field='uuid')

which is causing an N +1 select issue because select_related is not being used to pre-query the Person object. This means that in order to serialize the person field when serializing a Report, each Person must be queried for its uuid.

We likely want to use select_related on all SlugRelatedField and likely all nested serializers somewhere like here: https://github.com/tomchristie/django-rest-framework/blob/e437520217e20d500d641b95482d49484b1f24a7/rest_framework/serializers.py#L570

@tomchristie
Copy link
Member

It's actually been a conscious design decision to leave the select_related and prefetch_related down the end users.

Attempting to automatically add them would be helpful in many cases, but could also end up failing or giving worse performance in other cases. The other bit that's awkward is that we'd only want to add this to ModelSerializer (Serializer shouldn't have anything specific to Django models in it), which would end up introducing a further divide between what those two classes do, which we might or might not want.

Of course we'd also have to do the clevernesses of figuring out when to use prefetch_related and when to use select_related, so we'd be introducing a bunch of complexity.

Having said all that, might not be averse to building something like this in, in the future. Any thoughts from anyone else?

@tomchristie tomchristie changed the title select_related not being used -> many extra DB queries Automatically determine select_related and prefetch_related on ModelSerializer. Oct 17, 2014
@cancan101
Copy link
Contributor Author

A follow up question, would be "given that DRF does not currently do the
needed magic, how would I as a user specify to use select related?" I would
think that I would want to subclass and change the line of code I linked to
(ie subclass data property) or explicitly specify the queryset to the
serializer?

I feel at the very least more documentation is deserved. I was quite
surprised to see the large number of SQL queries occurring due to the slug
related usage.
On Oct 17, 2014 10:35 AM, "Tom Christie" notifications@github.com wrote:

It's actually been a conscious design decision to leave the select_related
and prefetch_related down the end users.

Attempting to automatically add them would be helpful in many cases, but
could also end up failing or giving worse performance in other cases. The
other bit that's awkward is that we'd only want to add this to
ModelSerializer (Serializer shouldn't have anything specific to Django
models in it), which would end up introducing a further divide between what
those two classes do, which we might or might not want.

Of course we'd also have to do the clevernesses of figuring out when to
use prefetch_related and when to use select_related, so we'd be
introducing a bunch of complexity.

Having said all that, might not be averse to building something like this
in, in the future. Any thoughts from anyone else?


Reply to this email directly or view it on GitHub
#1964 (comment)
.

@tomchristie
Copy link
Member

Assuming you're using generic views then specify it on the queryset attribute of view or viewset, eg...

queryset = Report.objects.all().select_related('person')

And yes, we should make to include some notes on that in the generic view section, plus also have a a section on general usage tips etc. Including:

  • Good API style.
  • querysets and select_related, prefetch_related.
  • DB indexes, particularly wrt filters, relationships, view lookups.
  • Model encapsulation.

@cancan101
Copy link
Contributor Author

Okay. That makes sense.

I am curious to hear what others think, but I would love to see some magic
here with automatically adding select related based on slug related usage.
On Oct 17, 2014 10:47 AM, "Tom Christie" notifications@github.com wrote:

Assuming you're using generic views then specify it on the queryset
attribute of view or viewset, eg...

queryset = Report.objects.all().select_related('person')

And yes, we should make to include some notes on that in the generic view
section, plus also have a a section on general usage tips etc. Including:

  • Good API style.
  • querysets and select_related, prefetch_related.
  • DB indexes, particularly wrt filters, relationships, view lookups.
  • Model encapsulation.


Reply to this email directly or view it on GitHub
#1964 (comment)
.

@tomchristie
Copy link
Member

"slug related usage" That's just one of many possibilities of course - nested serializers and other relationship types also benefit from this.

@tomchristie
Copy link
Member

At this point in time I'm closing this off in favor of the documentation ticket #1977.
Would I be happy to consider this at some point in the future? Sure, but it needs to start off life as a third party project. There's nothing to stop someone else from taking this on outside of core.

@Deepakdubey90
Copy link

Hi,
i'm trying to query with generic relation what should i use, either 'Select_related' or 'Prefetch_related'.
i'm having to two table user and contact.
i'm using contact table as generic(created generic relation from django view) .
so i'm making query like:
result = User.objects.filter().select_related('id')
in above queryset i'm passing "ID" to select_related . (ID is pk of "user" table and acting as Entity_object_id in "contact" table and Entity_content_type_id)
How can i get each user details with their contact details
for generic relation used link :https://docs.djangoproject.com/en/1.7/ref/contrib/contenttypes/#generic-relations.

@xordoquy
Copy link
Collaborator

@Deepakdubey90 please do use the mailing list, IRC or stack overflow for usage question.

@Deepakdubey90
Copy link

ok

@GeeWee
Copy link

GeeWee commented Apr 15, 2019

I'm working on this over at django-auto-prefetching

I'm not quite ready to publish to PyPi yet, but so far I have an algorithm that passes tests for all the relationships(fk, m2m, 1to1)
It recursively scans the fields of the serializer, fetching nested relations if needed.

If anyone wants to try it out before I'm done publishing (maybe find out where it doesn't work and submit an issue to the repo?) I'm sharing the first version of the code here:
https://gist.github.com/GeeWee/5f82fd11d5e9c95bfe61993c07624ccc

@GeeWee
Copy link

GeeWee commented Aug 9, 2019

First usable version of django-auto-prefetching is now live if anyone still has this problem

@thetarby
Copy link
Contributor

thetarby commented Aug 4, 2020

Hello everyone,

We faced the same issues in some projects and I have created a simple package to use in our projects. Then I have decided to publish it in github and pypi for anyone who is interested. Btw it is still being tested and there might be bugs. You can check it out on https://github.com/thetarby/django-auto-related

Simply what it does:

What django-rest gives us is the ability to know what will be accessed from the object before it is accessed by inspecting the source attribute of the fields defined in a serializer.

Then it traces those sources back on django fields instances and translates them to select_related, prefetch_related and only methods of django. Note that it does not use django-rest fields to decide what to prefetch but django fields because what django-rest says a related_field may not be a related field for the django or vice versa.

For instance, serializers.CharField(source=related_model.title) is not a related field for djangorest but in reality related_model should be prefetched to avoid n+1 problems.

If you have any feedback I would be glad to hear from you on github.

@ramwin
Copy link

ramwin commented Aug 5, 2020

First usable version of django-auto-prefetching is now live if anyone still has this problem

I'm using django-auto-prefeching in my company's project. It helps a lot. Thank you.

@GeeWee
Copy link

GeeWee commented Aug 5, 2020

You're very welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants
@cancan101 @tomchristie @xordoquy @Deepakdubey90 @GeeWee @ramwin @thetarby and others