[PATCH 3/7] REST: Use ModelMultipleChoiceField

Daniel Axtens dja at axtens.net
Thu May 10 02:14:38 AEST 2018


Stephen Finucane <stephen at that.guru> writes:

> On Wed, 2018-05-09 at 01:45 +1000, Daniel Axtens wrote:
>> 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.
>
> 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 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.
>
> 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 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?
>
> 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 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.
>
> 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 at lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/patchwork


More information about the Patchwork mailing list