Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090] #7258

erinspace · 2017-05-24T18:58:30Z

Note:

This is no longer an actual PR that would be merged, rather a proof of concept. Main focus is document linked on below JIRA ticket!

Purpose

Investigate using the DjangoFilterBackend for APIv2. This is just a proof-of-concept, not necessarily for merging, but just for reviewing and seeing what sorts of modifications using the 3rd party DjangoFilterBackend could entail.

Changes

Use DjangoFilterBackend for the UserList view and NodeList views
Create JSONAPIFilterSet using some of the same filter param logic to break down filter syntax
Custom filter properties that will filter with OR queries for two different fields when using filter syntax such as /?filter[given_name,family_name]=Doe
Also add OR functionality with one field, such as /?filter[family_name]=Jane,Dog
UserFilterSet and NodeFilterSet that facilitate filtering on node and user fields, consistent with what we have in the API currently

Side effects

No real ones at the moment, since it's only a proof of concept!

Other thoughts

A lot of messing around with standard functionality seemed to be needed to be done for our filter syntax to work properly with variable OR filters (see changes above). I'm not sure how well the built in solutions will work given the modifications we've done to get these things to work, but it could be alright! Just not sure if we'd need to alter the basics so much that we lose the benefits of using a maintained 3rd party library.

Ticket

https://openscience.atlassian.net/browse/OSF-7962
https://openscience.atlassian.net/browse/OSF-8090

sloria · 2017-05-24T19:37:39Z

api/base/filters.py

+
+    def __init__(self, data=None, *args, **kwargs):
+        if data:
+            new_qd = QueryDict('', mutable=True)


Why a QueryDict? Based on this, looks like data can be regular dict.

sloria · 2017-05-24T19:40:22Z

api/base/filters.py

@@ -61,6 +66,83 @@ def filter_queryset(self, request, queryset, view):
        return queryset


+class NewDjangoFilterMixin(django_filters.FilterSet):


This is a base class--not a mixin--so this should be renamed. Maybe JSONAPIFilterSet?

sloria · 2017-05-24T19:42:56Z

api/base/filters.py

+        super(NewDjangoFilterMixin, self).__init__(data=data, *args, **kwargs)
+
+    @property
+    def qs(self, *args, **kwargs):


Can we avoid having all the duplication with the code from FilterSet? Ideally, this method would just be:

qs = super(....).qs return self.filter_groups(qs)

I would think this is possible as long as you ensure self.filters is correct in __init__.

sloria · 2017-05-24T19:46:51Z

api/base/filters.py

+                    field_names = re.findall(self.FILTER_FIELDS, fields.strip())
+
+                    self.or_fields = []
+                    multiple_values = value.split(',')


Nit: This is a strange name--"values" is already plural, so "multiple_values" is redundant.

sloria · 2017-05-24T19:47:45Z

api/base/filters.py

+
+                    self.or_fields = []
+                    multiple_values = value.split(',')
+                    if len(multiple_values) > 1 or len(field_names) > 1:


Why is the length of multiple_values checked? Don't we only need to strip if there are more than one field_names?

the len of multiple_values is checked here cause if there are more than one, those also need to be sent to the or_fields list for a built OR query, independent if there are multiple field names

Ah, I think I see. But don't filters with multiple values need to be handled separately from filters with multiple field names anyway? Also, doesn't django-filter have a built in way to handle multiple values? Intuitively, ?full_name=Erin,Steve should be translated to .filter(fullname__in=['Erin', 'Steve']); I wonder if that's supported out-of-the-box.

I had a whole lot of trouble getting this to work out of the box -- theoretically, from reading through a few issues and their resolutions, the only mention I can find on how to handle those natively was through the CSV Widget -- though applying that in my testing of things didn't get me anywhere for a long while so I decided to just deal with it while dealing with other OR cases at the same time. I can dive back in tho and really try to figure out what's happening....

Cool cool, looks like it might not be supported OOB. MultipleChoiceFilter does something similar to what we want, except without a constrained set of "choices". So maybe we can at least steal some code from that class.

Also came across this issue, which might be worth looking into: carltongibson/django-filter#727

sloria · 2017-05-24T19:58:55Z

api/base/filters.py

+                    fields = match_dict['fields']
+                    field_names = re.findall(self.FILTER_FIELDS, fields.strip())
+
+                    self.or_fields = []


