[PATCH 3/7] REST: Use ModelMultipleChoiceField

Daniel Axtens dja at axtens.net
Wed May 9 01:45:11 AEST 2018


Stephen Finucane <stephen at that.guru> writes:

Some comments below.

> We use a modified version of this that allows us to query on multiple
> fields.

I think you're trying to say that we use a modified version of Django's
ModelMultipleChoiceField? but I'm not sure if "this that" refers to
something else.

>
> Signed-off-by: Stephen Finucane <stephen at that.guru>
> Fixes: #156
> ---
>  patchwork/api/filters.py                           | 103 ++++++++++++---------
>  patchwork/tests/api/test_patch.py                  |  15 ++-
>  .../improved-rest-filtering-bf68399270a9b245.yaml  |  10 +-
>  3 files changed, 82 insertions(+), 46 deletions(-)
>
> diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py
> index 030f9af3..b30499d0 100644
> --- a/patchwork/api/filters.py
> +++ b/patchwork/api/filters.py
> @@ -19,10 +19,11 @@
>  
>  from django.contrib.auth.models import User
>  from django.core.exceptions import ValidationError
> +from django.db.models import Q
>  from django_filters import FilterSet
>  from django_filters import IsoDateTimeFilter
> -from django_filters import ModelChoiceFilter
> -from django.forms import ModelChoiceField
> +from django_filters import ModelMultipleChoiceFilter
> +from django.forms import ModelMultipleChoiceField as BaseMultipleChoiceField
>  
>  from patchwork.models import Bundle
>  from patchwork.models import Check
> @@ -37,79 +38,96 @@ from patchwork.models import State
>  
>  # custom fields, filters
>  
> -class ModelMultiChoiceField(ModelChoiceField):
> -
> -    def _get_filters(self, value):
> -        raise NotImplementedError
> -
> -    def to_python(self, value):
> -        if value in self.empty_values:
> -            return None
> +class ModelMultipleChoiceField(BaseMultipleChoiceField):
>  
> +    def _get_filter(self, value):
>          try:
> -            filters = {'pk': int(value)}
> +            return 'pk', int(value)
>          except ValueError:
> -            filters = {self.alternate_lookup: value}
> -
> +            return self.alternate_lookup, value
> +
> +    def _check_values(self, value):
> +        """
> +        Given a list of possible PK values, returns a QuerySet of the
> +        corresponding objects. Raises a ValidationError if a given value is
> +        invalid (not a valid PK, not in the queryset, etc.)
> +        """
> +        # deduplicate given values to avoid creating many querysets or
> +        # requiring the database backend deduplicate efficiently.
>          try:
> -            value = self.queryset.get(**filters)
> -        except (ValueError, TypeError, self.queryset.model.DoesNotExist):
> -            raise ValidationError(self.error_messages['invalid_choice'],
> -                                  code='invalid_choice')
> -        return value
> +            value = frozenset(value)
> +        except TypeError:
> +            # list of lists isn't hashable, for example
> +            raise ValidationError(
> +                self.error_messages['list'],
> +                code='list',
> +            )
> +
> +        q_objects = Q()
> +
> +        for pk in value:
> +            key, val = self._get_filter(pk)
> +
> +            try:
> +                # NOTE(stephenfin): In contrast to the Django implementation
> +                # of this, we check to ensure each specified key exists and
> +                # fail if not. If we don't this, we can end up doing nothing
> +                # for the filtering which, to me, seems very confusing
> +                self.queryset.get(**{key: val})
You're doing duplicate lookups; one here and one in the
qs=self.queryset.filter(). I don't know if you can combine them in code
to avoid hitting the db repeatedly, and I don't know if it would ever be
massively speed critical, but you are having to go to the db twice to
get exactly the same data and that's unfortunate.

> +            except (ValueError, TypeError, self.queryset.model.DoesNotExist):
> +                raise ValidationError(
> +                    self.error_messages['invalid_pk_value'],
> +                    code='invalid_pk_value',
> +                    params={'pk': val},
> +                )
This isn't necessarily a PK if you're doing the alternate lookups.

