Skip to content

ordering on fields which use field.source #3398

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

Conversation

sehmaschine
Copy link

see #3390

Here's what I suggest:
b) if ordering_fields is __all__, keep the current behaviour (ordering on all model fields). See Notes below.
c) if ordering_fields is None, allow ordering on all serializer fields (allow field names and use field.source for the actual ordering).
d) if ordering_fields are explicitely defined, allow ordering on defined fields (allow field names and use field.source for the actual ordering).

Additional changes:
a) use serializer_class OR get_serializer_class (please note that I didn't change the error message).
b) distinguish between ordering_fields and valid_fields.
c) remove serializer method fields from valid_fields.

Notes:
a) I'm not sure about __all__ versus None, because ordering on model fields seems strange (since they are not necessarily part of the serializer). Howerver, I didn't want to change the current behaviour here.

# Default to allowing filtering on serializer fields
serializer_class = getattr(view, 'serializer_class')
if not ordering_fields == '__all__':
serializer_class = getattr(view, 'serializer_class') or view.get_serializer_class()
Copy link
Member

Choose a reason for hiding this comment

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

getattr will raise an exception (an AttributeError, usually) when the requested attribute is not found on the object. in this case, you are probably looking for something like

serializer_class = view.get_serializer_class() if hasattr(view, 'get_serializer_class) else view.serializer_class

or the rough equivalent of

serializer_class = getattr(view, 'serializer_class', None)

if serializer_class is None:
    serializer_class = view.get_serializer_class()

Then again, it may just be better to prefer view.get_serializer_class() instead of relying on the serializer_class attribute.

@kevin-brown
Copy link
Member

Note that there are two failing tests, one of which I've addressed in my in-line comment.

@sehmaschine
Copy link
Author

@kevin-brown thanks for checking that. unfortunately, I'm not able to run the DRF tests or provide additional tests for the new behaviour.

@tomchristie
Copy link
Member

Appreciate the feedback & work on this, but closing as per #3390.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants