Skip to content

Conversation

@kmwenja
Copy link
Contributor

@kmwenja kmwenja commented Oct 9, 2015

OrderingFilter backend checks whether a view specifies an
ordering_fields or serializer_class attribute. Views can however
override get_serializer_class to determine the serializer class
but the OrderingFilter doesn't catch this. This patch fixes that.

Here's an example of a view that would benefit from this patch:

class MyView(generics.ListAPIView):
    queryset = MyModel.objects.all()
    filter_backends = (filters.OrderingFilter, )

    def get_serializer_class(self):
        return MySerializer

This would formerly raise an ImproperlyConfigured error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why it doesn't ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xordoquy Sorry, I realized that the get_serializer_class method actually has its own assert to check for serializer_class: https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/generics.py#L123.

On that note, is it better to get the error from get_serializer_class (AssertionError) or from remove_invalid_fields (ImproperlyConfigured)? I can rewrite the patch to either catch the AssertionError in get_serializer_class or let it pass right up.

@xordoquy xordoquy added this to the 3.3.0 Release milestone Oct 9, 2015
@kmwenja
Copy link
Contributor Author

kmwenja commented Oct 9, 2015

@xordoquy I preferred raising ImproperlyConfigured (since that was the initial intended purpose) but I can quickly change that.

@kmwenja
Copy link
Contributor Author

kmwenja commented Oct 9, 2015

@xordoquy would it be presumptuous to update the docs at this point?

@xordoquy
Copy link
Contributor

xordoquy commented Oct 9, 2015

We'll take some time to review it first.

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
kmwenja added 3 commits May 26, 2016 07:59
OrderingFilter backend checks whether a view specifies an
ordering fields or serializer_class attribute. Views can however
override get_serializer_class to determine the serializer_class
but the OrderingFilter doesn't catch this. This patch fixes that.
just checking that the ImproperlyConfigured error is raised if
ordering_fields, serializer_class and get_serializer_class are
not provided in the view.
get_serializer_class raises an AssertionError if no serializer_class
is found (either as a class attribute or from an overriding method).
This commit catches it and raises ImproperlyConfigured instead.
@kmwenja kmwenja force-pushed the use-get-serializer-class-in-ordering-filter branch from c655de2 to bf498b2 Compare May 26, 2016 05:38
@codecov-io
Copy link

codecov-io commented May 26, 2016

Current coverage is 91.43%

Merging #3487 into master will increase coverage by <.01%

@@             master   #3487   diff @@
=======================================
  Files            51      49     -2   
  Lines          5476    5356   -120   
  Methods           0       0          
  Branches          0       0          
=======================================
- Hits           5005    4897   -108   
+ Misses          471     459    -12   
  Partials          0       0          

Powered by Codecov. Last updated by 35ace2e...f341e14

@kmwenja
Copy link
Contributor Author

kmwenja commented May 26, 2016

@xordoquy Updated the PR against master. Apologies for the lack of love.

@lovelydinosaur lovelydinosaur modified the milestones: 3.3.4 Release, 3.4.0 Release May 26, 2016
@lovelydinosaur
Copy link
Contributor

Perfectly reasonable, yup. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants