Skip to content

OrderingFilter should call get_serializer_class() to determine default fields. #3964

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

Closed
wants to merge 1 commit into from
Closed
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
5 changes: 3 additions & 2 deletions rest_framework/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,8 +227,9 @@ def get_valid_fields(self, queryset, view):

if valid_fields is None:
# Default to allowing filtering on serializer fields
serializer_class = getattr(view, 'serializer_class')
if serializer_class is None:
try:
serializer_class = view.get_serializer_class()
except AssertionError:
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to use except (AssertionError, AttributeError) here, in case get_serializer_class does not exist on the view (eg using basic APIView, rather than the generic classes)

msg = ("Cannot use %s on a view which does not have either a "
"'serializer_class' or 'ordering_fields' attribute.")
raise ImproperlyConfigured(msg % self.__class__.__name__)
Expand Down
48 changes: 48 additions & 0 deletions tests/test_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from decimal import Decimal

from django.conf.urls import url
from django.core.exceptions import ImproperlyConfigured
from django.core.urlresolvers import reverse
from django.db import models
from django.test import TestCase
Expand Down Expand Up @@ -585,6 +586,53 @@ def setUp(self):
)
OrderingFilterModel(title=title, text=text).save()

def test_get_valid_fields_from_explicit_serializer_class(self):
class OrderingListView(generics.ListAPIView):
queryset = OrderingFilterModel.objects.all()
serializer_class = OrderingFilterSerializer
filter_backends = (filters.OrderingFilter, )
ordering_fields = None

queryset = OrderingFilterModel.objects.all()
view = OrderingListView()
backend = filters.OrderingFilter()
valid_fields = backend.get_valid_fields(queryset, view)
expected_valid_fields = [(field.source or field_name, field.label)
for (field_name, field) in OrderingFilterSerializer().fields.items()
if not getattr(field, 'write_only', False) and not field.source == '*']
self.assertEqual(valid_fields, expected_valid_fields)

def test_get_valid_fields_from_explicit_get_serializer_class(self):
class OrderingListView(generics.ListAPIView):
queryset = OrderingFilterModel.objects.all()
filter_backends = (filters.OrderingFilter,)
ordering_fields = None

def get_serializer_class(self):
return OrderingFilterSerializer

queryset = OrderingFilterModel.objects.all()
view = OrderingListView()
backend = filters.OrderingFilter()
valid_fields = backend.get_valid_fields(queryset, view)
expected_valid_fields = [(field.source or field_name, field.label)
for (field_name, field) in OrderingFilterSerializer().fields.items()
if not getattr(field, 'write_only', False) and not field.source == '*']
self.assertEqual(valid_fields, expected_valid_fields)

def test_improperly_configured_error_from_get_valid_fields(self):
class OrderingListView(generics.ListAPIView):
queryset = OrderingFilterModel.objects.all()
filter_backends = (filters.OrderingFilter,)
ordering_fields = None
serializer_class = None

queryset = OrderingFilterModel.objects.all()
view = OrderingListView()
backend = filters.OrderingFilter()
with self.assertRaises(ImproperlyConfigured):
backend.get_valid_fields(queryset, view)

def test_ordering(self):
class OrderingListView(generics.ListAPIView):
queryset = OrderingFilterModel.objects.all()
Expand Down