-
Notifications
You must be signed in to change notification settings - Fork 772
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
Add QuerySetRequestMixin #494
Conversation
61039ee
to
5ab2477
Compare
Thanks |
This is on my list — except I want to make the request available, not just the |
Hi @carltongibson & @sassanh - out of curiosity what's the benefit to passing the full request instead of just the request user? My initial reaction is 👎, but I'm happy to change it since it's so straightforward to do. Some thoughts on request:
|
OK. I'll elaborate more later but... I've needed to filter on Accept-Languages header, and have had to do something like @kevin-brown's workaround before. Bottom line, whole request is context, we want the whole request, but it's optional, most of what we do is on the QueryDict. |
Oh cool - that makes sense. I'll update the PR. |
5ab2477
to
f14223b
Compare
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.
- Needs docs.
I'm inclined to add something like a RequestFilter
— which would look like the old MethodFilter
. This would always run — if self.parent.request is not None
— and would enable users ot define their own filtering logic in a method on the FilterSet
. (How best to aid/encourage validation...?)
@@ -307,11 +307,34 @@ class DurationFilter(Filter): | |||
field_class = forms.DurationField | |||
|
|||
|
|||
class ModelChoiceFilter(Filter): | |||
class QuerySetMixin(object): |
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.
This class needs a docstring. What's it for?
queryset = self.extra.get('queryset') | ||
|
||
if callable(queryset): | ||
return queryset(user=user) |
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 not following the API usage here — what's the intent?
It looks like you want to automatically filter to the user if provided... but queryset
is a django.db
QuerySet
instance right — so filter(...)
... ???
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.
Was thinking the API would be similar to a few of django's model field options, such as default
. You either pass the value (qs in this case) or a callable. In this case, the callable is passed the request or user (tbd, but leaning towards passing the request 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.
Actually - it would make the most sense to pass the request.
@@ -288,6 +288,9 @@ def __init__(self, data=None, queryset=None, prefix=None, strict=None): | |||
# What to do on on validation errors | |||
self.strict = self._meta.strict if strict is None else strict | |||
|
|||
self.request = request | |||
self.user = getattr(request, 'user', None) |
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 don't think we want to hold both references here. request.user
is fine.
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.
Makes sense, especially if the callable takes the request instead of the user.
@rpkilby
You can just pass in a dict with only the user, instead of a whole (faked) request, if you're only using the user in your tests. |
f14223b
to
95a8afa
Compare
Hi @carltongibson - updated the PR to be based on the
Could you give an example of possible Possible alternatives:
|
That should be the documented approach — it does away with the need for an extra type of weird Filter. (The whole point of this is to keep the filtering logic in the one-place, the |
95a8afa
to
7626d33
Compare
7626d33
to
30a226a
Compare
Hi @carltongibson - docs/tests have been added. Do they look sufficient? |
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.
Nice.
Exposes the request to be used during filtering. See DRF #3766, #3905, #3977.
FilterSet.request
, which is generally accessible through theFilter.parent
.ModelChoiceFilter
andModelMultipleChoiceFilter
throughQuerySetRequestMixin
.TODO: