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

Ignore any invalidly formed query parameters for OrderingFilter. #5131

Merged
merged 3 commits into from May 15, 2017

Conversation

vimarshc
Copy link
Contributor

@vimarshc vimarshc commented May 11, 2017

Closes #4989.

For using ordering filters, parameters are added to the URL in the following manner and the output is the listed in the increasing order of the passed parameter.

http://example.com/api/users?ordering=username

For decreasing order, the following URL is used:

http://example.com/api/users?ordering=-username

If multiple hyphens are added, it should be treated as an invalid ordering parameter and should be discarded. The failing testcase's error traceback is as follows:

 tests/test_filters.py:778: in test_incorrecturl_ordering
    response = view(request)
../venv-test/lib/python2.7/site-packages/django/views/decorators/csrf.py:58: in wrapped_view
    return view_func(*args, **kwargs)
../venv-test/lib/python2.7/site-packages/django/views/generic/base.py:68: in view
    return self.dispatch(request, *args, **kwargs)
rest_framework/views.py:489: in dispatch
    response = self.handle_exception(exc)
rest_framework/views.py:449: in handle_exception
    self.raise_uncaught_exception(exc)
rest_framework/views.py:486: in dispatch
    response = handler(request, *args, **kwargs)
rest_framework/generics.py:201: in get
    return self.list(request, *args, **kwargs)
rest_framework/mixins.py:40: in list
    queryset = self.filter_queryset(self.get_queryset())
rest_framework/generics.py:152: in filter_queryset
    queryset = backend().filter_queryset(self.request, queryset, self)
rest_framework/filters.py:277: in filter_queryset
    return queryset.order_by(*ordering)
../venv-test/lib/python2.7/site-packages/django/db/models/query.py:950: in order_by
    obj.query.add_ordering(*field_names)
../venv-test/lib/python2.7/site-packages/django/db/models/sql/query.py:1691: in add_ordering
    raise FieldError('Invalid order_by arguments: %s' % errors)
E   FieldError: Invalid order_by arguments: [u'--text']


@xordoquy
Copy link
Collaborator

xordoquy commented May 11, 2017

Nice work, thank you.
Willing to make the fix ?

@vimarshc
Copy link
Contributor Author

vimarshc commented May 11, 2017

Yea! Definitely.
I shall try to follow the style that is being followed in the codebase but are there any specific things that I should keep in mind?

@tomchristie
Copy link
Member

tomchristie commented May 11, 2017

Nice work! Thanks for progressing this.

Yup, discarding any filter parameter that starts with '--' seems like a fair strategy here.

@vimarshc
Copy link
Contributor Author

vimarshc commented May 15, 2017

Hey,

The approach that I have used here is as follows:

Every parameter that is passed for ordering to a queryset, django performs a check, matching it against a regex. The regular expression has been defined as ORDER_PATTERN in django.db.models.sql.constants.

This constant has been defined in the same file since django 1.6.

The regular expression is as follows:
ORDER_PATTERN = re.compile(r'\?|[-+]?[.\w]+$')

I'm using this regular expression to perform a check before it is passed to the queryset. If the parameter does not match the expression that parameter is discarded.

@tomchristie
Copy link
Member

tomchristie commented May 15, 2017

Nice one! 👍

@tomchristie tomchristie added this to the 3.6.4 Release milestone May 15, 2017
@tomchristie tomchristie changed the title #4989 - Added Failing Testcase. Ignore any invalidly formed query parameters for OrderingFilter. May 15, 2017
@tomchristie tomchristie merged commit 003c304 into encode:master May 15, 2017
1 check passed
@xordoquy
Copy link
Collaborator

xordoquy commented May 15, 2017

Nice job !

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

Successfully merging this pull request may close these issues.

None yet

3 participants