Stephen Finucane <step...@that.guru> writes:
> On Wed, 2018-05-09 at 01:45 +1000, Daniel Axtens wrote:
>> 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.
>
> 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 <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.
>
> 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='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?
>
> 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': '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.
>
> 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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