This looks suspicious--or_fields is getting reset on each iteration, so ?filter[given_name,family_name]=Erin&filter[something_else,another_thing]=foo would break.

sloria · 2017-05-24T20:02:53Z

api/base/filters.py

+                    if len(multiple_values) > 1 or len(field_names) > 1:
+                        for field_name in field_names:
+                            for value in multiple_values:
+                                self.or_fields.append({'field': field_name, 'value': value})


You'll need to handle multiple groups. Maybe it makes sense to use a dictionary, where the keys are sets of field names, and the value is the value.

{ set('given_name', 'family_name'): 'Erin', ... }

sloria · 2017-05-24T20:05:59Z

api/users/views.py

+    class Meta:
+        model = OSFUser
+        fields = ['id', 'full_name', 'given_name', 'middle_names', 'family_name']
+        strict = django_filters.constants.STRICTNESS.RAISE_VALIDATION_ERROR


Would it make sense to define strict on the base class (currently NewDjangoFilterMixin)?

Also, is there a way for us to raise the same error that we're doing now if the client passes an invalid field name?

osf.io/api/base/filters.py

Line 116 in 7e41c06

raise InvalidFilterFieldError(parameter='filter', value=field_name)

sloria · 2017-05-24T20:08:29Z

api/users/views.py

-
+        queryset = OSFUser.objects.filter(is_registered=True, date_disabled=None).order_by('-date_registered')
+        if self.request.version >= '2.3':
+            queryset = queryset.filter(merged_by__isnull=True)


Why was this added?

This was added for this test here that checks for version 2.3. Since django-filter is based on the view's get_queryset method (rather than our filter's relying on get_default_odm_query) -- I added the same checks on get_queryset as already existed on the odm query method

Ah, ok. Thanks for the clarification.

sloria · 2017-05-24T20:21:35Z

api_tests/users/views/test_user_list.py

@@ -261,6 +261,27 @@ def test_users_list_filter_multiple_fields_with_bad_filter(self):
        res = self.app.get(url, expect_errors=True)
        assert_equal(res.status_code, 400)

+    def test_users_list_or_filter(self):
+        self.john_doe = UserFactory(fullname='John Doe')


Assigning to self is unnecessary.

sloria

Pass finished

sloria · 2017-06-01T17:37:56Z

api/base/filters.py

+    QUERY_PATTERN = re.compile(r'^filter\[(?P<fields>((?:,*\s*\w+)*))\](\[(?P<op>\w+)\])?$')
+    FILTER_FIELDS = re.compile(r'(?:,*\s*(\w+)+)')
+
+    or_fields = {}


Why is this here?

sloria · 2017-06-01T17:39:27Z

api/base/filters.py

+
+class JSONAPIFilterSet(django_filters.FilterSet):
+
+    QUERY_PATTERN = re.compile(r'^filter\[(?P<fields>((?:,*\s*\w+)*))\](\[(?P<op>\w+)\])?$')


Is [op] respected? I don't see it being used anywhere

Not yet! This was used when parsing out the other operators used in other filters, such as eq, ne, etc, which aren't used in this endpoint that I picked. Perhaps this proof of concept should include porting a second endpoint that uses operators?

sloria · 2017-06-01T17:41:35Z

api/base/filters.py

+                if match:
+                    match_dict = match.groupdict()
+                    fields = match_dict['fields']
+                    field_names = re.findall(self.FILTER_FIELDS, fields.strip())


Since self.FILTER_FIELDS is a RegexObject, you should use self.FILTER_FIELDS.findall(...)

sloria · 2017-06-01T17:49:00Z

api/base/filters.py

+            data = new_data
+        super(JSONAPIFilterSet, self).__init__(data=data, *args, **kwargs)
+
+        for field in field_names:


This looks suspicious: field_names will be whatever happens to be set in the last iteration of the above loop. This validation should happen within the loop itself.

Actually, is this code necessary, since this check is happening in filter_groups?

I at first tried to do this check inside of the loop itself, but the only way I could access the fields that were supposed to be there was to access the form fields -- which is inaccessible until super(JSONAPIFilterSet, self).__init__(...) is called -- since self.form.fields isn't set until self.filters is set, which happens in the super's init.

