Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Next Next commit
Prevented unnecessary distinct() call in SearchFilter.
  • Loading branch information
charettes committed Feb 16, 2016
commit 1ada749a42488fc28bf9e5d5c75a0d1671796fa6
30 changes: 26 additions & 4 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 @@ -153,6 +154,25 @@ def construct_search(self, field_name):
else:
return "%s__icontains" % field_name

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

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
Contributor

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 {'^', '=', '@', '$'}:
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)
search_terms = self.get_search_terms(request)
Expand All @@ -173,10 +193,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 = ['', '^', '=', '@', '$']
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