>  
> +            q_objects |= Q(**{key: val})
>  
> -class ProjectChoiceField(ModelMultiChoiceField):
> +        qs = self.queryset.filter(q_objects)
> +
> +        return qs
> +
> +
> +class ProjectChoiceField(ModelMultipleChoiceField):
>  
>      alternate_lookup = 'linkname__iexact'
>  
>  
> -class ProjectFilter(ModelChoiceFilter):
> +class ProjectFilter(ModelMultipleChoiceFilter):
>  
>      field_class = ProjectChoiceField
>  
>  
> -class PersonChoiceField(ModelMultiChoiceField):
> +class PersonChoiceField(ModelMultipleChoiceField):
>  
>      alternate_lookup = 'email__iexact'
>  
>  
> -class PersonFilter(ModelChoiceFilter):
> +class PersonFilter(ModelMultipleChoiceFilter):
>  
>      field_class = PersonChoiceField
>  
>  
> -class StateChoiceField(ModelChoiceField):
> +class StateChoiceField(ModelMultipleChoiceField):
>  
> -    def prepare_value(self, value):
> -        if hasattr(value, '_meta'):
> -            return value.slug
> -        else:
> -            return super(StateChoiceField, self).prepare_value(value)
> -
> -    def to_python(self, value):
> -        if value in self.empty_values:
> -            return None
> +    def _get_filter(self, value):
>          try:
> -            value = ' '.join(value.split('-'))
> -            value = self.queryset.get(name__iexact=value)
> -        except (ValueError, TypeError, self.queryset.model.DoesNotExist):
> -            raise ValidationError(self.error_messages['invalid_choice'],
> -                                  code='invalid_choice')
> -        return value
> +            return 'pk', int(value)
> +        except ValueError:
> +            return 'name__iexact', ' '.join(value.split('-'))
>
>  
> -class StateFilter(ModelChoiceFilter):
> +class StateFilter(ModelMultipleChoiceFilter):
>  
>      field_class = StateChoiceField
>  
>  
> -class UserChoiceField(ModelMultiChoiceField):
> +class UserChoiceField(ModelMultipleChoiceField):
>  
>      alternate_lookup = 'username__iexact'
>  
>  
> -class UserFilter(ModelChoiceFilter):
> +class UserFilter(ModelMultipleChoiceFilter):
>  
>      field_class = UserChoiceField
>  
> @@ -125,8 +143,7 @@ class TimestampMixin(FilterSet):
>  
>  class ProjectMixin(FilterSet):
>  
> -    project = ProjectFilter(to_field_name='linkname',
> -                            queryset=Project.objects.all())
> +    project = ProjectFilter(queryset=Project.objects.all())
>  
>  
>  class SeriesFilter(ProjectMixin, TimestampMixin, FilterSet):
> diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py
> index 909c1eb4..373ee0d5 100644
> --- a/patchwork/tests/api/test_patch.py
> +++ b/patchwork/tests/api/test_patch.py
> @@ -71,8 +71,8 @@ class TestPatchAPI(APITestCase):
>          self.assertEqual(0, len(resp.data))
>  
>          person_obj = create_person(email='test at example.com')
> -        state_obj = create_state(name='Under Review')
>          project_obj = create_project(linkname='myproject')
> +        state_obj = create_state(name='Under Review')
>          patch_obj = create_patch(state=state_obj, project=project_obj,
>                                   submitter=person_obj)
This hunk could probably be dropped? unless there's a reason state has
to be after project?

>  
> @@ -117,6 +117,19 @@ class TestPatchAPI(APITestCase):
>              'submitter': 'test at example.org'})
>          self.assertEqual(0, len(resp.data))
>  
> +        state_obj_b = create_state(name='New')
> +        create_patch(state=state_obj_b)
> +        state_obj_c = create_state(name='RFC')
> +        create_patch(state=state_obj_c)
> +
> +        resp = self.client.get(self.api_url())
> +        self.assertEqual(3, len(resp.data))
> +        resp = self.client.get(self.api_url(), [('state', 'under-review')])
> +        self.assertEqual(1, len(resp.data))
> +        resp = self.client.get(self.api_url(), [('state', 'under-review'),
> +                                                ('state', 'new')])
> +        self.assertEqual(2, len(resp.data))
> +
>      def test_detail(self):
>          """Validate we can get a specific patch."""
>          patch = create_patch(
> diff --git a/releasenotes/notes/improved-rest-filtering-bf68399270a9b245.yaml b/releasenotes/notes/improved-rest-filtering-bf68399270a9b245.yaml
> index b1d12eb6..170e9621 100644
> --- a/releasenotes/notes/improved-rest-filtering-bf68399270a9b245.yaml
> +++ b/releasenotes/notes/improved-rest-filtering-bf68399270a9b245.yaml
> @@ -8,9 +8,15 @@ api:
>  
>         $ curl /covers/?submitter=stephen at that.guru
>    - |
> -    Bundles can be filtered by owner and checks by user using username. For
> -    example:
> +    Bundles can be filtered by owner, patches by delegate and checks by user
> +    using username. For example:
>  
>      .. code-block:: shell
>  
>         $ curl /bundles/?owner=stephenfin
> +  - |
> +    Filters can now be specified multiple times. For example:
> +
> +    .. code-block:: shell
> +
> +       $ curl /patches/?state=under-review&state=rfc

So IIUC, these create an OR relationship/a union - a patch can match any
of the states. I think this applies to all your multimatches? Perhaps
worth pointing out, just to ease the user into things.

Regards,
Daniel

> -- 
> 2.14.3
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork


More information about the Patchwork mailing list