Skip to content
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

Use get_serializer_class in ordering filter #3487

Merged

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.


view = OrderingListView.as_view()
request = factory.get('/', {'ordering': 'text'})
# BUG: I think this should raise ImproperlyConfigured
Copy link
Collaborator

@xordoquy xordoquy Oct 9, 2015

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

@kmwenja kmwenja Oct 9, 2015

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
Collaborator

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
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
@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.

@tomchristie tomchristie modified the milestones: 3.3.4 Release, 3.4.0 Release May 26, 2016
@tomchristie
Copy link
Member

tomchristie commented May 26, 2016

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.

None yet

4 participants