Skip to content

Prevented unnecessary distinct() call in SearchFilter. #3938

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

Merged
merged 2 commits into from
Jun 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions rest_framework/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
from django.db import models
from django.db.models.constants import LOOKUP_SEP
from django.template import loader
from django.utils import six
from django.utils.translation import ugettext_lazy as _
Expand Down Expand Up @@ -132,6 +133,12 @@ class SearchFilter(BaseFilterBackend):
# The URL query parameter used for the search.
search_param = api_settings.SEARCH_PARAM
template = 'rest_framework/filters/search.html'
lookup_prefixes = {
'^': 'istartswith',
'=': 'iexact',
'@': 'search',
'$': 'iregex',
}

def get_search_terms(self, request):
"""
Expand All @@ -142,16 +149,31 @@ def get_search_terms(self, request):
return params.replace(',', ' ').split()

def construct_search(self, field_name):
if field_name.startswith('^'):
return "%s__istartswith" % field_name[1:]
elif field_name.startswith('='):
return "%s__iexact" % field_name[1:]
elif field_name.startswith('@'):
return "%s__search" % field_name[1:]
if field_name.startswith('$'):
return "%s__iregex" % field_name[1:]
lookup = self.lookup_prefixes.get(field_name[0])
if lookup:
field_name = field_name[1:]
else:
return "%s__icontains" % field_name
lookup = 'icontains'
return LOOKUP_SEP.join([field_name, lookup])

def must_call_distinct(self, opts, lookups):
"""
Return True if 'distinct()' should be used to query the given lookups.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a link here to the Django WONTFIX ticket, or documentation, or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's undergoing work to allow using subqueries to filter m2m but I'm unsure about which ticket you are referring to. This was mostly inspired by how the Django's admin handle a similar case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about which ticket you are referring to.

No, I'm not sure either - I thought I'd remembered a django ticket referring to this that closed as WONTFIX, but I don't see anything like that now.

"""
for lookup in lookups:
if lookup[0] in self.lookup_prefixes:
lookup = lookup[1:]
parts = lookup.split(LOOKUP_SEP)
for part in parts:
field = opts.get_field(part)
if hasattr(field, 'get_path_info'):
# This field is a relation, update opts to follow the relation
path_info = field.get_path_info()
opts = path_info[-1].to_opts
if any(path.m2m for path in path_info):
# This field is a m2m relation so we know we need to call distinct
return True
return False

def filter_queryset(self, request, queryset, view):
search_fields = getattr(view, 'search_fields', None)
Expand All @@ -173,10 +195,12 @@ def filter_queryset(self, request, queryset, view):
]
queryset = queryset.filter(reduce(operator.or_, queries))

# Filtering against a many-to-many field requires us to
# call queryset.distinct() in order to avoid duplicate items
# in the resulting queryset.
return distinct(queryset, base)
if self.must_call_distinct(queryset.model._meta, search_fields):
# Filtering against a many-to-many field requires us to
# call queryset.distinct() in order to avoid duplicate items
# in the resulting queryset.
queryset = distinct(queryset, base)
return queryset

def to_html(self, request, queryset, view):
if not getattr(view, 'search_fields', None):
Expand Down
15 changes: 15 additions & 0 deletions tests/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,21 @@ class SearchListView(generics.ListAPIView):
response = view(request)
self.assertEqual(len(response.data), 1)

def test_must_call_distinct(self):
filter_ = filters.SearchFilter()
prefixes = [''] + list(filter_.lookup_prefixes)
for prefix in prefixes:
self.assertFalse(
filter_.must_call_distinct(
SearchFilterModelM2M._meta, ["%stitle" % prefix]
)
)
self.assertTrue(
filter_.must_call_distinct(
SearchFilterModelM2M._meta, ["%stitle" % prefix, "%sattributes__label" % prefix]
)
)


class OrderingFilterModel(models.Model):
title = models.CharField(max_length=20)
Expand Down