From 512846e4289b21ce34f0c1cb45f6453a703780a4 Mon Sep 17 00:00:00 2001 From: Oliver Sauder Date: Mon, 3 Jun 2019 12:04:14 +0200 Subject: [PATCH] Remove not always supported auto `select_related` Polymorphic models do not always support `select_related` as of https://django-polymorphic.readthedocs.io/en/stable/advanced.html?highlight=select_related#about-queryset-methods and might actually be slower in some cases as explained in https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html?highlight=usage#performance-improvements Hence reverting change of AutoPreloadMixin as it has not been released yet. --- CHANGELOG.md | 2 -- example/tests/test_performance.py | 2 +- example/views.py | 7 ++----- rest_framework_json_api/views.py | 32 ++++++------------------------- 4 files changed, 9 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3a9c9f5..54f73ec4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,6 @@ any parts of the framework not mentioned in the documentation should generally b ### Changed * Allow to define `select_related` per include using [select_for_includes](https://django-rest-framework-json-api.readthedocs.io/en/stable/usage.html#performance-improvements) -* Reduce number of queries to calculate includes by using `select_related` when possible * Use REST framework serializer functionality to extract includes. This adds support like using dotted notations in source attribute in `ResourceRelatedField`. @@ -31,7 +30,6 @@ any parts of the framework not mentioned in the documentation should generally b ### Deprecated * Deprecate `PrefetchForIncludesHelperMixin` use `PreloadIncludesMixin` instead -* Deprecate `AutoPrefetchMixin` use `AutoPreloadMixin` instead ## [2.7.0] - 2019-01-14 diff --git a/example/tests/test_performance.py b/example/tests/test_performance.py index 45c71aeb..ac8b5956 100644 --- a/example/tests/test_performance.py +++ b/example/tests/test_performance.py @@ -53,7 +53,7 @@ def test_query_count_include_author(self): 4. Author types prefetched 5. Entries prefetched """ - with self.assertNumQueries(4): + with self.assertNumQueries(5): response = self.client.get('/comments?include=author&page[size]=25') self.assertEqual(len(response.data['results']), 25) diff --git a/example/views.py b/example/views.py index 06e4c2f3..4d363032 100644 --- a/example/views.py +++ b/example/views.py @@ -12,7 +12,7 @@ from rest_framework_json_api.filters import OrderingFilter, QueryParameterValidationFilter from rest_framework_json_api.pagination import JsonApiPageNumberPagination from rest_framework_json_api.utils import format_drf_errors -from rest_framework_json_api.views import ModelViewSet, PreloadIncludesMixin, RelationshipView +from rest_framework_json_api.views import ModelViewSet, RelationshipView from example.models import Author, Blog, Comment, Company, Entry, Project, ProjectType from example.serializers import ( @@ -200,12 +200,9 @@ def get_queryset(self, *args, **kwargs): return super(CommentViewSet, self).get_queryset() -class CompanyViewset(PreloadIncludesMixin, viewsets.ModelViewSet): +class CompanyViewset(ModelViewSet): queryset = Company.objects.all() serializer_class = CompanySerializer - prefetch_for_includes = { - 'current_project': ['current_project'], - } class ProjectViewset(ModelViewSet): diff --git a/rest_framework_json_api/views.py b/rest_framework_json_api/views.py index a7a125f9..48c4f43e 100644 --- a/rest_framework_json_api/views.py +++ b/rest_framework_json_api/views.py @@ -115,11 +115,11 @@ def get_queryset(self, *args, **kwargs): return qs -class AutoPreloadMixin(object): - +class AutoPrefetchMixin(object): def get_queryset(self, *args, **kwargs): """ This mixin adds automatic prefetching for OneToOne and ManyToMany fields. """ - qs = super(AutoPreloadMixin, self).get_queryset(*args, **kwargs) + qs = super(AutoPrefetchMixin, self).get_queryset(*args, **kwargs) + included_resources = get_included_resources(self.request) for included in included_resources + ['__all__']: @@ -127,8 +127,6 @@ def get_queryset(self, *args, **kwargs): included_model = None levels = included.split('.') level_model = qs.model - # Suppose we can do select_related by default - can_select_related = True for level in levels: if not hasattr(level_model, level): break @@ -144,12 +142,6 @@ def get_queryset(self, *args, **kwargs): if not (is_forward_relation or is_reverse_relation): break - # Figuring out if relation should be select related rather than prefetch_related - # If at least one relation in the chain is not "selectable" then use "prefetch" - can_select_related &= ( - issubclass(field_class, (ForwardManyToOneDescriptor, ReverseOneToOneDescriptor)) - ) - if level == levels[-1]: included_model = field else: @@ -165,23 +157,11 @@ def get_queryset(self, *args, **kwargs): level_model = model_field.model if included_model is not None: - if can_select_related: - qs = qs.select_related(included.replace('.', '__')) - else: - qs = qs.prefetch_related(included.replace('.', '__')) + qs = qs.prefetch_related(included.replace('.', '__')) return qs -class AutoPrefetchMixin(AutoPreloadMixin): - - def __init__(self, *args, **kwargs): - warnings.warn("AutoPrefetchMixin is deprecated. " - "Use AutoPreloadMixin instead", - DeprecationWarning) - super(AutoPrefetchMixin, self).__init__(*args, **kwargs) - - class RelatedMixin(object): """ This mixin handles all related entities, whose Serializers are declared in "related_serializers" @@ -259,14 +239,14 @@ def get_related_instance(self): raise NotFound -class ModelViewSet(AutoPreloadMixin, +class ModelViewSet(AutoPrefetchMixin, PreloadIncludesMixin, RelatedMixin, viewsets.ModelViewSet): pass -class ReadOnlyModelViewSet(AutoPreloadMixin, +class ReadOnlyModelViewSet(AutoPrefetchMixin, RelatedMixin, viewsets.ReadOnlyModelViewSet): pass