The check is necessary here (or somewhere, at least), as the check in filter_groups only checks for invalid fields that are included in a group that will be part of an OR query (eg ?filter[wrong,field]) and not filters that will go through normal filtering later (eg ?filter[wrong_field]=whateves)

There might be a way to instead deal with this using django-filter's strict validation -- I tried for quite a long time to get this to work, and concluded that it was checking for an invalid filter value rather than an invalid filter itself -- which gets overlooked as far as I can tell by django-filter by default.

sloria · 2017-06-01T18:03:24Z

api/base/filters.py

+        self.form.is_valid()
+        or_keys = reduce(operator.or_, self.or_fields.keys(), set())
+        for key in or_keys:
+            self.form.cleaned_data.pop(key, None)


Rather than removing these keys from form.cleaned_data, would it make sense to prevent these fields from going through the form validation in the first place, i.e. preventing all the or_fields from going into self.form.fields?

sloria

Just a few minor suggested changes. Feel free to make those then move on to the more advanced use cases described in https://openscience.atlassian.net/browse/OSF-8090

sloria · 2017-06-09T18:54:36Z

api/base/filters.py

+        super(JSONAPIFilterSet, self).__init__(data=data, *args, **kwargs)
+
+        for field in field_names:
+            if field not in self.form.fields.keys():


Minor: No need to call keys(). value in some_dict is an O(1) operation.

sloria · 2017-06-12T20:20:05Z

api/users/views.py

        if check_permissions:
            # May raise a permission denied
            self.check_object_permissions(self.request, obj)
        return obj


-class UserList(JSONAPIBaseView, generics.ListAPIView, ODMFilterMixin):
+class UserFilterSet(JSONAPIFilterSet):


FilterSets and filter field classes should probably be separated into a separate file. Let's move this to api/users/filters.py.

sloria · 2017-06-12T20:20:13Z

api/users/views.py

@@ -29,7 +30,7 @@
 from rest_framework import permissions as drf_permissions
 from rest_framework import generics
 from rest_framework.exceptions import NotAuthenticated, NotFound
-from osf.models import Contributor, ExternalAccount, AbstractNode as Node, PreprintService, OSFUser as User
+from osf.models import Contributor, ExternalAccount, AbstractNode as Node, PreprintService, OSFUser


mfraezz

Couple minor comments, no blockers other than conflicts. LGTM 🐳

mfraezz · 2017-09-08T19:10:41Z

api/base/filters.py

+
+        return qs
+
+class NullModelMultipleChoiceCaseInsensitiveField(forms.ModelMultipleChoiceField):


Minor: I think the Django convention is to define custom fields in a fields.py file, but we haven't yet done that anywhere else in the API ¯\_(ツ)_/¯

mfraezz · 2017-09-08T19:29:23Z

api/base/filters.py

+    QUERY_PATTERN = re.compile(r'^filter\[(?P<fields>((?:,*\s*\w+)*))\](\[(?P<op>\w+)\])?$')
+    FILTER_FIELDS = re.compile(r'(?:,*\s*(\w+)+)')
+
+    # TODO - there must be a better way to recognize these


You can do something like if <class>._meta.get_field('<name>').many_to_many to check for M2M.
For date...

from django.db import connection if 'timestamp' in <class>._meta.get_field('<name>').db_type(connection)

Not sure how hard it is to get the class in scope here, though

Ah, that's good to know! I tried to get the class in scope here, but it's really tough, as far as I can tell my only args here are the field name and value being passed in by the filter....

[#OSF-7962]

- Put base class for filter into filters.py for use elsewhere that will deal with filter syntax - use that base class in the UserList view - Modify UserList get_queryset to also account for version - filter_groups method to deal with adding OR to filters in the same set

… for fullname

[#OSF-8090]

…hips

…from

sloria · 2018-04-13T16:12:18Z

Closing. See https://openscience.atlassian.net/browse/PLAT-492 for our analysis

sloria suggested changes May 24, 2017

View reviewed changes

erinspace force-pushed the feature/filter_backend branch 2 times, most recently from f21d1d5 to c1d836b Compare May 31, 2017 16:42

sloria suggested changes Jun 1, 2017

View reviewed changes

erinspace force-pushed the feature/filter_backend branch 2 times, most recently from ae1c69c to 0718b91 Compare June 2, 2017 14:10

sloria suggested changes Jun 12, 2017

View reviewed changes

erinspace force-pushed the feature/filter_backend branch from 0718b91 to 3ebedca Compare July 11, 2017 18:13

erinspace changed the title ~~Proof of concept: Refactoring UserList View to use django-filter [OSF-7962]~~ Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] Jul 11, 2017

erinspace changed the title ~~Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962]~~ Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090] Jul 11, 2017

