[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