-
Notifications
You must be signed in to change notification settings - Fork 301
WIP: Filter Backends #455
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
WIP: Filter Backends #455
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
-e . | ||
django>=2.0 | ||
django-debug-toolbar | ||
django-polymorphic>=2.0 | ||
django-filter>=2.0 | ||
factory-boy | ||
Faker | ||
isort | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
from django.db.models.sql.constants import ORDER_PATTERN | ||
from django_filters.rest_framework import DjangoFilterBackend | ||
from rest_framework.exceptions import ValidationError | ||
from rest_framework.filters import (BaseFilterBackend, OrderingFilter, | ||
SearchFilter) | ||
from rest_framework.settings import api_settings | ||
|
||
|
||
class JsonApiFilterMixin(object): | ||
""" | ||
class to share data among filtering backends | ||
""" | ||
jsonapi_query_keywords = ('sort', 'filter', 'fields', 'page', 'include') | ||
search_param = api_settings.SEARCH_PARAM | ||
ordering_param = 'sort' | ||
|
||
def __init__(self): | ||
self.filter_keys = [] | ||
|
||
|
||
class JsonApiQueryValidationFilter(JsonApiFilterMixin, BaseFilterBackend): | ||
""" | ||
A backend filter that validates query parameters for jsonapi spec conformance and raises a 400 error | ||
rather than silently ignoring unknown parameters or incorrect usage. | ||
|
||
set `allow_duplicate_filters = True` if you are OK with the same filter being repeated. | ||
|
||
TODO: For jsonapi error object compliance, must set jsonapi errors "parameter" for the ValidationError. | ||
This requires extending DRF/DJA Exceptions. | ||
""" | ||
allow_duplicated_filters = False | ||
|
||
def validate_query_params(self, request): | ||
""" | ||
Validate that query params are in the list of valid `jsonapi_query_keywords` | ||
Raises ValidationError if not. | ||
""" | ||
for qp in request.query_params.keys(): | ||
bracket = qp.find('[') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a regex would be better here such as: match = re.match(r'filter\[(.*?)\].*', qp) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to care about performance as this code executes for every request. Also the match would be something more complicated since it's searching for all keywords, whether or not they have brackets (e.g. |
||
if bracket >= 0: | ||
if qp[-1] != ']': | ||
raise ValidationError( | ||
'invalid query parameter: {}'.format(qp)) | ||
keyword = qp[:bracket] | ||
else: | ||
keyword = qp | ||
if keyword not in self.jsonapi_query_keywords: | ||
raise ValidationError( | ||
'invalid query parameter: {}'.format(keyword)) | ||
if not self.allow_duplicated_filters and len( | ||
request.query_params.getlist(qp)) > 1: | ||
raise ValidationError( | ||
'repeated query parameter not allowed: {}'.format(qp)) | ||
|
||
def filter_queryset(self, request, queryset, view): | ||
self.validate_query_params(request) | ||
return queryset | ||
|
||
|
||
class JsonApiOrderingFilter(JsonApiFilterMixin, OrderingFilter): | ||
""" | ||
The standard rest_framework.filters.OrderingFilter works mostly fine as is, but with .ordering_param = 'sort'. | ||
|
||
This implements http://jsonapi.org/format/#fetching-sorting and raises 400 if any sort field is invalid. | ||
""" | ||
ignore_bad_sort_fields = False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't provide such a option as I think DJA should simply follow the specification. If someone doesn't want to raise an error when sort field is invalid they can simply use the OrderingFilter of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Serves me right for trying to anticipate your review;-) I originally left it out:-) But see below... |
||
|
||
def remove_invalid_fields(self, queryset, fields, view, request): | ||
""" | ||
override remove_invalid_fields to raise a 400 exception instead of silently removing them. | ||
""" | ||
valid_fields = [item[0] for item in self.get_valid_fields(queryset, view, {'request': request})] | ||
bad_terms = [term for term in fields if term.lstrip('-') not in valid_fields and ORDER_PATTERN.match(term)] | ||
if bad_terms and not self.ignore_bad_sort_fields: | ||
raise ValidationError( | ||
'invalid sort parameter{}: {}'.format(('s' if len(bad_terms) > 1 else ''), ','.join(bad_terms))) | ||
return super(JsonApiOrderingFilter, self).remove_invalid_fields(queryset, fields, view, request) | ||
|
||
|
||
class JsonApiSearchFilter(JsonApiFilterMixin, SearchFilter): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you think it is better not to provide specific json api classes where there is no need? In this case we could simply provide the settings how to configure I would call the search param There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we just include Regarding the name of the filter, I like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of think Even though everyone can define the name as they like if we add it to DJA we have to come up with a recommendation. But this doesn't seem to be easy as the json api is silent on it and we are just two users and have two different opinions already 😄 While thinking it seems that adding a Also in DJA we don't have to reimplement all features which are in DRF but only implement what is different according to the json api specification. For this reasons I think we shouldn't provide a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's call it I agree with your reasoning for calling it something that doesn't imply every field. How about I'm OK with documenting to just use I'll drop the code and document example usage of SearchFilter. |
||
""" | ||
The (multi-field) rest_framework.filters.SearchFilter works just fine as is, but with a | ||
defined `filter[NAME]` such as `filter[all]` or `filter[_all_]` or something like that. | ||
|
||
This is not part of the jsonapi standard per-se, other than the requirement to use the `filter` | ||
keyword: This is an optional implementation of a style of filtering in which a single filter | ||
can implement a keyword search across multiple fields of a model as implemented by SearchFilter. | ||
""" | ||
pass | ||
|
||
|
||
class JsonApiFilterFilter(JsonApiFilterMixin, DjangoFilterBackend): | ||
""" | ||
Overrides django_filters.rest_framework.DjangoFilterBackend to use `filter[field]` query parameter. | ||
|
||
This is not part of the jsonapi standard per-se, other than the requirement to use the `filter` | ||
keyword: This is an optional implementation of style of filtering in which each filter is an ORM | ||
expression as implemented by DjangoFilterBackend and seems to be in alignment with an interpretation of | ||
http://jsonapi.org/recommendations/#filtering, including relationship chaining. | ||
Filters can be: | ||
- A resource field equality test: ?filter[foo]=123 | ||
- Apply other relational operators: ?filter[foo.in]=bar,baz or ?filter[count.ge]=7... | ||
- Membership in a list of values (OR): ?filter[foo]=abc,123,zzz (foo in ['abc','123','zzz']) | ||
- Filters can be combined for intersection (AND): ?filter[foo]=123&filter[bar]=abc,123,zzz&filter[...] | ||
- A related resource field for above tests: ?filter[foo.rel.baz]=123 (where `rel` is the relationship name) | ||
|
||
It is meaningless to intersect the same filter: ?filter[foo]=123&filter[foo]=abc will always yield nothing so | ||
detect this repeated appearance of the same filter in JsonApiQueryValidationFilter and complain there. | ||
""" | ||
|
||
def get_filterset(self, request, queryset, view): | ||
""" | ||
Validate that the `filter[field]` is defined in the filters and raise ValidationError if it's missing. | ||
|
||
While `filter` syntax and semantics is undefined by the jsonapi 1.0 spec, this behavior is consistent with the | ||
style used for missing query parameters: http://jsonapi.org/format/#query-parameters. In general, unlike | ||
django/DRF, jsonapi raises 400 rather than ignoring "bad" query parameters. | ||
""" | ||
fs = super(JsonApiFilterFilter, self).get_filterset( | ||
request, queryset, view) | ||
# TODO: change to have option to silently ignore bad filters | ||
for k in self.filter_keys: | ||
if k not in fs.filters: | ||
raise ValidationError("invalid filter[{}]".format(k)) | ||
return fs | ||
|
||
def get_filterset_kwargs(self, request, queryset, view): | ||
""" | ||
Turns filter[<field>]=<value> into <field>=<value> which is what DjangoFilterBackend expects | ||
""" | ||
self.filter_keys = [] | ||
# rewrite `filter[field]` query parameters to make DjangoFilterBackend work. | ||
data = request.query_params.copy() | ||
for qp, val in data.items(): | ||
if qp[: | ||
7] == 'filter[' and qp[-1] == ']' and qp != self.search_param: | ||
key = qp[7:-1].replace( | ||
'.', '__' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above a regex would make this more readable and the slices obsolete. Thinking about it again I guess replacing dots with __ wouldn't hurt for users who don't care about this notation as dots are disallowed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, I'm going to stop using yapf. It messed up the line breaks. It should be this: if qp[:7] == 'filter[' and qp[-1] == ']' and qp != self.search_param: |
||
) # convert jsonapi relationship path to Django ORM's __ notation | ||
# TODO: implement JSON_API_FORMAT_FIELD_NAMES conversions inbound. | ||
data[key] = val | ||
self.filter_keys.append(key) | ||
del data[qp] | ||
return { | ||
'data': data, | ||
'queryset': queryset, | ||
'request': request, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure how to get this right. Especially when an API designer wants to define additional query params with different naming per view it would also need to be configurable.
All would be possible to do somehow but I kind of think it makes the life more difficult of an api designer and the gain is very little in the default case.
Wouldn't it be good enough just to verify the params in the other filter backend classes whether they exists and leave this filter class out for now?
From a maintenance perspective I would prefer not to provide this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems inappropriate to verify the params in other classes as they only "own" their own params. Verification is already happening there. For instance,
filter[blah]
checks to see ifblah
is a defined filter.Some of the params are not even backend filters, like
include
which is elsewhere in the serializer code.The problem this class addresses is that a client would inadvertently misspell a param and then (if they don't notice that) wonder why the action they requested isn't happening. I'm all for making it easier on the client developer to notice these kinds of errors. If they want to "break" JSON:API and add additional parameters they can:
ignore_bad_sort_fields
as well; If you really want your API to be silent to all query parameter errors, this is needed.jsonapi_query_keywords
attribute (or add aJSON_API_QUERY_KEYWORDS
to settings although I've currently left that out to make it difficult to break the standard.)Needless to say I feel pretty strongly that this is a useful backend filter to help achieve JSON:API compliance for query parameter handling:
"If a server encounters a query parameter that does not follow the naming conventions above, and the server does not know how to process it as a query parameter from this specification, it MUST return
400 Bad Request
."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't address the different naming per view part.
I believe the attribute is actually defined for the view. It's certainly declared that way. I'm guessingI need to write unit tests for these permutations to confirm.backend_filters
classes are mixed in with the view class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this would make DJA better comply with JSON API spec which is one of ours goals of DJA.
I think though there is simple no hurry to implement this in DJA and I think we shouldn't rush into implementing this without having a proper understanding how to do it well.
Because once its landed in DJA and we will find that it is not a good fit we will have to go through a deprecation cycle to change it which makes maintaining DJA harder and the code more complex.
Another reason why I think this is hard to implement is that query params can be accessed wherever request is accessible. Query params are not only used in filter backends but also in other parts e.g. pagination is one example but I am sure there are others.
Also it kind of feels odd to add validation into a filter backend which should be there for filtering.
Maybe a middleware might be the better spot but I am not sure and would try to experiment with it first to get a clear understanding.
We could think of an experimental area for this. We haven't done this in the past and would need to rethink how to structure it though.
So why do we not just try it out in our projects (I can also think of one of our projects where it could be helpful)? Once we have it running in production and it is solid we can still consider adding it to DJA. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to finish up this PR, leaving it marked WIP, and then maybe we can make an experimental branch and merge it there? We can always rebase later. I have a working example and test case in my local code but wanted to merge in to the example and test cases here so others can try it and provide feedback.
I would argue that filtering for valid query parameters is a legit use of a filter;-) But this could be done as a view mixin too. I think middleware is too early in the stack as we haven't even gotten to DRF at that point, whereas backend filters are a DRF layer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sliverc FYI - I've closed the PR while I keep working on this. Do we want to create an experiment branch so others can test/contribute?
BTW, using DjangoFilterBackend adds a new dependency for
django-filter
. That could be a problem. It's not core Django or DRF but is the top-rated filtering package and the one reference from DRF docs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of
django-filter
. We have the same issue withdjango-polymorphic
. This is marked as optional dependency, we can do the same withdjango-filter
. We just have to make sure that DJA can still be used when this dependency is not installed - meaning we have to be careful with imports.Actually
django-filter
has the exact same issue just the other way around as it is also usable without Django REST Framework. All rest_framework related classes are part of therest_framework
module. We could create adjango_filters
module in our case.In terms of how to move forward:
I am an advocate of small PRs as they are simpler to review and there quicker to get merged.
What I would suggest how we move forward:
JSONAPIOrderingFilter
- this is a fairly straight forward change and can be merged easilyJSONAPIDjangoFilterBackend
with its own module approachJsonApiQueryValidationFilter
where we can experiment how this works best. (I will comment on the experimental discussion you started later on)How does this sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sliverc you wrote:
Actually there are no imports for django-polymorphic. I don't quite understand how this code works:
django-rest-framework-json-api/rest_framework_json_api/serializers.py
Line 215 in a320536
I don't quite understand what you mean by creating a
django_filters
module. Are you saying submit this as a PR to django-filter? As it stands right now, I'll either have to wrap theJSONAPIDjangoFilter
implementation with a try/except offrom django_filters.rest_framework import DjangoFilterBackend
or just makedjango-filter
a dependency for DJA. Please advise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, what I mean is to create a folder django_filters in DJA and add a init,py and backends.py to implement a custom backend. (Hint: Simplified each python file is called a module but more details at https://docs.python.org/3.6/tutorial/modules.html)
If a user of DJA doesn't import this newly created
django_filters
module or uses it in settings then he doesn't have to install django_filters.