erinspace changed the title ~~Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090]~~ [WIP] Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090] Jul 11, 2017

erinspace force-pushed the feature/filter_backend branch from 3ebedca to 7652434 Compare July 13, 2017 20:13

erinspace changed the title ~~[WIP] Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090]~~ Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090] Jul 13, 2017

erinspace force-pushed the feature/filter_backend branch 2 times, most recently from 4c1eebd to 5f3802b Compare July 14, 2017 18:16

mfraezz approved these changes Sep 8, 2017

View reviewed changes

erinspace force-pushed the feature/filter_backend branch 6 times, most recently from 53fa6a7 to a2d0ef3 Compare September 13, 2017 17:42

erinspace force-pushed the feature/filter_backend branch 2 times, most recently from d3f782b to 47e0276 Compare January 30, 2018 17:14

erinspace added 6 commits January 31, 2018 13:01

add djangofilterbackend for experimenting on userlist

5e41e58

Start of simple implementatin with user list view with django-filter

2cfc915

[#OSF-7962]

Add django-filter to requirements.txt

b6618aa

Add functionality and test for searching OR between values

5fd8e3d

Change name of FilterSet base class, other small adjustments

f756ae6

erinspace added 17 commits January 31, 2018 13:01

Refactor new tests a bit, add test for double filters

199eabc

Add MultiValueCharFilter and set as default for CharFields

231011e

Refactor user view a bit to inherit from base meta and use new filter…

675ae37

… for fullname

Add extra checks for invalid filter names and raise right error

5b02e3c

Refactor filterset methods a bit to be more efficent

9259f00

Port NodeList to use new django-filter functionality

ba63f7e

[#OSF-8090]

Remove keys call to make more effecient

2e97bcf

Move UserFilterSet to new file to seperate it

6698b2d

Update wording for filter error to match django-filter

5158228

Update base filterset to account for dates, operators, null relations…

c2eda44

…hips

Add NodeFilterSet for custom node filtering

36e125c

Post-process boolean filter values to be recognized by NullBooleanSelect

b46c4a5

Move NullModelMultipleChoiceCaseInsensitiveField to base/fields.py

13b4f8a

fix import conflicts and pytest conversions

3c2428f

Use the same datetime conversion as FilterMixin

23351de

hacks to get contributor filtering working properly

713af46

fix filterset to look for new created instead of date_created

a2fb4b4

erinspace force-pushed the feature/filter_backend branch from 47e0276 to a2fb4b4 Compare January 31, 2018 18:02

Mysterious need to remove duplicate empty_label where is this coming …

677739c

…from

erinspace force-pushed the feature/filter_backend branch from a31a1ad to 677739c Compare January 31, 2018 18:41

sloria closed this Apr 13, 2018

		@@ -61,6 +66,83 @@ def filter_queryset(self, request, queryset, view):
		return queryset


		class NewDjangoFilterMixin(django_filters.FilterSet):


		class JSONAPIFilterSet(django_filters.FilterSet):

		QUERY_PATTERN = re.compile(r'^filter\[(?P<fields>((?:,\s\w+)*))\](\[(?P<op>\w+)\])?$')


		return qs

		class NullModelMultipleChoiceCaseInsensitiveField(forms.ModelMultipleChoiceField):

Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090] #7258

Proof of concept: Refactoring UserListView an NodeListView to use django-filter [OSF-7962] [OSF-8090] #7258

Conversation

erinspace commented May 24, 2017 • edited

Note:

Purpose

Changes

Side effects

Other thoughts

Ticket

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloria May 25, 2017 • edited

Choose a reason for hiding this comment

erinspace May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloria left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfraezz left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloria commented Apr 13, 2018

erinspace commented May 24, 2017 •

edited

sloria May 25, 2017 •

edited

erinspace May 25, 2017 •

edited

mfraezz left a comment •

edited