[PATCH 6/6] REST: Resolve issues with filters
Stephen Finucane
stephen at that.guru
Wed May 17 03:20:54 AEST 2017
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
> 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