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

Do select_related for ?include #591

Closed
Anton-Shutik opened this issue Mar 7, 2019 · 4 comments · Fixed by #600
Closed

Do select_related for ?include #591

Anton-Shutik opened this issue Mar 7, 2019 · 4 comments · Fixed by #600

Comments

@Anton-Shutik
Copy link
Contributor

I've found that we have nice prefetch_for_includes option on the ModelViewSet.
I would also like select_for_includes mixin that will call add select_related and pass there values accordingly to the mapping.
If you like the idea I'll create PR for that

@Anton-Shutik
Copy link
Contributor Author

Anton-Shutik commented Mar 7, 2019

We can have a separate Mixin for that or just add the logic to exiting one.

class SelectForIncludesMixin(object):

    def get_queryset(self):
        """
        This viewset provides a helper attribute to select related models
        based on the include specified in the URL.

        __all__ can be used to specify a select which should be done regardless of the include

        .. code:: python

            # When MyViewSet is called with ?include=author it will prefetch author and authorbio
            class MyViewSet(viewsets.ModelViewSet):
                queryset = Book.objects.all()
                select_for_includes = {
                    '__all__': [],
                    'author': ['author', 'author__authorbio'],
                    'category.section': ['category']
                }
        """
        qs = super(SelectForIncludesMixin, self).get_queryset()
        if not hasattr(self, 'select_for_includes'):
            return qs

        includes = self.request.GET.get('include', '').split(',')
        for inc in includes + ['__all__']:
            selects = self.select_for_includes.get(inc)
            if selects:
                qs = qs.select_related(*selects)

        return qs

@Anton-Shutik
Copy link
Contributor Author

@sliverc What do you think about it ?

@sliverc
Copy link
Member

sliverc commented Mar 12, 2019

In corner cases it might be helpful to have a SelectForIncludesMixin but a user should not get into the habit of using select_for_includes for normal cases so if we add such a mixin it should come with a fixed AutoPrefetchMixin which also automatically selects select_related.

I think having one mixin for select/prefetch for includes and one for auto prefetching would be a good idea.

I guess we need to think about renaming the mixins (deprecating old name) as it might be confusing otherwise. Suggestions are welcome.

Related to #337

@Anton-Shutik
Copy link
Contributor Author

@sliverc I've created PR for that. Would you mind taking a look ?
Thanks

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

Successfully merging a pull request may close this issue.

2 participants