[3/7] REST: Use ModelMultipleChoiceField

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

Signed-off-by: Stephen Finucane <stephen@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(-)

Stephen Finucane <stephen@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@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@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@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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

On Wed, 2018-05-09 at 01:45 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen@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.

That's exactly what I'm trying to say :) As you can tell, these commit
messages were written after the fact (as I tried to break the changes
into manageable chunks) so they could do with some work.

> > 
> > Signed-off-by: Stephen Finucane <stephen@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.

Unfortunately it's necessary to do this otherwise, as noted, we can get
rubbish back out. I _think_ that the query itself shouldn't be too
expensive, given that we don't actually do anything with the result
(thus avoiding JOINs and the likes), so we should be OK here?

> > +            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.

True, but that's just a side-effect of re-using the existing message.

  https://github.com/django/django/blob/d1413c5d/django/forms/models.py
#L1274

I could override 'self.error_messages' but it seems unnecessary. Maybe
just a comment would do?

> >  
> > +            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@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?

As before, it's a ordering thing. It's no harm but it can be dropped.

> >  
> > @@ -117,6 +117,19 @@ class TestPatchAPI(APITestCase):
> >              'submitter': 'test@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@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.

Yeah, that would be a good addition to this release note. I should
probably add this to the docs too, if I didn't do so already.

Stephen

> Regards,
> Daniel
> 
> > -- 
> > 2.14.3
> > 
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork

Stephen Finucane <stephen@that.guru> writes:

> On Wed, 2018-05-09 at 01:45 +1000, Daniel Axtens wrote:
>> Stephen Finucane <stephen@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.
>
> That's exactly what I'm trying to say :) As you can tell, these commit
> messages were written after the fact (as I tried to break the changes
> into manageable chunks) so they could do with some work.
>
>> > 
>> > Signed-off-by: Stephen Finucane <stephen@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.
>
> Unfortunately it's necessary to do this otherwise, as noted, we can get
> rubbish back out. I _think_ that the query itself shouldn't be too
> expensive, given that we don't actually do anything with the result
> (thus avoiding JOINs and the likes), so we should be OK here?
>

I think we'll be fine. I suppose we'll get bug reports if not.

>> > +            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.
>
> True, but that's just a side-effect of re-using the existing message.
>
>   https://github.com/django/django/blob/d1413c5d/django/forms/models.py
> #L1274
>
> I could override 'self.error_messages' but it seems unnecessary. Maybe
> just a comment would do?
>
>> >  
>> > +            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@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?
>
> As before, it's a ordering thing. It's no harm but it can be dropped.

Ok, I'm leaving this and similar hunks then.

>
>> >  
>> > @@ -117,6 +117,19 @@ class TestPatchAPI(APITestCase):
>> >              'submitter': 'test@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@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.
>
> Yeah, that would be a good addition to this release note. I should
> probably add this to the docs too, if I didn't do so already.

Will do on merge.

Regards,
Daniel
>
> Stephen
>
>> Regards,
>> Daniel
>> 
>> > -- 
>> > 2.14.3
>> > 
>> > _______________________________________________
>> > Patchwork mailing list
>> > Patchwork@lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/patchwork