[PATCH 3/7] REST: Use ModelMultipleChoiceField
Stephen Finucane
stephen at that.guru
Wed May 9 02:13:46 AEST 2018
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?
> > + 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.
> >
> > @@ -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.
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