Stephen Finucane <step...@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 <step...@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='t...@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': 't...@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=step...@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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork