[PATCH 6/6] REST: Resolve issues with filters
Philippe Pepiot
philippe.pepiot at logilab.fr
Wed May 17 19:18:35 AEST 2017
On 05/16/2017 07:20 PM, Stephen Finucane wrote:
> On Tue, 2017-05-16 at 17:38 +0100, Stephen Finucane wrote:
>> On Tue, 2017-05-16 at 11:04 +0200, Philippe Pepiot wrote:
>>> On 05/16/2017 01:14 AM, Stephen Finucane wrote:
>>>> Turns out filtering patches using a series string wasn't as easy
>>>> as
>>>> we thought. We need to slugify State.name, but unfortunately that
>>>> isn't stored in the database. The tests were hiding this fact as
>>>> State objects created by 'tests.utils.create_state' don't have
>>>> spaces in them.
>>>>
>>>> Override custom versions of both django-filter's 'Filter' class
>>>> and
>>>> the Django 'Form' required by this, and update the tests to
>>>> prevent
>>>> a regression.
>>>>
>>>> Signed-off-by: Stephen Finucane <stephen at that.guru>
>>>> Fixes: 6222574 ("REST: filter patches by state name")
>>>> Cc: Philippe Pepiot <philippe.pepiot at logilab.fr>
>>>> ---
>>>> patchwork/api/filters.py | 36
>>>> +++++++++++++++++++++++++++++++-----
>>>> patchwork/api/patch.py | 2 +-
>>>> patchwork/models.py | 4 ++++
>>>> patchwork/tests/test_rest_api.py | 10 +++++-----
>>>> 4 files changed, 41 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py
>>>> index 12a54a8..00efc99 100644
>>>> --- a/patchwork/api/filters.py
>>>> +++ b/patchwork/api/filters.py
>>>> @@ -17,10 +17,11 @@
>>>> # along with Patchwork; if not, write to the Free Software
>>>> # Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>>> MA 02111-
>>>> 1307 USA
>>>>
>>>> +from django.core.exceptions import ValidationError
>>>> from django_filters import FilterSet
>>>> from django_filters import IsoDateTimeFilter
>>>> -from django_filters import CharFilter
>>>> from django_filters import ModelChoiceFilter
>>>> +from django.forms import ModelChoiceField
>>>>
>>>> from patchwork.compat import LOOKUP_FIELD
>>>> from patchwork.models import Bundle
>>>> @@ -30,6 +31,7 @@ from patchwork.models import Event
>>>> from patchwork.models import Patch
>>>> from patchwork.models import Project
>>>> from patchwork.models import Series
>>>> +from patchwork.models import State
>>>>
>>>>
>>>> class TimestampMixin(FilterSet):
>>>> @@ -59,15 +61,39 @@ class CoverLetterFilter(ProjectMixin,
>>>> TimestampMixin, FilterSet):
>>>> fields = ('project', 'series', 'submitter')
>>>>
>>>>
>>>> +class StateChoiceField(ModelChoiceField):
>>>> +
>>>> + 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
>>>> + 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
>>>
>>> Thanks for fixing this !
>>>
>>> It seems that the ValidationError is silently hidden:
>>>
>>> $ http 'http://localhost:8000/api/patches/?state=doesnotexists'
>>> HTTP/1.0 200 OK
>>> Allow: GET, HEAD, OPTIONS
>>> Content-Type: application/json
>>> Date: Tue, 16 May 2017 08:47:51 GMT
>>> Server: WSGIServer/0.2 CPython/3.5.2
>>> Vary: Accept, Cookie
>>>
>>> []
>>>
>>> Maybe modifying 'strict' behavior here ?
>>> http://django-filter.readthedocs.io/en/develop/ref/filterset.html#c
>>> us
>>> tomise-filter-generation-with-filter-overrides
>>>
>>
>> Yes, good idea. I'll submit a follow-up for this.
>
> Actually, looking into this it seems django-filter doesn't handle this
> properly: enabling the option results in a '500 (Server Error)', rather
> than a '400 (Bad Request)'. We should never bubble up a 5xx-class error
> unless there's a serious problem.
>
> Unless you're aware of a way to handle this better, I'd be reluctant to
> do this and would keep things as they are.
>
> Stephen
>
Ok let's keep this, it's still better than before, thanks.
Tested-by: Philippe Pepiot <philippe.pepiot at logilab.fr>
>> If you've tested this/reviewed this, feel free to say as much by
>> repling with a 'Tested-by' or 'Acked-by' (on this or any other patch
>> :))
>>
>> Stephen
>>
>>>> +
>>>> +
>>>> +class StateFilter(ModelChoiceFilter):
>>>> +
>>>> + field_class = StateChoiceField
>>>> +
>>>> +
>>>> class PatchFilter(ProjectMixin, FilterSet):
>>>>
>>>> - # TODO(stephenfin): We should probably be using a
>>>> ChoiceFilter
>>>> here?
>>>> - state = CharFilter(name='state__name')
>>>> + state = StateFilter(queryset=State.objects.all())
>>>>
>>>> class Meta:
>>>> model = Patch
>>>> - fields = ('project', 'series', 'submitter', 'delegate',
>>>> 'state',
>>>> - 'archived')
>>>> + fields = ('project', 'series', 'submitter', 'delegate',
>>>> + 'state', 'archived')
>>>>
>>>>
>>>> class CheckFilter(TimestampMixin, FilterSet):
>>>> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
>>>> index f0c7225..1f0ead1 100644
>>>> --- a/patchwork/api/patch.py
>>>> +++ b/patchwork/api/patch.py
>>>> @@ -70,7 +70,7 @@ class StateField(RelatedField):
>>>> self.fail('incorrect_type',
>>>> data_type=type(data).__name__)
>>>>
>>>> def to_representation(self, obj):
>>>> - return '-'.join(obj.name.lower().split())
>>>> + return obj.slug
>>>>
>>>> def get_queryset(self):
>>>> return State.objects.all()
>>>> diff --git a/patchwork/models.py b/patchwork/models.py
>>>> index 8913fac..0ed37ab 100644
>>>> --- a/patchwork/models.py
>>>> +++ b/patchwork/models.py
>>>> @@ -192,6 +192,10 @@ class State(models.Model):
>>>> ordering = models.IntegerField(unique=True)
>>>> action_required = models.BooleanField(default=True)
>>>>
>>>> + @property
>>>> + def slug(self):
>>>> + return '-'.join(self.name.lower().split())
>>>> +
>>>> def __str__(self):
>>>> return self.name
>>>>
>>>> diff --git a/patchwork/tests/test_rest_api.py
>>>> b/patchwork/tests/test_rest_api.py
>>>> index 70410d0..0956010 100644
>>>> --- a/patchwork/tests/test_rest_api.py
>>>> +++ b/patchwork/tests/test_rest_api.py
>>>> @@ -309,7 +309,7 @@ class TestPatchAPI(APITestCase):
>>>> self.assertEqual(patch_obj.id, patch_json['id'])
>>>> self.assertEqual(patch_obj.name, patch_json['name'])
>>>> self.assertEqual(patch_obj.msgid, patch_json['msgid'])
>>>> - self.assertEqual(patch_obj.state.name,
>>>> patch_json['state'])
>>>> + self.assertEqual(patch_obj.state.slug,
>>>> patch_json['state'])
>>>> self.assertIn(patch_obj.get_mbox_url(),
>>>> patch_json['mbox'])
>>>>
>>>> # nested fields
>>>> @@ -325,7 +325,8 @@ class TestPatchAPI(APITestCase):
>>>> self.assertEqual(status.HTTP_200_OK, resp.status_code)
>>>> self.assertEqual(0, len(resp.data))
>>>>
>>>> - patch_obj = create_patch()
>>>> + state_obj = create_state(name='Under Review')
>>>> + patch_obj = create_patch(state=state_obj)
>>>>
>>>> # anonymous user
>>>> resp = self.client.get(self.api_url())
>>>> @@ -338,10 +339,9 @@ class TestPatchAPI(APITestCase):
>>>> self.assertNotIn('diff', patch_rsp)
>>>>
>>>> # test filtering by state
>>>> - other_state = create_state()
>>>> - resp = self.client.get(self.api_url(), {'state':
>>>> patch_obj.state.name})
>>>> + resp = self.client.get(self.api_url(), {'state': 'under-
>>>> review'})
>>>> self.assertEqual([patch_obj.id], [x['id'] for x in
>>>> resp.data])
>>>> - resp = self.client.get(self.api_url(), {'state':
>>>> other_state.name})
>>>> + resp = self.client.get(self.api_url(), {'state':
>>>> 'missing-
>>>> state'})
>>>> self.assertEqual(0, len(resp.data))
>>>>
>>>> # authenticated user
>>>>
>>
>> _______________________________________________
>> Patchwork mailing list
>> Patchwork at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list