[PATCH v3 05/10] REST: Add Patches to the API
Finucane, Stephen
stephen.finucane at intel.com
Thu May 19 19:19:55 AEST 2016
On 18 May 22:30, Andy Doan wrote:
> This exposes patches via the REST API.
>
> Security Constraints:
> * Anyone (logged in or not) can read all objects.
> * No one can create/delete objects.
> * Project maintainers are allowed to update (ie "patch"
> attributes)
>
> NOTE: Patch.save was overridden incorrectly and had to be
> fixed to work with DRF.
>
> Signed-off-by: Andy Doan <andy.doan at linaro.org>
There's some things that could still be addressed. If you'd rather
fix these later, I can give sign-off now.
Stephen
> ---
> patchwork/models.py | 4 +-
> patchwork/rest_serializers.py | 39 +++++++++++++-
> patchwork/tests/test_rest_api.py | 113 ++++++++++++++++++++++++++++++++++++++-
> patchwork/views/rest_api.py | 19 ++++++-
> 4 files changed, 168 insertions(+), 7 deletions(-)
>
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 4d454c7..6324273 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -359,14 +359,14 @@ class Patch(Submission):
> for tag in tags:
> self._set_tag(tag, counter[tag])
>
> - def save(self):
> + def save(self, **kwargs):
> if not hasattr(self, 'state') or not self.state:
> self.state = get_default_initial_patch_state()
>
> if self.hash is None and self.diff is not None:
> self.hash = hash_patch(self.diff).hexdigest()
>
> - super(Patch, self).save()
> + super(Patch, self).save(**kwargs)
>
> self.refresh_tag_counts()
>
> diff --git a/patchwork/rest_serializers.py b/patchwork/rest_serializers.py
> index 0e2e371..9558d45 100644
> --- a/patchwork/rest_serializers.py
> +++ b/patchwork/rest_serializers.py
> @@ -17,9 +17,14 @@
> # along with Patchwork; if not, write to the Free Software
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> -from patchwork.models import Person, Project
> +import email.parser
>
> -from rest_framework.serializers import ModelSerializer
> +from django.core.urlresolvers import reverse
> +
> +from patchwork.models import Patch, Person, Project
nit: These should be in order:
* Standard library imports
* Third party imports
* Application imports
> +
> +from rest_framework.serializers import (
> + ListSerializer, ModelSerializer)
>
>
> class PersonSerializer(ModelSerializer):
> @@ -36,3 +41,33 @@ class PersonSerializer(ModelSerializer):
> class ProjectSerializer(ModelSerializer):
> class Meta:
> model = Project
> +
> +
> +class PatchListSerializer(ListSerializer):
> + '''Semi hack to make the list of patches more efficient'''
> + def to_representation(self, data):
> + del self.child.fields['content']
> + del self.child.fields['headers']
> + del self.child.fields['diff']
> + return super(PatchListSerializer, self).to_representation(data)
This works, but it is a hack like you say. I did some experimentation
and saw that defining 'exclude' via a Meta class doesn't work, sadly.
However, there do seem to be other ways to do this that might be worth
looking into, if you want:
https://stackoverflow.com/q/22616973
> +
> +class PatchSerializer(ModelSerializer):
> + class Meta:
> + model = Patch
> + list_serializer_class = PatchListSerializer
> + read_only_fields = ('project', 'name', 'date', 'submitter', 'diff',
> + 'content', 'hash', 'msgid')
> +
> + def to_representation(self, instance):
> + request = self.context.get('request', None)
> + data = super(PatchSerializer, self).to_representation(instance)
> +
> + data['project'] = request.build_absolute_uri(
> + reverse('api_1.0:project-detail', args=[data['project']]))
> + data['submitter'] = request.build_absolute_uri(
> + reverse('api_1.0:person-detail', args=[data['submitter']]))
> + headers = data.get('headers')
> + if headers:
> + data['headers'] = email.parser.Parser().parsestr(headers, True)
> + return data
Again, this works but 'HyperLinkedModelSerializer' seems like a natural
fit. If you go this route, you should look at 'SerializerMethodField'
to handle 'headers'.
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index 3f98595..7107dfa 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -25,8 +25,9 @@ from django.core.urlresolvers import reverse
> from rest_framework import status
> from rest_framework.test import APITestCase
>
> -from patchwork.models import Project
> -from patchwork.tests.utils import defaults, create_maintainer, create_user
> +from patchwork.models import Patch, Project
> +from patchwork.tests.utils import (
> + defaults, create_maintainer, create_user, create_patches, make_msgid)
>
>
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> @@ -157,3 +158,111 @@ class TestPersonAPI(APITestCase):
>
> resp = self.client.post(self.api_url(), {'email': 'foo at f.com'})
> self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> +
> + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> +class TestPatchAPI(APITestCase):
> + fixtures = ['default_states']
> +
> + @staticmethod
> + def api_url(item=None):
> + if item is None:
> + return reverse('api_1.0:patch-list')
> + return reverse('api_1.0:patch-detail', args=[item])
> +
> + def test_list_simple(self):
> + """Validate we can list a patch."""
> + patches = create_patches()
> + resp = self.client.get(self.api_url())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(1, len(resp.data))
> + patch = resp.data[0]
> + self.assertEqual(patches[0].name, patch['name'])
> +
> + # test while authenticated
> + user = create_user()
> + self.client.force_authenticate(user=user)
> + resp = self.client.get(self.api_url())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(1, len(resp.data))
> + patch = resp.data[0]
> + self.assertEqual(patches[0].name, patch['name'])
Perhaps assert that excluded fields are indeed excluded?
> + def test_get(self):
> + """Validate we can get a specific project."""
> + patches = create_patches()
> + resp = self.client.get(self.api_url(patches[0].id))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertEqual(patches[0].name, resp.data['name'])
> + self.assertIn(TestProjectAPI.api_url(patches[0].project.id),
> + resp.data['project'])
> + self.assertEqual(patches[0].msgid, resp.data['msgid'])
> + self.assertEqual(patches[0].diff, resp.data['diff'])
> + self.assertIn(TestPersonAPI.api_url(patches[0].submitter.id),
> + resp.data['submitter'])
> + self.assertEqual(patches[0].state.id, resp.data['state'])
> +
> + def test_anonymous_writes(self):
> + """Ensure anonymous "write" operations are rejected."""
> + patches = create_patches()
> + patch_url = self.api_url(patches[0].id)
> + resp = self.client.get(patch_url)
> + patch = resp.data
> + patch['msgid'] = 'foo'
> + patch['name'] = 'this should fail'
> +
> + # create
> + resp = self.client.post(self.api_url(), patch)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> + # update
> + resp = self.client.patch(patch_url, {'name': 'foo'})
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> + # delete
> + resp = self.client.delete(patch_url)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
Again, this could probably be merged with test(create|update|delete)
functions below.
> + def test_create(self):
> + """Ensure creations are rejected."""
> + create_patches()
> + patch = {
> + 'project': defaults.project.id,
> + 'submitter': defaults.patch_author_person.id,
> + 'msgid': make_msgid(),
> + 'name': 'test-create-patch',
> + 'diff': 'patch diff',
> + }
> +
> + user = create_maintainer(defaults.project)
> + user.is_superuser = True
> + user.save()
> + self.client.force_authenticate(user=user)
> + resp = self.client.post(self.api_url(), patch)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + def test_update(self):
> + """Ensure updates can be performed maintainers."""
> + patches = create_patches()
> +
> + # A maintainer can update
> + user = create_maintainer(defaults.project)
> + self.client.force_authenticate(user=user)
> + resp = self.client.patch(self.api_url(patches[0].id), {'state': 2})
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> +
> + # A normal user can't
> + user = create_user()
> + self.client.force_authenticate(user=user)
> + resp = self.client.patch(self.api_url(patches[0].id), {'state': 2})
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + def test_delete(self):
> + """Ensure deletions are rejected."""
> + patches = create_patches()
> +
> + user = create_maintainer(defaults.project)
> + user.is_superuser = True
> + user.save()
> + self.client.force_authenticate(user=user)
> + resp = self.client.delete(self.api_url(patches[0].id))
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> + self.assertEqual(1, Patch.objects.all().count())
> diff --git a/patchwork/views/rest_api.py b/patchwork/views/rest_api.py
> index dad9724..1e03c68 100644
> --- a/patchwork/views/rest_api.py
> +++ b/patchwork/views/rest_api.py
> @@ -19,7 +19,8 @@
>
> from django.conf import settings
>
> -from patchwork.rest_serializers import ProjectSerializer, PersonSerializer
> +from patchwork.rest_serializers import (
> + PatchSerializer, ProjectSerializer, PersonSerializer)
>
> from rest_framework import permissions
> from rest_framework.pagination import PageNumberPagination
> @@ -77,6 +78,21 @@ class PatchworkViewSet(ModelViewSet):
> return self.serializer_class.Meta.model.objects.all()
>
>
> +class PatchViewSet(PatchworkViewSet):
> + permission_classes = (PatchworkPermission,)
> + serializer_class = PatchSerializer
> +
> + def get_queryset(self):
> + qs = super(PatchViewSet, self).get_queryset(
> + ).prefetch_related(
> + 'check_set', 'tags'
> + ).select_related('state', 'submitter', 'delegate')
> + if 'pk' not in self.kwargs:
> + # we are doing a listing, we don't need these fields
> + qs = qs.defer('content', 'diff', 'headers')
> + return qs
> +
> +
> class PeopleViewSet(PatchworkViewSet):
> permission_classes = (AuthenticatedReadOnly, )
> serializer_class = PersonSerializer
> @@ -92,5 +108,6 @@ class ProjectViewSet(PatchworkViewSet):
>
>
> router = DefaultRouter()
> +router.register('patches', PatchViewSet, 'patch')
> router.register('people', PeopleViewSet, 'person')
> router.register('projects', ProjectViewSet, 'project')
> --
> 2.7.4
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list